fix Inlay hint for attribute does not match inherited annotation #846#3021
fix Inlay hint for attribute does not match inherited annotation #846#3021asukaminato0721 wants to merge 2 commits intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to address Issue #846 (mismatched type display for inherited class attributes) by preferring the solver-resolved class field type when retrieving a type at an attribute definition site, and adds a regression test around the hover output.
Changes:
- Added a hover regression test for an inherited
ClassVaroverridden in a subclass. - Updated
Transaction::get_type_atto, for Store-context identifiers, prefer a class-attribute lookup (get_class_attribute) when the identifier is defined inside a class body. - Introduced
class_attribute_type_at_definitionhelper to locate the enclosing class and resolve the field type via the solver.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pyrefly/lib/test/lsp/hover_type.rs |
Adds a regression test asserting hover type uses the inherited resolved field type for a subclass attribute assignment. |
pyrefly/lib/state/lsp.rs |
Adjusts hover/provide-type resolution on Store-context identifiers to prefer solver-resolved class attribute types at class attribute definition sites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for node in Ast::locate_node(&ast, identifier.range.start()) | ||
| .into_iter() | ||
| .skip(1) | ||
| { | ||
| match node { | ||
| AnyNodeRef::StmtFunctionDef(_) | AnyNodeRef::ExprLambda(_) => return None, | ||
| AnyNodeRef::StmtClassDef(class_def) => { | ||
| enclosing_class = Some(class_def.name.clone()); | ||
| break; | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
class_attribute_type_at_definition only treats function defs and lambdas as scope boundaries. Comprehensions/generator expressions also introduce their own scope, so a Store-context name inside a comprehension in a class body could be misinterpreted as a class attribute and incorrectly resolve to an inherited field type. Consider bailing out when Ast::locate_node includes AnyNodeRef::ExprListComp|ExprSetComp|ExprDictComp|ExprGenerator (and/or AnyNodeRef::Comprehension) similar to how other code treats these as disallowed scopes.
| let mut ty = if matches!(expr_context, ExprContext::Store) { | ||
| self.class_attribute_type_at_definition(handle, &id) | ||
| .or_else(|| self.get_type(handle, &key))? | ||
| } else { | ||
| self.get_type(handle, &key)? | ||
| }; |
There was a problem hiding this comment.
This change only affects Transaction::get_type_at (hover/provide-type). Inlay hints are generated via Transaction::inlay_hints and currently call get_type(handle, key) for Key::Definition, so the original Issue #846 symptom may still reproduce for variable-type inlay hints. If the goal is to fix inlay hints, the same “prefer resolved class field type” logic likely needs to be applied in the inlay hint path as well.
| #[test] | ||
| fn inherited_classvar_hover_prefers_resolved_field_type() { | ||
| let code = r#" | ||
| from typing import ClassVar | ||
|
|
||
| class A: | ||
| x: ClassVar[tuple[str, ...]] = ("A",) | ||
|
|
||
| class B(A): | ||
| x = ("B",) | ||
| # ^ | ||
|
|
||
| reveal = B.x | ||
| # ^ | ||
| "#; | ||
| let report = get_batched_lsp_operations_report(&[("main", code)], get_test_report); | ||
| assert_eq!( | ||
| r#" | ||
| # main.py | ||
| 8 | x = ("B",) | ||
| ^ | ||
| Hover Result: `tuple[str, ...]` | ||
|
|
||
| 11 | reveal = B.x | ||
| ^ | ||
| Hover Result: `tuple[str, ...]` | ||
| "# | ||
| .trim(), | ||
| report.trim(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This new test exercises hover behavior, but Issue #846 is about variable-type inlay hints showing an overly specific inferred type on inherited attributes. Since the inlay hint pipeline doesn’t use hover’s get_type_at path, it would be good to add/adjust a test in the inlay hint test suite to ensure the reported inlay hint output is fixed (and to prevent regressions).
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #846
where class-body store hovers now resolve through the computed class field type instead of the raw assignment binding, and that resolution is wired into get_type_at.
Test Plan
add test