bpo-42195: Ensure consistency of Callable's __args__ in collections.abc and typing#23060
bpo-42195: Ensure consistency of Callable's __args__ in collections.abc and typing#23060gvanrossum merged 31 commits into
Conversation
gvanrossum
left a comment
There was a problem hiding this comment.
I suppose it would be even nicer if the subclass was written in C and used by builtins.callable as well? So we can write callable[[int, int], str].
Sorry, something went wrong.
|
For now, I'll wait till we've decided how to go about doing this on the bpo. It seems that there are a few sensible approaches here. Apologies if I don't follow-up to your review that quickly as a result. |
Sorry, something went wrong.
dd71133 to
19d2973
Compare
November 19, 2020 04:48
|
@gvanrossum, I gave the issue more thought and I'm probably going ahead with the Python implementation since Supporting Although Edit: Sorry about the force push, I'm not a git sensei :(. |
Sorry, something went wrong.
1. allow equality between typing._PosArgs and collections.abc._PosArgs 2. allow equality checks between genericalias subclasses 3. add support to genericalias subclasses for union type expressions
|
@gvanrossum tests seem to be passing as of 327e1a5 with the _PosArgs implementation. Needs docs and whatsnew update (I'll do another PR for that if this gets accepted). Personally, the _PosArgs approach has grown on me quite a bit: it's a nice compromise. I managed to hack things together such that |
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
Sorry for leaving so many trivial code formatting comments. There are also a few important suggestions.
In general I am still not completely comfortable with this. In particular the change to how typing.Callable changes the form of its __args__ makes me worried about backwards compatibility (that code has been the same for a long time, unlike collections.abc.Callable, which is new in 3.9.0).
I also worry that 3.9.1rc1 is already out, so the earliest we could get this in would be 3.9.2. But if we ever want to fix this I think 3.9.2 is still better than 3.10 (let alone never).
Something makes me think that we're probably better off keeping typing.Callable unchanged and changing collections.abc.Callable so that its __args__ is a flat tuple. The trickery here is quite hard to follow. (Since I procrastinated on the review I feel this all the stronger. :-)
Sorry, something went wrong.
|
[me]
However, I also forgot that Shantanu warned in bpo-42195 that PEP 612 allows Callable[P, R] where P is a ParamSpec typevar. This could make a flat tuple ambiguous. (We should continue this discussion at the bpo issue.) |
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
Very close for the Python code. For the C code I am still not sure that this is the best refactoring. Basically we have some shared code (if not tuple, tuplify), then some non-shared code (allocate object), then some more shared code (set attributes), then some non-shared code (gc-track).
I think I may have led you astray by asking to share the code between the two. Or maybe twe could change the order to
- allocate
- tuplify
- set attrs
- gc-track
and then only put (2) and (3) in the shared helper function.
What do you think?
Sorry, something went wrong.
Ok, I did just that. This is really helpful and clear feedback. Thanks! |
Sorry, something went wrong.
|
Hi @Fidget-Spinner, this LGTM, but before I land it I have one question -- how will the PEP 612 implementation affect this? A particular thing to look into would be to see if we would run into trouble if people took this (or the original Callable implementations) and started using it with ParamSpec and Concatenate imported from typing_extensions? I fear that's not going to end well unless we remove most of the type checks. I wonder if we would have to simplify things so that
|
Sorry, something went wrong.
Yeah definitely agree here. I saw that ParamSpec in pyre_extensions already uses the inheriting from list hack to work with Callable. So do you want me to do that for both typing and collections.abc, or just collections.abc's Callable? |
Sorry, something went wrong.
|
Let's do that for both. This will make the introduction of ParamSpec much
simpler.
|
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
I think it's ready except for the commented-out blocks of code.
Also, I'd like to ask @pablogsal to see if he has time to review your C code (he has often caught things I missed in C code!).
Sorry, something went wrong.
gvanrossum
left a comment
There was a problem hiding this comment.
Actually @pablogsal may be busy, let's see if @brandtbucher is interested.
Sorry, something went wrong.
Sorry for the delay. I can give it a go this night :) |
Sorry, something went wrong.
pablogsal
left a comment
There was a problem hiding this comment.
I have reviewed the C code and it looks good to me! Good job @Fidget-Spinner !
Sorry, something went wrong.
Thank you for taking time out of your busy schedules to review this pablo and guido :). |
Sorry, something went wrong.
|
Okay, that's 3.10 down. @Fidget-Spinner, can you work on the backport to 3.9? There's one comment in typing.py indicating we need to do something different there. |
Sorry, something went wrong.
Sure, give me some time :). I'll make another PR to update docs too after the backport lands. |
Sorry, something went wrong.
This patch fixes bpo-42195 via changing the
GenericAliasbuiltin and subclassing forcollections.abc.Callable's__class_getitem__. The changes are:GenericAliassubclassable.collections.abc.Callable's__class_getitem__to allow for consistentency withtyping.Callable's__args__. This includes flattening the__args__.Callable[... T].Callable[[argtypes], resulttype]to prepare for PEP 612.https://bugs.python.org/issue42195