Enable incremental parsing via the LSP protocol #37
Conversation
With incremental parsing, the editor only sends a "diff" of what has
changed rather than the full file. For instance, it's pretty pointless
to send all of `all-packages.nix` from `nixpkgs` for a single-letter
change.
Given the benchmark of `rnix-parser`[1] it doesn't seem as the parsing
itself is the bottleneck a few people have reported[2], so this change
only replaces affected lines and re-parses everything using
`rnix-parser`.
While the current change seems usable on local tests, it has (at
least) two known issues:
* As soon as you type `inherit`, the LSP stalls and it seems it's due to
`rnix-parser` and steadily allocates more memory according to
`strace(1)`. I confirmed that it's indeed a problem of `rnix-parser`
by creating a file with `{ inherit` as content and the LSP refuses to
respond (with incremental parsing disabled).
* Given an expression like this:
``` nix
let a = 1; in
{
b = a;
}
```
When deleting line one and then line 2, the state ends up as `{{ }`.
This is because the range passed to LSP looks like this:
```
Range {
start: Position {
line: 1,
character: 0
},
end: Position {
line: 2,
character: 0
}
}
```
This also kills the second line of the expression above and as soon as
you delete this as well, the state will end up with bogus results.
It's a bit weird though since I got an equal message when deleting
more lines, so I'm not entirely sure if that's just a bug in the LSP
implementation of my neoVIM (and also the reason why I didn't just
insert a `\n` at the right vec entries).
[1] https://github.com/nix-community/rnix-parser/blob/v0.9.0/benches/all-packages.rs
[2] nix-community#27 (comment)
src/main.rs
Outdated
| last.chars().rev() | ||
| .take(last.len() - (end.character as usize)) | ||
| .collect::<String>() | ||
| .chars().rev().collect::<String>() |
There was a problem hiding this comment.
...and this is incredibly ugly, but I couldn't wrap my mind around a nicer solution here this afternoon ^^
There was a problem hiding this comment.
Perhaps last[end.character..last.len()].to_string()? Or maybe I'm misreading
…arator) gets deleted
|
Ok, using this for two days now and it seems as pasting seems still error-prone. Gotta investigate this a bit further. Nevertheless, feedback / testing from others is always welcome :) |
Rather than trying to mess around in an existing datastructure and thus running into tons of mean special cases since you have to take care of removing/adding lines on your own, we just write everything to a fresh string and use this for parsing and inside the HashMap containing the AST of each known Nix expression. Not sure if it works out the way I hope, but it seems a little bit better now. Will test it a while on my setup before I consider this ready.
|
Just pushed a new implementation that's hopefully better. Will use this for a while and we'll see :) |
|
cc @jD91mZM2 |
There was a problem hiding this comment.
This is great! There are a few .get(i as usize).unwrap() calls that could be replaced with [i as usize], but I think we might actually want to keep them and eventually replace .unwrap() with something else so we can have a panic-free codebase.
I haven't yet had a chance to try this PR locally, but hopefully it'll help with performance on large files.
|
I've tried it now, and it works great. Thank you @Ma27! |
|
Feel free to merge this PR, and thank you for implementing this! |
With incremental parsing, the editor only sends a "diff" of what has
changed rather than the full file. For instance, it's pretty pointless
to send all of
all-packages.nixfromnixpkgsfor a single-letterchange.
Given the benchmark of
rnix-parser[1] it doesn't seem as the parsingitself is the bottleneck a few people have reported[2], so this change
only replaces affected lines and re-parses everything using
rnix-parser.While the current change seems usable on local tests, it has (at
least) two known issues:
As soon as you type
inherit, the LSP stalls and it seems it's due tornix-parserand steadily allocates more memory according tostrace(1). I confirmed that it's indeed a problem ofrnix-parserby creating a file with
{ inheritas content and the LSP refuses torespond (with incremental parsing disabled).
Should be fixed with my second commit:
Given an expression like this:When deleting line one and then line 2, the state ends up as{{ }.This is because the range passed to LSP looks like this:
This also kills the second line of the expression above and as soon asyou delete this as well, the state will end up with bogus results.
It's a bit weird though since I got an equal message when deletingmore lines, so I'm not entirely sure if that's just a bug in the LSP
implementation of my neoVIM (and also the reason why I didn't just
insert a
\nat the right vec entries).[1] https://github.com/nix-community/rnix-parser/blob/v0.9.0/benches/all-packages.rs
[2] #27 (comment)
cc @jD91mZM2
It may make sense if you could test this as well and see if you find more issues :)
I'm not sure what to do about the second known issue I described btw.
Also, I guess I'll need to start writing a test suite for this, testing all of these cases when changing something isn't very nice :)