bpo-42195: Ensure consistency of Callable's __args__ in collections.abc and typing

[Fidget-Spinner]

This patch fixes bpo-42195 via changing the GenericAlias builtin and subclassing for collections.abc.Callable's __class_getitem__. The changes are:

1. Made GenericAlias subclassable.
2. Subclassed it in collections.abc.Callable's __class_getitem__ to allow for consistentency with typing.Callable's __args__. This includes flattening the __args__.

>>> import collections.abc
>>> alias = collections.abc.Callable[[int, str], dict]

>>> alias
collections.abc.Callable[[int, str], dict]

>>> alias.__args__
(int, str, dict)

3. Added tests for the above.
4. Remove bad typing tests which subclassed from Callable[... T].
5. Loosened type checks for Callable's argtypes in Callable[[argtypes], resulttype] to prepare for PEP 612.

https://bugs.python.org/issue42195

[gvanrossum]

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].

[Fidget-Spinner]

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].

There is work on that by Shantanu at this bpo and PR #22848.

[Fidget-Spinner]

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.

[Fidget-Spinner]

@gvanrossum, I gave the issue more thought and I'm probably going ahead with the Python implementation since callable genericalias seems contentious. In the future someone else can just replace the Python version with the C version if it comes to fruition.

Supporting __args__ for a Callable[[int, str], dict] as ((int, str), dict) is surprisingly rather inconvenient work. A few typing methods have to become recursive to account for nested tuples of types (mainly the ones that check for TypeVars and replacement of TypeVars). The C counterparts to those methods have to become recursive too. Update: We can represent the internal args as (Tuple[int, str], dict) which avoids the need for recursion.

Although __args__ of ((int, str), dict) is the cleanest IMO, the effort to achieve it versus just making both __args__ (int, str, dict) feels disproportionately high. Maybe this is why the original code author for typing (I'd assume Ivan) made a conscious decision to flatten the args.

Edit: Sorry about the force push, I'm not a git sensei :(.

[Fidget-Spinner]

@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 typing._PosArgs[x] == collections.abc._PosArgs[x] too. So typing.Callable[[int], str].__args__ == Callable[[int], str].__args__. (Note that typing.Callable[[int], str] != Callable[[int], str])

[gvanrossum]

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. :-)

[gvanrossum]

[me]

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. :-)

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.)

[gvanrossum]

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

1. allocate
2. tuplify
3. set attrs
4. gc-track

and then only put (2) and (3) in the shared helper function.

What do you think?

[gvanrossum]

Okay, for 3.10 I think this is the right solution.

For 3.9, I think we need to issue a DeprecationWarning(using the warnings module) with the same message (for both TypeErrors below) and return what was returned in 3.9.0.

[gvanrossum]

So in the 3.9 backport this file should be untouched.

[Fidget-Spinner]

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

1. allocate
2. tuplify
3. set attrs
4. gc-track

and then only put (2) and (3) in the shared helper function.

What do you think?

Ok, I did just that. This is really helpful and clear feedback. Thanks!

[gvanrossum]

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 Callable[X, Y] is valid for any X and Y, and falls into two cases:

• If X is a list, then __args__ becomes `(*X, Y)
• In all other cases, __args__ becomes (X, Y)

[Fidget-Spinner]

I wonder if we would have to simplify things so that Callable[X, Y] is valid for any X and Y, and falls into two cases:

• If X is a list, then __args__ becomes `(*X, Y)
• In all other cases, __args__ becomes (X, Y)

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?

[gvanrossum]

Let's do that for both. This will make the introduction of ParamSpec much simpler.

[gvanrossum]

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!).

[gvanrossum]

Actually @pablogsal may be busy, let's see if @brandtbucher is interested.

[pablogsal]

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 for the delay. I can give it a go this night :)

[pablogsal]

I have reviewed the C code and it looks good to me! Good job @Fidget-Spinner !

[Fidget-Spinner]

I have reviewed the C code and it looks good to me! Good job @Fidget-Spinner !

Thank you for taking time out of your busy schedules to review this pablo and guido :).

[gvanrossum]

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.

[Fidget-Spinner]

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.

Sure, give me some time :). I'll make another PR to update docs too after the backport lands.