Completion in arrays #1746
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,35 +13,55 @@ use nickel_lang_core::{ | |
|
|
||
| use crate::{identifier::LocIdent, requests::completion::CompletionItem, server::Server}; | ||
|
|
||
| /// A `Container` is something that has elements. | ||
| /// | ||
| /// The elements could have names (e.g. in a record) or not (e.g. in an array). | ||
| /// | ||
| /// You can use a [`FieldResolver`] to resolve terms, or terms with paths, to `Container`s. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub enum Container { | ||
| RecordTerm(RecordData), | ||
| Dict(Type), | ||
| RecordType(RecordRows), | ||
| Array(Type), | ||
| } | ||
|
|
||
| /// A `ChildId` identifies an element of a container. | ||
| #[derive(Clone, Copy, Debug, PartialEq)] | ||
| pub enum EltId { | ||
| /// A named element, for example of an array. | ||
| Ident(Ident), | ||
| /// An array element. | ||
| ArrayElt, | ||
| } | ||
|
|
||
| impl From<Ident> for EltId { | ||
| fn from(id: Ident) -> Self { | ||
| EltId::Ident(id) | ||
| } | ||
| } | ||
|
|
||
| impl Container { | ||
| /// If this `Container` has a field named `id`, returns its value. | ||
| fn get(&self, id: EltId) -> Option<FieldContent> { | ||
| match (self, id) { | ||
| (Container::RecordTerm(data), EltId::Ident(id)) => data | ||
| .fields | ||
| .get(&id) | ||
| .map(|field| FieldContent::RecordField(field.clone())), | ||
| (Container::Dict(ty), EltId::Ident(_)) => Some(FieldContent::Type(ty.clone())), | ||
| (Container::RecordType(rows), EltId::Ident(id)) => rows | ||
| .find_path(&[id]) | ||
| .map(|row| FieldContent::Type(row.typ.clone())), | ||
| (Container::Array(ty), EltId::ArrayElt) => Some(FieldContent::Type(ty.clone())), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| /// If this `Container` is a record term, try to retrieve the field named `id`. | ||
| fn get_field_and_loc(&self, id: Ident) -> Option<(LocIdent, &Field)> { | ||
| match self { | ||
| Container::RecordTerm(data) => data | ||
| .fields | ||
| .get_key_value(&id) | ||
| .map(|(id, fld)| (LocIdent::from(*id), fld)), | ||
|
|
@@ -51,19 +71,20 @@ impl FieldHaver { | |
|
|
||
| pub fn get_definition_pos(&self, id: Ident) -> Option<LocIdent> { | ||
| match self { | ||
| Container::RecordTerm(data) => data | ||
| .fields | ||
| .get_key_value(&id) | ||
| .map(|(id, _field)| (*id).into()), | ||
| Container::RecordType(rows) => rows.find_path(&[id]).map(|r| r.id.into()), | ||
| Container::Dict(_) => None, | ||
| Container::Array(_) => None, | ||
| } | ||
| } | ||
|
|
||
| /// Returns all fields in this `Container`, rendered as LSP completion items. | ||
| pub fn completion_items(&self) -> impl Iterator<Item = CompletionItem> + '_ { | ||
| match self { | ||
| Container::RecordTerm(data) => { | ||
| let iter = data.fields.iter().map(|(id, val)| CompletionItem { | ||
| label: ident_quoted(id), | ||
| detail: metadata_detail(&val.metadata), | ||
|
|
@@ -73,8 +94,10 @@ impl FieldHaver { | |
| }); | ||
| Box::new(iter) | ||
| } | ||
| Container::Dict(_) | Container::Array(_) => { | ||
| Box::new(std::iter::empty()) as Box<dyn Iterator<Item = _>> | ||
| } | ||
| Container::RecordType(rows) => { | ||
| let iter = rows.iter().filter_map(|r| match r { | ||
| RecordRowsIteratorItem::TailDyn => None, | ||
| RecordRowsIteratorItem::TailVar(_) => None, | ||
|
|
@@ -91,7 +114,7 @@ impl FieldHaver { | |
| } | ||
| } | ||
|
|
||
| /// [`Container`]s can have fields that are either record fields or types. | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| enum FieldContent { | ||
| RecordField(Field), | ||
|
|
@@ -219,8 +242,8 @@ impl<'a> FieldResolver<'a> { | |
| pub fn resolve_term_path( | ||
| &self, | ||
| rt: &RichTerm, | ||
| path: impl Iterator<Item = EltId>, | ||
| ) -> Vec<Container> { | ||
| let mut fields = self.resolve_term(rt); | ||
|
|
||
| for id in path { | ||
|
|
@@ -286,11 +309,13 @@ impl<'a> FieldResolver<'a> { | |
| ret | ||
| } | ||
|
|
||
| fn resolve_def_with_path(&self, def: &Def) -> Vec<Container> { | ||
| let mut fields = Vec::new(); | ||
|
|
||
| if let Some(val) = def.value() { | ||
| fields.extend_from_slice( | ||
| &self.resolve_term_path(val, def.path().iter().copied().map(EltId::Ident)), | ||
| ) | ||
| } | ||
| if let Some(meta) = def.metadata() { | ||
| fields.extend(self.resolve_annot(&meta.annotation)); | ||
|
|
@@ -306,7 +331,7 @@ impl<'a> FieldResolver<'a> { | |
| fields | ||
| } | ||
|
|
||
| fn resolve_annot(&'a self, annot: &'a TypeAnnotation) -> impl Iterator<Item = Container> + 'a { | ||
| annot | ||
| .contracts | ||
| .iter() | ||
|
|
@@ -319,10 +344,10 @@ impl<'a> FieldResolver<'a> { | |
| /// This a best-effort thing; it doesn't do full evaluation but it has some reasonable | ||
| /// heuristics. For example, it knows that the fields defined on a merge of two records | ||
| /// are the fields defined on either record. | ||
| pub fn resolve_term(&self, rt: &RichTerm) -> Vec<Container> { | ||
| let term_fields = match rt.term.as_ref() { | ||
| Term::Record(data) | Term::RecRecord(data, ..) => { | ||
| vec![Container::RecordTerm(data.clone())] | ||
| } | ||
| Term::Var(id) => { | ||
| let id = LocIdent::from(*id); | ||
|
|
@@ -347,6 +372,21 @@ impl<'a> FieldResolver<'a> { | |
| Vec::new() | ||
| } | ||
| } | ||
| Term::Array(_, attrs) => attrs | ||
| .pending_contracts | ||
|
There was a problem hiding this comment. I'm surprised that there's anything in There was a problem hiding this comment. Hm, I'm not actually sure. I'll check if removing it changes anything. There was a problem hiding this comment. @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) |
||
| .iter() | ||
| .filter_map(|ctr| { | ||
| if let Term::Type(ty) = ctr.contract.as_ref() { | ||
| if let TypeF::Array(elt_ty) = &ty.typ { | ||
| Some(Container::Array((**elt_ty).clone())) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect(), | ||
| Term::ResolvedImport(file_id) => self | ||
| .server | ||
| .cache | ||
|
|
@@ -358,7 +398,7 @@ impl<'a> FieldResolver<'a> { | |
| } | ||
| Term::Let(_, _, body, _) | Term::LetPattern(_, _, _, body) => self.resolve_term(body), | ||
| Term::Op1(UnaryOp::StaticAccess(id), term) => { | ||
| self.resolve_term_path(term, std::iter::once(id.ident().into())) | ||
| } | ||
| Term::Annotated(annot, term) => { | ||
| let defs = self.resolve_annot(annot); | ||
|
|
@@ -378,10 +418,11 @@ impl<'a> FieldResolver<'a> { | |
| combine(term_fields, typ_fields) | ||
| } | ||
|
|
||
| fn resolve_type(&self, typ: &Type) -> Vec<Container> { | ||
| match &typ.typ { | ||
| TypeF::Record(rows) => vec![Container::RecordType(rows.clone())], | ||
| TypeF::Dict { type_fields, .. } => vec![Container::Dict(type_fields.as_ref().clone())], | ||
| TypeF::Array(elt_ty) => vec![Container::Array(elt_ty.as_ref().clone())], | ||
| TypeF::Flat(rt) => self.resolve_term(rt), | ||
| _ => Default::default(), | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
EltIdwhich 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 returnNone).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
FieldHaverwith the methods you already implemented before this PR, and implement it forRecordData,Type, andRecordRows.A trait
Indexable(that's a bad name and it doesn't relate toFieldHaver, so it's doubly bad...) that is mostly doing the same but for array elements, which doesn't useIdentnorfieldin the method's name, and implement that forArray.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
ain{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 theaardvarkcompletion.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.