From 289641f5d28e7ea9bbd39153f48a93119a8c634e Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Thu, 11 Apr 2024 12:16:14 -0500 Subject: [PATCH 1/2] Extend the symbol range to include the rhs --- lsp/nls/src/requests/symbols.rs | 47 +++++++++++++++++-- lsp/nls/tests/inputs/symbols-basic.ncl | 2 + ...nls__tests__inputs__symbols-basic.ncl.snap | 2 +- ..._tests__inputs__symbols-recursion.ncl.snap | 2 +- 4 files changed, 46 insertions(+), 7 deletions(-) diff --git a/lsp/nls/src/requests/symbols.rs b/lsp/nls/src/requests/symbols.rs index 320e6d5673..a050141eee 100644 --- a/lsp/nls/src/requests/symbols.rs +++ b/lsp/nls/src/requests/symbols.rs @@ -36,10 +36,47 @@ fn symbols( .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 (file_id, id_span) = id.pos.into_opt()?.to_range(); + // We need to find the span of the name (that's id_span above), but also the + // span of the "whole value," whatever that means. In vscode, there's a little + // outline bar at the top that shows you which symbol you're currently in, and it + // works by checking whether the cursor is inside the "whole value" range. + // + // `last_pos` is going to be the end of this "value" span. We + // take it large enough to contain the field value (if there + // is one) and any other annotations that we can work out the + // positions of. + let mut last_pos = id_span.end; + + if let Some(val_span) = field.value.as_ref().and_then(|val| val.pos.into_opt()) { + last_pos = last_pos.max(val_span.end.to_usize()); + } + + if let Some(last_ty_pos) = field + .metadata + .annotation + .contracts + .iter() + .chain(field.metadata.annotation.typ.as_ref()) + .filter_map(|ty| ty.typ.pos.as_opt_ref()) + .map(|pos| pos.end.to_usize()) + .max() + { + last_pos = last_pos.max(last_ty_pos); + } + + let selection_range = crate::codespan_lsp::byte_span_to_range( + world.cache.files(), + file_id, + id_span.clone(), + ) + .ok()?; + let range = crate::codespan_lsp::byte_span_to_range( + world.cache.files(), + file_id, + id_span.start..last_pos, + ) + .ok()?; let children = max_depth .checked_sub(1) @@ -53,7 +90,7 @@ fn symbols( kind: SymbolKind::VARIABLE, tags: None, range, - selection_range: range, + selection_range, children, deprecated: None, }) diff --git a/lsp/nls/tests/inputs/symbols-basic.ncl b/lsp/nls/tests/inputs/symbols-basic.ncl index fba1a615b0..c40e49f6fb 100644 --- a/lsp/nls/tests/inputs/symbols-basic.ncl +++ b/lsp/nls/tests/inputs/symbols-basic.ncl @@ -9,6 +9,8 @@ let func = fun x => 1 in type_checked_block = { inner_name = { name = "value" }, } : _, + annotated | String, + docced | doc "The symbol range doesn't current include docs", } ### [[request]] ### type = "Symbols" 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 b9d7dc8fbd..d69f4ae4af 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,4 +2,4 @@ source: lsp/nls/tests/main.rs expression: output --- -[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: []]]] +[name (Variable / "String")@3:2-3:6 in 3:2-3:16: [], other_name (Variable / "Dyn")@4:2-4:12 in 4:2-6:3: [inner_name (Variable / "Dyn")@5:4-5:14 in 5:4-5:49: []], type_checked_block (Variable / "Dyn")@7:2-7:20 in 7:2-9:7: [inner_name (Variable / "{ name : String }")@8:4-8:14 in 8:4-8:35: [name (Variable / "String")@8:19-8:23 in 8:19-8:33: []]], annotated (Variable / "String")@10:2-10:11 in 10:2-10:20: [], docced (Variable / "Dyn")@11:2-11:8 in 11:2-11:8: []] diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-recursion.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-recursion.ncl.snap index b8263f827c..d9ab663023 100644 --- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-recursion.ncl.snap +++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__symbols-recursion.ncl.snap @@ -2,4 +2,4 @@ source: lsp/nls/tests/main.rs expression: output --- -[bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:19: []]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]] +[bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: [bar (Variable / "Dyn")@0:16-0:19 in 0:16-0:25: []]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]] From 76e91ea1ef502ddd81ff825a58a4ff3218c38772 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Thu, 11 Apr 2024 16:59:22 -0500 Subject: [PATCH 2/2] Review comments --- lsp/nls/src/requests/symbols.rs | 34 +++++++++----------------- lsp/nls/tests/inputs/symbols-basic.ncl | 2 +- 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/lsp/nls/src/requests/symbols.rs b/lsp/nls/src/requests/symbols.rs index a050141eee..145912b972 100644 --- a/lsp/nls/src/requests/symbols.rs +++ b/lsp/nls/src/requests/symbols.rs @@ -1,5 +1,6 @@ use lsp_server::{RequestId, Response, ResponseError}; use lsp_types::{DocumentSymbol, DocumentSymbolParams, SymbolKind}; +use nickel_lang_core::position::RawSpan; use nickel_lang_core::term::RichTerm; use nickel_lang_core::typ::Type; @@ -36,34 +37,23 @@ fn symbols( .flat_map(|rt| { rt.fields.into_iter().filter_map(|(id, field)| { let ty = type_lookups.idents.get(&id.into()); - let (file_id, id_span) = id.pos.into_opt()?.to_range(); + let id_pos = id.pos.into_opt()?; + let (file_id, id_span) = id_pos.to_range(); + // We need to find the span of the name (that's id_span above), but also the // span of the "whole value," whatever that means. In vscode, there's a little // outline bar at the top that shows you which symbol you're currently in, and it // works by checking whether the cursor is inside the "whole value" range. - // - // `last_pos` is going to be the end of this "value" span. We - // take it large enough to contain the field value (if there - // is one) and any other annotations that we can work out the - // positions of. - let mut last_pos = id_span.end; - - if let Some(val_span) = field.value.as_ref().and_then(|val| val.pos.into_opt()) { - last_pos = last_pos.max(val_span.end.to_usize()); - } - - if let Some(last_ty_pos) = field + // We take this range large enough to contain the field value + // (if there is one) and any other annotations that we can work + // out the positions of. + let val_span = field .metadata .annotation - .contracts .iter() - .chain(field.metadata.annotation.typ.as_ref()) - .filter_map(|ty| ty.typ.pos.as_opt_ref()) - .map(|pos| pos.end.to_usize()) - .max() - { - last_pos = last_pos.max(last_ty_pos); - } + .filter_map(|ty| ty.typ.pos.into_opt()) + .chain(field.value.as_ref().and_then(|val| val.pos.into_opt())) + .fold(id_pos, |a, b| RawSpan::fuse(a, b).unwrap_or(a)); let selection_range = crate::codespan_lsp::byte_span_to_range( world.cache.files(), @@ -74,7 +64,7 @@ fn symbols( let range = crate::codespan_lsp::byte_span_to_range( world.cache.files(), file_id, - id_span.start..last_pos, + val_span.to_range().1, ) .ok()?; diff --git a/lsp/nls/tests/inputs/symbols-basic.ncl b/lsp/nls/tests/inputs/symbols-basic.ncl index c40e49f6fb..98c699aa81 100644 --- a/lsp/nls/tests/inputs/symbols-basic.ncl +++ b/lsp/nls/tests/inputs/symbols-basic.ncl @@ -10,7 +10,7 @@ let func = fun x => 1 in inner_name = { name = "value" }, } : _, annotated | String, - docced | doc "The symbol range doesn't current include docs", + docced | doc "The symbol range doesn't currently include docs", } ### [[request]] ### type = "Symbols"