[ty] Improve protocol member type checking and relation handling#18847
Merged
AlexWaygood merged 14 commits intoastral-sh:mainfrom Jun 29, 2025
Merged
[ty] Improve protocol member type checking and relation handling#18847AlexWaygood merged 14 commits intoastral-sh:mainfrom
AlexWaygood merged 14 commits intoastral-sh:mainfrom
Conversation
Contributor
|
mtshiba
force-pushed
the
protocol-member-checks
branch
from
6f64b94 to
fd040c4
Compare
CodSpeed Instrumentation Performance ReportMerging #18847 will not alter performanceComparing Summary
|
CodSpeed WallTime Performance ReportMerging #18847 will not alter performanceComparing Summary
|
mtshiba
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
reviewed
Jun 26, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks! This looks okay overall, but I think it would be nice to implement invariance of mutable attribute members; it isn't really correct to treat them covariantly.
You could also consider adding a ProtocolMemberKind enum like I did in #18659? I think we'll need some way of distinguishing between method members, property members and everything else anyway, and your PR already starts to add that distinction
Member
|
Sorry for the delayed review!! |
Member
I'm curious how you plan to use protocols to implement that feature. For something like this: class Foo:
x: str
def f(foo: Foo):
if foo.x == "bar":
...It would be incorrect to narrow the type of |
Collaborator
Author
Yeah, you mean, for example, if such a "malicious" subclass is used, narrowing is unsound? class BadStr(str):
def __eq__(self, other):
return True
class Foo:
tag: str = BadStr("Foo")
value: int
class Bar:
tag: str = BadStr("Bar")
value: str
def _(x: Foo | Bar):
if x.tag == "Foo":
reveal_type(x.tag) # str, not Literal["Foo"]
reveal_type(x.value) # str | int, not intThe determination that |
Member
Exactly, yes! Okay, it sounds like you're already aware of the issue, which is great 👍 |
mtshiba
force-pushed
the
protocol-member-checks
branch
from
f6e81f0 to
b93f06a
Compare
mtshiba
commented
Jun 27, 2025
mtshiba
commented
Jun 27, 2025
carljm
reviewed
Jun 27, 2025
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
I made one inline comment here, but I think it's fine if this is merged with TODOs. I'd like @AlexWaygood to make the call on merging this, but let me know if there are any other parts of it you'd like me to review.
AlexWaygood
force-pushed
the
protocol-member-checks
branch
from
96d49d7 to
dfcf0e0
Compare
AlexWaygood
approved these changes
Jun 27, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks, this looks great to me! I pushed a few small changes.
Would you be able to take a quick look through the mypy_primer diff and see if the new diagnostics are all expected? Once that's done I'm happy to land this -- thank you!
Collaborator
Author
|
I found that the current protocol member check has a problem in the following case. def f(x: int | LiteralString):
if hasattr(x, "capitalize"):
reveal_type(x) # should be: (int & <Protocol with members 'capitalize'>) | LiteralString
else:
reveal_type(x) # should be: int & ~<Protocol with members 'capitalize'>It is inferred like this. The protocol synthesized from the predicate |
Member
|
Ah, nice catch. The synthesised protocols created from |
Collaborator
Author
|
Now all the new errors/warnings in mypy_primer look ok. |
AlexWaygood
approved these changes
Jun 29, 2025
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks again -- this is great work!
Member
|
I think we'll need to invest into improving the diagnostic if somebody tries to write to an attribute on a synthesized protocol that has that attribute available, but only as a read-only property member. But we can defer that for now. |
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
This PR adds handling for checking the types of protocol members to
Type::satisfies_protocolandType::is_disjoint_from.It only adds checks for simple members, not methods or properties for now.
The reason for adding this change is that
Protocolmember checks are required to implement advanced attribute narrowing (see astral-sh/ty#643). I understand that there are some issues with the currentProtocolsupport that need to be resolved, and if adding this PR would get in the way of solving those issues, I would like to wait until those issues are resolved.Test Plan
Protocoltests are added.