From cd4560e97ab2c08b966f43523b30dd59cf6ace2f Mon Sep 17 00:00:00 2001 From: zc he Date: Wed, 16 Apr 2025 19:43:21 +0800 Subject: [PATCH] fix(lsp): a panic caused by completion with decl_id out of range (#15576) Fixes a bug caused by #15536 Sorry about that, @fdncred # Description I've made the panic reproducible in the test case. TLDR: completer will sometimes return new decl_ids outside of the range of the engine_state passed in. # User-Facing Changes bug fix # Tests + Formatting +1 # After Submitting --- .../src/completions/exportable_completions.rs | 3 +- crates/nu-lsp/src/completion.rs | 105 +++++++++++------- tests/fixtures/lsp/completion/command.nu | 2 + 3 files changed, 66 insertions(+), 44 deletions(-) diff --git a/crates/nu-cli/src/completions/exportable_completions.rs b/crates/nu-cli/src/completions/exportable_completions.rs index 01a50841a3..db8bb6b143 100644 --- a/crates/nu-cli/src/completions/exportable_completions.rs +++ b/crates/nu-cli/src/completions/exportable_completions.rs @@ -69,7 +69,8 @@ impl Completer for ExportableCompletion<'_> { wrapped_name(name), Some(cmd.description().to_string()), None, - SuggestionKind::Command(cmd.command_type(), Some(*decl_id)), + // `None` here avoids arguments being expanded by snippet edit style for lsp + SuggestionKind::Command(cmd.command_type(), None), ); } } diff --git a/crates/nu-lsp/src/completion.rs b/crates/nu-lsp/src/completion.rs index bc28ed5058..7305778122 100644 --- a/crates/nu-lsp/src/completion.rs +++ b/crates/nu-lsp/src/completion.rs @@ -68,52 +68,60 @@ impl LanguageServer { let mut idx = 0; // use snippet as `insert_text_format` for command argument completion if let Some(SuggestionKind::Command(_, Some(decl_id))) = suggestion.kind { - let cmd = engine_state.get_decl(decl_id); - doc_string = Some(Self::get_decl_description(cmd, true)); - insert_text_format = Some(InsertTextFormat::SNIPPET); - let signature = cmd.signature(); - // add curly brackets around block arguments - // and keywords, e.g. `=` in `alias foo = bar` - let mut arg_wrapper = |arg: &PositionalArg, text: String, optional: bool| -> String { - idx += 1; - match &arg.shape { - SyntaxShape::Block | SyntaxShape::MatchBlock => { - format!("{{ ${{{}:{}}} }}", idx, text) - } - SyntaxShape::Keyword(kwd, _) => { - // NOTE: If optional, the keyword should also be in a placeholder so that it can be removed easily. - // Here we choose to use nested placeholders. Note that some editors don't fully support this format, - // but usually they will simply ignore the inner ones, so it should be fine. - if optional { - idx += 1; - format!( - "${{{}:{} ${{{}:{}}}}}", - idx - 1, - String::from_utf8_lossy(kwd), - idx, - text - ) - } else { - format!("{} ${{{}:{}}}", String::from_utf8_lossy(kwd), idx, text) + // NOTE: for new commands defined in current context, + // which are not present in the engine state, skip the documentation and snippet. + if engine_state.num_decls() > decl_id.get() { + let cmd = engine_state.get_decl(decl_id); + doc_string = Some(Self::get_decl_description(cmd, true)); + insert_text_format = Some(InsertTextFormat::SNIPPET); + let signature = cmd.signature(); + // add curly brackets around block arguments + // and keywords, e.g. `=` in `alias foo = bar` + let mut arg_wrapper = |arg: &PositionalArg, + text: String, + optional: bool| + -> String { + idx += 1; + match &arg.shape { + SyntaxShape::Block | SyntaxShape::MatchBlock => { + format!("{{ ${{{}:{}}} }}", idx, text) } + SyntaxShape::Keyword(kwd, _) => { + // NOTE: If optional, the keyword should also be in a placeholder so that it can be removed easily. + // Here we choose to use nested placeholders. Note that some editors don't fully support this format, + // but usually they will simply ignore the inner ones, so it should be fine. + if optional { + idx += 1; + format!( + "${{{}:{} ${{{}:{}}}}}", + idx - 1, + String::from_utf8_lossy(kwd), + idx, + text + ) + } else { + format!("{} ${{{}:{}}}", String::from_utf8_lossy(kwd), idx, text) + } + } + _ => format!("${{{}:{}}}", idx, text), } - _ => format!("${{{}:{}}}", idx, text), - } - }; + }; - for required in signature.required_positional { - snippet_text.push(' '); - snippet_text - .push_str(arg_wrapper(&required, required.name.clone(), false).as_str()); - } - for optional in signature.optional_positional { - snippet_text.push(' '); - snippet_text - .push_str(arg_wrapper(&optional, format!("{}?", optional.name), true).as_str()); - } - if let Some(rest) = signature.rest_positional { - idx += 1; - snippet_text.push_str(format!(" ${{{}:...{}}}", idx, rest.name).as_str()); + for required in signature.required_positional { + snippet_text.push(' '); + snippet_text + .push_str(arg_wrapper(&required, required.name.clone(), false).as_str()); + } + for optional in signature.optional_positional { + snippet_text.push(' '); + snippet_text.push_str( + arg_wrapper(&optional, format!("{}?", optional.name), true).as_str(), + ); + } + if let Some(rest) = signature.rest_positional { + idx += 1; + snippet_text.push_str(format!(" ${{{}:...{}}}", idx, rest.name).as_str()); + } } } // no extra space for a command with args expanded in the snippet @@ -329,6 +337,17 @@ mod tests { }) )); + // fallback completion on a newly defined command, + // the decl_id is missing in the engine state, this shouldn't cause any panic. + let resp = send_complete_request(&client_connection, script.clone(), 13, 9); + assert_json_include!( + actual: result_from_message(resp), + expected: serde_json::json!([ + // defined before the cursor + { "label": "config n foo bar", "detail": detail_str, "kind": 2 }, + ]) + ); + // inside parenthesis let resp = send_complete_request(&client_connection, script, 10, 34); assert!(result_from_message(resp).as_array().unwrap().contains( diff --git a/tests/fixtures/lsp/completion/command.nu b/tests/fixtures/lsp/completion/command.nu index ca2bd5cc51..6fb776f79b 100644 --- a/tests/fixtures/lsp/completion/command.nu +++ b/tests/fixtures/lsp/completion/command.nu @@ -10,3 +10,5 @@ def "config n foo bar" [ echo "🤔🐘" | str substring (str substring -) } + +config n # don't panic!