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

bpo-43224: Implement PEP 646 changes to typing.py #31021

Merged
merged 35 commits into from
Mar 8, 2022

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented Jan 30, 2022

These are the parts of the old PR (#30398) relevant to typing.py.

Note that we haven't yet merged the grammar PR (#31018), so for the time being all the tests just use Unpack. We can add tests using the actual star operator in a future PR.

https://bugs.python.org/issue43224

Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Show resolved Hide resolved
mrahtz and others added 5 commits January 30, 2022 20:58
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Lib/typing.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pradeep90 pradeep90 left a comment

Choose a reason for hiding this comment

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

(Reviewed three-fourths of it and got tired after a couple of hours. Will review the rest after the changes.)

Overall, I think we'd want to try other things beyond Unpack[Ts]: Unpack[tuple[int, ...]], Unpack[tuple[int, str]], and Unpack[tuple[int, Unpack[Ts], str]]. I've tried to point to cases inline.

I'd assumed all these comments had been posted weeks ago - I'd missed the part where you actually have to click the 'Submit code review' button to post them! Uff.

Ah, that clears up a mystery :) I was wondering why many comments were unaddressed.


Do we need to validate *args: <foo> somewhere? For example, *args: Ts vs *args: Unpack[Ts].

What if someone tries Union[*Ts] or Union[Ts]? Same for other special forms that call _type_check:

  • ClassVar
  • Optional
  • Final (i.e., x: Final[Ts])
  • TypeGuard
  • What about Concatenate[Ts, P] and Concatenate[*Ts, P]?

n00b question: how do I run the Python tests in my local clone of this repository? Wanted to experiment.

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Outdated Show resolved Hide resolved
Lib/typing.py Show resolved Hide resolved
@JelleZijlstra
Copy link
Member

n00b question: how do I run the Python tests in my local clone of this repository? Wanted to experiment.

First compile Python, as explained in "Quick reference" at https://devguide.python.org/. To run just the typing tests, the easiest way is just ./python.exe Lib/test/test_typing.py or similar depending on your OS and CWD.

@pradeep90
Copy link
Contributor

n00b question: how do I run the Python tests in my local clone of this repository? Wanted to experiment.

First compile Python, as explained in "Quick reference" at https://devguide.python.org/. To run just the typing tests, the easiest way is just ./python.exe Lib/test/test_typing.py or similar depending on your OS and CWD.

Thanks, Jelle. Should have RTFM'ed but was being lazy :|

@mrahtz Some tests fail locally (and on CI, apparently).

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@mrahtz
Copy link
Contributor Author

mrahtz commented Feb 2, 2022

@gvanrossum @JelleZijlstra @pradeep90 There's a high-level design point I've just realised we should discuss.

In the PEP itself, we stated that Unpack would primarily be for backwards compatibility. But in the current iteration of this PR, we're actually using it as the wrapper for unpacked types in general. For example, TypeVarTuple.__iter__ returns Unpack[self].

(@gvanrossum, in case you haven't followed the chain of comments on the PR so far, there were two main reasons to try doing it this way: 1. It means we can avoid the need for a middleman classes UnpackedTypeVarTuple and StarredTuple like we had before; and 2. It means we can also easily support unpacking of other types at runtime, which might be useful for people wanting to experiment with typing features - e.g. Jelle mentioned he'd like to experiment with Unpack[TypedDict type].)

This raises two questions:

  1. How much does it matter if the way that Unpack is used in typing.py doesn't match the spirit in which the PEP introduced it? (It feels a bit ugly to me, but I wonder whether I'm being too perfectionist. An alternative might be to have two separate things, Unpack as the backwards-compatible version of *, then a separate wrapper Unpacked for eliminating the middleman classes.)
  2. What should we do about unpacking a native tuple? In bpo-43224: Implement PEP 646 changes to genericaliasobject.c #31019, this essentially returns a special native 'starred tuple' type. I was about to change it so it instead returned typing.Unpack[self] - but I realise this is against the precedent set by the new union operator, where Foo | Bar also returns a special native type types.UnionType rather than typing.Union[Foo, Bar]. @gvanrossum in particular, do you know whether this was a deliberate decision? Is this a precedent we should respect?

@JelleZijlstra
Copy link
Member

Thanks for bringing this up! Here are my takes:

  1. I agree there's some tension here, but I'd rather keep the number of classes lower. Having a separate Unpacked and Unpack doing virtually the same thing would be confusing.
  2. types.UnionType was created specifically because we didn't want built-in syntax to create an instance of a Python-defined class. We should do the same for the PEP 646 syntax.

@gvanrossum
Copy link
Member

Exactly what Jelle says.

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Lib/test/test_typing.py Show resolved Hide resolved
@mrahtz
Copy link
Contributor Author

mrahtz commented Feb 6, 2022

@JelleZijlstra @gvanrossum Thanks for confirming re Unpack design. In that case, we'll stick with the plan where unpacking things from typing.py (that is, Tuple and TypeVarTuple) results in an Unpack[], and unpacking a native tuple results in a special version of the native tuple type that's starred.

@JelleZijlstra
Copy link
Member

(Fixed a merge conflict)

@mrahtz
Copy link
Contributor Author

mrahtz commented Mar 5, 2022

On the subject of what to do about type substitution, Pradeep had the excellent idea of moving discussion to a future PR, given how long even this PR has taken. For now I've excised all the extra logic required, instead raising NotImplementedError in the cases it means we currently don't support (re-purposing the corresponding tests so it's clear exactly which things we currently can't deal with).

For what it's worth, though, given that both Pradeep and Jelle lean towards thinking simple is better, for the follow-up PR, I'll try drafting a prototype of Jelle's suggestion: having the subscription operator just return a new GenericAlias.

@mrahtz
Copy link
Contributor Author

mrahtz commented Mar 5, 2022

P.S. bedevere-bot: I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@JelleZijlstra: please review the changes made to this pull request.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Let's do it.

I'm planning to merge this PR once 3.11.0a6 is out (cc @gvanrossum @Fidget-Spinner).

@JelleZijlstra
Copy link
Member

Congratulations!

@gvanrossum
Copy link
Member

Thanks @mrahtz for the code, and thanks @JelleZijlstra for the review. Thanks @pradeep90 for your help. This is a monumental change!

@mrahtz
Copy link
Contributor Author

mrahtz commented Mar 9, 2022

Fantastic! Thank you @JelleZijlstra and @pradeep90 for the review, and @gvanrossum for your continuing support :)

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.

8 participants