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 646] Allow cleanly substituting any tuple type for a TypeVarTuple #2162

Merged
merged 9 commits into from
Dec 8, 2021

Conversation

pradeep90
Copy link
Contributor

Our specification implicitly treated it as legal to replace a TypeVarTuple with a tuple such as Tuple[int, str]. However, we did not mention the natural implications for *args or Callable. We also did not address the behavior of unbounded tuples.

This led to artificial restrictions (such as "type variable tuples must be of known length").

This PR specifies that, wherever a Ts is used, we can legally replace it with Tuple[int, str] or Tuple[int, ...] or Tuple[int, *Ts, str]. Concretely, the following are valid:

  • *args: *Tuple[int, *Ts, str], etc.
  • Callable[[int, *Ts, str], None], etc.
  • Tuple[int, *Tuple[str, ...]], etc.

This replaces #2125 .

cc @mrahtz @gvanrossum

@gvanrossum
Copy link
Member

I'll review this once @mrahtz has approved it.

Copy link
Contributor

@mrahtz mrahtz left a comment

Choose a reason for hiding this comment

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

Lots of small comments, but basically looks good. Thanks, Pradeep, for getting this done so quickly!

pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I did not do a thorough review yet, but here are two nits.

In general, I worry a bit that this feels like a big rewrite, and as such the SC was right to un-approve the PEP and put it back in the queue. I doubt that they will end up rejecting it, but we are making more work for them, unless we add something like a cover letter when we resubmit saying "the update doesn't change anything fundamental, it just clarifies a bunch of stuff, and it makes one significant change (lifting the restriction on types with unknown length)." And they will still want to see for themselves that we're not (accidentally or intentionally) smuggling in something else.

pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated
@@ -414,6 +354,133 @@ Normal ``TypeVar`` instances can also be prefixed and/or suffixed:
z = prefix_tuple(x=0, y=(True, 'a'))
# Inferred type of z is Tuple[int, bool, str]

Unpacking a Tuple Type
Copy link
Member

Choose a reason for hiding this comment

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

We're already inconsistent, e.g. line 207 uses singular.

@pradeep90
Copy link
Contributor Author

@mrahtz Addressed all the comments.

@gvanrossum

In general, I worry a bit that this feels like a big rewrite

Yeah, I ignored the political aspect of it and just described from a technical point of view how tuple types affected certain sections :) Now that it's written down, we can trim any parts you feel aren't worth adding.

and as such the SC was right to un-approve the PEP and put it back in the queue. I doubt that they will end up rejecting it, but we are making more work for them, unless we add something like a cover letter when we resubmit saying "the update doesn't change anything fundamental, it just clarifies a bunch of stuff, and it makes one significant change (lifting the restriction on types with unknown length)." And they will still want to see for themselves that we're not (accidentally or intentionally) smuggling in something else.

A cover letter sounds good. We could say that we have preserved all of the old behavior and described some behavior that was previously unspecified or forbidden.

Main changes:

  1. Describe how to unpack tuple types.
  2. Remove section "Type Variable Tuples Must Have Known Length", which was an artificial restriction.
  3. Explain how Array without type arguments actually works.
  4. Explain how *args: *Tuple[<...>] works.
  5. Explain how Callable[[int, *Ts, str], None] works.

We can cut (4) if needed. Readers can infer it using (1).

@mrahtz
Copy link
Contributor

mrahtz commented Dec 2, 2021

@gvanrossum Just checking, are the next steps here for you to give us a thorough review of these changes, or for us to try making the delta smaller to minimise SC concerns? (If it's the former and you just haven't got round to it yet, no worries!)

@gvanrossum
Copy link
Member

The former. I will get to it.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I'm pretty happy here! Bunch of plural/singular nits, and one place where I think an example should actually be an error (but maybe I'm missing something).

pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Outdated
@@ -414,6 +354,133 @@ Normal ``TypeVar`` instances can also be prefixed and/or suffixed:
z = prefix_tuple(x=0, y=(True, 'a'))
# Inferred type of z is Tuple[int, bool, str]

Unpacking a Tuple Type
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing I am now definitely on the side of plurals.

pep-0646.rst Show resolved Hide resolved
pep-0646.rst Outdated Show resolved Hide resolved
pep-0646.rst Show resolved Hide resolved
Comment on lines +579 to +588
The behavior of a Callable containing an unpacked item, whether the
item is a ``TypeVarTuple`` or a tuple type, is to treat the elements
as if they were the type for ``*args``. So, ``Callable[[*Ts], None]``
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Comment on lines +598 to +607
When a generic class parameterised by a type variable tuple is used without
any type parameters, it behaves as if the type variable tuple was
substituted with ``Tuple[Any, ...]``:
Copy link
Member

Choose a reason for hiding this comment

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

Nice, isn't it! :-)

pep-0646.rst Outdated
unpacking ``Tuple[bool, bool]`` in the return type ``Tuple[float, *Ts,
float]`` gives back ``Tuple[float, bool, bool, float]``.

Unpacking an Unbounded Tuple Type
Copy link
Member

Choose a reason for hiding this comment

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

Plural...

pep-0646.rst Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

@pradeep90 Do you have time to finish this PR this week? We also have to consider #2177 and I'd like to get this moving to the SC again.

@pradeep90 pradeep90 force-pushed the master branch 2 times, most recently from 321cb78 to 2548e68 Compare December 8, 2021 21:04
@pradeep90
Copy link
Contributor Author

Done! Sorry, was working with Steven on the callable PEP. Lmk if you have further thoughts :)

@mrahtz We'll probably need to update the *args: *Ts grammar section to mention *Tuple[<...>].

@gvanrossum
Copy link
Member

Aargh! I wish you wouldn't force-push. Now GH won't show "commits since last review". But no worries, I'll figure it out.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM. I will merge this now, then we can move on to the other PR for this PEP, and then we can send it back to the SC (you can start drafting a cover letter now).

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

Successfully merging this pull request may close these issues.

4 participants