Add "goto definition" support for the import term (#1756)

* Make goto definition work for the import term

* Add a test

lsp/nls/src/analysis.rs

@@ -2,6 +2,7 @@ use std::collections::HashMap;
 
 use codespan::FileId;
 use nickel_lang_core::{
+    position::RawSpan,
     term::{BinaryOp, RichTerm, Term, Traverse, TraverseControl},
     typ::{Type, TypeF},
     typecheck::{reporting::NameReg, TypeTables, TypecheckVisitor, UnifType},
@@ -272,15 +273,15 @@ impl AnalysisRegistry {
         self.analysis.get(&file)?.usage_lookup.def(ident)
     }
 
-    pub fn get_usages(&self, ident: &LocIdent) -> impl Iterator<Item = &LocIdent> {
+    pub fn get_usages(&self, span: &RawSpan) -> impl Iterator<Item = &LocIdent> {
         fn inner<'a>(
             slf: &'a AnalysisRegistry,
-            ident: &LocIdent,
+            span: &RawSpan,
         ) -> Option<impl Iterator<Item = &'a LocIdent>> {
-            let file = ident.pos.as_opt_ref()?.src_id;
-            Some(slf.analysis.get(&file)?.usage_lookup.usages(ident))
+            let file = span.src_id;
+            Some(slf.analysis.get(&file)?.usage_lookup.usages(span))
         }
-        inner(self, ident).into_iter().flatten()
+        inner(self, span).into_iter().flatten()
     }
 
     pub fn get_env(&self, rt: &RichTerm) -> Option<&crate::usage::Environment> {

lsp/nls/src/requests/goto.rs

@@ -1,6 +1,9 @@
 use lsp_server::{RequestId, Response, ResponseError};
 use lsp_types::{GotoDefinitionParams, GotoDefinitionResponse, Location, ReferenceParams};
-use nickel_lang_core::term::{record::FieldMetadata, RichTerm, Term, UnaryOp};
+use nickel_lang_core::{
+    position::RawSpan,
+    term::{record::FieldMetadata, RichTerm, Term, UnaryOp},
+};
 use serde_json::Value;
 
 use crate::{
@@ -11,24 +14,31 @@ use crate::{
     server::Server,
 };
 
-fn get_defs(term: &RichTerm, ident: Option<LocIdent>, server: &Server) -> Option<Vec<LocIdent>> {
+fn get_defs(term: &RichTerm, ident: Option<LocIdent>, server: &Server) -> Option<Vec<RawSpan>> {
     let resolver = FieldResolver::new(server);
     let ret = match (term.as_ref(), ident) {
         (Term::Var(id), _) => {
             let id = LocIdent::from(*id);
             let def = server.analysis.get_def(&id)?;
             let cousins = resolver.get_cousin_defs(def);
             if cousins.is_empty() {
-                vec![def.ident()]
+                vec![def.ident().pos.unwrap()]
             } else {
-                cousins.into_iter().map(|(loc, _)| loc).collect()
+                cousins
+                    .into_iter()
+                    .filter_map(|(loc, _)| loc.pos.into_opt())
+                    .collect()
             }
         }
         (Term::Op1(UnaryOp::StaticAccess(id), parent), _) => {
             let parents = resolver.resolve_term(parent);
             parents
                 .iter()
-                .filter_map(|parent| parent.get_definition_pos(id.ident()))
+                .filter_map(|parent| {
+                    parent
+                        .get_definition_pos(id.ident())
+                        .and_then(|def| def.pos.into_opt())
+                })
                 .collect()
         }
         (Term::LetPattern(_, pat, value, _), Some(hovered_id)) => {
@@ -43,18 +53,26 @@ fn get_defs(term: &RichTerm, ident: Option<LocIdent>, server: &Server) -> Option
             let parents = resolver.resolve_term_path(value, path.iter().copied().map(EltId::Ident));
             parents
                 .iter()
-                .filter_map(|parent| parent.get_definition_pos(last.ident()))
+                .filter_map(|parent| {
+                    parent
+                        .get_definition_pos(last.ident())
+                        .and_then(|def| def.pos.into_opt())
+                })
                 .collect()
         }
+        (Term::ResolvedImport(file), _) => {
+            let pos = server.cache.terms().get(file)?.term.pos;
+            vec![pos.into_opt()?]
+        }
         _ => {
             return None;
         }
     };
     Some(ret)
 }
 
-fn ids_to_locations(ids: impl IntoIterator<Item = LocIdent>, server: &Server) -> Vec<Location> {
-    let mut spans: Vec<_> = ids.into_iter().filter_map(|id| id.pos.into_opt()).collect();
+fn ids_to_locations(ids: impl IntoIterator<Item = RawSpan>, server: &Server) -> Vec<Location> {
+    let mut spans: Vec<_> = ids.into_iter().collect();
 
     // The sort order of our response is a little arbitrary. But we want to deduplicate, and we
     // don't want the response to be random.
@@ -113,20 +131,26 @@ pub fn handle_references(
     // Maybe the position is pointing straight at the definition already.
     // In that case, def_locs won't have the definition yet; so add it.
     if let Some(id) = server.lookup_ident_by_position(pos)? {
-        def_locs.push(id);
-        if let Some(parent) = term {
-            // If `id` is a field name in a record, we can search through cousins
-            // to find more definitions.
-            if matches!(parent.as_ref(), Term::RecRecord(..) | Term::Record(_)) {
-                let def = Def::Field {
-                    ident: id,
-                    value: None,
-                    record: parent.clone(),
-                    metadata: FieldMetadata::default(),
-                };
-                let resolver = FieldResolver::new(server);
-                let cousins = resolver.get_cousin_defs(&def);
-                def_locs.extend(cousins.into_iter().map(|(loc, _)| loc))
+        if let Some(span) = id.pos.into_opt() {
+            def_locs.push(span);
+            if let Some(parent) = term {
+                // If `id` is a field name in a record, we can search through cousins
+                // to find more definitions.
+                if matches!(parent.as_ref(), Term::RecRecord(..) | Term::Record(_)) {
+                    let def = Def::Field {
+                        ident: id,
+                        value: None,
+                        record: parent.clone(),
+                        metadata: FieldMetadata::default(),
+                    };
+                    let resolver = FieldResolver::new(server);
+                    let cousins = resolver.get_cousin_defs(&def);
+                    def_locs.extend(
+                        cousins
+                            .into_iter()
+                            .filter_map(|(loc, _)| loc.pos.into_opt()),
+                    )
+                }
             }
         }
     }
@@ -137,7 +161,7 @@ pub fn handle_references(
     let mut usages: Vec<_> = def_locs
         .iter()
         .flat_map(|id| server.analysis.get_usages(id))
-        .cloned()
+        .filter_map(|id| id.pos.into_opt())
         .collect();
 
     if params.context.include_declaration {

lsp/nls/src/usage.rs

@@ -37,7 +37,8 @@ pub struct UsageLookup {
     // This could be made more general (we might want to map arbitrary positions to
     // environments) but its enough for us now.
     def_table: HashMap<RawSpan, Environment>,
-    usage_table: HashMap<LocIdent, Vec<LocIdent>>,
+    // Maps from spans of idents to the places where they are referenced.
+    usage_table: HashMap<RawSpan, Vec<LocIdent>>,
     // The list of all the symbols (and their locations) in the document.
     //
     // Currently, variables bound in `let` bindings and record fields count as symbols.
@@ -53,9 +54,9 @@ impl UsageLookup {
     }
 
     /// Return all the usages of `ident`.
-    pub fn usages(&self, ident: &LocIdent) -> impl Iterator<Item = &LocIdent> {
+    pub fn usages(&self, span: &RawSpan) -> impl Iterator<Item = &LocIdent> {
         self.usage_table
-            .get(ident)
+            .get(span)
             .map(|v| v.iter())
             .unwrap_or([].iter())
     }
@@ -185,7 +186,9 @@ impl UsageLookup {
                     Term::Var(id) => {
                         let id = LocIdent::from(*id);
                         if let Some(def) = env.get(&id.ident) {
-                            self.usage_table.entry(def.ident()).or_default().push(id);
+                            if let Some(span) = def.ident().pos.into_opt() {
+                                self.usage_table.entry(span).or_default().push(id);
+                            }
                         }
                         TraverseControl::Continue
                     }
@@ -234,7 +237,10 @@ pub(crate) mod tests {
         let x2 = locced(x, file, 17..18);
         let table = UsageLookup::new(&rt, &Environment::new());
 
-        assert_eq!(table.usages(&x0).cloned().collect::<Vec<_>>(), vec![x1, x2]);
+        assert_eq!(
+            table.usages(&x0.pos.unwrap()).cloned().collect::<Vec<_>>(),
+            vec![x1, x2]
+        );
         assert_eq!(table.def(&x1), table.def(&x2));
         assert_eq!(table.def(&x0), table.def(&x1));
 
@@ -254,9 +260,21 @@ pub(crate) mod tests {
         let baz1 = locced("baz", file, 56..59);
         let table = UsageLookup::new(&rt, &Environment::new());
 
-        assert_eq!(table.usages(&x0).cloned().collect::<Vec<_>>(), vec![x1]);
-        assert_eq!(table.usages(&a0).cloned().collect::<Vec<_>>(), vec![a1]);
-        assert_eq!(table.usages(&baz0).cloned().collect::<Vec<_>>(), vec![baz1]);
+        assert_eq!(
+            table.usages(&x0.pos.unwrap()).cloned().collect::<Vec<_>>(),
+            vec![x1]
+        );
+        assert_eq!(
+            table.usages(&a0.pos.unwrap()).cloned().collect::<Vec<_>>(),
+            vec![a1]
+        );
+        assert_eq!(
+            table
+                .usages(&baz0.pos.unwrap())
+                .cloned()
+                .collect::<Vec<_>>(),
+            vec![baz1]
+        );
 
         let x_def = table.def(&x1).unwrap();
         assert_eq!(x_def.ident(), x0);

lsp/nls/tests/inputs/goto-cross-file.ncl

@@ -8,6 +8,11 @@ in
 ### [[request]]
 ### type = "GotoDefinition"
 ### textDocument.uri = "file:///goto.ncl"
+### position = { line = 1, character = 16 }
+###
+### [[request]]
+### type = "GotoDefinition"
+### textDocument.uri = "file:///goto.ncl"
 ### position = { line = 3, character = 2 }
 ###
 ### [[request]]

lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__goto-cross-file.ncl.snap

@@ -2,6 +2,7 @@
 source: lsp/nls/tests/main.rs
 expression: output
 ---
+file:///dep.ncl:0:0-0:15
 file:///goto.ncl:1:2-1:8
 file:///dep.ncl:0:2-0:5