Repository: tweag/nickel (rust)

Issue:
Refine lsp document symbols
Currently, a document symbols request return a flat list of all symbols defined in records or let bindings. This is probably not the best behavior:

- we probably want to leave out symbols defined in let bindings
- symbols defined in record fields should be reported hierarchically

See #1623 for some more context.

Pull request:
Improve document symbols
This changes the "document symbols" request to something that I think is more useful: it lists only "publicly visible" symbols, by which I mean symbols that someone importing the file can access using field paths. In particular, local let bindings are no longer included, nor are fields in records that are only visible in local function bodies and let bindings.

The symbols are also presented hierarchically now.

Fixes #1627 

Patch:
diff --git a/examples/arrays/arrays.ncl b/examples/arrays/arrays.ncl
index 7d3e9e44..66978ba6 100644
--- a/examples/arrays/arrays.ncl
+++ b/examples/arrays/arrays.ncl
@@ -11,15 +11,16 @@ let my_array_lib = {
         let tail = std.array.drop_first arr in
         [f head] @ map f tail,
 
-  fold : forall a b. (a -> b -> b) -> Array a -> b -> b = fun f arr first =>
+  fold : forall a b. (a -> b -> b) -> b -> Array a -> b = fun f first arr =>
       if arr == [] then
         first
       else
         let head = std.array.first arr in
         let tail = std.array.drop_first arr in
-        f head (fold f tail first),
+        f head (fold f first tail),
 }
 in
 # Compute `7!`
-let l = my_array_lib.map (fun x => x + 1) [1, 2, 3, 4, 5, 6] in
-my_array_lib.fold (fun x acc => x * acc) l 1
+[1, 2, 3, 4, 5, 6]
+|> my_array_lib.map ((+) 1)
+|> my_array_lib.fold (*) 1
diff --git a/examples/config-gcc/config-gcc.ncl b/examples/config-gcc/config-gcc.ncl
index 745c1ed6..bc4a5ab8 100644
--- a/examples/config-gcc/config-gcc.ncl
+++ b/examples/config-gcc/config-gcc.ncl
@@ -6,25 +6,27 @@ let GccFlag =
   # We only allow the following flags
   let available = ["W", "c", "S", "e", "o"] in
   fun label value =>
-    if std.is_string value then
-      if std.string.length value > 0
-      && std.array.any (fun x => x == std.string.substring 0 1 value) available then
-        value
-      else
-        std.contract.blame_with_message "unknown flag %{value}" label
-    else if std.is_record value then
-      if std.record.has_field "flag" value && std.record.has_field "arg" value then
-        if std.array.any (fun x => x == value.flag) available then
-          #Normalize the tag to a string
-          value.flag ++ value.arg
+    std.typeof value
+    |> match {
+      'String =>
+        if std.string.length value > 0
+        && std.array.any ((==) (std.string.substring 0 1 value)) available then
+          value
         else
-          std.contract.blame_with_message "unknown flag %{value.flag}" label
-      else
-        std.contract.blame_with_message
-          "bad record structure: missing field `flag` or `arg`"
-          label
-    else
-      std.contract.blame_with_message "expected record or string" label
+          std.contract.blame_with_message "unknown flag %{value}" label,
+      'Record =>
+        if std.record.has_field "flag" value && std.record.has_field "arg" value then
+          if std.array.any ((==) value.flag) available then
+            #Normalize the tag to a string
+            value.flag ++ value.arg
+          else
+            std.contract.blame_with_message "unknown flag %{value.flag}" label
+        else
+          std.contract.blame_with_message
+            "bad record structure: missing field `flag` or `arg`"
+            label,
+      _ => std.contract.blame_with_message "expected record or string" label,
+    }
 in
 
 let Path =
@@ -87,7 +89,9 @@ let Contract = {
 }
 in
 
-{
-  flags = ["Wextra", { flag = "o", arg = "stuff.o" }],
-  optimization_level = 2,
-} | Contract
+(
+  {
+    flags = ["Wextra", { flag = "o", arg = "stuff.o" }],
+    optimization_level = 2,
+  } | Contract
+)
diff --git a/examples/foreach-pattern/foreach-pattern-on-import.ncl b/examples/foreach-pattern/foreach-pattern-on-import.ncl
index 328d6076..c9f81958 100644
--- a/examples/foreach-pattern/foreach-pattern-on-import.ncl
+++ b/examples/foreach-pattern/foreach-pattern-on-import.ncl
@@ -1,16 +1,13 @@
 # test = 'ignore'
-let users =
-  (import "data_users.yml").users
-  |> std.array.map
-    (
-      fun name =>
-        {
-          username = name,
-          email = "%{name}@nickel-lang.org"
-        }
-    )
-in
-
 {
-  posix_users = users
+  posix_users =
+    (import "data_users.yml").users
+    |> std.array.map
+      (
+        fun name =>
+          {
+            username = name,
+            email = "%{name}@nickel-lang.org"
+          }
+      )
 }
diff --git a/examples/foreach-pattern/foreach-pattern.ncl b/examples/foreach-pattern/foreach-pattern.ncl
index b3c28f2e..a2c873b3 100644
--- a/examples/foreach-pattern/foreach-pattern.ncl
+++ b/examples/foreach-pattern/foreach-pattern.ncl
@@ -1,16 +1,13 @@
 # test = 'pass'
-let users =
-  ["jane", "pete", "richie"]
-  |> std.array.map
-    (
-      fun name =>
-        {
-          username = name,
-          email = "%{name}@nickel-lang.org"
-        }
-    )
-in
-
 {
-  usernames = users
+  user =
+    ["jane", "pete", "richie"]
+    |> std.array.map
+      (
+        fun name =>
+          {
+            username = name,
+            email = "%{name}@nickel-lang.org",
+          }
+      )
 }
diff --git a/examples/merge-priorities/security.ncl b/examples/merge-priorities/security.ncl
index 04792378..abcf103b 100644
--- a/examples/merge-priorities/security.ncl
+++ b/examples/merge-priorities/security.ncl
@@ -3,7 +3,9 @@
   server.host.options | priority 10 = "TLS",
 
   # force make it impossible to override this value with false
-  firewall.enabled | force = true,
-  firewall.type | default = "iptables",
-  firewall.open_ports | priority 5 = [21, 80, 443],
+  firewall = {
+    enabled | force = true,
+    type | default = "iptables",
+    open_ports | priority 5 = [21, 80, 443],
+  },
 }
diff --git a/examples/merge-priorities/server.ncl b/examples/merge-priorities/server.ncl
index 8eeb7fd9..a640418c 100644
--- a/examples/merge-priorities/server.ncl
+++ b/examples/merge-priorities/server.ncl
@@ -1,6 +1,8 @@
 # test = 'ignore'
 {
-  server.host.ip | default = "182.168.1.1",
-  server.host.port | default = 80,
-  server.host.name | default = "hello-world.net",
+  server.host = {
+    ip | default = "182.168.1.1",
+    port | default = 80,
+    name | default = "hello-world.net",
+  },
 }
diff --git a/examples/merge/server.ncl b/examples/merge/server.ncl
index 1f2f875d..402adf06 100644
--- a/examples/merge/server.ncl
+++ b/examples/merge/server.ncl
@@ -1,6 +1,8 @@
 # test = 'ignore'
 {
-  server.host.ip = "182.168.1.1",
-  server.host.port = 80,
-  server.host.name = "hello-world.net",
+  server.host = {
+    ip = "182.168.1.1",
+    port = 80,
+    name = "hello-world.net",
+  },
 }
diff --git a/examples/record-contract/record-contract.ncl b/examples/record-contract/record-contract.ncl
index ca6f6d18..e71c2e31 100644
--- a/examples/record-contract/record-contract.ncl
+++ b/examples/record-contract/record-contract.ncl
@@ -39,25 +39,28 @@ let KubernetesConfig = {
 
   apiVersion | String,
 
-  metadata = {
-    name | String,
-    labels.app | String,
-  },
+  metadata
+    | {
+      name | String,
+      labels.app | String,
+    },
 
-  spec = {
-    replicas
-      | std.number.PosNat
-      | doc "The number of replicas"
-      | default
-      = 1,
+  spec
+    | {
+      replicas
+        | std.number.PosNat
+        | doc "The number of replicas"
+        | default
+        = 1,
 
-    selector.matchLabels.app | String,
+      selector.matchLabels.app | String,
 
-    template = {
-      metadata.labels.app | String,
-      spec.containers | Array Container,
+      template
+        | {
+          metadata.labels.app | String,
+          spec.containers | Array Container,
+        },
     },
-  },
 }
 in
 
diff --git a/lsp/lsp-harness/src/output.rs b/lsp/lsp-harness/src/output.rs
index e1502d3d..cc4416e3 100644
--- a/lsp/lsp-harness/src/output.rs
+++ b/lsp/lsp-harness/src/output.rs
@@ -183,11 +183,13 @@ impl LspDebug for lsp_types::DocumentSymbol {
         let name = &self.name;
         let kind = self.kind;
         let detail = self.detail.clone().unwrap_or_default();
+        let children = self.children.as_deref().unwrap_or(&[]);
         write!(
             w,
-            "{name} ({kind:?} / {detail:?})@{} in {}",
+            "{name} ({kind:?} / {detail:?})@{} in {}: {}",
             self.selection_range.debug_str(),
-            self.range.debug_str()
+            self.range.debug_str(),
+            Iter(children.iter()).debug_str(),
         )
     }
 }
diff --git a/lsp/nls/src/requests/symbols.rs b/lsp/nls/src/requests/symbols.rs
index d411e6e2..770e7f9f 100644
--- a/lsp/nls/src/requests/symbols.rs
+++ b/lsp/nls/src/requests/symbols.rs
@@ -1,10 +1,59 @@
 use lsp_server::{RequestId, Response, ResponseError};
 use lsp_types::{DocumentSymbol, DocumentSymbolParams, SymbolKind};
+use nickel_lang_core::term::RichTerm;
 use nickel_lang_core::typ::Type;
 
+use crate::analysis::CollectedTypes;
 use crate::cache::CacheExt as _;
+use crate::field_walker::{FieldResolver, Record};
 use crate::server::Server;
 use crate::term::RawSpanExt;
+use crate::world::World;
+
+// Returns a hierarchy of "publicly accessible" symbols in a term.
+//
+// Basically, if the term "evaluates" (in the sense of FieldResolver's heuristics) to a record,
+// all fields in that record count as publicly accessible symbols. Then we recurse into
+// each of those.
+fn symbols(
+    world: &World,
+    type_lookups: &CollectedTypes<Type>,
+    rt: &RichTerm,
+) -> Vec<DocumentSymbol> {
+    let resolver = FieldResolver::new(world);
+    let root_records = resolver.resolve_path(rt, [].into_iter());
+    root_records
+        .into_iter()
+        .filter_map(|rec| match rec {
+            Record::RecordTerm(data) => Some(data),
+            Record::RecordType(_) => None,
+        })
+        .flat_map(|rt| {
+            rt.fields.into_iter().filter_map(|(id, field)| {
+                let ty = type_lookups.idents.get(&id.into());
+                let (file_id, span) = id.pos.into_opt()?.to_range();
+                let range =
+                    crate::codespan_lsp::byte_span_to_range(world.cache.files(), file_id, span)
+                        .ok()?;
+
+                let children = field.value.map(|v| symbols(world, type_lookups, &v));
+
+                #[allow(deprecated)]
+                // because the `deprecated` field is... wait for it... deprecated.
+                Some(DocumentSymbol {
+                    name: id.ident().to_string(),
+                    detail: ty.map(Type::to_string),
+                    kind: SymbolKind::VARIABLE,
+                    tags: None,
+                    range,
+                    selection_range: range,
+                    children,
+                    deprecated: None,
+                })
+            })
+        })
+        .collect()
+}
 
 pub fn handle_document_symbols(
     params: DocumentSymbolParams,
@@ -17,32 +66,13 @@ pub fn handle_document_symbols(
         .file_id(&params.text_document.uri)?
         .ok_or_else(|| crate::error::Error::FileNotFound(params.text_document.uri.clone()))?;
 
-    let usage_lookups = &server.world.file_analysis(file_id)?.usage_lookup;
     let type_lookups = &server.world.file_analysis(file_id)?.type_lookup;
+    let term = server.world.cache.get_ref(file_id);
 
-    let mut symbols = usage_lookups
-        .symbols()
-        .filter_map(|ident| {
-            let (file_id, span) = ident.pos.into_opt()?.to_range();
-            let range =
-                crate::codespan_lsp::byte_span_to_range(server.world.cache.files(), file_id, span)
-                    .ok()?;
-            let ty = type_lookups.idents.get(&ident);
-
-            #[allow(deprecated)] // because the `deprecated` field is... wait for it... deprecated.
-            Some(DocumentSymbol {
-                name: ident.ident.to_string(),
-                detail: ty.map(Type::to_string),
-                kind: SymbolKind::VARIABLE,
-                tags: None,
-                range,
-                selection_range: range,
-                children: None,
-                deprecated: None,
-            })
-        })
-        .collect::<Vec<_>>();
-
+    let mut symbols = term
+        .map(|t| symbols(&server.world, type_lookups, t))
+        .unwrap_or_default();
+    // Sort so the response is deterministic.
     symbols.sort_by_key(|s| s.range.start);
 
     server.reply(Response::new_ok(id, symbols));
diff --git a/lsp/nls/src/usage.rs b/lsp/nls/src/usage.rs
index 6aef367a..68903fd2 100644
--- a/lsp/nls/src/usage.rs
+++ b/lsp/nls/src/usage.rs
@@ -86,11 +86,6 @@ impl UsageLookup {
             .and_then(|span| self.def_table.get(span))
     }
 
-    /// Return the list of symbols in the document.
-    pub fn symbols(&self) -> impl Iterator<Item = LocIdent> + '_ {
-        self.syms.keys().cloned()
-    }
-
     fn add_sym(&mut self, def: Def) {
         self.syms.insert(def.ident(), def);
     }


Test patch:
diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-basic.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-basic.ncl.snap
index 6bb0416a..b9d7dc8f 100644
--- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-basic.ncl.snap
+++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-basic.ncl.snap
@@ -2,5 +2,4 @@
 source: lsp/nls/tests/main.rs
 expression: output
 ---
-[foo (Variable / "Number")@0:4-0:7 in 0:4-0:7, func (Variable / "Dyn")@1:4-1:8 in 1:4-1:8, name (Variable / "String")@3:2-3:6 in 3:2-3:6, other_name (Variable / "Dyn")@4:2-4:12 in 4:2-4:12, inner_name (Variable / "Dyn")@5:4-5:14 in 5:4-5:14, inner_binding (Variable / "Number")@5:21-5:34 in 5:21-5:34, type_checked_block (Variable / "Dyn")@7:2-7:20 in 7:2-7:20, inner_name (Variable / "{ name : String }")@8:4-8:14 in 8:4-8:14, name (Variable / "String")@8:19-8:23 in 8:19-8:23]
-
+[name (Variable / "String")@3:2-3:6 in 3:2-3:6: [], other_name (Variable / "Dyn")@4:2-4:12 in 4:2-4:12: [inner_name (Variable / "Dyn")@5:4-5:14 in 5:4-5:14: []], type_checked_block (Variable / "Dyn")@7:2-7:20 in 7:2-7:20: [inner_name (Variable / "{ name : String }")@8:4-8:14 in 8:4-8:14: [name (Variable / "String")@8:19-8:23 in 8:19-8:23: []]]]