fix LSP: Completions for attribute override definitions #2431#2631
fix LSP: Completions for attribute override definitions #2431#2631asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes LSP completion behavior when defining/overriding members inside a subclass by surfacing relevant base-class members, and refactors attribute completion generation into a shared helper (with a new interaction test covering the scenario).
Changes:
- Add class-body completion logic to suggest class/base members during method defs and attribute assignments.
- Refactor attribute completion item generation into a reusable helper.
- Add an LSP interaction test for override-member completions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyrefly/lib/lsp/wasm/completion.rs |
Adds class-body override completion path and refactors attribute completion generation. |
pyrefly/lib/test/lsp/lsp_interaction/completion.rs |
Adds a regression test ensuring base-class members appear in subclass override completions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn enclosing_class_def_for_completion( | ||
| ast: &ModModule, | ||
| position: TextSize, | ||
| ) -> Option<(&StmtClassDef, bool)> { | ||
| let covering_nodes = Ast::locate_node(ast, position); | ||
| let mut saw_function = false; | ||
| for node in covering_nodes.iter() { | ||
| match node { | ||
| AnyNodeRef::StmtFunctionDef(_) => { | ||
| saw_function = true; | ||
| } | ||
| AnyNodeRef::StmtClassDef(class_def) => { | ||
| return Some((class_def, saw_function)); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| None | ||
| } | ||
|
|
||
| fn add_attribute_completions_for_type( | ||
| &self, | ||
| handle: &Handle, | ||
| base_type: Type, | ||
| expected_type: Option<&Type>, | ||
| filter: Option<&Identifier>, | ||
| completions: &mut Vec<RankedCompletion>, | ||
| ) { | ||
| self.ad_hoc_solve(handle, "completion_attributes", |solver| { | ||
| let matcher = filter.map(|_| SkimMatcherV2::default().smart_case()); | ||
| solver | ||
| .completions(base_type, None, true) | ||
| .iter() | ||
| .filter(|attr| { | ||
| if let Some(filter) = filter | ||
| && let Some(ref matcher) = matcher | ||
| { | ||
| matcher | ||
| .fuzzy_match(attr.name.as_str(), filter.as_str()) | ||
| .is_some() | ||
| } else { | ||
| true | ||
| } | ||
| }) | ||
| .for_each(|attr| { | ||
| let kind = match attr.ty { | ||
| Some(Type::BoundMethod(_)) => Some(CompletionItemKind::METHOD), | ||
| Some(Type::Function(_) | Type::Overload(_)) => { | ||
| Some(CompletionItemKind::FUNCTION) | ||
| } | ||
| Some(Type::Module(_)) => Some(CompletionItemKind::MODULE), | ||
| Some(Type::ClassDef(_)) => Some(CompletionItemKind::CLASS), | ||
| _ => Some(CompletionItemKind::FIELD), | ||
| }; | ||
| let detail = attr | ||
| .ty | ||
| .clone() | ||
| .map(|t| t.as_lsp_string(LspDisplayMode::Hover)); | ||
| let documentation = self.get_docstring_for_attribute(handle, attr); | ||
| let is_incompatible = self.is_incompatible_with_expected_type( | ||
| handle, | ||
| expected_type, | ||
| attr.ty.as_ref(), | ||
| ); | ||
| let source = if attr.is_reexport { | ||
| CompletionSource::Reexport | ||
| } else { | ||
| CompletionSource::Local | ||
| }; | ||
| completions.push(RankedCompletion { | ||
| item: CompletionItem { | ||
| label: attr.name.as_str().to_owned(), | ||
| detail, | ||
| kind, | ||
| documentation, | ||
| tags: if attr.is_deprecated { | ||
| Some(vec![CompletionItemTag::DEPRECATED]) | ||
| } else { | ||
| None | ||
| }, | ||
| ..Default::default() | ||
| }, | ||
| source, | ||
| is_incompatible, | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| fn add_class_body_override_completions( | ||
| &self, | ||
| handle: &Handle, | ||
| position: TextSize, | ||
| identifier: &Identifier, | ||
| allow_inside_function: bool, | ||
| completions: &mut Vec<RankedCompletion>, | ||
| ) { | ||
| let Some(ast) = self.get_ast(handle) else { | ||
| return; | ||
| }; | ||
| let Some((class_def, saw_function)) = | ||
| Self::enclosing_class_def_for_completion(ast.as_ref(), position) | ||
| else { | ||
| return; | ||
| }; | ||
| if saw_function && !allow_inside_function { | ||
| return; | ||
| } |
There was a problem hiding this comment.
add_class_body_override_completions calls Ast::locate_node via enclosing_class_def_for_completion, but completion_sorted_opt_with_incomplete already calls identifier_at, which itself does a full Ast::locate_node traversal. This adds a second full AST walk on every completion in MethodDef/Expr(Store) contexts, which can be a noticeable perf hit on large files. Consider reusing the covering-nodes chain from the existing identifier_at call (e.g., return covering nodes alongside IdentifierWithContext, or plumb an enclosing_class/saw_function flag through IdentifierContext) so the class lookup doesn’t require another traversal.
| self.ad_hoc_solve(handle, "completion_attributes", |solver| { | ||
| let matcher = filter.map(|_| SkimMatcherV2::default().smart_case()); | ||
| solver | ||
| .completions(base_type, None, true) | ||
| .iter() | ||
| .filter(|attr| { | ||
| if let Some(filter) = filter | ||
| && let Some(ref matcher) = matcher | ||
| { | ||
| matcher | ||
| .fuzzy_match(attr.name.as_str(), filter.as_str()) | ||
| .is_some() | ||
| } else { | ||
| true | ||
| } | ||
| }) |
There was a problem hiding this comment.
The PR description says override members are "filtered by the typed prefix", but add_attribute_completions_for_type uses SkimMatcherV2::fuzzy_match, which will match non-prefix/skip-character patterns (e.g. "em" matching "error_message"). If prefix filtering is the intended behavior here, switch this filter to a starts_with (optionally case-insensitive) check instead of fuzzy matching; otherwise, consider updating the PR description to reflect fuzzy matching.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #2431
Added class-body completion logic to surface base-class members (filtered by the fuzzy match) and refactored attribute completion generation into a shared helper.
Test Plan
Added an LSP interaction test for override completions.