[ty] Check method definitions on subclasses for Liskov violations #21436
+1,719
−185
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-18 19:14:13.775177666 +0000
+++ new-output.txt 2025-11-18 19:14:17.566209433 +0000
@@ -1,4 +1,4 @@
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a885bb4/src/function/execute.rs:465: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/a885bb4/src/function/execute.rs:465: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__`
|
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 10.11%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,149 | 0 | 0 |
unused-ignore-comment |
0 | 439 | 0 |
| Total | 3,149 | 439 | 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
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
67640d1 to
cd2d59c
Compare
AlexWaygood
commented
Nov 17, 2025
Comment on lines
+551
to
+554
| if self.db().should_check_file(self.file()) { | ||
| self.check_class_definitions(); | ||
| self.check_overloaded_functions(node); | ||
| } |
Member
Author
There was a problem hiding this comment.
This is an attempt to reduce the performance hit from this PR. A few regressions went down a few percentage points as a result of this change, but I wouldn't say this made a huge impact on perf -- it seemed to improve things fairly significantly in terms of memory usage, however.
AlexWaygood
commented
Nov 17, 2025
Comment on lines
+3797
to
+3836
| pub(super) fn components_excluding_self(&self) -> Vec<String> { | ||
| let body_scope = self.class.body_scope(self.db); | ||
| let file = body_scope.file(self.db); | ||
| let module_ast = parsed_module(self.db, file).load(self.db); | ||
| let index = semantic_index(self.db, file); | ||
| let file_scope_id = body_scope.file_scope_id(self.db); | ||
|
|
||
| let mut name_parts = vec![]; | ||
|
|
||
| // Skips itself | ||
| for (_, ancestor_scope) in index.ancestor_scopes(file_scope_id).skip(1) { | ||
| let node = ancestor_scope.node(); | ||
|
|
||
| match ancestor_scope.kind() { | ||
| ScopeKind::Class => { | ||
| if let Some(class_def) = node.as_class() { | ||
| name_parts.push(class_def.node(&module_ast).name.as_str().to_string()); | ||
| } | ||
| } | ||
| ScopeKind::Function => { | ||
| if let Some(function_def) = node.as_function() { | ||
| name_parts.push(format!( | ||
| "<locals of function '{}'>", | ||
| function_def.node(&module_ast).name.as_str() | ||
| )); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| if let Some(module) = file_to_module(self.db, file) { | ||
| let module_name = module.name(self.db); | ||
| name_parts.push(module_name.as_str().to_string()); | ||
| } | ||
|
|
||
| name_parts.reverse(); | ||
| name_parts | ||
| } | ||
| } |
Member
Author
There was a problem hiding this comment.
(this method used to be called qualified_name_components, it used to be a method on ClassLiteral, and it used to be in display.rs)
Comment on lines
-279
to
-285
| /// Returns the components of the qualified name of this class, excluding this class itself. | ||
| /// | ||
| /// For example, calling this method on a class `C` in the module `a.b` would return | ||
| /// `["a", "b"]`. Calling this method on a class `D` inside the namespace of a method | ||
| /// `m` inside the namespace of a class `C` in the module `a.b` would return | ||
| /// `["a", "b", "C", "<locals of function 'm'>"]`. | ||
| fn qualified_name_components(self, db: &'db dyn Db) -> Vec<String> { |
Member
Author
There was a problem hiding this comment.
(moved to class.rs for better reusability)
AlexWaygood
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
Contributor
|
Not sure what's up with the ConstraintSet mdtests failing only on MacOS? |
Member
Author
I have no idea. I haven't touched any code to do with constraint sets in this PR, nor have I added any MacOS-specific code! The test also passes locally for me on my MacBook, but seems to be failing very persistently in CI on this PR. @dcreager, any idea why this PR might be causing the test to start failing here...? The only significant way I think that this PR could impact other areas of ty is that we have to fetch the type for all attributes defined in the body of every class we check (so we're doing a lot more types in every file we check). |
AlexWaygood
force-pushed
the
alex/basic-liskov
branch
from
1ce83df to
e4b2174
Compare
Member
|
Do we iterate over hash maps or rely on salsa ids for ordering? |
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.
Comment on lines
+139
to
+143
| If a child class's method definition is Liskov-compatible with the method definition on its parent | ||
| class, Liskov compatibility must also nonetheless be checked with respect to the method definition | ||
| on its grandparent class. This is because type checkers will treat the child class as a subtype of | ||
| the grandparent class just as much as they treat it as a subtype of the parent class, so | ||
| substitutability with respect to the grandparent class is just as important: |
Contributor
There was a problem hiding this comment.
Not that it needs to be specifically mentioned here, but I think the strongest case for this is gradual types, because it means you can have cases where each individual parent-child relationship passes Liskov checking, but the grandparent-child relationship does not:
class Grandparent:
x: int
class Parent(Grandparent):
x: Any
class Child(Parent):
x: str| return; | ||
| } | ||
|
|
||
| let class_specialized = class.identity_specialization(db); |
Contributor
There was a problem hiding this comment.
I think it would be good to add some tests for method overrides involving generics. The behavior should fall out from callable subtyping, but it wouldn't hurt to have some such tests for overrides specifically.
| 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.
|
|
||
| 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.
Co-authored-by: Carl Meyer <[email protected]>
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.
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