-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial handling of incomplete input in nls #1541
Conversation
|
||
// The definition of this identifier is unlikely to belong to the | ||
// environment we started with, especially because the enviroment | ||
// mechanism is only used for providing definitions to incompletely | ||
// parsed input. | ||
let env = Environment::new(); | ||
let defs = def.resolve_terms(&env, server); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that just an optimization? I.e. would it still be correct to pass along the environment given to resolve
instead of a new environment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can actually make a difference, although maybe not in any case that matters. For example:
let y = x in
let x = { bar = 1 } in
[ y.
(with the cursor at the end). The incomplete part of the input has initial environment with y bound to Term::Var(x)
and x
bound to {bar = 1}
When trying to provide completions, we follow the initial environment to replace y by Term::Var(x)
. Then the correct thing to do is to see that x
is undefined and stop looking for completions, but if we don't reset the environment we'll fall back to the other x
and erroneously provide "bar" as a completion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But would
let x = { bar = 1 } in
let y = x in
[ y.
provide the bar
completion then? I'm probably missing something about the environment discipline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because for the part of the input that parses, environments get attached to every term. So the Term::Var(x)
in y = x
will get inserted into the defs usage table with its own environment.
The extra env
parameter to field resolution is an extra environment that only applies for the first lookup. It's to compensate for the fact that [ y.
doesn't get parsed and so it doesn't get an environment attached. I'll add some better docs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation!
This adds support for parsing incomplete input to nls, enabling the new term-based completer to be used in situations like
let x = some.path
(where the cursor is at the end).Fixes #1045.