More aggressive type/contract deduplication on hover (#1984)

* More aggressive deduplication on hover.

Deduplicate not just the type annotations, but also the contract
annotations. Also, try to report Dyn less often.

* Add hover metadata for the field of a record.

* Review comments

* Add a comment

lsp/nls/src/requests/hover.rs

@@ -12,9 +12,10 @@ use serde_json::Value;
 use crate::{
     cache::CacheExt,
     diagnostic::LocationCompat,
-    field_walker::{FieldResolver, Record},
+    field_walker::{Def, FieldResolver, Record},
     identifier::LocIdent,
     server::Server,
+    utils::dedup,
     world::World,
 };
 
@@ -94,6 +95,10 @@ fn ident_hover(ident: LocIdent, world: &World) -> Option<HoverData> {
                     ret.metadata.push(cousin.metadata);
                 }
             }
+
+            if let Def::Field { metadata, .. } = def {
+                ret.metadata.push(metadata.clone());
+            }
         }
     }
 
@@ -156,57 +161,40 @@ pub fn handle(
     if let Some(hover) = hover_data {
         let mut contents = Vec::new();
 
-        // If we can't determine a static type through the typechecker because we are outside of a
-        // statically typed block, but the term points to a definition with a type annotation, we
-        // use this annotation insted.
-        let mut type_annots: Vec<_> = hover
+        // Collect all the type and contract annotations we can find. We don't distinguish between them
+        // (and we deduplicate annotations if they're present as both types and contracts). However, we
+        // do give some special attention to the inferred static type if there is one: we list it first.
+        let mut annotations: Vec<_> = hover
             .metadata
             .iter()
-            .filter_map(|m| Some(m.annotation.typ.as_ref()?.typ.to_string()))
+            .flat_map(|m| m.annotation.iter().map(|typ| typ.typ.to_string()))
+            .chain(
+                hover
+                    .values
+                    .iter()
+                    .flat_map(annotated_contracts)
+                    .map(|contract| contract.label.typ.to_string()),
+            )
             .collect();
+        dedup(&mut annotations);
 
         let ty = hover
             .ty
             .as_ref()
             .map(Type::to_string)
-            // Unclear whether it's useful to report `Dyn` all the time when there's no type found,
-            // but it matches the old behavior.
             .unwrap_or_else(|| "Dyn".to_owned());
 
-        // If the type is `Dyn`, and we can find a type annotation somewhere in the metadata, we
-        // use the latter instead, which will be more precise.
-        let ty = if ty == "Dyn" {
-            // Ordering isn't meaningful here: metadata are aggregated from merged values (and
-            // merge is commutative). This list will also be sorted for deduplication later anyway.
-            // So we just pop the last one.
-            type_annots.pop().unwrap_or(ty)
-        } else {
-            // If the type is both statically known and present as an annotation, we don't want to
-            // report a second time with the other contracts, so we remove it from the list.
-            //
-            // Note that there might be duplicates, and we need to remove them all, hence the
-            // `retain` (instead of a potentially more performant `iter().position(..)` followed by
-            // `swap_remove`).
-            type_annots.retain(|annot| annot != &ty);
+        // There's no point in repeating the static type in the annotations.
+        if let Some(idx) = annotations.iter().position(|a| a == &ty) {
+            annotations.remove(idx);
+        }
 
-            ty
-        };
+        // Only report a Dyn type if there's no more useful information.
+        if ty != "Dyn" || annotations.is_empty() {
+            contents.push(nickel_string(ty));
+        }
 
-        contents.push(nickel_string(ty));
-
-        let mut contracts: Vec<_> = hover
-            .metadata
-            .iter()
-            .flat_map(|m| &m.annotation.contracts)
-            .chain(hover.values.iter().flat_map(annotated_contracts))
-            .map(|contract| contract.label.typ.to_string())
-            .chain(type_annots)
-            .collect();
-
-        contracts.sort();
-        contracts.dedup();
-
-        contents.extend(contracts.into_iter().map(nickel_string));
+        contents.extend(annotations.into_iter().map(nickel_string));
 
         // Not sure how to do documentation merging yet, so pick the first non-empty one.
         let doc = hover.metadata.iter().find_map(|m| m.doc.as_ref());

lsp/nls/src/utils.rs

@@ -1,3 +1,5 @@
+use std::collections::HashSet;
+
 use nickel_lang_core::{
     cache::Cache,
     environment::Environment,
@@ -41,3 +43,11 @@ pub(crate) fn initialize_stdlib(
     }
     initial_env
 }
+
+/// De-duplicate a vec without changing the order. The first instance of each unique
+/// element will be kept.
+pub fn dedup<T: std::hash::Hash + Eq + Clone>(xs: &mut Vec<T>) {
+    let mut seen = HashSet::new();
+    // Clone is needed because the signature of retain doesn't let us keep the reference.
+    xs.retain(|x| seen.insert(x.clone()));
+}

lsp/nls/tests/inputs/hover-basic.ncl

@@ -18,3 +18,11 @@ record.foo.bar
 ### type = "Hover"
 ### textDocument.uri = "file:///main.ncl"
 ### position = { line = 6, character = 12 }
+### [[request]]
+### type = "Hover"
+### textDocument.uri = "file:///main.ncl"
+### position = { line = 1, character = 3 }
+### [[request]]
+### type = "Hover"
+### textDocument.uri = "file:///main.ncl"
+### position = { line = 2, character = 5 }

lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-basic.ncl.snap

@@ -9,8 +9,11 @@ Dyn
 Dyn
 ```, middle]
 <6:0-6:14>[```nickel
-Dyn
-```, ```nickel
 Number
 ```, innermost]
-
+<1:2-1:5>[```nickel
+Dyn
+```, middle]
+<2:4-2:7>[```nickel
+Number
+```, innermost]

lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-cousin.ncl.snap

@@ -7,18 +7,13 @@ Dyn
 ```, outer]
 <1:10-1:13>[```nickel
 Number
-```, ```nickel
-Number
 ```, inner]
 <2:9-2:12>[```nickel
 Dyn
 ```, outer]
 <2:9-2:16>[```nickel
-Dyn
-```, ```nickel
 Number
 ```, inner]
 <3:6-3:10>[```nickel
 Dyn
 ```, longer path]
-

lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-pattern.ncl.snap

@@ -15,13 +15,8 @@ Dyn
 { bar : Dyn }
 ```]
 <9:2-9:11>[```nickel
-Dyn
-```, ```nickel
 Number
 ```, innermost]
 <10:2-10:11>[```nickel
-Dyn
-```, ```nickel
 Number
 ```, innermost]
-