Skip to content

gh-95337: update TypeVarTuple example #95338

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

Merged
merged 8 commits into from
Aug 30, 2022
Merged

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented Jul 27, 2022

Notice that "T" is only used once in the function signature, which Pylance will highlight as an error.
In this case, Any is just as meaningful as T (and I would argue semantically better, we don't care what the first element is)
All of the examples (checked with reveal_type) are still valid.

Notice that "T" is only used once in the function signature, which Pylance will highlight as an error.
In this case, Any is just as meaningful as T (and I would argue semantically better, we don't care what the first element is)
All of the examples (checked with reveal_type) are still valid.
@gvanrossum
Copy link
Member

@mrahtz @pradeep90 Do you agree?

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.

In this case, Any is just as meaningful as T (and I would argue semantically better, we don't care what the first element is)

I'm against removing the T. It's there to demonstrate how tuples are split into T and *Ts and what each variable is bound to. Otherwise, there's nothing in the docs to show this key behavior.

Given that this example is for explanation, I don't think we should change it just to avoid Pylance lint errors.

Notice that "T" is only used once in the function signature, which Pylance will highlight as an error.

If you feel really strongly about the lint error, you could change it to split_first_and_rest(xs: tuple[T, *Ts]) -> tuple[T, tuple[*Ts]]: .... But that complicates the introduction to a concept that can already be hard to understand. I think the example is fine as it is.

@adriangb
Copy link
Contributor Author

adriangb commented Jul 27, 2022

Given that this example is for explanation, I don't think we should change it just to avoid Pylance lint errors.

That's fair, I wasn't trying to pose that as a substantial point, that's just how I noticed it.

I'm against removing the T. It's there to demonstrate how tuples are split into T and *Ts and what each variable is bound to. Otherwise, there's nothing in the docs to show this key behavior.

Then I think we should change the example to fully demonstrate that behavior (i.e. the more complicated version you just proposed or similar). As is, that behavior is encoded in comments but not the code. Personally I think Any demonstrates TypeVarTuple well and keeps the example simple (simpler in fact because there's one less type variable to grok).

@mrahtz
Copy link
Contributor

mrahtz commented Aug 2, 2022

Sorry for the slow reply - busy week.

I agree with:

It's there to demonstrate how tuples are split into T and *Ts and what each variable is bound to. Otherwise, there's nothing in the docs to show this key behavior.

To put it another way, I think it's important to emphasise that TypeVarTuples aren't anything 'special' - that the mental model is "TypeVarTuples just unpack into something that looks like a bunch of TypeVars, so it can be used with other regular TypeVars" - as opposed to "TypeVarTuple is this special thing that needs to be used on its own and I have to arbitrarily star", which is the nuance I think a reader might get from

def remove_first_element(tup: tuple[Any, *Ts]) -> tuple[*Ts]:
    return tup[1:]

(Ok, yes, this example does at least mix a TypeVarTuple with an Any - but that might not be enough to give the impression that TypeVarTuples can also be mixed with regular TypeVars.)

Having said that, I also agree with:

Then I think we should change the example to fully demonstrate that behavior (i.e. the more complicated version you just proposed or similar).

More than just fixing Pylance lint, I do think it's important our examples implicitly instruct the user in what kinds of things are valid to do. Since (iiuc?) most type checkers do enforce this thing that it's not valid to have an unreferenced type variable, I think it would be good to avoid giving the user the impression that this might be possible.

But that complicates the introduction to a concept that can already be hard to understand. I think the example is fine as it is.

It does make it more complicated, but only a little bit - iiuc:

 def move_first_element_to_last(tup: tuple[T, *Ts]) -> tuple[*Ts, T]:
   return (*tup[1:], tup[0])

If the reader understands the splitting of tuple[T, *Ts] in the argument type, I think they also should be fine with it in the return type - also there's also the nice parallel of the star in the method body to the star in the type signature, emphasising that star in the type signature has the same meaning as regular unpacking.

Having said that, I don't feel strongly about this, so Pradeep, if you still have hesitations, I trust your judgement, and we can leave it as it is.

@adriangb
Copy link
Contributor Author

adriangb commented Aug 2, 2022

Thank you for the feedback folks. It sounds like the consensus is that if there was going to be a change made it would be in the direction of the more complex example that uses the type var in the return type, so I'll at least change this PR to propose that instead of using Any.

@JelleZijlstra
Copy link
Member

I like Matthew's idea of having the example move the first tuple element to the end: it's a fairly realistic example and yet not too complicated.

@adriangb
Copy link
Contributor Author

adriangb commented Aug 2, 2022

That's a great example @mrahtz , I had missed it. I like that it also uses tuple[*Ts, T] which shows that the TypeVarTuple can be unpacked anywhere and not just the last/remainder.

For completeness, I added the invalid case of move_first_element_to_last(tup=())

@AlexWaygood AlexWaygood added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Aug 2, 2022
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I like this PR!


# T is bound to int, Ts is bound to ()
# Return value is (), which has type tuple[()]
remove_first_element(tup=(1,))
# Return value is (1, ), which has type tuple[int]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think PEP 8 says there shouldn't be a space after the comma here: (1,)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

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.

Lgtm. Thanks, Adrian!

@adriangb adriangb changed the title gh-95337: update TypeVarTuple example to remove unused TypeVar gh-95337: update TypeVarTuple example Aug 3, 2022
@JelleZijlstra JelleZijlstra merged commit 07f12b5 into python:main Aug 30, 2022
@miss-islington
Copy link
Contributor

Thanks @adriangb for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @adriangb and @JelleZijlstra, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 07f12b5c1567581aa77d523e462b0e7f75c1f05c 3.10

@bedevere-bot
Copy link

GH-96431 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 30, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 30, 2022
Co-authored-by: Alex Waygood <[email protected]>
(cherry picked from commit 07f12b5)

Co-authored-by: Adrian Garcia Badaracco <[email protected]>
@AlexWaygood
Copy link
Member

Oops, my bad, no TypeVarTuples in 3.10

@AlexWaygood AlexWaygood removed the needs backport to 3.10 only security fixes label Aug 30, 2022
miss-islington added a commit that referenced this pull request Aug 30, 2022
Co-authored-by: Alex Waygood <[email protected]>
(cherry picked from commit 07f12b5)

Co-authored-by: Adrian Garcia Badaracco <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news topic-typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants