[ty] Prefer declared base class attribute over inferred attribute on subclass #20764
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unsupported-operator |
0 | 0 | 514 |
possibly-missing-attribute |
13 | 10 | 11 |
invalid-argument-type |
5 | 11 | 2 |
unresolved-attribute |
9 | 0 | 0 |
invalid-assignment |
2 | 0 | 1 |
non-subscriptable |
2 | 0 | 0 |
not-iterable |
2 | 0 | 0 |
| Total | 33 | 21 | 528 |
sharkdp
force-pushed
the
david/fix-1067
branch
from
79fbd20 to
961c77c
Compare
sharkdp
force-pushed
the
david/fix-1067
branch
from
961c77c to
8c87e6f
Compare
Contributor
|
sharkdp
force-pushed
the
david/fix-1067
branch
from
aea57b1 to
ab3f52d
Compare
sharkdp
commented
Oct 8, 2025
| scope: ScopeId<'db>, | ||
| name: &str, | ||
| ) -> PlaceAndQualifiers<'db> { | ||
| pub(crate) fn class_member<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Member<'db> { |
Contributor
Author
There was a problem hiding this comment.
We could think about moving this to the newly created member module...
sharkdp
force-pushed
the
david/fix-1067
branch
from
ab3f52d to
f51819b
Compare
sharkdp
commented
Oct 8, 2025
| /// about the type, type qualifiers, boundness/declaredness, and additional | ||
| /// metadata (e.g. whether or not the member was declared) | ||
| #[derive(Debug, Clone, PartialEq, Eq, salsa::Update, get_size2::GetSize)] | ||
| pub(crate) struct Member<'db> { |
Contributor
Author
There was a problem hiding this comment.
I don't love the fact that we now have this Member(PlaceAndQualifiers(Place(Type))) onion structure, but each single layer has its own dedicated purpose and is used in a lot of places.
I considered adding is_declared to PlaceAndQualifiers somehow, as this flag might be relevant in other areas as well. This would be a huge refactoring, but more importantly, I think that we probably want to add new flags to Member soon (or use bitflags) in order to convey even more metadata (e.g. is this an implicit instance attribute? is the attribute a data/non-data descriptor?).
Contributor
There was a problem hiding this comment.
This feels like it intersects with discussions we've had a long time ago already (and several TODO comments we have) about how the Boundness part of Place currently conflates both "boundness" and "declaredness" in a way that isn't always consistent. I think we always vaguely had the intention of cleaning that up, but it never became a priority. it seems like this new flag is providing the same metadata that would be provided by that refactor, but at a new outer layer, mostly in order to avoid the need for a big refactor of Place?
Contributor
Author
There was a problem hiding this comment.
I opened astral-sh/ty#1341 to track this.
sharkdp
force-pushed
the
david/fix-1067
branch
from
f51819b to
05df818
Compare
sharkdp
commented
Oct 8, 2025
| if boundness == Boundness::Bound { | ||
| if union.is_empty() { | ||
| // Short-circuit, no need to allocate inside the union builder | ||
| if is_declared { |
Contributor
Author
There was a problem hiding this comment.
This (and the few lines below) is the key change. Everything else is just refactoring (and recording whether or not a particular Place is declared or inferred).
sharkdp
commented
Oct 8, 2025
Member
That's a weird diagnostic. Shouldn't |
Contributor
Author
I stumbled over the exact same thing and assumed it was just me. But we should definitely improve the diagnostic message here*. The problem is not the type of the object that we're trying to assign. The problem is in the type of the object that we're acessing the attribute on (
* ideally by adding a subdiagnostic that explains the underlying reason why the assignment to the attribute on that union type has failed (because the assignment to that attribute on one particular element failed). |
Member
|
Ahhh, I see. Thanks! In that case, the type of the object we're trying to assign to the |
Contributor
Author
That would be one way to improve the diagnostic, yes. The other thing that I proposed would also split up that union type and add to yours: "… because it can not be set on union element |
Member
|
yes -- both sound good ;) but not for this PR, of course. |
carljm
approved these changes
Oct 10, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks good, thank you! I definitely think we should go ahead with this, as the impact is all positive. The Member struct seems like it could still make sense for other things we want to add to it, though it does seem to me like ideally bound-vs-declared would be recorded with better precision all the way down in Place instead.
| /// about the type, type qualifiers, boundness/declaredness, and additional | ||
| /// metadata (e.g. whether or not the member was declared) | ||
| #[derive(Debug, Clone, PartialEq, Eq, salsa::Update, get_size2::GetSize)] | ||
| pub(crate) struct Member<'db> { |
Contributor
There was a problem hiding this comment.
This feels like it intersects with discussions we've had a long time ago already (and several TODO comments we have) about how the Boundness part of Place currently conflates both "boundness" and "declaredness" in a way that isn't always consistent. I think we always vaguely had the intention of cleaning that up, but it never became a priority. it seems like this new flag is providing the same metadata that would be provided by that refactor, but at a new outer layer, mostly in order to avoid the need for a big refactor of Place?
dcreager
added a commit
that referenced
this pull request
Oct 13, 2025
…tity * origin/main: (24 commits) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) Update Rust crate libc to v0.2.177 (#20832) Update Rust crate memchr to v2.7.6 (#20834) ...
dcreager
added a commit
that referenced
this pull request
Oct 13, 2025
* main: (25 commits) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) Update Rust crate libc to v0.2.177 (#20832) ...
dcreager
added a commit
that referenced
this pull request
Oct 14, 2025
…rable * origin/main: (26 commits) [ty] Add separate type for typevar "identity" (#20813) [ty] Diagnostic for generic classes that reference typevars in enclosing scope (#20822) Update Python compatibility from 3.13 to 3.14 in README.md (#20852) [syntax-errors]: break outside loop F701 (#20556) [ty] Treat `Callable`s as bound-method descriptors in special cases (#20802) [ty] Do not bind self to non-positional parameters (#20850) Fix syntax error false positives on parenthesized context managers (#20846) [ty] Remove 'pre-release software' warning (#20817) Render unsupported syntax errors in formatter tests (#20777) [ty] Treat functions, methods, and dynamic types as function-like `Callable`s (#20842) [ty] Move logic for `super()` inference to a new `types::bound_super` submodule (#20840) [ty] Fix false-positive diagnostics on `super()` calls (#20814) [ty] Move `class_member` to `member` module (#20837) [`ruff`] Use DiagnosticTag for more flake8 and numpy rules (#20758) [ty] Prefer declared base class attribute over inferred attribute on subclass (#20764) [ty] Log files that are slow to type check (#20836) Update cargo-bins/cargo-binstall action to v1.15.7 (#20827) Update CodSpeedHQ/action action to v4.1.1 (#20828) Update Rust crate pyproject-toml to v0.13.7 (#20835) Update Rust crate anstream to v0.6.21 (#20829) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When accessing an (instance) attribute on a given class, we were previously traversing its MRO, and building a union of types (if the attribute was available on multiple classes in the MRO) until we found a definitely bound symbol. The idea was that possibly unbound symbols in a subclass might only partially shadow the underlying base class attribute.
This behavior was problematic for two reasons:
self.x = None), we would have stopped iterating, even if there might be ax: str | Nonedeclaration in a base class (the bug reported in Prefer declared attribute on base class over inferred attribute on subclass ty#1067).self.x = 1in methodSub.foo), we might stop looking and miss another implicit instance attribute assignment in a base class method (e.g.self.x = 2in methodBase.bar).With this fix, we still iterate the MRO of the class, but we only stop iterating if we find a definitely declared symbol. In this case, we only return the declared attribute type. Otherwise, we keep building a union of inferred attribute types.
The implementation here seemed to be the easiest fix for astral-sh/ty#1067 that also kept the ecosystem impact low (the changes that I see all look correct). However, as the Markdown tests show, there are other things to fix in this area. For example, we should do a similar thing for class attributes. This is more involved, though (affects many different areas and probably involves a change to our descriptor protocol implementation), so I'd like to postpone this to a follow-up.
closes astral-sh/ty#1067
Test Plan
Updated Markdown tests, including a regression test for astral-sh/ty#1067.