[ty] Treat Hashable, and similar protocols, equivalently to object for subtyping/assignability#20284
Merged
AlexWaygood merged 8 commits intomainfrom Sep 10, 2025
Merged
[ty] Treat Hashable, and similar protocols, equivalently to object for subtyping/assignability#20284AlexWaygood merged 8 commits intomainfrom
Hashable, and similar protocols, equivalently to object for subtyping/assignability#20284AlexWaygood merged 8 commits intomainfrom
Conversation
Contributor
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-09-10 10:36:03.633952560 +0000
+++ new-output.txt 2025-09-10 10:36:06.644971417 +0000
@@ -1,6 +1,6 @@
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a3ffa22/src/function/execute.rs:228:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(b416)): execute: too many cycle iterations`
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a3ffa22/src/function/execute.rs:228:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(1267a)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a3ffa22/src/function/execute.rs:228:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(b816)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a3ffa22/src/function/execute.rs:228:25 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(12a7a)): 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__` |
Contributor
|
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
1 | 378 | 2 |
invalid-assignment |
0 | 12 | 0 |
no-matching-overload |
0 | 7 | 0 |
unused-ignore-comment |
4 | 0 | 0 |
possibly-unbound-attribute |
0 | 3 | 0 |
type-assertion-failure |
0 | 3 | 0 |
unresolved-attribute |
3 | 0 | 0 |
not-iterable |
0 | 2 | 0 |
unsupported-operator |
0 | 2 | 0 |
invalid-return-type |
0 | 0 | 1 |
non-subscriptable |
0 | 1 | 0 |
redundant-cast |
1 | 0 | 0 |
| Total | 9 | 408 | 3 |
CodSpeed Instrumentation Performance ReportMerging #20284 will improve performances by 4.42%Comparing Summary
Benchmarks breakdown
|
CodSpeed WallTime Performance ReportMerging #20284 will not alter performanceComparing Summary
|
This comment was marked as resolved.
This comment was marked as resolved.
AlexWaygood
commented
Sep 7, 2025
This comment was marked as resolved.
This comment was marked as resolved.
fc9d21a to
4f913aa
Compare
Member
Author
|
Applying this diff locally buys back basically all the lost performance on the incremental benchmark: diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs
index 46aab242b2..fdaebb0bf9 100644
--- a/crates/ty_python_semantic/src/types.rs
+++ b/crates/ty_python_semantic/src/types.rs
@@ -1350,21 +1350,19 @@ impl<'db> Type<'db> {
C::always_satisfiable(db)
}
- // Any structural types that are "supertypes of `object`"
- // are in fact equivalent to `object`, and should therefore
- // be treated equivalently to `object` when it comes to assignability/subtyping
- (_, Type::ProtocolInstance(target))
- if Type::object(db)
- .satisfies_protocol(db, target, relation, visitor)
- .is_always_satisfied(db) =>
- {
- C::always_satisfiable(db)
- }
-
// `Never` is the bottom type, the empty set.
// It is a subtype of all other types.
(Type::Never, _) => C::always_satisfiable(db),
+ // `Any` is only a subtype of `object` -- but if a protocol's interface can be satisfied by `object`,
+ // then `object` is equivalent to the protocol, meaning that `Any` can also be a subtype of that
+ // equivalent-to-object protocol.
+ (Type::Dynamic(_), Type::ProtocolInstance(proto)) => {
+ C::from_bool(db, relation.is_assignability()).or(db, || {
+ Type::object(db).satisfies_protocol(db, proto, relation, visitor)
+ })
+ }
+
// Dynamic is only a subtype of `object` and only a supertype of `Never`; both were
// handled above. It's always assignable, though.
(Type::Dynamic(_), _) | (_, Type::Dynamic(_)) => {
diff --git a/crates/ty_python_semantic/src/types/instance.rs b/crates/ty_python_semantic/src/types/instance.rs
index 8714c824aa..c278b181d9 100644
--- a/crates/ty_python_semantic/src/types/instance.rs
+++ b/crates/ty_python_semantic/src/types/instance.rs
@@ -114,6 +114,13 @@ impl<'db> Type<'db> {
.when_all(db, |member| {
member.is_satisfied_by(db, self, relation, visitor)
})
+ .or(db, || {
+ let object = Type::object(db);
+ if self == object {
+ return C::unsatisfiable(db);
+ }
+ object.satisfies_protocol(db, protocol, relation, visitor)
+ })
}
}
@@ -524,13 +531,16 @@ impl<'db> ProtocolInstanceType<'db> {
self,
db: &'db dyn Db,
other: Self,
- _relation: TypeRelation,
+ relation: TypeRelation,
visitor: &HasRelationToVisitor<'db, C>,
) -> C {
other
.inner
.interface(db)
.is_sub_interface_of(db, self.inner.interface(db), visitor)
+ .or(db, || {
+ Type::object(db).satisfies_protocol(db, other, relation, visitor)
+ })
}But I'm really not a fan of how repetitive that makes the logic -- it means encoding in three separate places that protocols which are supertypes of |
Contributor
|
Another option that might buy back the performance also is to eagerly simplify when constructing the protocol type (as we elsewhere prefer to do with equivalent types when we can), so that an annotation of There's probably yet another option where we keep "equivalent to object" as a special flag on the protocol struct, and just check it once when the protocol is constructed rather than on every subtype check. Maybe those won't actually be net positive for performance, if there are lots of protocol types constructed but then never actually checked for assignability or subtyping? I think if those aren't good options, it's probably worth doing the repetitive version here (with comments cross-referencing the location). The regression is only >5% on the incremental benchmark, but it shows up as 1-2% across the board. Having to check a protocol against object literally every time it is used in a subtype or assignability check is pretty significant. |
Contributor
|
Actually there might be another option here that gets the best of both worlds performance-wise, plus the DRYness -- if we make |
Member
Author
Yeah, I considered that, but I think it'll just be a really confusing user experience if we produce error messages like this: from typing import Hashable
def f(x: Hashable):
x.foo # error: object of type `object` has no attribute `foo`From a user's perspective: why on earth are we talking about |
Member
Author
Nice, that seems to have done the trick! We're now seeing speedups on some of the benchmarks 🥳 |
7e26957 to
c65bd20
Compare
c65bd20 to
ca79486
Compare
carljm
approved these changes
Sep 9, 2025
| } | ||
|
|
||
| fn initial<'db>(_db: &'db dyn Db, _value: ProtocolInstanceType<'db>, _: ()) -> bool { | ||
| false |
Contributor
There was a problem hiding this comment.
Probably doesn't matter in practice, but I suspect this should be true, or we will fail to recognize some recursive protocols as equivalent to object when in fact they should be. The co-inductive assumption here should be true -- if there were any part of the protocol that object does not satisfy, we would still correctly end up with an overall false.
Member
Author
There was a problem hiding this comment.
Hmm I'm not sure there are any possible recursive protocols that would be equivalent to object? But I haven't tried very hard to think of any 😄
Contributor
There was a problem hiding this comment.
If you take any protocol that is equivalent to object, and replace one of the types of its members with itself, I think that would do it?
Member
Author
There was a problem hiding this comment.
I added a regression test for recursive protocols that are supertypes of object. It didn't fail with the code as I had it before, but that may be just because we still don't look at the types of methods when doing assignability/subtyping checks for protocols (and this PR blocks doing that!). And the regression tests seem like a useful edge case to test explicitly, anyway.
I switched this to true, as per your suggestion!
…` for subtyping/assignability
ca79486 to
c518a97
Compare
This was referenced Sep 10, 2025
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
2 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
This PR fixes astral-sh/ty#1132 by treating any supertype of
objectthe same asobjectfor the purposes of subtyping/assignability. This fixes some serious false positives that we have onmain, which make ty hard to use on code that makes heavy use of the popularpandaslibrary. It does so at the cost of introducing serious false negatives for the same users. For now, this seems like a good trade-off; in the future, however, we may wish to explore rolling this back and explore doing less eager simplifications of unions when the unions include structural types. This would improve compatibility with other type checkers and (in my opinion) probably lead to a more intuitive user experience. If this is merged, I'll open a followup issue to track doing that.Test Plan
Mdtests added.