-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[PEP 695] Allow Self return types with contravariance #17786
Conversation
Fix variance inference in this fragment from a typing conformance test: ``` class ClassA[T1, T2, T3](list[T1]): def method1(self, a: T2) -> None: ... def method2(self) -> T3: ... ``` Previously T2 was incorrectly inferred as invariant due to `list` having methods that return `Self`. Be more flexible with return types to allow inferring contravariance for type variables even if there are `Self` return types, in particular. We could probably make this even more lenient, but after thinking about this for a while, I wasn't sure what the most general rule would be, so I decided to just make a tweak to support the likely most common use case (which is probably actually not that common either). Link to conformance test: https://github.com/python/typing/blob/main/conformance/tests/generics_variance_inference.py#L15C1-L20C12
This comment has been minimized.
This comment has been minimized.
The build failure looks like a flake. |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
@@ -213,7 +213,7 @@ b: Invariant[int] | |||
if int(): | |||
a = b # E: Incompatible types in assignment (expression has type "Invariant[int]", variable has type "Invariant[object]") | |||
if int(): | |||
b = a # E: Incompatible types in assignment (expression has type "Invariant[object]", variable has type "Invariant[int]") | |||
b = a |
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.
I think this is not right. It happens because with current implementation we check return type after bind_self()
, so we can't distinguish situation with an explicit Self
from situation where return type just happens to (accidentally) match current class. I understand that Self
-types are inherently (slightly) unsafe, but I don't want this unsafety to "leak" to code that doesn't use Self
.
I think it makes sense to limit the return type erasure only to situations where the original callable type contained an explicit Self
type (or the "old style" self type).
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.
Btw when looking at this I found a bug in variance inference that affects protocols as well. In particular, all three examples above should be inferred as covariant (or even bivariant, but we don't have that).
Also to clarify, the unsafety I meant above relates to examples like
class C[T]:
def meth(self, x: T, y: C[T]) -> C[T]: ...
that should not be contravariant, while it seems to me with the proposed PR it will be. But after playing with this more, inference doesn't behave as it should on master either. This needs some cleanup. @JukkaL we can discuss this in our meeting on Tuesday.
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.
As per our discussion, you can merge this now (to unblock making PEP 695 official), and I will do some variance inference overhaul later.
Fix variance inference in this fragment from a typing conformance test:
Previously T2 was incorrectly inferred as invariant due to
list
having methods that returnSelf
. Be more flexible with return types to allow inferring contravariance for type variables even if there areSelf
return types, in particular.We could probably make this even more lenient, but after thinking about this for a while, I wasn't sure what the most general rule would be, so I decided to just make a tweak to support the likely most common use case (which is probably actually not that common either).
Link to conformance test:
https://github.com/python/typing/blob/main/conformance/tests/generics_variance_inference.py#L15C1-L20C12