-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[ty] Make TypeIs invariant in its type argument #20428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
| ## TypeIs | ||
|
|
||
| ```toml | ||
| [environment] | ||
| python-version = "3.13" | ||
| ``` | ||
|
|
||
| `TypeIs[T]` is invariant in `T`. See the [typing spec][typeis-spec] for a justification. | ||
|
|
||
| ```py | ||
| from typing import TypeIs | ||
| from ty_extensions import is_assignable_to, is_subtype_of, static_assert | ||
|
|
||
| class A: | ||
| pass | ||
|
|
||
| class B(A): | ||
| pass | ||
|
|
||
| assert not is_subtype_of(TypeIs[B], TypeIs[A]) | ||
| assert not is_subtype_of(TypeIs[A], TypeIs[B]) | ||
| assert not is_assignable_to(TypeIs[B], TypeIs[A]) | ||
| assert not is_assignable_to(TypeIs[A], TypeIs[B]) | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't TypeIs's variance already tested in is_subtype_of.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is interesting. The pre-existence of these tests implies the invariance was already being enforced? I'll have to look further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the tests you added here fail before the code change in this PR?
It seems to me that what you are testing for here is assignability/subtyping of TypeIs types themselves (which I think was already correctly invariant), but the code change you made would not impact that, it would instead affect variance inference when you have e.g. TypeIs[T] as part of a method signature or as an attribute on a PEP695 generic class with typevar T. Which seems like a rare/unusual case, but still worth getting right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I missed that there was existing logic for the in variance of TypeIs. Changed the tests, and also fixed assert -> static_assert 🙈
a3e61a7 to
d83dd35
Compare
d83dd35 to
d909c68
Compare
carljm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Summary
What it says on the tin. See the typing spec for justification.
Test Plan
Add more tests to PEP 695
variance.mdsuite.