Add cousin search to goto and hover #1670
Conversation
lsp/nls/src/field_walker.rs
Outdated
| /// If this definition came from a record field, this is the containing | ||
| /// record. Note that having a parent record is mutually exclusive with | ||
| /// having a non-empty path, because non-empty paths only come from let | ||
| /// pattern bindings. |
There was a problem hiding this comment.
Would that make sense to encode this invariant as a type, instead of a comment (a simple enum with maybe some helper functions)?
| // We're traversing up the tree starting at the parent, so this is the | ||
| // path to the parent (not the original def). | ||
| if let Some(parent_path) = ancestors.path() { | ||
| let uncles = self.resolve_term_path(&ancestor, parent_path.iter().copied()); |
There was a problem hiding this comment.
So the uncles are the other leaf of the merge expression enclosing the current term? I think those method could benefit from being described at a high level in the documentation (even more if they're public API, despite "public API" being a bit less meaningful for NLS)
| // TODO: This usage map is based only on static scoping, and not on our "extended" | ||
| // scopes that we build up dynamically based on merges. Improving this probably | ||
| // requires building the extended scopes at static analysis time. |
There was a problem hiding this comment.
What exactly do you mean by that? We don't have dynamic scoping in Nickel, so you can't use a field that isn't part of the record, even if it will be brought by merging later. Or are you talking about context completion, e.g. in the case {foo | String} & { [.] } you would like to provide completion for foo?
There was a problem hiding this comment.
For context completion this already works, but one example that doesn't currently work is a "goto references" request for the first foo in
{ foo = 1 } & { bar = foo }
There was a problem hiding this comment.
nickel> { foo = 1 } & { bar = foo }
error: unbound identifier `foo`
┌─ <repl-input-0>:1:23
│
1 │ { foo = 1 } & { bar = foo }
│ ^^^ this identifier is unbound
Nickel doesn't have dynamic scoping, so this is invalid. Or did you mean {foo = 1} & {bar = foo, foo}, which would be the "valid" version? But then, I think it's ok to go to definition to the foo in the RHS. That's what you would do for a function (say (fun foo => foo) 1, you wouldn't go to 1, I mean you could but it's not really expected from the LSP).
There was a problem hiding this comment.
Sorry, that's what I get for trying to minimize an example without actually checking it 😅. But yes, currently we go to only the definition on the RHS whereas I'd like to go to both definitions (GotoDefinition can have multiple responses). Also, when asking for References on the LHS foo, I'd like to respond with the RHS foo.
There was a problem hiding this comment.
Ah, I didn't even know goto definition could show multiple locations (and haven't noticed on Nickel). Now it makes sense, thanks!
When encountering trees of merges and annotations in the AST, we've been doing some analysis to include "cousins" into the local scope when generating completions. This PR extends this analysis to hover and goto definition/references. For example, hovering over the first
fooin the following example now correctly brings up the doc metadata.