Context completion #1584
Context completion #1584
Conversation
jneem
force-pushed
the
env-completion
branch
2 times, most recently
from
73b9f10
to
2608a34
Compare
| @@ -30,6 +30,57 @@ pub mod interface; | |||
|
|
|||
| pub type Environment = nickel_lang_core::environment::Environment<Ident, ItemId>; | |||
|
|
|||
| #[derive(Clone, Debug)] | |||
| pub struct ParentLookup { | |||
| table: HashMap<RichTermPtr, RichTerm>, | |||
There was a problem hiding this comment.
It's a bit sad to go through a hashmap, while we could use pointers directly. But this might be the most reasonable short-term. In the future, how disrupting would that be to define a type like:
pub struct NotRichTerm {
term: SharedTerm,
pos: TermPos,
parent: Option<std::rc::Weak<NotRichTerm>>,
}And use that instead of RichTerm?
Secondly, regarding RichTermptr, it's a detail but do we really care about the position when hashing, or could we simply hash the pointer alone?
There was a problem hiding this comment.
(PS: I meant use that instead of RichTerm in the LSP, not in the whole codebase)
There was a problem hiding this comment.
I think we don't care about the position, and cargo test agrees so I'll drop it. I'll do the pointer-chasing in a follow-up, if that's ok. I'd like to prioritize getting rid of the linearizer.
| let mut traverse_merge = | ||
| |rt: &RichTerm, parent: &Option<RichTerm>| -> TraverseControl<Option<RichTerm>, ()> { | ||
| if let Some(parent) = parent { | ||
| table.insert(RichTermPtr(rt.clone()), parent.clone()); |
There was a problem hiding this comment.
Just as safety check, are we correctly dropping the table when e.g. the current file is parsed an linearized again? Otherwise we could keep some Rc alive.
There was a problem hiding this comment.
Good point, we are not dropping the table yet. I'll fix that.
There was a problem hiding this comment.
Another solution is to use weak pointers
lsp/nls/src/linearization/mod.rs
Outdated
| self.position_lookups | ||
| .insert(file_id, PositionLookup::new(term)); | ||
| self.usage_lookups.insert(file_id, UsageLookup::new(term)); | ||
| self.parent_lookups.insert(file_id, ParentLookup::new(term)); |
There was a problem hiding this comment.
I think it's fine for now, and this needs to be measured, but it might be more efficient to do only one traversal and all the transformation/analysis at each node - if possible of course - for cache/memory access reasons.
lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-basic.ncl.snap
Outdated
Show resolved
Hide resolved
lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-dict.ncl.snap
Outdated
Show resolved
Hide resolved
jneem
force-pushed
the
env-completion
branch
from
2608a34
to
cd68b93
Compare
|
Ok, I rebased and I also changed the behavior to use either context completion or env completion, but not both at once. At the same time, I disabled the old completer by default so that the new behavior is easier to see. What do you think of the test output now, @yannham ? |
There was a problem hiding this comment.
Looks good when only selecting one source of completion 👍
I wonder if we have any tests for context completion (as in, completion when defining a field inside a record literal based on parents)? I only grepped quickly over the completion inputs for test but only saw dot-triggered completion mostly. Doesn't have to be in this PR, but this could be interesting to monitor as well.
|
Good point, there were one or two cases in the old tests that hit env completion, but I added a few more and already found (and fixed) an issue 😅 |
jneem
force-pushed
the
env-completion
branch
from
b24ce69
to
5ca2277
Compare
This adds completions from things that are "in scope," liberally interpreted. This includes things that are actually in scope, but also things that we can tell will be "brought into" scope by merges and contract annotations.
Fixes #1499. This also makes (at least according to the current regression tests) the new completer a superset of the old one. I'll be planning to remove the old one soonish. (After 1.2, obviously, and also after adding some more comprehensive tests.)
One behavioral question I have is whether it makes sense to include this scope-based completion if we detect that they are in the middle of a record path. The old completer did, so I am doing it also. But since we offer more completions than the old completer, maybe it makes too many false positives?