Enhance completion details for builtins if Nix 2.4 is available #27
Conversation
A hidden feature of Nix 2.4+ (a.k.a. `nixUnstable`) is `nix
__dump-builtins` which returns a JSON giving details about built in
functions.
This change basically does the following things:
* It checks whether Nix 2.4 is available on `$PATH`[1]. In that case
built-in functions are retrieved via `nix __dump-builtins`[2].
If not, the hard-coded list of builtins is used as a fallback.
* If builtins can be retrieved from Nix, the following additional things
are added to `CompletionItem`:
* The `documentation`[3] field containing markdown documentation for the
function is added. This will be shown in editors as one is used to
it from e.g. doc-block comments in other languages.
* A very basic parameter info is provided. E.g. for
`builtins.removeAttrs` the popup shows `Lambda: set -> list ->
Result`. While this is fairly basic, it's IMHO a small benefit to
make sure one's not forgetting the order of parameters.
Please note that this will currently display *wrong* info if
a built-in is called via `lib.flip`.
* If `**DEPRECATED.**` is at the beginning of `documentation`, the
built-in is marked in the LSP response as `deprecated`. While this
is a gross hack, it ensures that the only deprecated builtin
(`builtins.toPath`) is actually marked as such.
[1] Please note that this means that you can wrap `$PATH` for `rnix-lsp`
to have this feature without being forced to use `nixUnstable` on
your system.
[2] As this is only an optional feature (and hence a loose dependency to
Nix) I decided against using an FFI and thus keeping the build
process rather simple.
[3] https://microsoft.github.io/language-server-protocol/specification#textDocument_completion
|
Okay.... I'll really have to do some memory profiling here. I just left a neovim window open for a few days (with a Nix-based side-project in it) and now when I resumed to work on it, rnix-lsp from this branch ate up wayyyy too much RAM. |
|
Do you think this branch is to blame, or rnix-lsp as a whole? We might have a leak somewhere, not sure where but I wouldn't be surprised |
|
I have no idea. Just realized that Since I haven't tested |
|
Damn, I begin to regret that I |
|
If you have patience, try opening a large file like |
|
@jD91mZM2 so I can occasionally reproduce this, but I didn't find a reasonable pattern yet. Also, sometimes neovim crashed with an out-of-memory error (which shouldn't be caused by Since I'm currently missing time & energy for a technical deep-dive in another project, I'm not really sure if I find something meaningful. Could you check the performance of current master (and probably this PR) to confirm my assumption? If that's true, we could consider this mergable (and investigate the issues in another ticket), though I think we may want to add some caching to the builtins implementation first :) |
I'm already aware Give your mark I understood your question properly, and I'll merge it! |
That sounds nice :) Regarding the merge: we may want to do some caching for the builtins here, but I didn't check whether this makes a difference and I shouldn't start another procrastination-project while actually learning for exams :D So if you're fine with the current state, we could merge this, see if your optimizations are sufficient and then decide whether this needs optimization or not. If you agree with this, feel free to merge :) |
|
Massive thanks for this, man! You're amazing |
|
@jD91mZM2 so I guess incremental parsing is the way to go:
I think that incremental parsing wouldn't just save memory, it could also make rnix-lsp more responsive. As you probably know, just switching the sync strategy to Do you want to take care of this at some point or should I? Though I'm not sure when/whether I'll get to this :) |
|
@jD91mZM2 okay, even though I can't reproduce it too often, @ony had probably more luck, hence I'd suggest to revert for now and see if it gets better (otherwise the problem may be related to the most recent crate updates). We should probably work on incremental parsing here first before we re-integrate this. |
…le (nix-community#27)" This reverts commit fd1ad49.
|
Just reviewed this PR and I see no change that can be suspected for memory leak. My only concern is I don't think leak is caused by this PR. |
|
Not so sure about that, let me explain:
What I'm not so sure about is if this change made things significantly worse or whether updates of the parser and stuff like this are the root-cause. That's why I filed #34 to give you a revision without this change to test. As you seem to hit this more often than I am, it'd be awesome if you could check whether this change makes things better. |
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)
* Enable incremental parsing via the LSP protocol
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] #27 (comment)
* Don't swallow empty lines if the previous one (including the `\n` separator) gets deleted
* Simplify (& hopefully fix) incremental parsing
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.
* Fix newlines
Note: this isn't completed ready yet:
.unwrap()-calls where better handling may be useful if e.g. the format ofnix __dump-builtinschanges in the future.A hidden feature of Nix 2.4+ (a.k.a.
nixUnstable) isnix __dump-builtinswhich returns a JSON giving details about built infunctions.
This change basically does the following things:
It checks whether Nix 2.4 is available on
$PATH[1]. In that casebuilt-in functions are retrieved via
nix __dump-builtins[2].If not, the hard-coded list of builtins is used as a fallback.
If builtins can be retrieved from Nix, the following additional things
are added to
CompletionItem:The
documentation[3] field containing markdown documentation for thefunction is added. This will be shown in editors as one is used to
it from e.g. doc-block comments in other languages.
A very basic parameter info is provided. E.g. for
builtins.removeAttrsthe popup showsLambda: set -> list -> Result. While this is fairly basic, it's IMHO a small benefit tomake sure one's not forgetting the order of parameters.
Please note that this will currently display wrong info if
a built-in is called via
lib.flip.If
**DEPRECATED.**is at the beginning ofdocumentation, thebuilt-in is marked in the LSP response as
deprecated. While thisis a gross hack, it ensures that the only deprecated builtin
(
builtins.toPath) is actually marked as such.[1] Please note that this means that you can wrap
$PATHforrnix-lspto have this feature without being forced to use
nixUnstableonyour system.
[2] As this is only an optional feature (and hence a loose dependency to
Nix) I decided against using an FFI and thus keeping the build
process rather simple.
[3] https://microsoft.github.io/language-server-protocol/specification#textDocument_completion
cc @jD91mZM2