From dc8b50a21ea8e5ce691e063386e6ab05f0d2c62b Mon Sep 17 00:00:00 2001 From: blindfs Date: Tue, 8 Apr 2025 11:08:06 +0800 Subject: [PATCH] fix(lsp): several edge cases of inaccurate references --- crates/nu-lsp/src/ast.rs | 80 ++++++----- crates/nu-lsp/src/goto.rs | 2 +- crates/nu-lsp/src/hover.rs | 2 +- crates/nu-lsp/src/lib.rs | 2 +- crates/nu-lsp/src/symbols.rs | 4 +- crates/nu-lsp/src/workspace.rs | 214 ++++++++++++++++++++++------ tests/fixtures/lsp/workspace/bar.nu | 2 +- tests/fixtures/lsp/workspace/baz.nu | 10 ++ tests/fixtures/lsp/workspace/foo.nu | 12 ++ 9 files changed, 247 insertions(+), 81 deletions(-) create mode 100644 tests/fixtures/lsp/workspace/baz.nu diff --git a/crates/nu-lsp/src/ast.rs b/crates/nu-lsp/src/ast.rs index fb25e9139a..7baa5bb4bd 100644 --- a/crates/nu-lsp/src/ast.rs +++ b/crates/nu-lsp/src/ast.rs @@ -7,15 +7,20 @@ use nu_protocol::{ use std::sync::Arc; /// Adjust span if quoted -fn strip_quotes(span: Span, working_set: &StateWorkingSet) -> Span { - let text = String::from_utf8_lossy(working_set.get_span_contents(span)); +fn strip_quotes(span: Span, working_set: &StateWorkingSet) -> (Vec, Span) { + let text = working_set.get_span_contents(span); if text.len() > 1 - && ((text.starts_with('"') && text.ends_with('"')) - || (text.starts_with('\'') && text.ends_with('\''))) + && ((text.starts_with(b"\"") && text.ends_with(b"\"")) + || (text.starts_with(b"'") && text.ends_with(b"'"))) { - Span::new(span.start.saturating_add(1), span.end.saturating_sub(1)) + ( + text.get(1..text.len() - 1) + .expect("Invalid quoted span!") + .to_vec(), + Span::new(span.start.saturating_add(1), span.end.saturating_sub(1)), + ) } else { - span + (text.to_vec(), span) } } @@ -70,9 +75,8 @@ fn try_find_id_in_def( } } } - let span = strip_quotes(span?, working_set); - let name = working_set.get_span_contents(span); - let decl_id = Id::Declaration(working_set.find_decl(name).or_else(|| { + let (name, span) = strip_quotes(span?, working_set); + let decl_id = Id::Declaration(working_set.find_decl(&name).or_else(|| { // for defs inside def // TODO: get scope by position // https://github.com/nushell/nushell/issues/15291 @@ -114,8 +118,8 @@ fn try_find_id_in_mod( Argument::Positional(expr) => { let name = expr.as_string()?; let module_id = working_set.find_module(name.as_bytes())?; - let found_id = Id::Module(module_id); - let found_span = strip_quotes(arg.span(), working_set); + let found_id = Id::Module(module_id, name.as_bytes().to_vec()); + let found_span = strip_quotes(arg.span(), working_set).1; id_ref .is_none_or(|id_r| found_id == *id_r) .then_some((found_id, found_span)) @@ -160,14 +164,16 @@ fn try_find_id_in_use( (*decl_id == *decl_id_ref).then_some(Id::Declaration(*decl_id)) }), // this is only for argument `members` - Some(Id::Module(module_id_ref)) => module.submodules.get(name).and_then(|module_id| { - (*module_id == *module_id_ref).then_some(Id::Module(*module_id)) - }), + Some(Id::Module(module_id_ref, name_ref)) => { + module.submodules.get(name).and_then(|module_id| { + (*module_id == *module_id_ref && name_ref == name) + .then_some(Id::Module(*module_id, name.to_vec())) + }) + } None => module .submodules .get(name) - .cloned() - .map(Id::Module) + .map(|id| Id::Module(*id, name.to_vec())) .or(module.decls.get(name).cloned().map(Id::Declaration)) .or(module.constants.get(name).cloned().map(Id::Variable)), _ => None, @@ -178,16 +184,17 @@ fn try_find_id_in_use( // Get module id if required let module_name = call.arguments.first()?; let span = module_name.span(); - if let Some(Id::Module(id_ref)) = id { + let (span_content, clean_span) = strip_quotes(span, working_set); + if let Some(Id::Module(id_ref, name_ref)) = id { // still need to check the rest, if id not matched - if module_id == *id_ref { - return Some((Id::Module(module_id), strip_quotes(span, working_set))); + if module_id == *id_ref && *name_ref == span_content { + return Some((Id::Module(module_id, span_content.to_vec()), clean_span)); } } if let Some(pos) = location { // first argument of `use`/`hide` should always be module name if span.contains(*pos) { - return Some((Id::Module(module_id), strip_quotes(span, working_set))); + return Some((Id::Module(module_id, span_content.to_vec()), clean_span)); } } @@ -200,14 +207,13 @@ fn try_find_id_in_use( let name = e.as_string()?; Some(( find_by_name(name.as_bytes())?, - strip_quotes(item_expr.span, working_set), + strip_quotes(item_expr.span, working_set).1, )) }) }) }; - let arguments = call.arguments.get(1..)?; - for arg in arguments { + for arg in call.arguments.get(1..)?.iter().rev() { let Argument::Positional(expr) = arg else { continue; }; @@ -216,7 +222,7 @@ fn try_find_id_in_use( } let matched = match &expr.expr { Expr::String(name) => { - find_by_name(name.as_bytes()).map(|id| (id, strip_quotes(expr.span, working_set))) + find_by_name(name.as_bytes()).map(|id| (id, strip_quotes(expr.span, working_set).1)) } Expr::List(items) => search_in_list_items(items), Expr::FullCellPath(fcp) => { @@ -248,7 +254,7 @@ fn try_find_id_in_overlay( id: Option<&Id>, ) -> Option<(Id, Span)> { let check_location = |span: &Span| location.is_none_or(|pos| span.contains(*pos)); - let module_from_parser_info = |span: Span| { + let module_from_parser_info = |span: Span, name: &str| { let Expression { expr: Expr::Overlay(Some(module_id)), .. @@ -256,18 +262,22 @@ fn try_find_id_in_overlay( else { return None; }; - let found_id = Id::Module(*module_id); + let found_id = Id::Module(*module_id, name.as_bytes().to_vec()); id.is_none_or(|id_r| found_id == *id_r) - .then_some((found_id, strip_quotes(span, working_set))) + .then_some((found_id, strip_quotes(span, working_set).1)) }; - // NOTE: `overlay_expr` doesn't work for `overlay hide` + // NOTE: `overlay_expr` doesn't exist for `overlay hide` let module_from_overlay_name = |name: &str, span: Span| { - let found_id = Id::Module(working_set.find_overlay(name.as_bytes())?.origin); + let found_id = Id::Module( + working_set.find_overlay(name.as_bytes())?.origin, + name.as_bytes().to_vec(), + ); id.is_none_or(|id_r| found_id == *id_r) - .then_some((found_id, strip_quotes(span, working_set))) + .then_some((found_id, strip_quotes(span, working_set).1)) }; - for arg in call.arguments.iter() { + // check `as alias` first + for arg in call.arguments.iter().rev() { let Argument::Positional(expr) = arg else { continue; }; @@ -275,11 +285,11 @@ fn try_find_id_in_overlay( continue; }; let matched = match &expr.expr { - Expr::String(name) => module_from_parser_info(expr.span) + Expr::String(name) => module_from_parser_info(expr.span, name) .or_else(|| module_from_overlay_name(name, expr.span)), // keyword 'as' Expr::Keyword(kwd) => match &kwd.expr.expr { - Expr::String(name) => module_from_parser_info(kwd.expr.span) + Expr::String(name) => module_from_parser_info(kwd.expr.span, name) .or_else(|| module_from_overlay_name(name, kwd.expr.span)), _ => None, }, @@ -349,7 +359,9 @@ fn find_id_in_expr( FindMapResult::Found((Id::CellPath(var_id, tail), span)) } } - Expr::Overlay(Some(module_id)) => FindMapResult::Found((Id::Module(*module_id), span)), + Expr::Overlay(Some(module_id)) => { + FindMapResult::Found((Id::Module(*module_id, vec![]), span)) + } // terminal value expressions Expr::Bool(_) | Expr::Binary(_) diff --git a/crates/nu-lsp/src/goto.rs b/crates/nu-lsp/src/goto.rs index f84d4c0e9e..995b05ae21 100644 --- a/crates/nu-lsp/src/goto.rs +++ b/crates/nu-lsp/src/goto.rs @@ -55,7 +55,7 @@ impl LanguageServer { let var = working_set.get_variable(*var_id); Some(var.declaration_span) } - Id::Module(module_id) => { + Id::Module(module_id, _) => { let module = working_set.get_module(*module_id); module.span } diff --git a/crates/nu-lsp/src/hover.rs b/crates/nu-lsp/src/hover.rs index 29e106c751..2634f44061 100644 --- a/crates/nu-lsp/src/hover.rs +++ b/crates/nu-lsp/src/hover.rs @@ -178,7 +178,7 @@ impl LanguageServer { working_set.get_decl(decl_id), false, )), - Id::Module(module_id) => { + Id::Module(module_id, _) => { let description = working_set .get_module_comments(module_id)? .iter() diff --git a/crates/nu-lsp/src/lib.rs b/crates/nu-lsp/src/lib.rs index 111a736b8c..9c98c2d500 100644 --- a/crates/nu-lsp/src/lib.rs +++ b/crates/nu-lsp/src/lib.rs @@ -43,7 +43,7 @@ pub(crate) enum Id { Variable(VarId), Declaration(DeclId), Value(Type), - Module(ModuleId), + Module(ModuleId, Vec), CellPath(VarId, Vec), External(String), } diff --git a/crates/nu-lsp/src/symbols.rs b/crates/nu-lsp/src/symbols.rs index 0a02d383a8..b3b6e81c13 100644 --- a/crates/nu-lsp/src/symbols.rs +++ b/crates/nu-lsp/src/symbols.rs @@ -124,7 +124,7 @@ impl SymbolCache { range, }) } - Id::Module(module_id) => { + Id::Module(module_id, _) => { let module = working_set.get_module(module_id); let span = module.span?; if !doc_span.contains(span.start) { @@ -165,7 +165,7 @@ impl SymbolCache { .chain((0..working_set.num_modules()).filter_map(|id| { Self::get_symbol_by_id( working_set, - Id::Module(ModuleId::new(id)), + Id::Module(ModuleId::new(id), vec![]), doc, &cached_file.covered_span, ) diff --git a/crates/nu-lsp/src/workspace.rs b/crates/nu-lsp/src/workspace.rs index 05701cd7e2..3886abbd1c 100644 --- a/crates/nu-lsp/src/workspace.rs +++ b/crates/nu-lsp/src/workspace.rs @@ -101,9 +101,14 @@ impl LanguageServer { let (id, cursor_span) = find_id(&block, &working_set, &location)?; let mut refs = find_reference_by_id(&block, &working_set, &id); let definition_span = Self::find_definition_span_by_id(&working_set, &id); - if let Some(extra_span) = - Self::reference_not_in_ast(&id, &working_set, definition_span, file_span, cursor_span) - { + if let Some(extra_span) = Self::reference_not_in_ast( + &id, + &String::from_utf8_lossy(working_set.get_span_contents(cursor_span)), + &working_set, + definition_span, + file_span, + cursor_span, + ) { if !refs.contains(&extra_span) { refs.push(extra_span); } @@ -333,6 +338,7 @@ impl LanguageServer { /// which is not covered in the AST fn reference_not_in_ast( id: &Id, + name_ref: &str, working_set: &StateWorkingSet, definition_span: Option, file_span: Span, @@ -340,17 +346,20 @@ impl LanguageServer { ) -> Option { if let (Id::Variable(_), Some(decl_span)) = (&id, definition_span) { if file_span.contains_span(decl_span) && decl_span.end > decl_span.start { - let leading_dashes = working_set - .get_span_contents(decl_span) + let content = working_set.get_span_contents(decl_span); + let leading_dashes = content .iter() // remove leading dashes for flags .take_while(|c| *c == &b'-') .count(); let start = decl_span.start + leading_dashes; - return Some(Span { - start, - end: start + sample_span.end - sample_span.start, - }); + return content + .get(leading_dashes..) + .is_some_and(|name| name.starts_with(name_ref.as_bytes())) + .then_some(Span { + start, + end: start + sample_span.end - sample_span.start, + }); } } None @@ -436,6 +445,7 @@ impl LanguageServer { .unwrap_or(Span::unknown()); if let Some(extra_span) = Self::reference_not_in_ast( &id_tracker.id, + &id_tracker.name, &working_set, definition_span, file_span, @@ -635,7 +645,7 @@ mod tests { script.push("foo.nu"); let script = path_to_uri(&script); - open_unchecked(&client_connection, script.clone()); + open_unchecked(&client_connection, script); } #[test] @@ -659,14 +669,16 @@ mod tests { open_unchecked(&client_connection, script.clone()); - let message_num = 5; + let message_num = 6; let messages = send_reference_request(&client_connection, script.clone(), 0, 12, message_num); assert_eq!(messages.len(), message_num); + let mut has_response = false; for message in messages { match message { Message::Notification(n) => assert_eq!(n.method, "$/progress"), Message::Response(r) => { + has_response = true; let result = r.result.unwrap(); let array = result.as_array().unwrap(); assert!(array.contains(&serde_json::json!( @@ -687,6 +699,7 @@ mod tests { _ => panic!("unexpected message type"), } } + assert!(has_response); } #[test] @@ -717,14 +730,16 @@ mod tests { // thus changing the cached `StateDelta` open_unchecked(&client_connection, script_foo); - let message_num = 5; + let message_num = 6; let messages = send_reference_request(&client_connection, script.clone(), 0, 23, message_num); assert_eq!(messages.len(), message_num); + let mut has_response = false; for message in messages { match message { Message::Notification(n) => assert_eq!(n.method, "$/progress"), Message::Response(r) => { + has_response = true; let result = r.result.unwrap(); let array = result.as_array().unwrap(); assert!(array.contains(&serde_json::json!( @@ -745,6 +760,61 @@ mod tests { _ => panic!("unexpected message type"), } } + assert!(has_response); + } + + #[test] + fn module_path_reference_in_workspace() { + let mut script = fixtures(); + script.push("lsp"); + script.push("workspace"); + let (client_connection, _recv) = initialize_language_server( + None, + serde_json::to_value(InitializeParams { + workspace_folders: Some(vec![WorkspaceFolder { + uri: path_to_uri(&script), + name: "random name".to_string(), + }]), + ..Default::default() + }) + .ok(), + ); + script.push("baz.nu"); + let script = path_to_uri(&script); + + open_unchecked(&client_connection, script.clone()); + + let message_num = 6; + let messages = + send_reference_request(&client_connection, script.clone(), 0, 12, message_num); + assert_eq!(messages.len(), message_num); + let mut has_response = false; + for message in messages { + match message { + Message::Notification(n) => assert_eq!(n.method, "$/progress"), + Message::Response(r) => { + has_response = true; + let result = r.result.unwrap(); + let array = result.as_array().unwrap(); + assert!(array.contains(&serde_json::json!( + { + "uri": script.to_string().replace("baz", "bar"), + "range": { "start": { "line": 0, "character": 4 }, "end": { "line": 0, "character": 12 } } + } + ) + )); + assert!(array.contains(&serde_json::json!( + { + "uri": script.to_string(), + "range": { "start": { "line": 6, "character": 4 }, "end": { "line": 6, "character": 12 } } + } + ) + )); + } + _ => panic!("unexpected message type"), + } + } + assert!(has_response); } #[test] @@ -768,7 +838,7 @@ mod tests { open_unchecked(&client_connection, script.clone()); - let message_num = 5; + let message_num = 6; let messages = send_rename_prepare_request( &client_connection, script.clone(), @@ -778,19 +848,24 @@ mod tests { false, ); assert_eq!(messages.len(), message_num); + let mut has_response = false; for message in messages { match message { Message::Notification(n) => assert_eq!(n.method, "$/progress"), - Message::Response(r) => assert_json_eq!( - r.result, - serde_json::json!({ - "start": { "line": 6, "character": 13 }, - "end": { "line": 6, "character": 20 } - }), - ), + Message::Response(r) => { + has_response = true; + assert_json_eq!( + r.result, + serde_json::json!({ + "start": { "line": 6, "character": 13 }, + "end": { "line": 6, "character": 20 } + }), + ) + } _ => panic!("unexpected message type"), } } + assert!(has_response); if let Message::Response(r) = send_rename_request(&client_connection, script.clone(), 6, 11) { @@ -817,7 +892,7 @@ mod tests { assert!( changs_bar.contains( &serde_json::json!({ - "range": { "start": { "line": 0, "character": 20 }, "end": { "line": 0, "character": 27 } }, + "range": { "start": { "line": 0, "character": 22 }, "end": { "line": 0, "character": 29 } }, "newText": "new" }) )); @@ -847,7 +922,7 @@ mod tests { open_unchecked(&client_connection, script.clone()); - let message_num = 5; + let message_num = 6; let messages = send_rename_prepare_request( &client_connection, script.clone(), @@ -857,19 +932,24 @@ mod tests { false, ); assert_eq!(messages.len(), message_num); + let mut has_response = false; for message in messages { match message { Message::Notification(n) => assert_eq!(n.method, "$/progress"), - Message::Response(r) => assert_json_eq!( - r.result, - serde_json::json!({ - "start": { "line": 3, "character": 3 }, - "end": { "line": 3, "character": 8 } - }), - ), + Message::Response(r) => { + has_response = true; + assert_json_eq!( + r.result, + serde_json::json!({ + "start": { "line": 3, "character": 3 }, + "end": { "line": 3, "character": 8 } + }), + ) + } _ => panic!("unexpected message type"), } } + assert!(has_response); if let Message::Response(r) = send_rename_request(&client_connection, script.clone(), 3, 5) { @@ -934,25 +1014,29 @@ mod tests { } else { panic!("Progress not cancelled"); }; + let mut has_response = false; for message in messages { match message { Message::Notification(n) => assert_eq!(n.method, "$/progress"), // the response of the preempting hover request - Message::Response(r) => assert_json_eq!( - r.result, - serde_json::json!({ - "contents": { - "kind": "markdown", - "value": "\n---\n### Usage \n```nu\n foo str {flags}\n```\n\n### Flags\n\n `-h`, `--help` - Display the help message for this command\n\n" - } - }), - ), + Message::Response(r) => { + has_response = true; + assert_json_eq!( + r.result, + serde_json::json!({ + "contents": { + "kind": "markdown", + "value": "\n---\n### Usage \n```nu\n foo str {flags}\n```\n\n### Flags\n\n `-h`, `--help` - Display the help message for this command\n\n" + } + }), + ) + } _ => panic!("unexpected message type"), } } + assert!(has_response); - if let Message::Response(r) = send_rename_request(&client_connection, script.clone(), 6, 11) - { + if let Message::Response(r) = send_rename_request(&client_connection, script, 6, 11) { // should not return any changes assert_json_eq!(r.result.unwrap()["changes"], serde_json::json!({})); } else { @@ -998,7 +1082,7 @@ mod tests { let (client_connection, _recv) = initialize_language_server(None, None); open_unchecked(&client_connection, script.clone()); - let message = send_document_highlight_request(&client_connection, script.clone(), 3, 5); + let message = send_document_highlight_request(&client_connection, script, 3, 5); let Message::Response(r) = message else { panic!("unexpected message type"); }; @@ -1010,4 +1094,52 @@ mod tests { ]), ); } + + #[test] + fn document_highlight_module_alias() { + let mut script = fixtures(); + script.push("lsp"); + script.push("workspace"); + script.push("baz.nu"); + let script = path_to_uri(&script); + + let (client_connection, _recv) = initialize_language_server(None, None); + open_unchecked(&client_connection, script.clone()); + + let message = send_document_highlight_request(&client_connection, script, 4, 17); + let Message::Response(r) = message else { + panic!("unexpected message type"); + }; + assert_json_eq!( + r.result, + serde_json::json!([ + { "range": { "start": { "line": 0, "character": 24 }, "end": { "line": 0, "character": 30 } }, "kind": 1 }, + { "range": { "start": { "line": 4, "character": 13 }, "end": { "line": 4, "character": 19 } }, "kind": 1 } + ]), + ); + } + + /// TODO: associate the module record with the submodule name in `use`? + #[test] + fn document_highlight_module_record() { + let mut script = fixtures(); + script.push("lsp"); + script.push("workspace"); + script.push("baz.nu"); + let script = path_to_uri(&script); + + let (client_connection, _recv) = initialize_language_server(None, None); + open_unchecked(&client_connection, script.clone()); + + let message = send_document_highlight_request(&client_connection, script, 8, 0); + let Message::Response(r) = message else { + panic!("unexpected message type"); + }; + assert_json_eq!( + r.result, + serde_json::json!([ + { "range": { "start": { "line": 8, "character": 1 }, "end": { "line": 8, "character": 8 } }, "kind": 1 }, + ]), + ); + } } diff --git a/tests/fixtures/lsp/workspace/bar.nu b/tests/fixtures/lsp/workspace/bar.nu index 947043d791..65f4ae09ed 100644 --- a/tests/fixtures/lsp/workspace/bar.nu +++ b/tests/fixtures/lsp/workspace/bar.nu @@ -1,4 +1,4 @@ -use foo.nu [ foooo "foo str" ] +use ./foo.nu [ foooo "foo str" ] export def "bar str" [ ] { diff --git a/tests/fixtures/lsp/workspace/baz.nu b/tests/fixtures/lsp/workspace/baz.nu new file mode 100644 index 0000000000..2f1aea642b --- /dev/null +++ b/tests/fixtures/lsp/workspace/baz.nu @@ -0,0 +1,10 @@ +overlay use ./foo.nu as prefix --prefix +alias aname = prefix mod name sub module cmd name +aname +prefix foo str +overlay hide prefix + +use ./foo.nu [ "mod name" cst_mod ] + +$cst_mod."sub module".var_name +mod name sub module cmd name diff --git a/tests/fixtures/lsp/workspace/foo.nu b/tests/fixtures/lsp/workspace/foo.nu index 3af354b032..22a55bef9e 100644 --- a/tests/fixtures/lsp/workspace/foo.nu +++ b/tests/fixtures/lsp/workspace/foo.nu @@ -5,3 +5,15 @@ export def foooo [ } export def "foo str" [] { "foo" } + +export module "mod name" { + export module "sub module" { + export def "cmd name" [] { } + } +} + +export module cst_mod { + export module "sub module" { + export const var_name = "const value" + } +}