add evaluator #38

aaronjanse · 2021-05-15T04:39:23Z

Related to #31.

I created this PR to show what integration with an evaluator will look like. We'll also have to figure out how to review all this code (maybe split it across many PRs?), if we want to merge it at all.

This particular PR adds hover-to-see-value functionality. it's requires very little modification to main.rs. Using the evaluator for completions and goto-definition would similarly require very little modification to main.rs.

How it works

The evaluator works by associating rnix-parser text ranges with lazy Nix values. It uses the gc crate for garbage collection.

The heart of the evaluator is the following struct, with comments added:

#[derive(Clone, Trace, Finalize)]
pub struct Tree {
    // Associates an expression with its position in a text file.
    // We call `children()` and recurse to find the value at a 
    // given cursor position. This is also useful for `goto`.
    #[unsafe_ignore_trace]
    pub range: Option<TextRange>,
    // Tracks variable bindings in scope and also the file path.
    pub scope: Gc<Scope>,
    // Processed rnix-parser nodes, contains child `Tree`s
    pub source: TreeSource,
    // Lazily evaluated NixValue. GcCell lets a Tree cache
    // evaluations without needing a mutable reference to `self`.
    pub value: GcCell<Option<Gc<NixValue>>>,
    // Hash of the syntax used to define this Tree. If we assume
    // that the same syntax evaluates to the same `value` (after
    // variables are substituted with their value, etc), then we
    // can re-use cached evaluations across file edits.
    // NOTE: this doesn't detect changes to imported files.
    pub hash: GcCell<Option<String>>,
}

Files

tests.rs has a few basic tests for the evaluator.
value.rs defines NixValue, which holds Nix primitive types such as int/string/bool.
scope.rs defines Scope, which handles scoping for Nix variables.
parse.rs converts a rnix-parser node into a Tree, a struct with associates a range of text with a lazily-evaluated NixValue and scope.
eval.rs converts a Tree's lazy value placeholder to a concrete NixValue.
builtins.rs uses macros to define a massive NixBuiltin enum. This is where all Nix builtins are implemented.
derivation.nix is a written-from-scratch implementation of nixpkgs/nix's derivation.nix, which defines a derivation function that calls derivationStrict. Note that the code looks similar due to needing to create sets with the same attribute names; the implementations themselves work in entirely different ways.

Code review

Perhaps it make sense to split these changes across a bunch of PRs? If so, I could start by submitting a PR with an arithmetic-only-evaluator then follow-up with PRs to slowly add back functionality until we can evaluate Nixpkgs again.

aaronjanse · 2021-05-15T04:41:25Z

src/eval.rs

+        Ok(self.eval()?.as_map()?.keys().cloned().collect())
+    }
+
+    fn hash_uncached(&self) -> Result<String, EvalError> {


Hashing (explained in the PR description) is an ugly hack at the moment. This PR is mostly just to show how integration with an evaluator would work if rnix-lsp wanted to do so.

aaronjanse · 2021-05-15T04:45:54Z

Oh for context, see the working proof of concept with all features enabled:

nixos-discourse · 2021-06-06T13:28:32Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/best-ide-for-writing-nix-expressions/13454/9

zimbatm · 2021-06-07T20:04:23Z

My rust foo is too low to be able to properly review this PR. Maybe @Mic92 or @rizary can help?

Mic92 · 2021-06-07T21:17:25Z

Does maybe @tazjin also want to have a look at it?

aaronjanse · 2021-06-07T21:24:47Z

To clarify, this PR is way too large to properly review its code. But if we're on board with the general idea (the PR description along with the general structure of the diff), I'd be happy to make smaller PRs that would be easier to review and discuss each step of the way

aaronjanse · 2021-06-07T21:28:00Z

Oh, we also might want to consider exposing the functionality of this PR as a Rust crate, since it seems like it would be generally helpful for the Nix community to have a way to evaluate and inspect Nix code without shelling out

Ma27

First of all, incredible, awesome work! ❤️

Below are a few comments from a first review, though I haven't thorougly tested it yet.

Perhaps it make sense to split these changes across a bunch of PRs? If so, I could start by submitting a PR with an arithmetic-only-evaluator then follow-up with PRs to slowly add back functionality until we can evaluate Nixpkgs again.

I think splitting this into multiple commits or suggesting where to start when reviewing might help :)

It uses the gc crate for garbage collection.

Perhaps I'm missing something obvious, but can you explain, why this is needed? I'm not against it, but I doubt that I fully understood the why behind this decision :)

Ma27 · 2021-06-18T18:59:51Z

src/builtins.rs

+                    let a: &NixValue = x.borrow();
+                    a.clone()
+                }))),
+            Err(_) => map.insert("value".to_string(), Gc::new(


builtins.tryEval does only catch errors from throw and assert.

Yep. We'll want a error type for user-generated errors. This will also help if we later to do the red squiggley diagnostic thing to show exactly which user input (e.g. a nixos option value) is causing an assertion to fail somewhere else.

Ma27 · 2021-06-18T19:00:48Z

src/builtins.rs

+            .replace(")", "\\)")
+            .replace("%%PIPE%%", "|")
+            .replace("%%L_PAREN%%", "(")
+            .replace("%%R_PAREN%%", ")");


Would you mind elaborating here what's this about? :)

Ah this is a very hacky way to handle this. I don't want this hack in rnix-lsp, so I'll need to replace it before putting it in any PRs I want merged

Ma27 · 2021-06-18T19:01:43Z

src/builtins.rs

+    "unsafeGetAttrPos <?>" ; UnsafeGetAttrPos1(_0: String) => |_param: Gc<Tree>, _attr: &String| {
+        let mut map = HashMap::new();
+        map.insert("column".to_string(), Gc::new(Tree::from_concrete(
+            NixValue::Integer(1)


This seems a little bit unexpected. Why not adding an unimplemented-error here? Or is this used too often to do this?

Ah, iirc unsafeGetAttrPor is required for evaluating nixpkgs, so I needed to "implement" it to see if an evaluating lsp is even possible. Like many things, I will make sure to provide an actual implementation before putting it in any non-draft PRs to rnix-lsp.

Ma27 · 2021-06-18T19:03:38Z

src/builtins.rs

+    "compareVersions" ; CompareVersions => |param| {
+        Ok(Gc::new(NixValue::Lambda(NixLambda::Builtin(NixBuiltin::CompareVersions1(param)))))
+    }
+    "compareVersions <?>" ; CompareVersions1(_0: Gc<Tree>) => |param: Gc<Tree>, left: &Gc<Tree>| {


Can't we use some kind of semver crate in here?

Perhaps, although we'll want to make sure we're using the exact same logic as official Nix. I'll make sure to link to the relevant section in the Nix manual in code comments.

Semver crates would be likely not bug-compatible with nix itself.

Ma27 · 2021-06-18T19:09:51Z

src/lookup.rs

+use crate::scope::Scope;

 lazy_static! {
    static ref BUILTINS: Vec<String> = vec![


(note for later: we should replace this list by reading the keys from builtins.rs.

Ma27 · 2021-06-18T19:10:53Z

src/main.rs

+            Err(e) => format!("{}", e),
+        };
+        Some((Some(range), val))
+    }


Now that we can load attr keys here as well: should we use this procedure for the completions as well?

Pretty much. In nix-eval-lsp, the only major difference is some code at the end pushing completions to a list:

https://github.com/aaronjanse/nix-eval-lsp/blob/0c263b802d83b3988924d6db55a6a0ca60faf578/src/main.rs#L235

Note that we'll want to merge rnix-lsp logic with evaluator logic to allow graceful fallback to static analysis when the evaluator gets confused.

aaronjanse · 2021-06-18T19:31:50Z

I think splitting this into multiple commits or suggesting where to start when reviewing might help :)

Oh definitely. For this PR, I recommend focusing on mostly the description. For actually reviewing code, I think it would make sense for me to make a PR that only handles arithmetic, would would make the code much easier to review. After that's merged, I could make PRs adding more features in small steps.

aaronjanse · 2021-06-18T19:34:46Z

Also to clarify, I don't think the code itself is ready to be merged. But I wanted to make sure we're both on board with the structure before I spend significant time making the implementation do exactly what we want.

aaronjanse · 2021-06-18T19:42:57Z

It uses the gc crate for garbage collection.

Perhaps I'm missing something obvious, but can you explain, why this is needed? I'm not against it, but I doubt that I fully understood the why behind this decision :)

No worries. The official Nix evaluator using a tracing garbage collector, and the Nix language is designed for garbage collection. I originally tried to implement this with borrowing and Rc, but that's unable to handle many scenarios. For example, consider how garbage collection needs to work for Nixpkgs's implementation of the fixed-point operator (lib/fixed-points.nix#L3-L19):

let
  fix = (f: let x = f x; in x);
  attrs = (self: { x = 1; y = self.x; });
in
  (fix attrs).x

I also found this helpful blog post about garbage collectors in Rust, written by the author of the gc crate (and much more).

Ma27 · 2021-06-18T20:11:52Z

Oh definitely. For this PR, I recommend focusing on mostly the description

The overall structure makes sense to me, however I'm not an expert on how libexpr of Nix is implemented internally, so I cannot say if there are potential flaws that I'm missing.

For actually reviewing code, I think it would make sense for me to make a PR that only handles arithmetic, would would make the code much easier to review. After that's merged, I could make PRs adding more features in small steps.

I think such an incremental progress where we (or at least I'd be up for that) could dogfood the changes by using them locally makes sense.

My next task here would be to finish incremental parsing (I think I found a small bug), but let me know if I can help!

Mic92 · 2021-06-18T20:12:06Z

@Ma27 do you want to adopt this project? You could also maintain this temporary until we find someone else.

Ma27 · 2021-06-18T20:35:29Z

@Mic92 Aaron and I already have commit access and we decided that they will become the "official" LSP maintainer and I'll become the "official" rnix-parser maintainer even though we'll work on both projects ;)

Mic92 · 2021-06-18T20:47:25Z

@Mic92 Aaron and I already have commit access and we decided that they will become the "official" LSP maintainer and I'll become the "official" rnix-parser maintainer even though we'll work on both projects ;)

I updated the repo description to reflect that.

aaronjanse · 2021-06-26T06:44:43Z

Status update: I'm busy for the next few days, but next week I'll submit a PR that does basic arithmetic. I think that should be more helpful for discussion than this draft PR

nayr7 · 2022-05-20T10:58:30Z

Is there any blockage concerning this PR ? I'm very interested since I use Nix everyday, what is the current state of this PR ?

aaronjanse commented May 15, 2021

View reviewed changes

zimbatm requested a review from jD91mZM2 May 16, 2021 21:46

aaronjanse mentioned this pull request Jun 10, 2021

support outside of vscode? aaronjanse/nix-eval-lsp#3

Open

Ma27 reviewed Jun 18, 2021

View reviewed changes

lapp0 mentioned this pull request Jun 23, 2021

Integrate Language Server nix-gui/nix-gui#2

Closed

add evaluator

92ba7ee

aaronjanse force-pushed the add_evaluator branch from 71d1d23 to 92ba7ee Compare June 30, 2021 18:25

aaronjanse mentioned this pull request Jul 1, 2021

evaluate arithmetic #45

Merged

Ma27 mentioned this pull request Jul 5, 2021

Possible features and ideas #41

Open

11 tasks

Uh oh!

Uh oh!

add evaluator #38

Are you sure you want to change the base?

add evaluator #38

Uh oh!

Conversation

aaronjanse commented May 15, 2021 • edited by Ma27 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How it works

Files

Code review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronjanse commented May 15, 2021

Uh oh!

nixos-discourse commented Jun 6, 2021

Uh oh!

zimbatm commented Jun 7, 2021

Uh oh!

Mic92 commented Jun 7, 2021

Uh oh!

aaronjanse commented Jun 7, 2021

Uh oh!

aaronjanse commented Jun 7, 2021

Uh oh!

Ma27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mic92 Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronjanse commented Jun 18, 2021

Uh oh!

aaronjanse commented Jun 18, 2021

Uh oh!

aaronjanse commented Jun 18, 2021

Uh oh!

Ma27 commented Jun 18, 2021

Uh oh!

Mic92 commented Jun 18, 2021

Uh oh!

Ma27 commented Jun 18, 2021

Uh oh!

Mic92 commented Jun 18, 2021

Uh oh!

aaronjanse commented Jun 26, 2021

Uh oh!

nayr7 commented May 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

aaronjanse commented May 15, 2021 •

edited by Ma27

Loading

Mic92 Jul 1, 2021 •

edited

Loading