From 8877431ee79caa2e85d96e06dc9ec62e1149635f Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Mon, 15 Dec 2025 09:23:32 -0500 Subject: [PATCH] [ty] Suppress keyword argument completions unless we're in the "arguments" Otherwise, given a case like this: ``` (lambda foo: ( + 1))(2) ``` we'll offer _argument_ completions for `foo` at the cursor position. While we do actually want to offer completions for `foo` in this context, it is currently difficult to do so. But we definitely don't want to offer completions for `foo` as an argument to a function here. Which is what we were doing. We also add an end-to-end test here to verify that the actual label we offer in completion suggestions includes the `=` suffix. Closes https://github.com/astral-sh/ruff/pull/21970 --- crates/ty_ide/src/completion.rs | 16 ++++- crates/ty_ide/src/signature_help.rs | 5 ++ crates/ty_server/tests/e2e/completions.rs | 59 +++++++++++++++++++ crates/ty_server/tests/e2e/main.rs | 36 +++++++++-- crates/ty_server/tests/e2e/signature_help.rs | 37 ++++++++++++ ...ignature_help__works_in_function_name.snap | 39 ++++++++++++ 6 files changed, 184 insertions(+), 8 deletions(-) create mode 100644 crates/ty_server/tests/e2e/signature_help.rs create mode 100644 crates/ty_server/tests/e2e/snapshots/e2e__signature_help__works_in_function_name.snap diff --git a/crates/ty_ide/src/completion.rs b/crates/ty_ide/src/completion.rs index 905ca92c0b..670b5b2ec7 100644 --- a/crates/ty_ide/src/completion.rs +++ b/crates/ty_ide/src/completion.rs @@ -504,6 +504,18 @@ fn detect_function_arg_completions<'db>( parsed: &ParsedModuleRef, offset: TextSize, ) -> Option>> { + // But be careful: this isn't as simple as just finding a call + // expression. We also have to make sure we are in the "arguments" + // portion of the call. Otherwise we risk incorrectly returning + // something for `()(arg1, arg2)`-style expressions. + if !covering_node(parsed.syntax().into(), TextRange::empty(offset)) + .ancestors() + .take_while(|node| !node.is_statement()) + .any(|node| node.is_arguments()) + { + return None; + } + let sig_help = signature_help(db, file, offset)?; let set_function_args = detect_set_function_args(parsed, offset); @@ -2558,7 +2570,7 @@ def frob(): ... assert_snapshot!( builder.skip_keywords().skip_builtins().build().snapshot(), - @"foo=", + @"", ); } @@ -2572,7 +2584,7 @@ def frob(): ... assert_snapshot!( builder.skip_keywords().skip_builtins().build().snapshot(), - @"foo=", + @"", ); } diff --git a/crates/ty_ide/src/signature_help.rs b/crates/ty_ide/src/signature_help.rs index a4746f1563..33fb922686 100644 --- a/crates/ty_ide/src/signature_help.rs +++ b/crates/ty_ide/src/signature_help.rs @@ -124,6 +124,11 @@ fn get_call_expr( })?; // Find the covering node at the given position that is a function call. + // Note that we are okay with the range being anywhere within a call + // expression, even if it's not in the arguments portion of the call + // expression. This is because, e.g., a user can request signature + // information at a call site, and this should ideally work anywhere + // within the call site, even at the function name. let call = covering_node(root_node, token.range()) .find_first(|node| { if !node.is_expr_call() { diff --git a/crates/ty_server/tests/e2e/completions.rs b/crates/ty_server/tests/e2e/completions.rs index 23e074a715..e5a5448027 100644 --- a/crates/ty_server/tests/e2e/completions.rs +++ b/crates/ty_server/tests/e2e/completions.rs @@ -283,3 +283,62 @@ TypedDi Ok(()) } + +/// Tests that completions for function arguments will +/// show a `=` suffix. +#[test] +fn function_parameter_shows_equals_suffix() -> Result<()> { + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +import re +re.match('', '', fla +"; + + let mut server = TestServerBuilder::new()? + .with_initialization_options(ClientOptions::default().with_auto_import(false)) + .with_workspace(workspace_root, None)? + .with_file(foo, foo_content)? + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(foo, foo_content, 1); + let _ = server.await_notification::(); + + let completions = server.completion_request(&server.file_uri(foo), Position::new(1, 20)); + + insta::assert_json_snapshot!(completions, @r#" + [ + { + "label": "flags=", + "kind": 6, + "detail": "int", + "sortText": "0", + "insertText": "flags=" + }, + { + "label": "FloatingPointError", + "kind": 7, + "detail": "", + "documentation": "Floating-point operation failed.\n", + "sortText": "1" + }, + { + "label": "PythonFinalizationError", + "kind": 7, + "detail": "", + "documentation": "Operation blocked during Python finalization.\n", + "sortText": "2" + }, + { + "label": "float", + "kind": 7, + "detail": "", + "documentation": "Convert a string or number to a floating-point number, if possible.\n", + "sortText": "3" + } + ] + "#); + + Ok(()) +} diff --git a/crates/ty_server/tests/e2e/main.rs b/crates/ty_server/tests/e2e/main.rs index d0ac098527..8c6d378857 100644 --- a/crates/ty_server/tests/e2e/main.rs +++ b/crates/ty_server/tests/e2e/main.rs @@ -36,6 +36,7 @@ mod notebook; mod publish_diagnostics; mod pull_diagnostics; mod rename; +mod signature_help; use std::collections::{BTreeMap, HashMap, VecDeque}; use std::num::NonZeroUsize; @@ -54,7 +55,8 @@ use lsp_types::notification::{ }; use lsp_types::request::{ Completion, DocumentDiagnosticRequest, HoverRequest, Initialize, InlayHintRequest, - PrepareRenameRequest, Request, Shutdown, WorkspaceConfiguration, WorkspaceDiagnosticRequest, + PrepareRenameRequest, Request, Shutdown, SignatureHelpRequest, WorkspaceConfiguration, + WorkspaceDiagnosticRequest, }; use lsp_types::{ ClientCapabilities, CompletionItem, CompletionParams, CompletionResponse, @@ -64,11 +66,11 @@ use lsp_types::{ DocumentDiagnosticParams, DocumentDiagnosticReportResult, FileEvent, Hover, HoverParams, InitializeParams, InitializeResult, InitializedParams, InlayHint, InlayHintClientCapabilities, InlayHintParams, NumberOrString, PartialResultParams, Position, PreviousResultId, - PublishDiagnosticsClientCapabilities, Range, TextDocumentClientCapabilities, - TextDocumentContentChangeEvent, TextDocumentIdentifier, TextDocumentItem, - TextDocumentPositionParams, Url, VersionedTextDocumentIdentifier, WorkDoneProgressParams, - WorkspaceClientCapabilities, WorkspaceDiagnosticParams, WorkspaceDiagnosticReportResult, - WorkspaceEdit, WorkspaceFolder, + PublishDiagnosticsClientCapabilities, Range, SignatureHelp, SignatureHelpParams, + SignatureHelpTriggerKind, TextDocumentClientCapabilities, TextDocumentContentChangeEvent, + TextDocumentIdentifier, TextDocumentItem, TextDocumentPositionParams, Url, + VersionedTextDocumentIdentifier, WorkDoneProgressParams, WorkspaceClientCapabilities, + WorkspaceDiagnosticParams, WorkspaceDiagnosticReportResult, WorkspaceEdit, WorkspaceFolder, }; use ruff_db::system::{OsSystem, SystemPath, SystemPathBuf, TestSystem}; use rustc_hash::FxHashMap; @@ -940,6 +942,28 @@ impl TestServer { None => vec![], } } + + /// Sends a `textDocument/signatureHelp` request for the document at the given URL and position. + pub(crate) fn signature_help_request( + &mut self, + uri: &Url, + position: Position, + ) -> Option { + let signature_help_id = self.send_request::(SignatureHelpParams { + text_document_position_params: TextDocumentPositionParams { + text_document: TextDocumentIdentifier { uri: uri.clone() }, + position, + }, + work_done_progress_params: lsp_types::WorkDoneProgressParams::default(), + context: Some(lsp_types::SignatureHelpContext { + trigger_kind: SignatureHelpTriggerKind::INVOKED, + trigger_character: None, + is_retrigger: false, + active_signature_help: None, + }), + }); + self.await_response::(&signature_help_id) + } } impl fmt::Debug for TestServer { diff --git a/crates/ty_server/tests/e2e/signature_help.rs b/crates/ty_server/tests/e2e/signature_help.rs new file mode 100644 index 0000000000..87cb98bb0f --- /dev/null +++ b/crates/ty_server/tests/e2e/signature_help.rs @@ -0,0 +1,37 @@ +use anyhow::Result; +use lsp_types::{Position, notification::PublishDiagnostics}; +use ruff_db::system::SystemPath; +use ty_server::ClientOptions; + +use crate::TestServerBuilder; + +/// Tests that we get signature help even when the cursor +/// is on the function name. +/// +/// This is a regression test to ensure we don't accidentally +/// cause this case to stop working. +#[test] +fn works_in_function_name() -> Result<()> { + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +import re +re.match('', '') +"; + + let mut server = TestServerBuilder::new()? + .with_initialization_options(ClientOptions::default()) + .with_workspace(workspace_root, None)? + .with_file(foo, foo_content)? + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(foo, foo_content, 1); + let _ = server.await_notification::(); + + let signature_help = server.signature_help_request(&server.file_uri(foo), Position::new(1, 6)); + + insta::assert_json_snapshot!(signature_help); + + Ok(()) +} diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__signature_help__works_in_function_name.snap b/crates/ty_server/tests/e2e/snapshots/e2e__signature_help__works_in_function_name.snap new file mode 100644 index 0000000000..19e4d2d1b0 --- /dev/null +++ b/crates/ty_server/tests/e2e/snapshots/e2e__signature_help__works_in_function_name.snap @@ -0,0 +1,39 @@ +--- +source: crates/ty_server/tests/e2e/signature_help.rs +expression: signature_help +--- +{ + "signatures": [ + { + "label": "(pattern: str | Pattern[str], string: str, flags: int = Literal[0]) -> Match[str] | None", + "documentation": "Try to apply the pattern at the start of the string, returning/na Match object, or None if no match was found.\n", + "parameters": [ + { + "label": "pattern: str | Pattern[str]" + }, + { + "label": "string: str" + }, + { + "label": "flags: int = Literal[0]" + } + ] + }, + { + "label": "(pattern: bytes | Pattern[bytes], string: Buffer, flags: int = Literal[0]) -> Match[bytes] | None", + "parameters": [ + { + "label": "pattern: bytes | Pattern[bytes]" + }, + { + "label": "string: Buffer" + }, + { + "label": "flags: int = Literal[0]" + } + ] + } + ], + "activeSignature": 0, + "activeParameter": 0 +}