-
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
Completion in arrays #1746
Completion in arrays #1746
Conversation
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 like the fact that adding this feature is mostly a small chunk of boring but expected code, which is a good sign that the current architecture of the LSP is way better than before 🙂
#[derive(Clone, Debug, PartialEq)] | ||
pub enum FieldHaver { | ||
pub enum Container { |
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 wonder: would that make sense to make this a trait instead? It seems that most of what you do below is to match on the type of container, and then apply specific behavior. It sounds a lot like what traits are for.
Also, it feels slightly odd to mix array and records, leading to a rather vague notion of container (I guess it's why this is hard to give it a meaningful name) and an EltId
which also feels like it smashes two different things together to then match on the different possibilities and apply specific behavior (also, if I'm understanding correctly, some illegal cases such as trying to get an ident from an array aren't statically excluded but just return None
).
At this point I might be oblivious to important details that make the current approach actually justified, sorry if this is the case. But from a distance, would that lead to much code duplication to have:
A trait FieldHaver
with the methods you already implemented before this PR, and implement it for RecordData
, Type
, and RecordRows
.
A trait Indexable
(that's a bad name and it doesn't relate to FieldHaver
, so it's doubly bad...) that is mostly doing the same but for array elements, which doesn't use Ident
nor field
in the method's name, and implement that for Array
.
What do you think?
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.
Good points, I will think more about this over the break. I think there is at least one situation where you do want to dynamically handle either a record field or an array element, but it's probably a smaller use case and there's probably a better organization.
To elaborate, suppose we're trying to complete at the a
in {foo = [{ a }]} | {foo | Array { aardvark | String }}
. The way we currently do this without any evaluation is to first construct the path to the place we want completion. In some weird notation I just made up, this path looks like . foo / !!
(where the !!
means an array index -- we don't track what the index actually was). Then we follow the same path on the contract {foo | Array { aardvark | String }}
to find the aardvark
completion.
So the path does need to alternate at runtime between field accesses and array indices, and it might also happen that the path-follower tries to match an array index against a record (say, if the contract had been {foo | {_: {aardvark | String}}}
). I don't know of any valid nickel code where this will happen, but of course nls also needs to handle invalid nickel code.
lsp/nls/src/field_walker.rs
Outdated
@@ -347,6 +372,21 @@ impl<'a> FieldResolver<'a> { | |||
Vec::new() | |||
} | |||
} | |||
Term::Array(_, attrs) => attrs | |||
.pending_contracts |
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'm surprised that there's anything in pending_contracts
at this point. If I recall correctly, pending_contracts
on array attributes is only filled at evaluation time when applying an Array
contract, which updates this field. It's a runtime thing. Do you get anything out of it here in practice?
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.
Hm, I'm not actually sure. I'll check if removing it changes anything.
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.
@jneem let's proceed with this version, code structure is less important. However, maybe we should flag this now with either a comment or remove it altogether (at worse re-add it later if it turns out to do something in practice, but I doubt it)
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.
While code structure is important for long-term maintainability, the code in this PR is still entirely reasonable and brings a nice feature for 1.4. Let's proceed in this state (beside my comment around pending_contracts, to solve first), include this in 1.4 and from there think harder about the right way to structure this.
This makes the completer aware of array contracts, allowing it to provide completion in more circumstances. The tl;dr is that
[{ a }] | Array { aardvark | String }
will now provide the completion "aardvark" after the "a".Fixes #1732