[ty] Improve @override, @final and Liskov checks in cases where there are multiple reachable definitions#21767
Merged
AlexWaygood merged 5 commits intomainfrom Dec 3, 2025
Merged
Conversation
AlexWaygood
force-pushed
the
alex/first-declaration
branch
from
972c956 to
a6650e9
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
AlexWaygood
changed the title
@override, @final and Liskov checks in cases where there are multiple reachable definitions
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unused-ignore-comment |
1 | 11 | 0 |
invalid-method-override |
9 | 0 | 0 |
invalid-argument-type |
2 | 0 | 0 |
| Total | 12 | 11 | 0 |
Member
Author
|
Okay, the weird new Liskov violations on mypy's vendored version of typeshed are because e.g. ty thinks that the |
AlexWaygood
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
commented
Dec 3, 2025
Member
Author
There was a problem hiding this comment.
@BurntSushi -- both of the tests being changed in this file have FIXME comments above them. But I'm not sure if the changes this PR makes to the tests fixes the problems, or makes the problems worse 😆 I'm not totally sure I understand what these tests are meant to be asserting
Member
Author
There was a problem hiding this comment.
LMK if you consider this a regression and I can fix it in a followup. It shouldn't be too hard to track whether a member has multiple definitions and propagate that information upwards, if that's what the autocomplete machinery wants to know
Member
There was a problem hiding this comment.
I think the idea here was just trying to test what happens when we try to insert an import for a symbol that is already imported, but whose imports are conditional. I think in this case we don't want to add any new imports. But we weren't doing that before either. So I think this is fine for now.
carljm
approved these changes
Dec 3, 2025
| def method3(self) -> None: ... # error: [override-of-final-method] | ||
| def method4(self) -> None: ... # error: [override-of-final-method] | ||
|
|
||
| # TODO: we should emit Liskov violations here too: |
Contributor
There was a problem hiding this comment.
It seems like we do? At least at the first definition...
Member
Author
There was a problem hiding this comment.
no, we emit a diagnostic stating that an @final method has been overridden (which is not to my mind a Liskov violation), but we are silent about the fact that the type has also been incompatibly overridden (which is, to my mind, a Liskov violation)
… multiple reachable definitions
AlexWaygood
force-pushed
the
alex/first-declaration
branch
from
87d8593 to
d2e8da5
Compare
dcreager
added a commit
that referenced
this pull request
Dec 3, 2025
* origin/main: [ty] Improve `@override`, `@final` and Liskov checks in cases where there are multiple reachable definitions (#21767) [ty] Extend `invalid-explicit-override` to also cover properties decorated with `@override` that do not override anything (#21756) [ty] Enable LRU collection for parsed module (#21749) [ty] Support typevar-specialized dynamic types in generic type aliases (#21730) Add token based `parenthesized_ranges` implementation (#21738) [ty] Default-specialization of generic type aliases (#21765) [ty] Suppress false positives when `dataclasses.dataclass(...)(cls)` is called imperatively (#21729) [syntax-error] Default type parameter followed by non-default type parameter (#21657) new module for parsing ranged suppressions (#21441) [ty] `type[T]` is assignable to an inferable typevar (#21766) Fix syntax error false positives for `await` outside functions (#21763) [ty] Improve diagnostics for unsupported comparison operations (#21737) Move `Token`, `TokenKind` and `Tokens` to `ruff-python-ast` (#21760) [ty] Don't confuse multiple occurrences of `typing.Self` when binding bound methods (#21754) Use our org-wide Renovate preset (#21759) Delete `my-script.py` (#21751) [ty] Move `all_members`, and related types/routines, out of `ide_support.rs` (#21695)
dcreager
added a commit
that referenced
this pull request
Dec 3, 2025
* origin/main: [ty] Reachability constraints: minor documentation fixes (#21774) [ty] Fix non-determinism in `ConstraintSet.specialize_constrained` (#21744) [ty] Improve `@override`, `@final` and Liskov checks in cases where there are multiple reachable definitions (#21767) [ty] Extend `invalid-explicit-override` to also cover properties decorated with `@override` that do not override anything (#21756) [ty] Enable LRU collection for parsed module (#21749) [ty] Support typevar-specialized dynamic types in generic type aliases (#21730) Add token based `parenthesized_ranges` implementation (#21738) [ty] Default-specialization of generic type aliases (#21765) [ty] Suppress false positives when `dataclasses.dataclass(...)(cls)` is called imperatively (#21729) [syntax-error] Default type parameter followed by non-default type parameter (#21657)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
(Stacked on top of #21756; review that first.)
Fixes astral-sh/ty#1677. Currently we only keeping track of a
Definitionif there was only one single reachable definition for aPlace. Now, we always keep track of the firstDefinition, even if there are multiple reachable definitions for thatPlace.Test Plan
mdtests