Take the ancestor path into account when env-completing from uncles. (#…

…1661)

* Track paths in the parent chain

* Don't offer the completion at the cursor

* Fix rebase issues

* Improve parent iter

* Fix docs

lsp/nls/src/analysis.rs

@@ -2,7 +2,8 @@ use std::collections::HashMap;
 
 use codespan::FileId;
 use nickel_lang_core::{
-    term::{RichTerm, Traverse, TraverseControl},
+    identifier::Ident,
+    term::{RichTerm, Term, Traverse, TraverseControl},
     typ::{Type, TypeF},
     typecheck::{reporting::NameReg, TypeTables, TypecheckVisitor, UnifType},
 };
@@ -15,51 +16,132 @@ use crate::{
     usage::{Environment, UsageLookup},
 };
 
-#[derive(Default, Debug)]
+#[derive(Clone, Debug)]
+pub struct Parent {
+    term: RichTerm,
+    child_name: Option<Ident>,
+}
+
+impl From<RichTerm> for Parent {
+    fn from(term: RichTerm) -> Self {
+        Parent {
+            term,
+            child_name: None,
+        }
+    }
+}
+
+#[derive(Clone, Debug, Default)]
 pub struct ParentLookup {
-    table: HashMap<RichTermPtr, RichTerm>,
+    table: HashMap<RichTermPtr, Parent>,
 }
 
 impl ParentLookup {
     pub fn new(rt: &RichTerm) -> Self {
         let mut table = HashMap::new();
-        let mut traverse_merge =
-            |rt: &RichTerm, parent: &Option<RichTerm>| -> TraverseControl<Option<RichTerm>, ()> {
-                if let Some(parent) = parent {
-                    table.insert(RichTermPtr(rt.clone()), parent.clone());
+
+        fn traversal(
+            rt: &RichTerm,
+            parent: &Option<Parent>,
+            acc: &mut HashMap<RichTermPtr, Parent>,
+        ) -> TraverseControl<Option<Parent>, ()> {
+            if let Some(parent) = parent {
+                acc.insert(RichTermPtr(rt.clone()), parent.clone());
+            }
+            match rt.as_ref() {
+                Term::Record(data) | Term::RecRecord(data, _, _) => {
+                    for (name, field) in &data.fields {
+                        if let Some(child) = &field.value {
+                            let parent = Parent {
+                                term: rt.clone(),
+                                child_name: Some(name.ident()),
+                            };
+                            child.traverse_ref(
+                                &mut |rt, parent| traversal(rt, parent, acc),
+                                &Some(parent),
+                            );
+                        }
+                    }
+                    TraverseControl::SkipBranch
                 }
-                TraverseControl::ContinueWithScope(Some(rt.clone()))
-            };
+                _ => TraverseControl::ContinueWithScope(Some(rt.clone().into())),
+            }
+        }
 
-        rt.traverse_ref(&mut traverse_merge, &None);
+        rt.traverse_ref(&mut |rt, parent| traversal(rt, parent, &mut table), &None);
 
         ParentLookup { table }
     }
 
-    pub fn parent(&self, rt: &RichTerm) -> Option<&RichTerm> {
+    pub fn parent(&self, rt: &RichTerm) -> Option<&Parent> {
         self.table.get(&RichTermPtr(rt.clone()))
     }
 
-    pub fn parent_chain<'a>(&'a self, rt: &'a RichTerm) -> ParentChainIter<'_> {
+    pub fn parent_chain(&self, rt: &RichTerm) -> ParentChainIter<'_> {
+        let next = self.parent(rt).cloned();
         ParentChainIter {
             table: self,
-            next: Some(rt),
+            path: Some(Vec::new()),
+            next,
         }
     }
 }
 
+/// Essentially an iterator over pairs of `(ancestor, reversed_path_to_the_original)`.
+///
+/// For example, if we are iterating over the AST of `foo.bar.baz`, the iterator
+/// should return
+/// - ancestor `foo.bar`, path \[`baz`\]; and then
+/// - ancestor `foo`, path [`baz`, `bar`].
+///
+/// If, during our iteration, we encounter an ancestor that isn't a record then the
+/// path will be none from then on. For example, if we traverse `(some_fn foo.bar.baz).quux`
+/// starting from the `baz` AST node then the first couple of terms will have paths like
+/// the previous example, but after that we'll get
+/// - ancestor `(some_fn foo.bar.baz)`, path `None`; and then
+/// - ancestor `(some_fn foo.bar.baz).quux`, path `None`.
+///
+/// This is a "streaming iterator" in the sense that the returned data borrows from
+/// our state. Since streaming iterators are not (yet) in rust's stdlib, we don't
+/// implement any traits here, but just do it by hand.
+///
+/// For borrowck reasons, the iteration is done in two parts: `next` advances the iterator
+/// and returns just the term part. `path` retrieves the path corresponding to the previous
+/// `next` call.
 pub struct ParentChainIter<'a> {
     table: &'a ParentLookup,
-    next: Option<&'a RichTerm>,
+    path: Option<Vec<Ident>>,
+    next: Option<Parent>,
 }
 
-impl<'a> Iterator for ParentChainIter<'a> {
-    type Item = &'a RichTerm;
+impl<'a> ParentChainIter<'a> {
+    pub fn next(&mut self) -> Option<RichTerm> {
+        if let Some(next) = self.next.take() {
+            if let Some((ident, path)) = next.child_name.zip(self.path.as_mut()) {
+                path.push(ident);
+            }
+            if !matches!(
+                next.term.as_ref(),
+                Term::Record(_) | Term::RecRecord(..) | Term::Annotated(..) | Term::Op2(..)
+            ) {
+                self.path = None;
+            }
+            self.next = self.table.parent(&next.term).cloned();
+
+            Some(next.term)
+        } else {
+            None
+        }
+    }
+
+    pub fn path(&self) -> Option<&[Ident]> {
+        self.path.as_deref()
+    }
 
-    fn next(&mut self) -> Option<Self::Item> {
-        if let Some(next) = self.next {
-            self.next = self.table.parent(next);
-            Some(next)
+    /// Peek at the grandparent.
+    pub fn peek_gp(&self) -> Option<&RichTerm> {
+        if let Some(Parent { term, .. }) = &self.next {
+            self.table.parent(term).map(|gp| &gp.term)
         } else {
             None
         }

lsp/nls/src/field_walker.rs

@@ -1,4 +1,4 @@
-use lsp_types::{CompletionItem, CompletionItemKind, Documentation, MarkupContent, MarkupKind};
+use lsp_types::{CompletionItemKind, Documentation, MarkupContent, MarkupKind};
 use nickel_lang_core::{
     identifier::Ident,
     pretty::ident_quoted,
@@ -9,7 +9,7 @@ use nickel_lang_core::{
     typ::{RecordRows, RecordRowsIteratorItem, Type, TypeF},
 };
 
-use crate::{identifier::LocIdent, server::Server};
+use crate::{identifier::LocIdent, requests::completion::CompletionItem, server::Server};
 
 /// A `FieldHaver` is something that... has fields.
 ///
@@ -56,7 +56,7 @@ impl FieldHaver {
                     detail: metadata_detail(&val.metadata),
                     kind: Some(CompletionItemKind::Property),
                     documentation: metadata_doc(&val.metadata),
-                    ..Default::default()
+                    ident: Some((*id).into()),
                 });
                 Box::new(iter)
             }
@@ -168,14 +168,17 @@ impl<'a> FieldResolver<'a> {
     /// Resolve a record path iteratively.
     ///
     /// Returns all the field-having objects that the final path element refers to.
-    pub fn resolve_term_path(&self, rt: &RichTerm, mut path: &[Ident]) -> Vec<FieldHaver> {
+    pub fn resolve_term_path(
+        &self,
+        rt: &RichTerm,
+        path: impl Iterator<Item = Ident>,
+    ) -> Vec<FieldHaver> {
         let mut fields = self.resolve_term(rt);
 
-        while let Some((id, tail)) = path.split_first() {
-            path = tail;
+        for id in path {
             let values = fields
                 .iter()
-                .filter_map(|haver| haver.get(*id))
+                .filter_map(|haver| haver.get(id))
                 .collect::<Vec<_>>();
             fields.clear();
 
@@ -201,7 +204,7 @@ impl<'a> FieldResolver<'a> {
         let mut fields = Vec::new();
 
         if let Some(val) = &def.value {
-            fields.extend_from_slice(&self.resolve_term_path(val, &def.path))
+            fields.extend_from_slice(&self.resolve_term_path(val, def.path.iter().copied()))
         }
         if let Some(meta) = &def.metadata {
             fields.extend(self.resolve_annot(&meta.annotation));
@@ -252,7 +255,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, &[id.ident()])
+                self.resolve_term_path(term, std::iter::once(id.ident()))
             }
             Term::Annotated(annot, term) => {
                 let defs = self.resolve_annot(annot);

lsp/nls/src/requests/completion.rs

@@ -1,6 +1,6 @@
 use log::debug;
 use lsp_server::{RequestId, Response, ResponseError};
-use lsp_types::{CompletionItem, CompletionItemKind, CompletionParams};
+use lsp_types::{CompletionItemKind, CompletionParams};
 use nickel_lang_core::{
     cache::{self, InputFormat},
     identifier::Ident,
@@ -16,19 +16,30 @@ use std::path::PathBuf;
 use crate::{
     cache::CacheExt,
     field_walker::{FieldHaver, FieldResolver},
+    identifier::LocIdent,
     incomplete,
     server::Server,
     usage::Environment,
 };
 
-fn remove_duplicates(items: &Vec<CompletionItem>) -> Vec<CompletionItem> {
-    let mut seen: Vec<CompletionItem> = Vec::new();
+fn remove_duplicates_and_myself(
+    items: &[CompletionItem],
+    cursor: RawPos,
+) -> Vec<lsp_types::CompletionItem> {
+    let mut seen_labels = HashSet::new();
+    let mut ret = Vec::new();
     for item in items {
-        if !seen.iter().any(|seen_item| seen_item.label == item.label) {
-            seen.push(item.clone())
+        if let Some(ident) = item.ident {
+            if ident.pos.contains(cursor) {
+                continue;
+            }
+        }
+
+        if seen_labels.insert(&item.label) {
+            ret.push(item.clone().into());
         }
     }
-    seen
+    ret
 }
 
 fn extract_static_path(mut rt: RichTerm) -> (RichTerm, Vec<Ident>) {
@@ -73,12 +84,33 @@ fn sanitize_record_path_for_completion(
     }
 }
 
+#[derive(Default, Debug, PartialEq, Eq, Clone)]
+pub struct CompletionItem {
+    pub label: String,
+    pub detail: Option<String>,
+    pub kind: Option<CompletionItemKind>,
+    pub documentation: Option<lsp_types::Documentation>,
+    pub ident: Option<LocIdent>,
+}
+
+impl From<CompletionItem> for lsp_types::CompletionItem {
+    fn from(my: CompletionItem) -> Self {
+        Self {
+            label: my.label,
+            detail: my.detail,
+            kind: my.kind,
+            documentation: my.documentation,
+            ..Default::default()
+        }
+    }
+}
+
 fn record_path_completion(term: RichTerm, server: &Server) -> Vec<CompletionItem> {
     log::info!("term based completion path: {term:?}");
 
     let (start_term, path) = extract_static_path(term);
 
-    let defs = FieldResolver::new(server).resolve_term_path(&start_term, &path);
+    let defs = FieldResolver::new(server).resolve_term_path(&start_term, path.iter().copied());
     defs.iter().flat_map(FieldHaver::completion_items).collect()
 }
 
@@ -111,30 +143,37 @@ fn env_completion(rt: &RichTerm, server: &Server) -> Vec<CompletionItem> {
 
     // Iterate through all ancestors of our term, looking for identifiers that are "in scope"
     // because they're in an uncle/aunt/cousin that gets merged into our direct ancestors.
-    if let Some(parents) = server.analysis.get_parent_chain(rt) {
+    if let Some(mut parents) = server.analysis.get_parent_chain(rt) {
         // We're only interested in adding identifiers from terms that are records or
         // merges/annotations of records. But actually we can skip the records, because any
         // records that are our direct ancestor have already contributed to `env`.
-        let env_term = |rt: &RichTerm| {
+        let is_env_term = |rt: &RichTerm| {
+            matches!(
+                rt.as_ref(),
+                Term::Op2(BinaryOp::Merge(_), _, _) | Term::Annotated(_, _) | Term::RecRecord(..)
+            )
+        };
+
+        let is_merge_term = |rt: &RichTerm| {
             matches!(
                 rt.as_ref(),
                 Term::Op2(BinaryOp::Merge(_), _, _) | Term::Annotated(_, _)
             )
         };
 
-        let mut parents = parents.peekable();
-        while let Some(rt) = parents.next() {
+        while let Some(p) = parents.next() {
             // If a parent and a grandparent were both merges, we can skip the parent
             // because the grandparent will have a superset of its fields. This prevents
             // quadratic behavior on long chains of merges.
-            if let Some(gp) = parents.peek() {
-                if env_term(gp) {
+            if let Some(gp) = parents.peek_gp() {
+                if is_merge_term(gp) {
                     continue;
                 }
             }
 
-            if env_term(rt) {
-                let records = resolver.resolve_term(rt);
+            if is_env_term(&p) {
+                let path = parents.path().unwrap_or_default();
+                let records = resolver.resolve_term_path(&p, path.iter().rev().copied());
                 items.extend(records.iter().flat_map(FieldHaver::completion_items));
             }
         }
@@ -189,7 +228,7 @@ pub fn handle_completion(
         (None, None) => Vec::new(),
     };
 
-    let completions = remove_duplicates(&completions);
+    let completions = remove_duplicates_and_myself(&completions, pos);
 
     server.reply(Response::new_ok(id.clone(), completions));
     Ok(())
@@ -199,7 +238,7 @@ fn handle_import_completion(
     import: &OsString,
     params: &CompletionParams,
     server: &mut Server,
-) -> io::Result<Vec<CompletionItem>> {
+) -> io::Result<Vec<lsp_types::CompletionItem>> {
     debug!("handle import completion");
 
     let current_file = params
@@ -254,7 +293,7 @@ fn handle_import_completion(
             } else {
                 CompletionItemKind::Folder
             };
-            CompletionItem {
+            lsp_types::CompletionItem {
                 label: entry
                     .path
                     .file_name()

lsp/nls/src/requests/hover.rs

@@ -68,7 +68,7 @@ fn ident_hover(ident: LocIdent, server: &Server) -> Option<HoverData> {
     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.as_ref()) {
-            let parents = resolver.resolve_term_path(val, path);
+            let parents = resolver.resolve_term_path(val, path.iter().copied());
             let (values, metadata) = values_and_metadata_from_field(parents, *last);
             ret.values = values;
             ret.metadata = metadata;