impl Completions: Auto add function call parenthesis #2207#2254
impl Completions: Auto add function call parenthesis #2207#2254asukaminato0721 wants to merge 6 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in completion behavior to automatically insert () for function/method completion items (using snippets when supported), exposed via python.analysis.completeFunctionParens and validated with an LSP interaction test.
Changes:
- Add
python.analysis.completeFunctionParensVS Code setting and plumb it through the server’s analysis config. - Detect client snippet support and emit snippet-based
insertText(name($0)) when available, otherwise plain text (name()). - Add an LSP interaction test asserting snippet completion behavior.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/test/lsp/lsp_interaction/completion.rs | Adds an interaction test to validate snippet insertion when auto-parens is enabled. |
| pyrefly/lib/state/lsp.rs | Implements auto-parens mutation of completion items for functions/methods behind new flags. |
| pyrefly/lib/lsp/non_wasm/workspace.rs | Adds complete_function_parens to the deserialized analysis config. |
| pyrefly/lib/lsp/non_wasm/server.rs | Wires config + client snippet capability into completion request handling. |
| lsp/package.json | Exposes python.analysis.completeFunctionParens in VS Code settings UI. |
| Cargo.lock | Updates lockfile checksum entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| list.items.iter().any(|item| { | ||
| item.label == "spam" | ||
| && item.insert_text.as_deref() == Some("spam($0)") | ||
| && item.insert_text_format == Some(InsertTextFormat::SNIPPET) | ||
| }) |
There was a problem hiding this comment.
The new behavior has two branches (snippet vs. non-snippet clients), but this test only asserts the snippet path. Consider adding a companion test (or an additional assertion) with snippetSupport: false to verify the plain-text insertion uses spam() and does not set insertTextFormat to SNIPPET.
pyrefly/lib/state/lsp.rs
Outdated
| pub fn completion_with_incomplete_with_function_parens( | ||
| &self, | ||
| handle: &Handle, | ||
| position: TextSize, | ||
| import_format: ImportFormat, |
There was a problem hiding this comment.
completion_with_incomplete_with_function_parens adds multiple boolean parameters, which makes call sites harder to read and easy to misuse. Consider introducing a small options struct (e.g., CompletionOptions { complete_function_parens, supports_snippets, ... }) or an enum/bitflags to keep the API self-documenting as more completion features are added.
There was a problem hiding this comment.
I think this is good advice. We should probably add something simple here
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
kinto0
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
Summary
Fixes part of #2207
Added optional auto‑parentheses for function/method completion items behind
python.analysis.completeFunctionParens, with snippet insertion when the client supports it.Plumbed the setting through LSP config, exposed it in the VS Code settings.
Test Plan
Added an LSP interaction test that asserts snippet behavior.