From ebbb4f294d6d6884e271d04221d874a72678266c Mon Sep 17 00:00:00 2001 From: jneem Date: Tue, 9 Jan 2024 12:08:40 -0600 Subject: [PATCH] Completion in arrays (#1746) * Rename FieldHaver -> Container and add an Array variant * Support arrays in the field resolver * Add a test * Do better on the renaming * Don't look at pending_contracts --- lsp/nls/src/analysis.rs | 67 ++++++++++++-- lsp/nls/src/field_walker.rs | 88 ++++++++++++------- lsp/nls/src/requests/completion.rs | 13 +-- lsp/nls/src/requests/goto.rs | 4 +- lsp/nls/src/requests/hover.rs | 8 +- lsp/nls/tests/inputs/completion-array.ncl | 14 +++ ...__tests__inputs__completion-array.ncl.snap | 7 ++ 7 files changed, 151 insertions(+), 50 deletions(-) create mode 100644 lsp/nls/tests/inputs/completion-array.ncl create mode 100644 lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-array.ncl.snap diff --git a/lsp/nls/src/analysis.rs b/lsp/nls/src/analysis.rs index 6d502353cc..26403e71bc 100644 --- a/lsp/nls/src/analysis.rs +++ b/lsp/nls/src/analysis.rs @@ -2,14 +2,13 @@ use std::collections::HashMap; use codespan::FileId; use nickel_lang_core::{ - identifier::Ident, term::{BinaryOp, RichTerm, Term, Traverse, TraverseControl}, typ::{Type, TypeF}, typecheck::{reporting::NameReg, TypeTables, TypecheckVisitor, UnifType}, }; use crate::{ - field_walker::Def, + field_walker::{Def, EltId}, identifier::LocIdent, position::PositionLookup, term::RichTermPtr, @@ -19,7 +18,7 @@ use crate::{ #[derive(Clone, Debug)] pub struct Parent { term: RichTerm, - child_name: Option, + child_name: Option, } impl From for Parent { @@ -54,7 +53,7 @@ impl ParentLookup { if let Some(child) = &field.value { let parent = Parent { term: rt.clone(), - child_name: Some(name.ident()), + child_name: Some(name.ident().into()), }; child.traverse_ref( &mut |rt, parent| traversal(rt, parent, acc), @@ -64,6 +63,19 @@ impl ParentLookup { } TraverseControl::SkipBranch } + Term::Array(arr, _) => { + for elt in arr.iter() { + let parent = Parent { + term: rt.clone(), + child_name: Some(EltId::ArrayElt), + }; + elt.traverse_ref( + &mut |rt, parent| traversal(rt, parent, acc), + &Some(parent), + ); + } + TraverseControl::SkipBranch + } _ => TraverseControl::ContinueWithScope(Some(rt.clone().into())), } } @@ -110,7 +122,7 @@ impl ParentLookup { /// `next` call. pub struct ParentChainIter<'a> { table: &'a ParentLookup, - path: Option>, + path: Option>, next: Option, } @@ -122,7 +134,11 @@ impl<'a> ParentChainIter<'a> { } if !matches!( next.term.as_ref(), - Term::Record(_) | Term::RecRecord(..) | Term::Annotated(..) | Term::Op2(..) + Term::Record(_) + | Term::RecRecord(..) + | Term::Annotated(..) + | Term::Op2(..) + | Term::Array(..) ) { self.path = None; } @@ -172,7 +188,7 @@ impl<'a> ParentChainIter<'a> { None } - pub fn path(&self) -> Option<&[Ident]> { + pub fn path(&self) -> Option<&[EltId]> { self.path.as_deref() } @@ -349,3 +365,40 @@ impl TypeCollector { CollectedTypes { terms, idents } } } + +#[cfg(test)] +mod tests { + use assert_matches::assert_matches; + use nickel_lang_core::{identifier::Ident, term::Term}; + + use crate::{ + field_walker::EltId, + position::tests::parse, + usage::{tests::locced, Environment, UsageLookup}, + }; + + use super::ParentLookup; + + #[test] + fn parent_chain() { + let (file, rt) = parse("{ foo = [{ bar = 1 }] }"); + let bar_id = Ident::new("bar"); + let bar = locced(bar_id, file, 11..14); + + let parent = ParentLookup::new(&rt); + let usages = UsageLookup::new(&rt, &Environment::new()); + let bar_rt = usages.def(&bar).unwrap().value().unwrap(); + + let p = parent.parent(bar_rt).unwrap(); + assert_eq!(p.child_name, Some(EltId::Ident(bar_id))); + assert_matches!(p.term.as_ref(), Term::RecRecord(..)); + + let gp = parent.parent(&p.term).unwrap(); + assert_eq!(gp.child_name, Some(EltId::ArrayElt)); + assert_matches!(gp.term.as_ref(), Term::Array(..)); + + let ggp = parent.parent(&gp.term).unwrap(); + assert_matches!(ggp.child_name, Some(EltId::Ident(_))); + assert_matches!(ggp.term.as_ref(), Term::RecRecord(..)); + } +} diff --git a/lsp/nls/src/field_walker.rs b/lsp/nls/src/field_walker.rs index 31dc6e82d8..a2fd9d2e87 100644 --- a/lsp/nls/src/field_walker.rs +++ b/lsp/nls/src/field_walker.rs @@ -13,35 +13,55 @@ use nickel_lang_core::{ use crate::{identifier::LocIdent, requests::completion::CompletionItem, server::Server}; -/// A `FieldHaver` is something that... has fields. +/// A `Container` is something that has elements. /// -/// You can use a [`FieldResolver`] to resolve terms, or terms with paths, to `FieldHaver`s. +/// 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 FieldHaver { +pub enum Container { RecordTerm(RecordData), Dict(Type), RecordType(RecordRows), + Array(Type), } -impl FieldHaver { - /// If this `FieldHaver` has a field named `id`, returns its value. - fn get(&self, id: Ident) -> Option { - match self { - FieldHaver::RecordTerm(data) => data +/// 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 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 { + match (self, id) { + (Container::RecordTerm(data), EltId::Ident(id)) => data .fields .get(&id) .map(|field| FieldContent::RecordField(field.clone())), - FieldHaver::Dict(ty) => Some(FieldContent::Type(ty.clone())), - FieldHaver::RecordType(rows) => rows + (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 `FieldHaver` is a record term, try to retrieve the field named `id`. + /// 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 { - FieldHaver::RecordTerm(data) => data + 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 { match self { - FieldHaver::RecordTerm(data) => data + Container::RecordTerm(data) => data .fields .get_key_value(&id) .map(|(id, _field)| (*id).into()), - FieldHaver::RecordType(rows) => rows.find_path(&[id]).map(|r| r.id.into()), - FieldHaver::Dict(_) => None, + Container::RecordType(rows) => rows.find_path(&[id]).map(|r| r.id.into()), + Container::Dict(_) => None, + Container::Array(_) => None, } } - /// Returns all fields in this `FieldHaver`, rendered as LSP completion items. + /// Returns all fields in this `Container`, rendered as LSP completion items. pub fn completion_items(&self) -> impl Iterator + '_ { match self { - FieldHaver::RecordTerm(data) => { + 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) } - FieldHaver::Dict(_) => Box::new(std::iter::empty()) as Box>, - FieldHaver::RecordType(rows) => { + Container::Dict(_) | Container::Array(_) => { + Box::new(std::iter::empty()) as Box> + } + Container::RecordType(rows) => { let iter = rows.iter().filter_map(|r| match r { RecordRowsIteratorItem::TailDyn => None, RecordRowsIteratorItem::TailVar(_) => None, @@ -91,7 +114,7 @@ impl FieldHaver { } } -/// [`FieldHaver`]s can have fields that are either record fields or types. +/// [`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, - ) -> Vec { + path: impl Iterator, + ) -> Vec { 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 { + fn resolve_def_with_path(&self, def: &Def) -> Vec { let mut fields = Vec::new(); if let Some(val) = def.value() { - fields.extend_from_slice(&self.resolve_term_path(val, def.path().iter().copied())) + 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 + 'a { + fn resolve_annot(&'a self, annot: &'a TypeAnnotation) -> impl Iterator + '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 { + pub fn resolve_term(&self, rt: &RichTerm) -> Vec { let term_fields = match rt.term.as_ref() { Term::Record(data) | Term::RecRecord(data, ..) => { - vec![FieldHaver::RecordTerm(data.clone())] + vec![Container::RecordTerm(data.clone())] } Term::Var(id) => { let id = LocIdent::from(*id); @@ -358,7 +383,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())) + self.resolve_term_path(term, std::iter::once(id.ident().into())) } Term::Annotated(annot, term) => { let defs = self.resolve_annot(annot); @@ -378,10 +403,11 @@ impl<'a> FieldResolver<'a> { combine(term_fields, typ_fields) } - fn resolve_type(&self, typ: &Type) -> Vec { + fn resolve_type(&self, typ: &Type) -> Vec { match &typ.typ { - TypeF::Record(rows) => vec![FieldHaver::RecordType(rows.clone())], - TypeF::Dict { type_fields, .. } => vec![FieldHaver::Dict(type_fields.as_ref().clone())], + 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(), } diff --git a/lsp/nls/src/requests/completion.rs b/lsp/nls/src/requests/completion.rs index ec9252be06..82cadc3271 100644 --- a/lsp/nls/src/requests/completion.rs +++ b/lsp/nls/src/requests/completion.rs @@ -15,7 +15,7 @@ use std::path::PathBuf; use crate::{ cache::CacheExt, - field_walker::{FieldHaver, FieldResolver}, + field_walker::{Container, EltId, FieldResolver}, identifier::LocIdent, incomplete, server::Server, @@ -110,8 +110,9 @@ fn record_path_completion(term: RichTerm, server: &Server) -> Vec Vec { @@ -124,14 +125,14 @@ fn env_completion(rt: &RichTerm, server: &Server) -> Vec { // If the current term is a record, add its fields. (They won't be in the environment, // because that's the environment *of* the current term. And we don't want to treat - // all possible FieldHavers here, because for example if the current term is a Term::Var + // all possible Containers here, because for example if the current term is a Term::Var // that references a record, we don't want it.) if matches!(rt.as_ref(), Term::RecRecord(..)) { items.extend( resolver .resolve_term(rt) .iter() - .flat_map(FieldHaver::completion_items), + .flat_map(Container::completion_items), ); } @@ -144,7 +145,7 @@ fn env_completion(rt: &RichTerm, server: &Server) -> Vec { // TODO: This adds our merge "cousins" to the environment, but we should also // be adding our merge "uncles" and "great-uncles". let records = resolver.resolve_term_path(&p, path.iter().rev().copied()); - items.extend(records.iter().flat_map(FieldHaver::completion_items)); + items.extend(records.iter().flat_map(Container::completion_items)); } } diff --git a/lsp/nls/src/requests/goto.rs b/lsp/nls/src/requests/goto.rs index 331a2cce2e..6db4dbf137 100644 --- a/lsp/nls/src/requests/goto.rs +++ b/lsp/nls/src/requests/goto.rs @@ -6,7 +6,7 @@ use serde_json::Value; use crate::{ cache::CacheExt, diagnostic::LocationCompat, - field_walker::{Def, FieldResolver}, + field_walker::{Def, EltId, FieldResolver}, identifier::LocIdent, server::Server, }; @@ -40,7 +40,7 @@ fn get_defs(term: &RichTerm, ident: Option, server: &Server) -> Option path.reverse(); let (last, path) = path.split_last()?; let path: Vec<_> = path.iter().map(|id| id.ident()).collect(); - let parents = resolver.resolve_term_path(value, path.iter().copied()); + let parents = resolver.resolve_term_path(value, path.iter().copied().map(EltId::Ident)); parents .iter() .filter_map(|parent| parent.get_definition_pos(last.ident())) diff --git a/lsp/nls/src/requests/hover.rs b/lsp/nls/src/requests/hover.rs index e08ab6af6c..57749261e2 100644 --- a/lsp/nls/src/requests/hover.rs +++ b/lsp/nls/src/requests/hover.rs @@ -12,7 +12,7 @@ use serde_json::Value; use crate::{ cache::CacheExt, diagnostic::LocationCompat, - field_walker::{FieldHaver, FieldResolver}, + field_walker::{Container, EltId, FieldResolver}, identifier::LocIdent, server::Server, }; @@ -50,13 +50,13 @@ fn nickel_string(s: String) -> MarkedString { } fn values_and_metadata_from_field( - parents: Vec, + parents: Vec, ident: Ident, ) -> (Vec, Vec) { let mut values = Vec::new(); let mut metadata = Vec::new(); for parent in parents { - if let FieldHaver::RecordTerm(r) = parent { + if let Container::RecordTerm(r) = parent { if let Some(field) = r.fields.get(&ident) { values.extend(field.value.iter().cloned()); metadata.push(field.metadata.clone()); @@ -79,7 +79,7 @@ fn ident_hover(ident: LocIdent, server: &Server) -> Option { if let Some(def) = server.analysis.get_def(&ident) { let resolver = FieldResolver::new(server); if let Some(((last, path), val)) = def.path().split_last().zip(def.value()) { - let parents = resolver.resolve_term_path(val, path.iter().copied()); + let parents = resolver.resolve_term_path(val, path.iter().copied().map(EltId::Ident)); let (values, metadata) = values_and_metadata_from_field(parents, *last); ret.values = values; ret.metadata = metadata; diff --git a/lsp/nls/tests/inputs/completion-array.ncl b/lsp/nls/tests/inputs/completion-array.ncl new file mode 100644 index 0000000000..8950fc5039 --- /dev/null +++ b/lsp/nls/tests/inputs/completion-array.ncl @@ -0,0 +1,14 @@ +### /file.ncl +{ + a = [ { f } ] | Array { foo }, + b = { c = [ { b } ] }, +} | { b | { c | Array { bar } }, .. } +### [[request]] +### type = "Completion" +### textDocument.uri = "file:///file.ncl" +### position = { line = 1, character = 11 } +### +### [[request]] +### type = "Completion" +### textDocument.uri = "file:///file.ncl" +### position = { line = 2, character = 17 } diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-array.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-array.ncl.snap new file mode 100644 index 0000000000..a79a06302e --- /dev/null +++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__completion-array.ncl.snap @@ -0,0 +1,7 @@ +--- +source: lsp/nls/tests/main.rs +expression: output +--- +[a, b, foo, std] +[a, b, bar, c, std] +