[ty] Proper assignability/subtyping checks for protocols with method members#20165
Merged
AlexWaygood merged 9 commits intomainfrom Sep 12, 2025
Merged
[ty] Proper assignability/subtyping checks for protocols with method members#20165AlexWaygood merged 9 commits intomainfrom
AlexWaygood merged 9 commits intomainfrom
Conversation
Contributor
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-09-12 10:07:15.533009560 +0000
+++ new-output.txt 2025-09-12 10:07:18.541030789 +0000
@@ -720,6 +720,12 @@
protocols_definition.py:159:1: error[invalid-assignment] Object of type `Concrete3_Bad4` is not assignable to `Template3`
protocols_definition.py:160:1: error[invalid-assignment] Object of type `Concrete3_Bad5` is not assignable to `Template3`
protocols_definition.py:219:1: error[invalid-assignment] Object of type `Concrete4_Bad2` is not assignable to `Template4`
+protocols_definition.py:285:1: error[invalid-assignment] Object of type `Concrete5_Bad1` is not assignable to `Template5`
+protocols_definition.py:286:1: error[invalid-assignment] Object of type `Concrete5_Bad2` is not assignable to `Template5`
+protocols_definition.py:287:1: error[invalid-assignment] Object of type `Concrete5_Bad3` is not assignable to `Template5`
+protocols_definition.py:288:1: error[invalid-assignment] Object of type `Concrete5_Bad4` is not assignable to `Template5`
+protocols_definition.py:289:1: error[invalid-assignment] Object of type `Concrete5_Bad5` is not assignable to `Template5`
+protocols_generic.py:40:1: error[invalid-assignment] Object of type `Concrete1` is not assignable to `Proto1[int, str]`
protocols_merging.py:52:1: error[invalid-assignment] Object of type `SCConcrete2` is not assignable to `SizedAndClosable1`
protocols_merging.py:53:1: error[invalid-assignment] Object of type `SCConcrete2` is not assignable to `SizedAndClosable2`
protocols_merging.py:54:1: error[invalid-assignment] Object of type `SCConcrete2` is not assignable to `SizedAndClosable3`
@@ -848,5 +854,5 @@
typeddicts_usage.py:28:1: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Invalid key access on 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 849 diagnostics
+Found 855 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details. |
AlexWaygood
commented
Aug 30, 2025
Contributor
|
This comment was marked as resolved.
This comment was marked as resolved.
CodSpeed Instrumentation Performance ReportMerging #20165 will improve performances by 13.92%Comparing Summary
Benchmarks breakdown
|
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
40 | 4 | 0 |
no-matching-overload |
39 | 0 | 0 |
unused-ignore-comment |
0 | 23 | 0 |
invalid-return-type |
18 | 1 | 0 |
invalid-assignment |
8 | 2 | 0 |
redundant-cast |
0 | 1 | 0 |
type-assertion-failure |
1 | 0 | 0 |
| Total | 106 | 31 | 0 |
AlexWaygood
force-pushed
the
alex/method-members
branch
from
1657253 to
27a967d
Compare
AlexWaygood
force-pushed
the
alex/method-members
branch
from
d8208b0 to
b8b11db
Compare
CodSpeed WallTime Performance ReportMerging #20165 will not alter performanceComparing Summary
|
AlexWaygood
force-pushed
the
alex/method-members
branch
3 times, most recently
from
54de803 to
80db138
Compare
AlexWaygood
force-pushed
the
alex/method-members
branch
from
80db138 to
90c7da7
Compare
AlexWaygood
force-pushed
the
alex/method-members
branch
2 times, most recently
from
910b683 to
73c80bb
Compare
AlexWaygood
changed the base branch from
alex/protocol-nominal
to
alex/generic-method-tests
AlexWaygood
force-pushed
the
alex/generic-method-tests
branch
from
aef602f to
7b94e44
Compare
AlexWaygood
force-pushed
the
alex/method-members
branch
2 times, most recently
from
7041a3a to
a947ad4
Compare
AlexWaygood
commented
Sep 11, 2025
Comment on lines
+2231
to
+2232
| # TODO: this should pass | ||
| static_assert(is_subtype_of(TypeOf[satisfies_foo], Foo)) # error: [static-assert-error] |
Member
Author
There was a problem hiding this comment.
I believe this is because we don't consider Foo a fully static type currently, because it has an unannotated self parameter
AlexWaygood
commented
Sep 11, 2025
Comment on lines
557
to
564
| if any_over_type(db, proto_member_as_bound_method, &|t| { | ||
| matches!(t, Type::TypeVar(_)) | ||
| }) { | ||
| // TODO: proper validation for generic methods on protocols | ||
| return C::always_satisfiable(db); | ||
| } |
Member
Author
There was a problem hiding this comment.
I wanted to get a MVP of this feature out before digging into why we have lots of false positives for method members with function-scoped generic contexts
AlexWaygood
force-pushed
the
alex/method-members
branch
from
95aeb55 to
c155a9a
Compare
AlexWaygood
requested review from
carljm,
dcreager and
sharkdp
as code owners
Member
Author
Quite unexpected, but I'll take it!! The |
carljm
approved these changes
Sep 11, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Nice job extracting so many fixes along the way and leaving this such a small diff!
Comment on lines
+3266
to
+3269
| if name == "__call__" && matches!(self, Type::Callable(_) | Type::DataclassTransformer(_)) { | ||
| return Place::bound(self).into(); |
Contributor
There was a problem hiding this comment.
Do we have any direct tests of this (that the __call__ of a callable type is itself), or only indirect tests via protocols? Would it be worth a direct test for it, to make it clearer what's happening if it ever regresses?
Member
Author
There was a problem hiding this comment.
What's changing here is a bit subtle. On main we are already able to infer that for an object c of a callable type T, the type of c.__call__ will also be T. But naively looking up the __call__ attribute directly on c isn't good enough for protocol assignability/subtyping -- it would mean that this assertion would not pass, for example, because Foo.__iter__ has type Callable[[Foo], str] rather than Callable[[], Foo]. (The enum class object itself is iterable, but so is each enum member. Iterating over the enum members uses str.__iter__ because each enum member is an instance of str; iterating over the enum class object itself uses EnumType.__iter__.):
from typing import Iterable
from enum import StrEnum
from ty_extensions import static_assert, is_assignable_to, TypeOf
class Foo(StrEnum):
X = "foo"
Y = "bar"
static_assert(is_assignable_to(TypeOf[Foo], Iterable[Foo]))>>> from enum import StrEnum
>>> class Foo(StrEnum):
... X = "foo"
... Y = "bar"
...
>>> list(Foo)
[<Foo.X: 'foo'>, <Foo.Y: 'bar'>]
>>> list(Foo.X)
['f', 'o', 'o']Therefore to get the "instance get-type" of a method member, we can't simply lookup the type of that member on an object; instead we must lookup the type of that member on the object's meta-type and then invoke the descriptor protocol on the result of that lookup. On most types that works great, but not on Callable types, because we currently give the meta-type of a Callable type as being just type (it's a very lossy operation currently!). Ideally we would synthesize some kind of meta-protocol as the meta-type of a Callable type (I'd like to do that at some point!) so that it's not so lossy, but that feels out of scope for this PR.
TL;DR: no, I don't think it's really possible to add an isolated unit test for this that doesn't involve protocols. It's a change that's very specific to the way we need to look up the __call__ attribute on Callable types when determining whether a Callable type is a subtype of a protocol type with a __call__ method.
However, I will add a comment to this bit of code noting that while this isn't inaccurate, it's basically a workaround until we have a better answer for the meta-type of Callable types.
Member
Author
There was a problem hiding this comment.
I'm actually getting rid of this again in #20363 FWIW, and replacing it with special-casing somewhere else instead 😄
…members remove now-unneeded changes
AlexWaygood
force-pushed
the
alex/method-members
branch
from
c155a9a to
a1ede87
Compare
This was referenced Sep 12, 2025
AlexWaygood
added a commit
that referenced
this pull request
Sep 12, 2025
#20367) ## Summary #20165 added a lot of false positives around calls to `builtins.open()`, because our missing support for PEP-613 type aliases means that we don't understand typeshed's overloads for `builtins.open()` at all yet, and therefore always select the first overload. This didn't use to matter very much, but now that we have a much stricter implementation of protocol assignability/subtyping it matters a lot, because most of the stdlib functions dealing with I/O (`pickle`, `marshal`, `io`, `json`, etc.) are annotated in typeshed as taking in protocols of some kind. In lieu of full PEP-613 support, which is blocked on various things and might not land in time for our next alpha release, this PR adds some temporary special-casing for `builtins.open()` to avoid the false positives. We just infer `Todo` for anything that isn't meant to match typeshed's first `open()` overload. This should be easy to rip out again once we have proper support for PEP-613 type aliases, which hopefully should be pretty soon! ## Test Plan Added an mdtest
sharkdp
reviewed
Sep 15, 2025
Comment on lines
+544
to
+552
| let Place::Type(attribute_type, Boundness::Bound) = other | ||
| .invoke_descriptor_protocol( | ||
| db, | ||
| self.name, | ||
| Place::Unbound.into(), | ||
| InstanceFallbackShadowsNonDataDescriptor::No, | ||
| MemberLookupPolicy::default(), | ||
| ) | ||
| .place |
Contributor
There was a problem hiding this comment.
Why do you call invoke_descriptor_protocol directly here instead of going through Type::member? Does this work correctly if other is a union/intersection (is that already handled at a higher level)? And does it handle classmethods, method accesses on class objects etc. correctly?
Member
Author
There was a problem hiding this comment.
Why do you call
invoke_descriptor_protocoldirectly here instead of going throughType::member?
I answered this in my reply to Carl here
Does this work correctly if
otheris a union/intersection (is that already handled at a higher level)?
I think so... I should probably add more tests around this. Is there a reason why you think this might not work for unions/intersections?
And does it handle classmethods, method accesses on class objects etc. correctly?
I should definitely add more tests around this too, thanks. Note that there are some more checks that I'd like to do (such as checking the type on the meta-type as well as the type on the instance-type) which we can't yet do while we consider methods with unannotated self/cls to be non-fully-static. This PR is really an MVP
Contributor
There was a problem hiding this comment.
Is there a reason why you think this might not work for unions/intersections?
Not sure, but I think member does the "distribute this operation over unions/intersections" part of invoke_descriptor_protocol.
If you plan to add more tests for both of these cases, we should be fine. Thanks.
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
When determining whether a nominal type
Nsatisfies a protocol typePwith a method memberP.x, check that the signature of the methodN.xis a subtype of the signature ofP.xbefore deciding thatNis a subtype ofP.Note that subtyping between two different types that are both protocols is a separate code path in
instance.rs, and this PR doesn't touch that code path (I'll do that separately). So this doesn't yet provide a fix for e.g. astral-sh/ty#1089 or astral-sh/ty#733.Fixes astral-sh/ty#637.
Fixes astral-sh/ty#889.
Test Plan
QUICKCHECK_TESTS=1000000 cargo test --release -p ty_python_semantic -- --ignored types::property_tests::stableto check that theall_type_assignable_to_iterable_are_iterabletest can be safely marked as stable.Typing conformance suite impact
These all look good! All the lines where we're emitting new diagnostics have
# Ecomments next to them.Ecosystem impact
I analysed nearly all the mypy_primer diff in #20165 (comment). I don't think there's anything there that indicates a bug in protocol assignability/subtyping logic. Most new hits are due to one of:
builtins.open()(because that function uses PEP-613 type aliases). This is such a common source of new false positives that it might be worth us adding some temporary special-casing foropen()where we infer it as returningTodo?