bpo-44801: Check arguments in substitution of ParamSpec in Callable #27585
Conversation
serhiy-storchaka
added
type-bug
serhiy-storchaka
requested review from
gvanrossum and
rhettinger
as code owners
There was a problem hiding this comment.
Phew, that was harder than I expected to review 😉 .
Thanks for fixing this Serhiy, it looks good to me (and also seems like it follows the PEP more closely now :).
We might not be able to backport this until 3.10.1. I don't think such a huge change can land in rc2.
| @@ -492,7 +487,13 @@ def __getitem__(self, item): | |||
| new_args = [] | |||
| for arg in self.__args__: | |||
| if _is_typevarlike(arg): | |||
| arg = subst[arg] | |||
| if _is_param_expr(arg): | |||
There was a problem hiding this comment.
Checking for something like _is_paramspec would be clearer IMO, but we don't have that function and it's kind of a waste to make a function just for this one-time use.
| C1 = Callable[P, T] | ||
| # substitution | ||
| self.assertEqual(C1[int, str], Callable[[int], str]) | ||
| self.assertEqual(C1[[int], str], Callable[[int], str]) |
There was a problem hiding this comment.
Hmm I feel a little strange about this. I feel that the new form is correct, but I also feel the old form should work. There's not much ambiguity in using the old form.
There was a problem hiding this comment.
The PEP explicitly claim that only substituting ParamSpec with the parameters expression should be accepted. It will also simplify the future code.
There was a problem hiding this comment.
Ah ok I see it now:
def f(x: X[int, int]) -> str: ... # Rejected
Thanks for the reminder.
| self.assertEqual(C1[[int, str], str], Callable[[int, str], str]) | ||
| self.assertEqual(C1[[], str], Callable[[], str]) |
| @@ -4656,6 +4672,15 @@ class Z(Generic[P]): | |||
| self.assertEqual(G5.__parameters__, G6.__parameters__) | |||
| self.assertEqual(G5, G6) | |||
|
|
|||
| G7 = Z[int] | |||
| self.assertEqual(G7.__args__, ((int,),)) | |||
There was a problem hiding this comment.
This isn't a critique of your PR, but a minor regret of mine: Looking at G5, G6, G7, I realized their __args__ should be (int, str, bool) or (int, ), etc.; not nested tuple with types. Nested tuple makes repr weird, and also nested tuple means any nested TypeVar or ParamSpec won't appear in __parameters__ and so can't be substituted.
I don't remember what made me code it like that back then, but it could be me not wanting to make the _GenericAlias.__getitem__ code complicated just for ParamSpec.
There was a problem hiding this comment.
It does not work for user generics because they can have more than one ParamSpec. For example:
class X(Generic[P1, P2]):
f: Callable[P1, int]
g: Callable[P2, str]
G1 = X[[int, str], [bytes]]
G2 = X[int, [str, bytes]]
assert G1.__args__ == ((int, str), (bytes,))
assert G2.__args__ == ((int,) (str, bytes))If flatten arguments, __args__ of G1 and G2 would be the same ((int, str, bytes)). But they are clearly different types.
I'll add a special test for this.
There was a problem hiding this comment.
Right, thanks for pointing that out! I forgot there was ambiguity with multiple ParamSpec. Hopefully some future PEP makes this clearer.
|
Thanks @serhiy-storchaka for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
…ythonGH-27585) (cherry picked from commit 3875a69) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-27598 is a backport of this pull request to the 3.10 branch. |
https://bugs.python.org/issue44801