diff --git a/lsp/nls/src/requests/hover.rs b/lsp/nls/src/requests/hover.rs index 653406cceb..8106ab69f4 100644 --- a/lsp/nls/src/requests/hover.rs +++ b/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 { 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); - - 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()); diff --git a/lsp/nls/src/utils.rs b/lsp/nls/src/utils.rs index 1b34a84217..6b482f02eb 100644 --- a/lsp/nls/src/utils.rs +++ b/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(xs: &mut Vec) { + 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())); +} diff --git a/lsp/nls/tests/inputs/hover-basic.ncl b/lsp/nls/tests/inputs/hover-basic.ncl index 7957e344e8..5bd12c98b5 100644 --- a/lsp/nls/tests/inputs/hover-basic.ncl +++ b/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 } diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-basic.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-basic.ncl.snap index 269a6c01fa..b3331331b8 100644 --- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-basic.ncl.snap +++ b/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] - diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-cousin.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-cousin.ncl.snap index c5ca3dede0..bcbb920f6a 100644 --- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-cousin.ncl.snap +++ b/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] - diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-pattern.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-pattern.ncl.snap index b836268ed5..8999b109a3 100644 --- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-pattern.ncl.snap +++ b/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] -