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

SubtypeVisitor: treat ParamSpec as if it was a covariant TypeVar #12096

Closed
wants to merge 5 commits into from

Conversation

Akuli
Copy link
Contributor

@Akuli Akuli commented Jan 29, 2022

Description

Fixes #12033. The problem was that Foo[<nothing>] was not a subtype of Foo[_P], where _P is a paramspec. This caused a weird problem where Foo[_P] was not a subtype of itself, because it got rewritten as Foo[<nothing>] being a subtype of Foo[_P].

I'm still not sure if this is correct, as this is my first PR that does something nontrivial.

Test Plan

Tests mostly copied from #12034 (an alternative PR to fix the same problem).

@github-actions

This comment has been minimized.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks much better than my solution 👍

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is roughly what I did in #11847 but better -- I'd love to see this merged :D

@jhance
Copy link
Collaborator

jhance commented Mar 29, 2022

Can you explain why we treat it as covariant and not invariant type variable?

The semantic analyzer constructs param spec already with a variance attached (always INVARIANT because PEP doesn't specify what to do for other variances)

@A5rocks
Copy link
Contributor

A5rocks commented Mar 30, 2022

In my case because I want [a: int, b: str] to be accepted as [int, str]. See this test case:

[case testParamSpecVariance]
from typing import Callable, Generic
from typing_extensions import ParamSpec
_P = ParamSpec("_P")
class Job(Generic[_P]):
def __init__(self, target: Callable[_P, None]) -> None: ...
def into_callable(self) -> Callable[_P, None]: ...
class A:
def func(self, var: int) -> None: ...
def other_func(self, job: Job[[int]]) -> None: ...
job = Job(A().func)
reveal_type(job) # N: Revealed type is "__main__.Job[[var: builtins.int]]"
A().other_func(job) # This should NOT error (despite the keyword)
# and yet the keyword should remain
job.into_callable()(var=42)
job.into_callable()(x=42) # E: Unexpected keyword argument "x"
# similar for other functions
def f1(n: object) -> None: ...
def f2(n: int) -> None: ...
def f3(n: bool) -> None: ...
# just like how this is legal...
a1: Callable[[bool], None]
a1 = f3
a1 = f2
a1 = f1
# ... this is also legal
a2: Job[[bool]]
a2 = Job(f3)
a2 = Job(f2)
a2 = Job(f1)
# and this is not legal
def f4(n: bytes) -> None: ...
a1 = f4 # E: Incompatible types in assignment (expression has type "Callable[[bytes], None]", variable has type "Callable[[bool], None]")
a2 = Job(f4) # E: Argument 1 to "Job" has incompatible type "Callable[[bytes], None]"; expected "Callable[[bool], None]"
# nor is this:
a4: Job[[int]]
a4 = Job(f3) # E: Argument 1 to "Job" has incompatible type "Callable[[bool], None]"; expected "Callable[[int], None]"
a4 = Job(f2)
a4 = Job(f1)
# just like this:
a3: Callable[[int], None]
a3 = f3 # E: Incompatible types in assignment (expression has type "Callable[[bool], None]", variable has type "Callable[[int], None]")
a3 = f2
a3 = f1
[builtins fixtures/tuple.pyi]

This was because some counterexample though it's so long ago I don't remember exactly why nor if there was another way.

And to be clear: TypeVarTuples specifies that they are invariant, I believe ParamSpecTypes don't have such a thing in the PEP (unless I'm misremembering, which is possible).

@Akuli
Copy link
Contributor Author

Akuli commented Mar 30, 2022

Can you explain why we treat it as covariant and not invariant type variable?

If I change it to INVARIANT, we again get a situation where Foo[<nothing>] is not a subtype of Foo[_P], where _P is a ParamSpec, and the test fails with these errors:

  main:10: error: Argument 1 to "f" has incompatible type "Type[OnlyParamSpec[Any]]"; expected "Type[OnlyParamSpec[Any]]" (diff)
  main:14: error: Argument 1 to "f" has incompatible type "Type[MixedWithTypeVar1[Any, Any]]"; expected "Type[MixedWithTypeVar1[Any, Any]]" (diff)
  main:18: error: Argument 1 to "f" has incompatible type "Type[MixedWithTypeVar2[Any, Any]]"; expected "Type[MixedWithTypeVar2[Any, Any]]" (diff)

I haven't figured out why "Foo[_P] is a subtype of Foo[_P]" gets rewritten as "Foo[<nothing>] is a subtype of Foo[_P]".

@github-actions

This comment has been minimized.

@Akuli Akuli marked this pull request as draft April 9, 2022 15:55
@Akuli
Copy link
Contributor Author

Akuli commented Apr 9, 2022

I'm not sure if we still need this. The tests pass without this PR.

@github-actions

This comment has been minimized.

@A5rocks
Copy link
Contributor

A5rocks commented Apr 10, 2022

I'm not sure if we still need this. The tests pass without this PR.

IMO yes. Why? Because this fixes my worse yet similar fix (which is why this is working now in master :P) in 272de9a

@Akuli
Copy link
Contributor Author

Akuli commented Apr 10, 2022

How exactly is my fix better? What kind of code would fail the type check with your fix and pass with my fix, or vice versa?

@A5rocks
Copy link
Contributor

A5rocks commented Apr 10, 2022

With TypeVarTuple, the variance is invariant but it falls under the else branch which is covariant in master. With yours it's invariant.

@Akuli Akuli marked this pull request as ready for review April 10, 2022 17:01
@Akuli
Copy link
Contributor Author

Akuli commented Apr 10, 2022

As far as I can tell, mypy doesn't really support TypeVarTuple yet, so we can't test this PR properly.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@cdce8p cdce8p mentioned this pull request Apr 13, 2022
@hauntsaninja
Copy link
Collaborator

Thanks for the PR! A lot has changed here, but looks like core of this change is on master

@Akuli Akuli deleted the paramspec branch September 16, 2023 19:10
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.

@final and ParamSpec
5 participants