fix [lsp][completion] rank results based on compatibility in context #1653#2315
fix [lsp][completion] rank results based on compatibility in context #1653#2315asukaminato0721 wants to merge 3 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements compatibility-aware completion ordering to address issue #1653, where incompatible suggestions should be ranked lower than compatible ones in call-argument contexts. The implementation computes the expected argument type at call sites and demotes local variables and attributes that are incompatible with that type.
Changes:
- Added type compatibility checking for completion items in call-argument contexts
- Modified completion sorting to demote incompatible suggestions using a 'z' suffix in sort_text
- Added LSP interaction test verifying compatible suggestions rank higher than incompatible ones
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyrefly/lib/lsp/wasm/completion.rs | Implements compatibility checking logic with helper methods, integrates expected type computation into completion flows for local variables and attributes, and applies demotion via sort_text manipulation |
| pyrefly/lib/test/lsp/lsp_interaction/completion.rs | Adds test case verifying that compatible variables are ranked before incompatible ones in function call contexts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn expected_call_argument_type(&self, handle: &Handle, position: TextSize) -> Option<Type> { | ||
| let (callables, chosen_overload_index, active_argument, _) = | ||
| self.get_callables_from_call(handle, position)?; | ||
| let callable = callables.get(chosen_overload_index)?.clone(); | ||
| let params = Self::normalize_singleton_function_type_into_params(callable)?; | ||
| let arg_index = Self::active_parameter_index(¶ms, &active_argument)?; | ||
| let param = params.get(arg_index)?; | ||
| Some(param.as_type().clone()) | ||
| } |
There was a problem hiding this comment.
This method calls Self::normalize_singleton_function_type_into_params (line 291) and Self::active_parameter_index (line 292), but these methods are not defined in the Transaction impl block in completion.rs. These methods exist in signature_help.rs but are not accessible from this module. This will cause a compilation error.
The methods need to either be:
- Moved to a shared location accessible by both modules
- Re-implemented in completion.rs
- Imported from signature_help.rs if they are made public
Looking at the codebase, similar methods exist in signature_help.rs (lines 272-286 and 288-300) and inlay_hints.rs (lines 72-87).
This comment has been minimized.
This comment has been minimized.
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
thanks!!
looks pretty good for now from a few nits and red CI. a refactor at some point making it clear where and when sort_text should added would also be appreciated (how do local variable completions compete with the other types of completions?), but that is a pre-existing issue in this diff.
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. ✅ |
2 participants
Summary
Fixes #1653
Implemented compatibility-aware completion ordering for call-argument contexts by computing the expected argument type and demoting incompatible local/attribute suggestions
Test Plan
Added an LSP interaction test that asserts compatible suggestions are ranked ahead of incompatible ones