More aggressive type/contract deduplication on hover

lsp/nls/src/requests/hover.rs

@@ -12,7 +12,7 @@ use serde_json::Value;
 use crate::{
     cache::CacheExt,
     diagnostic::LocationCompat,
-    field_walker::{FieldResolver, Record},
+    field_walker::{Def, FieldResolver, Record},
     identifier::LocIdent,
     server::Server,
     world::World,
@@ -94,6 +94,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 +160,47 @@ 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.)
+        let mut annotations: Vec<_> = hover
             .metadata
             .iter()
-            .filter_map(|m| Some(m.annotation.typ.as_ref()?.typ.to_string()))
+            .flat_map(|m| {
+                let maybe_typ = m.annotation.typ.as_ref().map(|typ| typ.typ.to_string());
+                let contracts = m
+                    .annotation
+                    .contracts
+                    .iter()
+                    .map(|c| c.label.typ.to_string());
+                maybe_typ.into_iter().chain(contracts)
+            })
+            .chain(
+                hover
+                    .values
+                    .iter()
+                    .flat_map(annotated_contracts)
+                    .map(|contract| contract.label.typ.to_string()),
+            )
             .collect();
+        annotations.sort();
+        annotations.dedup();
 
         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);
-
-            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();
+        // 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);
+        }
 
-        contracts.sort();
-        contracts.dedup();
+        // Only report a Dyn type if there's no more useful information.
+        if ty != "Dyn" || annotations.is_empty() {
+            contents.push(nickel_string(ty));
+        }
 
-        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/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
+Number
+```, innermost]
+<1:2-1:5>[```nickel
 Dyn
-```, ```nickel
+```, 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]
-