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: Initial implementation of PEP 646 in typing.py #24527

Closed
wants to merge 5 commits into from

Conversation

mrahtz
Copy link
Contributor

@mrahtz mrahtz commented Feb 14, 2021

Currently this only supports the Unpack[Ts] form, but once the changes from PEP 637 are merged, *Ts also works with no extra changes.

I don't think this is finished yet - we need more tests, especially for aliases - but this is enough to begin discussion.

Notes:

  • I've renamed _TypeVarLike to _BoundVarianceMixin, since TypeVarTuple doesn't support bounds or variance yet, and it would be confusing if TypeVarTuple wasn't a _TypeVarLike. I've created _TypeVarTypes as a replacement.
  • I've renamed _check_generic to _check_type_parameter_count to make its purpose clearer, and overhauled its body to support TypeVarTuples.
  • In _check_type_parameter_count, I've removed the check on whether cls is a generic a) so that the function only does one thing, and b) because I couldn't see any way for the function to be called on a class which wasn't generic (and none of the tests fail, so shrug).

https://bugs.python.org/issue43224

@mrahtz mrahtz changed the title Initial implementation of PEP 646 in typing.py bpo-43224: Initial implementation of PEP 646 in typing.py Feb 14, 2021
@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Feb 14, 2021

Hi! I only took a brief look (sorry I'll try to do a more thorough review if the PEP is accepted), but here's a possible general checklist of things you may have to do:

  • Add to typing.py (done)
  • Add to PEP585 GenericAlias objects' __parameters__ (unless you don't want to treat TypeVarTuple as a TypeVar in builtin generics, sorry I'm not too clear about the PEP's implementation details). You can see how PEP612 ParamSpec added itself in PR bpo-41559: Implement PEP 612 - Add ParamSpec and Concatenate to typing #23702 (it's in Objects/genericaliasobject.c).
  • Ensure some support in typing.get_args and typing.get_origin. Though I think this depends on the extent that the PEP would like to support runtime correctness.

Please correct me if you think I missed anything.

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.

This looks great. Thanks for working on this, Matthew!

Apart from the suggestions above from @Fidget-Spinner, could you also look at typing_extensions.py? I saw a few uses of isinstance(..., TypeVar). Would be worth checking if they need updating.

https://github.com/python/typing/blob/c7a981aaab7b20b93c911f1e5dd0ab2a8e30fd6c/typing_extensions/src_py3/typing_extensions.py#L1136

Also, could you add tests for Tensor[()] and Tensor (without parameters)?

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

I'm catching up on old items. Is this ready to merge from your POV?

@gvanrossum
Copy link
Member

I'm catching up on old items. Is this ready to merge from your POV?

Never mind, I didn't read this, I thought it was another PEP update. As an implementation update it will have to wait (but the proof-of-concept implementation is very much appreciated).

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Mar 1, 2021

I'm catching up on old items. Is this ready to merge from your POV?

Thanks for doing this. I don't think PEP 646 is accepted by the SC yet so no.

Edit: woops messages posted at the same time and crossed

@mrahtz
Copy link
Contributor Author

mrahtz commented Mar 2, 2021

Sorry, yes, should have said - just a proof of concept for now, posted here so it would get some initial feedback from the right people.

@gvanrossum gvanrossum marked this pull request as draft March 2, 2021 16:50
@github-actions
Copy link

github-actions bot commented Apr 2, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Apr 2, 2021
@mrahtz mrahtz closed this Jul 11, 2021
@mrahtz mrahtz deleted the pep646 branch July 11, 2021 11:11
@mrahtz
Copy link
Contributor Author

mrahtz commented Dec 8, 2021

This PR is closed, but @Fidget-Spinner, I wanted to ask your advice on something.

After a lot of churn on the PEP itself, I'm restarting work on the CPython implementation. I was going to tackle adding support to genericaliasobject.c, but I happened to notice that some of the work in #23702 you referenced was backed out in 859577c. Before going into this, I thought I should ask - is there anything I should be aware of here? Is genericaliasobject.c still the right place to be working in order to make e.g. list[*Ts] work properly (e.g. adding the corresponding TypeVarTuple to list[*Ts].__parameters__)?

Thanks!

@Fidget-Spinner
Copy link
Member

@mrahtz Sorry, I've lost track of PEP 646 after its many revisions, so I'm not sure what the [*Ts] evaluates to now (does it just evaluate to list[TypeVarTuple[...]] ?).

To clarify, if you want a base implementation for type checkers without the runtime stuff, genericaliasobject.c probably needs no modification, it supports completely arbitrary subscriptions like list[1]. If you want the runtime stuff, you can implement it in either C or Python depending on your needs:

  1. Python way: TypeVarTuple[].__parameters__ should contain itself. This is kind of hacky, but the C genericalias objects blindly take any iterable it sees in any object's __parameters__ and append them to its own. Example:
Y = type("Y", (), {})
y = Y()
y.__parameters__ = (1,)
>>> list[y].__parameters__
(1,)
  1. C "legit" way: modify genericaliasobject.c like what you mentioned above to identify TypeVarTuple directly.

PS. I would love to review the C and typing.py code for the implementation. While I'm useless at the grammar and parser, I'm somewhat better at the bytecode interpreter, builtin objects, and typing.py stuff.

@pradeep90
Copy link
Contributor

@mrahtz Could you explain what you mean by list[*Ts]? That's not a valid type as per our PEP. Is this something needed for generic aliases or for some edge case in the runtime implementation?

@mrahtz
Copy link
Contributor Author

mrahtz commented Dec 10, 2021

@pradeep90 Oops, sorry, I meant tuple[*Ts].

@Fidget-Spinner Ah, yeah, tuple[*Ts] basically evaluates to tuple[iter(next(Ts))] now. And great to hear there's a Python way of handling this now! I'll have a play around with this, and let you know if I have any questions - thank you for your kind offer to review stuff :)

@gvanrossum
Copy link
Member

(Why are we discussing this in a closed PR? :-)

Anyway, surely you meant tuple[next(iter(Ts))], not the other way around. But, I don't like the possibility of throwing away extra values which that spelling implies. It is actually hard to find a closed expression that explains what tuple[*Ts] means, since it has a special meaning to type checkers but a different meaning at runtime. I think at runtime it would be equivalent to tuple[tuple(Ts)].

Also, from the runtime's POV tuple is not special, and we need to assign a meaning to X[*Y] for any X and Y. IMO it should always mean X[tuple(Y)]. We're now getting in dangerous territory though, since when keyword args for subscripts were being discussed, python-ideas nearly exploded over the question of what argument X.__getitem__ should see when Y is exactly one item long: does this call X.__getitem__(tuple(Y)) or X.__getitem__(tuple(Y)[0])? (This is relevant because X[1] does not pass a tuple, but X[1, 2] passes the tuple (1, 2). This in turn is a 20-plus-year-old API design issue for __getitem__ that we can't fix.)

@mrahtz mrahtz mannequin mentioned this pull request Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants