Skip to content
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

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Sep 19, 2024

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

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.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 19, 2024

The build failure looks like a flake.

Copy link
Contributor

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
Copy link
Member

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).

Copy link
Member

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.

Copy link
Member

@ilevkivskyi ilevkivskyi left a 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.

@JukkaL JukkaL merged commit ecfab6a into master Sep 24, 2024
19 checks passed
@JukkaL JukkaL deleted the variance-self-type branch September 24, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants