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

Basic ParamSpec Concatenate and literal support #11847

Merged
merged 50 commits into from
Apr 7, 2022

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Dec 26, 2021

Description

I was planning on adding a ParamSpec-based interface to something I'm making but... it turns out I need Concatenate and Z[[int]] syntax.

Closes #11833
Closes #12276 (needs test)
Closes #12257 (needs test; not sure how to test cache issues)
Refs #8645
External ref python/typeshed#4827

This PR adds a new Parameters proper type to represent parameters (more later in this description), along with adding a Concatenate operator.

Test Plan

I copied all (seemingly relevant) tests from PEP 612. I plan on copying test cases from issues and thinking of a few of my own later.

Notes

(not a real section but seemed important enough)

  • Concatenate support does not use a ProperType. That would be too much new code for an operator, in my opinion. Thus, it prepends to a ParamSpecType's .prefix.
  • I would really really like CallableType to take in a parameters: ParamSpecType | Parameters -- this would simplify some code, and it fits better in your head. However, this would be a breaking change, so I did not do this (along with the fact that making basic changes in this direction led to 300 type errors when self-checking).
    - Concatenate was super unspecified by the PEP in my opinion. I ended up making it work wherever ParamSpec could be (or so I think)... Which means Concatenate[int, Concatenate[str, P]] works!
  • Error messages could be improved, but I'm not sure where I can get the context to improve those.

Now this program works:
```py
from typing_extensions import ParamSpec
from typing import Generic, Callable

P = ParamSpec("P")

class Z(Generic[P]):
	def call(self, *args: P.args, **kwargs: P.kwargs) -> None:
		pass

n: Z[[int]]

reveal_type(n)  # N: Revealed type is "repro.Z[[builtins.int]]"

def f(n: Z[P]) -> Z[P]:
	...

reveal_type(f)  # N: Revealed type is "def [P] (n: repro.Z[P`-1]) -> repro.Z[P`-1]"
reveal_type(f(n))  # N: Revealed type is "repro.Z[[builtins.int]]"
```
This program:
```py
from typing_extensions import ParamSpec, Concatenate
from typing import Generic

P = ParamSpec("P")

class Z(Generic[P]): ...

n: Z[[int]]

reveal_type(n)

def f(n: Z[P]) -> Z[Concatenate[bool, Concatenate[str, P]]]: ...

reveal_type(f)
reveal_type(f(n))
```

outputs:
```
repro.py:10: note: Revealed type is "repro.Z[[builtins.int]]"
repro.py:14: note: Revealed type is "def [P] (n: repro.Z[P`-1]) -> repro.Z[Concatenate[builtins.bool, builtins.str, P`-1]]"
repro.py:15: note: Revealed type is "repro.Z[[builtins.bool, builtins.str, builtins.int]]"
```

Next up: checking inputs match prefixes and why does it only work when
cache exists?
It wasn't actually cache...
mypy/typeanal.py Outdated Show resolved Hide resolved
@A5rocks A5rocks marked this pull request as ready for review December 27, 2021 01:53
@@ -506,6 +506,17 @@ def visit_param_spec(self, t: ParamSpecType) -> ProperType:
else:
return self.default(self.s)

def visit_parameters(self, t: Parameters) -> ProperType:
# TODO: is this the right variance?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is correct, we need any instance of the return type to be compatible iwth both, which seems to be the intersection of their arguments e.g. meet_types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if its possible to test these independently (I assume we can add tests in the semanal section that simply test this, if not thats a problem in itself) the "Add all the visitors" section of the diff should probably be migrated to a separate PR with its own semanal tests.


# This is a heuristic: it will be checked later anyways but the error
# message may be worse.
with self.set_allow_param_spec_literals(info.has_param_spec_type):
Copy link
Collaborator

@jhance jhance Mar 17, 2022

Choose a reason for hiding this comment

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

Existing precedent seems to be that anal_type does its own pushing/popping.

IMO (and we can do this in a separate PR so that this one is a minor tweak) we should do something like having the parameters to anal_type be some sort of dataclass, and then allow passing mutators to the dataclass to anal_array at which point it will push/pop the options. This avoids having N things that are pushing/popping to some sort of stack.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Its also really confusing how the state set by set_allow_param_spec_literals compares to the value allow_param_spec send to self.anal_array and self.anal_type. If the distinction is necessary it should be documented somewhere what the difference is.

@ygale
Copy link

ygale commented Mar 22, 2022

@A5rocks Great work, thanks! I also hit the same roadblock, and I'm glad to see that you are working on it.

Your idea to generalize where Concatenate can occur is interesting. Do you have any code samples where this would be useful? The thing is, both PEP 612 and the Python Library docs explicitly forbid this. The CPython interpreter rejects your sample usage of that syntax with

TypeError: The last parameter to Concatenate should be a ParamSpec variable.

So to do this, we would need to submit PRs to change the CPython source code, the docs, and the PEP. For those to have a chance of being accepted, we would need convincing justifications of the need for the change, not just "because it was there". mypy would still need to reject that syntax for now, but we could guard the rejection by version numbers, in anticipation of removing the restriction in future versions of Python after the PRs are accepted.

Truthfully, I'm not really sure why this syntax is needed, other than aesthetics, because Concatenate[int, Concatenate[str, P]] seems to be equivalent to Concatenate[int, str, P]. But if you have an idea about why it is needed, I'd be happy to help out with the PRs.

EDIT: To be clear, the simplest solution is to stick with the current syntax: Concatenate is allowed only as the first parameter of Callable, nowhere else. And the last parameter of Concatenate must be a ParamSpec variable. Programs that violate either of those rules are rejected by the type checker.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 22, 2022

@A5rocks This close to being ready to be merged! If you can fix the merge conflicts and CI failures, and address feedback in the next 1 week or so, we could probably include this in the next mypy feature release (planned to be out in the first half of April).

@A5rocks
Copy link
Contributor Author

A5rocks commented Mar 26, 2022

@ygale

The thing is, both PEP 612 and the Python Library docs explicitly forbid this.

Yep I just somehow misread the syntax definition in PEP 612, fixed.


@cdce8p

In my last comment I mentioned one more issue I wasn't able to isolate. Since then I did take another look at it. From the looks of it, it seems to be caused by a combination of overload, a Generic class, and passing an invalid type with type: ignore. Maybe you've an idea.

Looked a bit at it: so the overload resolution is failing for exactly what you predicted: the type ignore. In a nutshell, the overloads are checked through until the first without type errors upon calling it -- this yields 'repro.py:39: error: Argument 2 to "run_job" has incompatible type "str"; expected "int"' for the second overload due to a lacking type: ignore in the synthesized check. Thus, mypy thinks none of the overloads work and then chooses the first (arbitrary choice, just for typing reasons).

I'm not sure a solution would be in scope for this PR (as a proper solution would involve propagating type ignores)

EDIT: filed #12454

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core)
+ homeassistant/helpers/event.py:116: error: Unused "type: ignore" comment
+ homeassistant/helpers/event.py:117: error: Unused "type: ignore" comment
+ homeassistant/components/tplink/entity.py:22: error: Unused "type: ignore" comment
+ homeassistant/components/tplink/entity.py:23: error: Unused "type: ignore" comment
+ homeassistant/components/sonarr/sensor.py:112: error: Unused "type: ignore" comment
+ homeassistant/components/sonarr/sensor.py:113: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:18: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:19: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:27: error: Unused "type: ignore" comment
+ homeassistant/components/zabbix/__init__.py:93: error: Argument 3 has incompatible type "Callable[[datetime], bool]"; expected "Union[HassJob[Optional[Awaitable[None]]], Callable[[datetime], Optional[Awaitable[None]]]]"  [arg-type]
+ homeassistant/components/zabbix/__init__.py:93: error: Incompatible return value type (got "bool", expected "Optional[Awaitable[None]]")  [return-value]
+ homeassistant/components/plugwise/util.py:18: error: Unused "type: ignore" comment
+ homeassistant/components/plugwise/util.py:19: error: Unused "type: ignore" comment
+ homeassistant/components/roku/helpers.py:32: error: Unused "type: ignore" comment
+ homeassistant/components/roku/helpers.py:33: error: Unused "type: ignore" comment
+ homeassistant/components/webostv/media_player.py:101: error: Unused "type: ignore" comment
+ homeassistant/components/webostv/media_player.py:102: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:38: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:47: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:55: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:61: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:62: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/media.py:138: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, SupportsIndex, SupportsTrunc]"  [arg-type]
+ homeassistant/components/vlc_telnet/media_player.py:79: error: Unused "type: ignore" comment
+ homeassistant/components/vlc_telnet/media_player.py:80: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:82: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:83: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:95: error: Unused "type: ignore" comment

@cdce8p
Copy link
Collaborator

cdce8p commented Mar 27, 2022

Looked a bit at it: so the overload resolution is failing for exactly what you predicted: the type ignore. In a nutshell, the overloads are checked through until the first without type errors upon calling it -- this yields 'repro.py:39: error: Argument 2 to "run_job" has incompatible type "str"; expected "int"' for the second overload due to a lacking type: ignore in the synthesized check. Thus, mypy thinks none of the overloads work and then chooses the first (arbitrary choice, just for typing reasons).

Thanks for taking the time! I already left a #12454 (comment), but the gist is that I should just fix the argument types itself. So not an issue for this PR.

--
Not completely sure what's left here. All major issues seem to be fixed and the primer output looks good. There is the TypeAlias support that's still missing #11855 and some other (minor) issues which might not be fixed yet: topic-paramspec PEP 612, ParamSpec, Concatenate
As you said though, these can be addressed later. It would be awesome if the PR would be part of the next release.

The failed windows CI tests seem unrelated. They fail on the master branch, too.

@cdce8p
Copy link
Collaborator

cdce8p commented Mar 31, 2022

Windows tests have been fixed in #12477

A5rocks and others added 3 commits April 5, 2022 10:09
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
pros:
 - more readable IMO
 - more concise
 - standardization of output

cons:
 - can be easy to miss double backets
 - I feel like this is a small deviation from normal mypy
   representations... but I have nothing to back that up.

Overall, I think it's a good thing to try at least.
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core)
+ homeassistant/helpers/event.py:116: error: Unused "type: ignore" comment
+ homeassistant/helpers/event.py:117: error: Unused "type: ignore" comment
+ homeassistant/components/tplink/entity.py:22: error: Unused "type: ignore" comment
+ homeassistant/components/tplink/entity.py:23: error: Unused "type: ignore" comment
+ homeassistant/components/sonarr/sensor.py:112: error: Unused "type: ignore" comment
+ homeassistant/components/sonarr/sensor.py:113: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:18: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:19: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:27: error: Unused "type: ignore" comment
+ homeassistant/components/zabbix/__init__.py:93: error: Argument 3 has incompatible type "Callable[[datetime], bool]"; expected "Union[HassJob[Optional[Awaitable[None]]], Callable[[datetime], Optional[Awaitable[None]]]]"  [arg-type]
+ homeassistant/components/zabbix/__init__.py:93: error: Incompatible return value type (got "bool", expected "Optional[Awaitable[None]]")  [return-value]
+ homeassistant/components/plugwise/util.py:18: error: Unused "type: ignore" comment
+ homeassistant/components/plugwise/util.py:19: error: Unused "type: ignore" comment
+ homeassistant/components/roku/helpers.py:32: error: Unused "type: ignore" comment
+ homeassistant/components/roku/helpers.py:33: error: Unused "type: ignore" comment
+ homeassistant/components/webostv/media_player.py:101: error: Unused "type: ignore" comment
+ homeassistant/components/webostv/media_player.py:102: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:38: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:47: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:55: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:61: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:62: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/media.py:138: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, SupportsIndex, SupportsTrunc]"  [arg-type]
+ homeassistant/components/vlc_telnet/media_player.py:79: error: Unused "type: ignore" comment
+ homeassistant/components/vlc_telnet/media_player.py:80: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:82: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:83: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:95: error: Unused "type: ignore" comment

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core)
+ homeassistant/helpers/event.py:116: error: Unused "type: ignore" comment
+ homeassistant/helpers/event.py:117: error: Unused "type: ignore" comment
+ homeassistant/components/tplink/entity.py:22: error: Unused "type: ignore" comment
+ homeassistant/components/tplink/entity.py:23: error: Unused "type: ignore" comment
+ homeassistant/components/sonarr/sensor.py:112: error: Unused "type: ignore" comment
+ homeassistant/components/sonarr/sensor.py:113: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:18: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:19: error: Unused "type: ignore" comment
+ homeassistant/components/evil_genius_labs/util.py:27: error: Unused "type: ignore" comment
+ homeassistant/components/zabbix/__init__.py:93: error: Argument 3 has incompatible type "Callable[[datetime], bool]"; expected "Union[HassJob[Optional[Awaitable[None]]], Callable[[datetime], Optional[Awaitable[None]]]]"  [arg-type]
+ homeassistant/components/zabbix/__init__.py:93: error: Incompatible return value type (got "bool", expected "Optional[Awaitable[None]]")  [return-value]
+ homeassistant/components/plugwise/util.py:18: error: Unused "type: ignore" comment
+ homeassistant/components/plugwise/util.py:19: error: Unused "type: ignore" comment
+ homeassistant/components/roku/helpers.py:32: error: Unused "type: ignore" comment
+ homeassistant/components/roku/helpers.py:33: error: Unused "type: ignore" comment
+ homeassistant/components/webostv/media_player.py:101: error: Unused "type: ignore" comment
+ homeassistant/components/webostv/media_player.py:102: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:38: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:47: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:55: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:61: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/helpers.py:62: error: Unused "type: ignore" comment
+ homeassistant/components/sonos/media.py:138: error: Argument 1 to "int" has incompatible type "Optional[Any]"; expected "Union[str, bytes, SupportsInt, SupportsIndex, SupportsTrunc]"  [arg-type]
+ homeassistant/components/vlc_telnet/media_player.py:79: error: Unused "type: ignore" comment
+ homeassistant/components/vlc_telnet/media_player.py:80: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:82: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:83: error: Unused "type: ignore" comment
+ homeassistant/components/dlna_dmr/media_player.py:95: error: Unused "type: ignore" comment

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

This looks good now! The core functionality looks solid, and it will be great to have this in the next release. Remaining corner cases and TODOs can be worked on in separate PRs.

Thank you for working on this -- this is the biggest remaining 3.10 feature that mypy has been missing.

@JukkaL JukkaL merged commit 07d8878 into python:master Apr 7, 2022
@A5rocks
Copy link
Contributor Author

A5rocks commented Apr 7, 2022

Thanks!

FYI, #12096 should be checked out (I put similar code in the else branch rather than a new elif, not realizing that typevarlikes other than typevar could be invariant, like typevartuple)

@AlexWaygood
Copy link
Member

Just want to say that I'm really excited to see this get merged — thanks for all your work on this @A5rocks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants