Repository navigation
fix Inlay hint for attribute does not match inherited annotation #846 - #3021
asukaminato0721 wants to merge 2 commits into
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.
| #[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).
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cd97ccc to
822048b
Compare
822048b to
a7d8246
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
stroxler
left a comment
stroxler
left a comment
There was a problem hiding this comment.
Thanks! This seems like a promising direction. A few suggestions of things to improve before we merge:
-
Respect comprehension scopes. Resolve class-field ownership from bindings, or stop lookup at comprehension boundaries. With
C.x: strand[x for x in (1, 2)], the comprehension target incorrectly hovers asstrinstead ofLiteral[1, 2]. Add a regression test for this shadowing case. -
Preserve
Selfat declarations. Use the resolved declaration field type rather than class-access lookup. Hoveringx: Self | None = NoneinsideCcurrently showsC | None, losing its subclass-relative meaning. Add hover and computed-type coverage. -
Wire the fix into inlay hints. Share resolved field lookup with the inlay-hint path and test it directly using a nonliteral RHS. An override inheriting
ClassVar[tuple[str, ...]]still receives atuple[str]hint; the existing test covers hover only.
|
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