[ty] Check method definitions on subclasses for Liskov violations #21436
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
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-11-23 17:50:26.942738321 +0000
+++ new-output.txt 2025-11-23 17:50:30.674772221 +0000
@@ -1,4 +1,4 @@
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/17bc55d/src/function/execute.rs:469:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(19ea3)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/17bc55d/src/function/execute.rs:469:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(1a6a3)): execute: too many cycle iterations`
_directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
_directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
_directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__`
@@ -868,6 +868,7 @@
qualifiers_final_annotation.py:166:1: error[invalid-assignment] Reassignment of `Final` symbol `TEN` is not allowed: Reassignment of `Final` symbol
qualifiers_final_decorator.py:21:16: error[subclass-of-final-class] Class `Derived1` cannot inherit from final class `Base1`
qualifiers_final_decorator.py:89:9: error[invalid-overload] `@final` decorator should be applied only to the overload implementation
+qualifiers_final_decorator.py:118:9: error[invalid-method-override] Invalid override of method `method`: Definition is incompatible with `Base5_2.method`
specialtypes_any.py:86:1: error[type-assertion-failure] Type `int` does not match asserted type `int | @Todo(instance attribute on class with dynamic base)`
specialtypes_never.py:19:22: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Never`
specialtypes_never.py:32:12: error[invalid-return-type] Return type does not match returned value: expected `int`, found `Literal["whatever works"]`
@@ -1001,5 +1002,5 @@
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Unknown key "title" for TypedDict `Movie`: Unknown key "title"
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
-Found 1003 diagnostics
+Found 1004 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details.
|
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
2 times, most recently
from
ec973f2 to
cc3b9a8
Compare
CodSpeed Performance ReportMerging #21436 will degrade performances by 6.73%Comparing Summary
Benchmarks breakdown
Footnotes
|
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
cc3b9a8 to
9f9ac36
Compare
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-method-override |
3,484 | 0 | 0 |
unused-ignore-comment |
0 | 499 | 0 |
| Total | 3,484 | 499 | 0 |
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
41911e1 to
3c4d3fc
Compare
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
65bdb0d to
ebbedb7
Compare
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
5475da6 to
be95e57
Compare
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
be95e57 to
da0cdfc
Compare
carljm
reviewed
Nov 18, 2025
| from typing import Literal | ||
|
|
||
| class Date: | ||
| # error: [invalid-method-override] |
Contributor
There was a problem hiding this comment.
Not necessary to decide in this PR, but I wonder if we should special-case ignoring object.__setattr__ for Liskov purposes (and separately implement validation of a correct __setattr__ signature). TBH I'm not even sure why typeshed has object.__setattr__, and I don't think there is any soundness need for subclasses of object to respect Liskov compatibility with the object.__setattr__ definition in typeshed.
Member
Author
There was a problem hiding this comment.
Yes, I think you're right.
I was also wondering about special-case ignoring object.__hash__. It's obviously common to define unhashable classes in Python, and it's often the correct thing to do to add __hash__ = None if your class is mutable. Having a type checker yell at you every time you have to do that has never felt helpful to me.
I can propose them both (either together or separately) as followups. I'd rather keep them out of this PR, since either special case (while, IMO, well-motivated) would be a deviation from the behaviour of other type checkers.
| class::CodeGeneratorKind, | ||
| context::InferContext, | ||
| diagnostic::report_invalid_method_override, | ||
| ide_support::{MemberWithDefinition, all_declarations_and_bindings}, |
Contributor
There was a problem hiding this comment.
I think this PR should idealy move this stuff out of ide_support? It's clearly no longer just for IDE support.
Member
Author
There was a problem hiding this comment.
I do agree, but I'm a bit loath to do that in this PR because it'll dramatically increase the merge conflicts (and because this PR is already a big PR). I'd prefer to do that as an immediate followup, if that's okay
|
|
||
| for supercls in class.iter_mro(db).skip(1).filter_map(ClassBase::into_class) { | ||
| let Place::Defined(type_on_supercls, _, _) = | ||
| Type::instance(db, supercls).member(db, &member.name).place |
Contributor
There was a problem hiding this comment.
Is there an own_instance_member method we could use here instead, to short-circuit this "find and then skip if not in our own scope" dance?
It feels odd (and potentially inefficient) that if we have a long inheritance chain of length N, and a method defined at the top of it (say on object) and then overridden at the leaf, we will do O(N^2) mro traversal, as we (wastefully) traverse the full MRO of every intervening type in the inheritance chain, only to find the same super-class method on object every time, and then skip it because it's not defined in our own scope.
Or alternatively, if we can't use an own_instance_member, what about checking the place table before doing the member lookup?
The break below gives a false sense of efficiency. If the method doesn't exist anywhere in the MRO, we will only traverse the MRO once, which is good -- but the same would be true if we did the "outer" loop once and did own_instance_member for each type in the MRO. When the method does exist somewhere up the MRO, this way does a lot more traversal.
Member
Author
There was a problem hiding this comment.
Is there an
own_instance_membermethod we could use here instead, to short-circuit this "find and then skip if not in our own scope" dance?
There is a method by that name, yes. But it emulates looking up an attribute directly in self.__dict__ (it does not include class attributes defined directly in the class body). We'd have to call own_class_member as well as own_instance_member to do this correctly.
But looking at this again, this would have the advantage that it would include synthesized members, since own_class_member calls own_synthesized_member. I think we're currently incorrectly skipping over synthesized members (from dataclasses/namedtuples/etc.) on superclasses, because they don't appear in the symbol table for those superclasses.
So your suggestion probably brings correctness benefits as well as performance benefits. Thanks!
Contributor
There was a problem hiding this comment.
Type::member is Salsa-cached, but I think that doesn't help here as much as we'd hope? The internal own-member stuff that is done for each class in the MRO is not cached, so we are still doing the N^2 walk -- but we cache the final lookup result for each class in the MRO. Which is useful if that attribute is later looked up on that superclass, but still wasted if not.
Member
Author
There was a problem hiding this comment.
Okay, I've tried to optimise this a bit. I'm a bit reticent to call own_class_member directly, because (at least for now) we only care about the type accessed on instances of the class. To get the class-member-as-viewed-by-instances, you'd really need to call invoke_descriptor_protocol (which invokes own_class_member) rather than own_class_member directly, and calling Type::invoke_descriptor_protocol undermines any optimisation effect we'd get from avoiding calling Type::member() on the nominal-instance type (it'll recurse up the class hierarchy until it finds a class that has the attribute).
For now I've moved the place_table check above the Type::member call, so that we check whether it's defined in the class body before we do the Type::member call. But that won't work in the future when we extend Liskov checking to also check implicit instance attributes (etc.)
Member
Author
There was a problem hiding this comment.
The good news is that the perf regression on the latest version of this PR is indeed much reduced -- now at <7%!
Member
It might cause the typevar ordering to be different, which would cause the internal BDD structure to change. I'm seeing this on another PR, too, so I will try to push up a separate fix for it. |
Member
#21524 fixes the macos CI failures I was getting on #21414. I was getting a slightly different test failure than you, but my hope is that they had the same underlying cause. You can cherry-pick that commit if you want to test soon, or wait till that lands on |
dcreager
added a commit
that referenced
this pull request
Nov 19, 2025
We're seeing flaky test failures on macos, which seems to be caused by different Salsa ID orderings on the different platforms. Constraint set BDDs order their internal nodes based on the Salsa IDs of the interned typevar structs, and we had some code that depended on variable ordering in an unexpected way. This patch definitely fixes the macos test failure on #21414, and hopefully fixes it on #21436, too.
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
c4b5edb to
69ad201
Compare
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
69ad201 to
7f7169a
Compare
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
e597eba to
7d57fee
Compare
carljm
approved these changes
Nov 21, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Nice!
There were some comments from previous review accidentlly left not addressed; I just unresolved those prior comments rather than adding new ones.
Comment on lines
+110
to
+112
| Some(CodeGeneratorKind::TypedDict) => { | ||
| MethodKind::Synthesized(SynthesizedMethodKind::TypedDict) | ||
| } |
Contributor
There was a problem hiding this comment.
It's not valid to define methods on a TypedDict (or subclass of a TypedDict) at all -- we should emit a diagnostic on that (separate PR). Seems like it then shouldn't be possible to hit this case (until we are checking non-methods on the subclass).
Member
|
Ohh, I would have needed this feature today when working on ty_benchmark where claude messed up the overrides... |
Co-authored-by: Carl Meyer <carl@astral.sh>
…into alex/basic-liskov
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
2 times, most recently
from
fb5edc9 to
947a03a
Compare
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
947a03a to
1d6c816
Compare
dcreager
added a commit
that referenced
this pull request
Nov 24, 2025
* origin/main: (27 commits) [ty] Add hint about resolved Python version when a user attempts to import a member added on a newer version (#21615) Use release commit for actions/checkout (#21610) [ty] Add failing mdtest for known `Protocol` panic (#21594) [`parser`] Fix panic when parsing IPython escape command expressions (#21480) Fix cargo shear in CI (#21609) Update actions/checkout digest to c2d88d3 (#21601) Update dependency ruff to v0.14.6 (#21603) Update astral-sh/setup-uv action to v7.1.4 (#21602) Update Rust crate clap to v4.5.53 (#21604) Update taiki-e/install-action action to v2.62.56 (#21608) Update Rust crate hashbrown to v0.16.1 (#21605) Update Rust crate indexmap to v2.12.1 (#21606) Update Rust crate syn to v2.0.111 (#21607) [ty] Check method definitions on subclasses for Liskov violations (#21436) [ty] Fix panic for unclosed string literal in type annotation position (#21592) [ty] Fix rendering of unused suppression diagnostic (#21580) [ty] Improve lsp handling of hover/goto on imports (#21572) [ty] Improve diagnostics when a submodule is not available as an attribute on a module-literal type (#21561) [ty] Improve concise diagnostics for invalid exceptions when a user catches a tuple of objects (#21578) [ty] upgrade salsa (#21575) ...
dcreager
added a commit
that referenced
this pull request
Nov 24, 2025
…d-typevar * origin/main: (30 commits) [ty] Double click to insert inlay hint (#21600) [ty] Switch the error code from `unresolved-attribute` to `possibly-missing-attribute` for submodules that may not be available (#21618) [ty] Substitute for `typing.Self` when checking protocol members (#21569) [ty] Don't suggest things that aren't subclasses of `BaseException` after `raise` [ty] Add hint about resolved Python version when a user attempts to import a member added on a newer version (#21615) Use release commit for actions/checkout (#21610) [ty] Add failing mdtest for known `Protocol` panic (#21594) [`parser`] Fix panic when parsing IPython escape command expressions (#21480) Fix cargo shear in CI (#21609) Update actions/checkout digest to c2d88d3 (#21601) Update dependency ruff to v0.14.6 (#21603) Update astral-sh/setup-uv action to v7.1.4 (#21602) Update Rust crate clap to v4.5.53 (#21604) Update taiki-e/install-action action to v2.62.56 (#21608) Update Rust crate hashbrown to v0.16.1 (#21605) Update Rust crate indexmap to v2.12.1 (#21606) Update Rust crate syn to v2.0.111 (#21607) [ty] Check method definitions on subclasses for Liskov violations (#21436) [ty] Fix panic for unclosed string literal in type annotation position (#21592) [ty] Fix rendering of unused suppression diagnostic (#21580) ...
AlexWaygood
added a commit
that referenced
this pull request
Nov 24, 2025
#21598) ## Summary Building on #21436. There's nothing conceptually more complicated about this, it just requires its own set of tests and its own subdiagnostic hint. I also uncovered another inconsistency between mypy/pyright/pyrefly, which is fun. In this case, I suggest we go with pyright's behaviour. ## Test Plan mdtests/snapshots
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
A basic implementation of Liskov assignability.
Currently we only check methods on subclasses: not attributes or properties. These should also obviously be checked as well, but they're deferred for now. Similarly, even for methods, there is much that is currently deferred: no checks are done for staticmethods or classmethods for now, and we only check definitions inferred as
Type::FunctionLiterals.The main thing missing from the checks implemented here is that the diagnostics don't do a good job of explaining why a particular override violates the Liskov Substitution Principle. Other type checkers do a great job at this: they'll tell you that the return annotation is invalid, or that you've changed a parameter name, etc. All of that is very difficult for us to implement until something like #19580 is implemented to propagate up information about why an assignability or subtyping check failed. I think we'll definitely need something like that eventually, but I'm loath to do that until the new constraint solver has landed, since it'll touch the same area of code and cause a lot of merge conflicts!
Another thing to note (which looks like it comes up a lot in the ecosystem) is that other type checkers are less strict than us in some respects. For example, pyright ignores Liskov issues stemming from a parameter on a method override having the wrong name if the method is a dunder method. We could consider pushing "parameter renaming issues" into a separate (disabled-by-default) error code, but once again this is very difficult to do without something like #19580.
Test Plan
Mdtests and snapshots