Improve hover output for let patterns (#1696)

* Improve hover output for let patterns

* Be a little less eager to combine

* Less clones

core/src/typecheck/mod.rs

@@ -1402,6 +1402,11 @@ fn walk<V: TypecheckVisitor>(
             }
 
             let pattern_ty = destructuring::build_pattern_type_walk_mode(state, &ctxt, pat)?;
+            for item in pattern_ty.iter() {
+                if let GenericUnifRecordRowsIteratorItem::Row(row) = item {
+                    visitor.visit_ident(&row.id, row.typ.clone());
+                }
+            }
             destructuring::inject_pattern_variables(state, &mut ctxt.type_env, pat, pattern_ty);
 
             walk(state, ctxt, visitor, rt)
@@ -1773,6 +1778,12 @@ fn check<V: TypecheckVisitor>(
                 ctxt.type_env.insert(x.ident(), ty_let);
             }
 
+            for item in pattern_rows_type.iter() {
+                if let GenericUnifRecordRowsIteratorItem::Row(row) = item {
+                    visitor.visit_ident(&row.id, row.typ.clone());
+                }
+            }
+
             destructuring::inject_pattern_variables(
                 state,
                 &mut ctxt.type_env,

lsp/nls/src/requests/hover.rs

@@ -1,6 +1,7 @@
 use lsp_server::{RequestId, Response, ResponseError};
 use lsp_types::{Hover, HoverContents, HoverParams, LanguageString, MarkedString, Range};
 use nickel_lang_core::{
+    combine::Combine,
     identifier::Ident,
     position::RawSpan,
     term::{record::FieldMetadata, LabeledType, RichTerm, Term, UnaryOp},
@@ -16,14 +17,24 @@ use crate::{
     server::Server,
 };
 
-#[derive(Debug)]
+#[derive(Debug, Default)]
 struct HoverData {
     values: Vec<RichTerm>,
     metadata: Vec<FieldMetadata>,
-    span: RawSpan,
+    span: Option<RawSpan>,
     ty: Option<Type>,
 }
 
+impl Combine for HoverData {
+    fn combine(mut left: Self, mut right: Self) -> Self {
+        left.values.append(&mut right.values);
+        left.metadata.append(&mut right.metadata);
+        left.ty = left.ty.or(right.ty);
+        left.span = left.span.or(right.span);
+        left
+    }
+}
+
 fn annotated_contracts(rt: &RichTerm) -> &[LabeledType] {
     match rt.as_ref() {
         Term::Annotated(annot, _) => &annot.contracts,
@@ -61,7 +72,7 @@ fn ident_hover(ident: LocIdent, server: &Server) -> Option<HoverData> {
     let mut ret = HoverData {
         values: Vec::new(),
         metadata: Vec::new(),
-        span,
+        span: Some(span),
         ty,
     };
 
@@ -92,7 +103,7 @@ fn ident_hover(ident: LocIdent, server: &Server) -> Option<HoverData> {
 
 fn term_hover(rt: &RichTerm, server: &Server) -> Option<HoverData> {
     let ty = server.analysis.get_type(rt).cloned();
-    let span = rt.pos.into_opt()?;
+    let span = rt.pos.into_opt();
 
     match rt.as_ref() {
         Term::Op1(UnaryOp::StaticAccess(id), parent) => {
@@ -124,15 +135,21 @@ pub fn handle(
         .cache
         .position(&params.text_document_position_params)?;
 
-    let hover_data = server
+    let ident_hover_data = server
         .lookup_ident_by_position(pos)?
         .and_then(|ident| ident_hover(ident, server));
 
-    let hover_data = match hover_data {
-        Some(h) => Some(h),
-        None => server
-            .lookup_term_by_position(pos)?
-            .and_then(|rt| term_hover(rt, server)),
+    let term = server.lookup_term_by_position(pos)?;
+    let term_hover_data = term.and_then(|rt| term_hover(rt, server));
+
+    // We combine the hover information from the term (which can have better type information)
+    // and the ident (which can have better metadata), but only when hovering over a `Var`.
+    // In general, the term and the ident can have different meanings (like when hovering over
+    // the `x` in `let x = ... in y`) and so it would be confusing to combine them.
+    let hover_data = if matches!(term.map(AsRef::as_ref), Some(Term::Var(_))) {
+        Combine::combine(ident_hover_data, term_hover_data)
+    } else {
+        ident_hover_data.or(term_hover_data)
     };
 
     if let Some(hover) = hover_data {
@@ -169,7 +186,9 @@ pub fn handle(
             req_id,
             Hover {
                 contents: HoverContents::Array(contents),
-                range: Some(Range::from_span(&hover.span, server.cache.files())),
+                range: hover
+                    .span
+                    .map(|s| Range::from_span(&s, server.cache.files())),
             },
         ));
     } else {

lsp/nls/tests/inputs/hover-pattern-typed.ncl

@@ -0,0 +1,16 @@
+### /main.ncl
+(let { x } = { x = 1 } in x) : _
+### [[request]]
+### type = "Hover"
+### textDocument.uri = "file:///main.ncl"
+### position = { line = 0, character = 7 }
+###
+### [[request]]
+### type = "Hover"
+### textDocument.uri = "file:///main.ncl"
+### position = { line = 0, character = 15 }
+###
+### [[request]]
+### type = "Hover"
+### textDocument.uri = "file:///main.ncl"
+### position = { line = 0, character = 26 }

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

@@ -0,0 +1,14 @@
+---
+source: lsp/nls/tests/main.rs
+expression: output
+---
+<0:7-0:8>[```nickel
+Number
+```]
+<0:15-0:16>[```nickel
+Number
+```]
+<0:26-0:27>[```nickel
+Number
+```]
+

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

@@ -11,9 +11,9 @@ Dyn
 <8:2-8:11>[```nickel
 Dyn
 ```, middle]
-<9:2-9:7>[```nickel
-Dyn
-```, middle]
+<9:2-9:7>[middle, ```nickel
+{ bar : Dyn }
+```]
 <9:2-9:11>[```nickel
 Dyn
 ```, ```nickel