Skip to content

bpo-44801: Check arguments in substitution of ParamSpec in Callable #27585

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.

Sign up for GitHub

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 3 commits into from
Aug 4, 2021
Merged

bpo-44801: Check arguments in substitution of ParamSpec in Callable #27585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd2ccff
bpo-44801: Check arguments in substitution of ParamSpec in Callable
serhiy-storchaka Aug 2, 2021
7583789
Merge branch 'main' into callable-paramspec-subst
serhiy-storchaka Aug 4, 2021
6770ca9
Add a test for multiple paramspecs in user generics.
serhiy-storchaka Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 26 additions & 26 deletions Lib/_collections_abc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,22 +426,16 @@ class _CallableGenericAlias(GenericAlias):
__slots__ = ()

def __new__(cls, origin, args):
return cls.__create_ga(origin, args)

@classmethod
def __create_ga(cls, origin, args):
if not isinstance(args, tuple) or len(args) != 2:
if not (isinstance(args, tuple) and len(args) == 2):
raise TypeError(
"Callable must be used as Callable[[arg, ...], result].")
t_args, t_result = args
if isinstance(t_args, (list, tuple)):
ga_args = tuple(t_args) + (t_result,)
# This relaxes what t_args can be on purpose to allow things like
# PEP 612 ParamSpec. Responsibility for whether a user is using
# Callable[...] properly is deferred to static type checkers.
else:
ga_args = args
return super().__new__(cls, origin, ga_args)
if isinstance(t_args, list):
args = (*t_args, t_result)
elif not _is_param_expr(t_args):
raise TypeError(f"Expected a list of types, an ellipsis, "
f"ParamSpec, or Concatenate. Got {t_args}")
return super().__new__(cls, origin, args)

@property
def __parameters__(self):
Expand All @@ -456,15 +450,15 @@ def __parameters__(self):
return tuple(dict.fromkeys(params))

def __repr__(self):
if _has_special_args(self.__args__):
if len(self.__args__) == 2 and _is_param_expr(self.__args__[0]):
return super().__repr__()
return (f'collections.abc.Callable'
f'[[{", ".join([_type_repr(a) for a in self.__args__[:-1]])}], '
f'{_type_repr(self.__args__[-1])}]')

def __reduce__(self):
args = self.__args__
if not _has_special_args(args):
if not (len(args) == 2 and _is_param_expr(args[0])):
args = list(args[:-1]), args[-1]
return _CallableGenericAlias, (Callable, args)

Expand All @@ -479,10 +473,11 @@ def __getitem__(self, item):
param_len = len(self.__parameters__)
if param_len == 0:
raise TypeError(f'{self} is not a generic class')
if (param_len == 1
and isinstance(item, (tuple, list))
and len(item) > 1) or not isinstance(item, tuple):
if not isinstance(item, tuple):
item = (item,)
if (param_len == 1 and _is_param_expr(self.__parameters__[0])
and item and not _is_param_expr(item[0])):
item = (list(item),)
item_len = len(item)
if item_len != param_len:
raise TypeError(f'Too {"many" if item_len > param_len else "few"}'
Expand All @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code will gone after moving the logic in ParamSpec.__getitem__ in #27511.

I am planning first to fix most bugs and to cover more cases by tests, and only then apply #27511, so we may not need to backport it.

arg = subst[arg]
if not _is_param_expr(arg):
raise TypeError(f"Expected a list of types, an ellipsis, "
f"ParamSpec, or Concatenate. Got {arg}")
else:
arg = subst[arg]
# Looks like a GenericAlias
elif hasattr(arg, '__parameters__') and isinstance(arg.__parameters__, tuple):
subparams = arg.__parameters__
Expand All @@ -502,32 +503,31 @@ def __getitem__(self, item):
new_args.append(arg)

# args[0] occurs due to things like Z[[int, str, bool]] from PEP 612
if not isinstance(new_args[0], (tuple, list)):
if not isinstance(new_args[0], list):
t_result = new_args[-1]
t_args = new_args[:-1]
new_args = (t_args, t_result)
return _CallableGenericAlias(Callable, tuple(new_args))


def _is_typevarlike(arg):
obj = type(arg)
# looks like a TypeVar/ParamSpec
return (obj.__module__ == 'typing'
and obj.__name__ in {'ParamSpec', 'TypeVar'})

def _has_special_args(args):
"""Checks if args[0] matches either ``...``, ``ParamSpec`` or
def _is_param_expr(obj):
"""Checks if obj matches either a list of types, ``...``, ``ParamSpec`` or
``_ConcatenateGenericAlias`` from typing.py
"""
if len(args) != 2:
return False
obj = args[0]
if obj is Ellipsis:
return True
if isinstance(obj, list):
return True
obj = type(obj)
names = ('ParamSpec', '_ConcatenateGenericAlias')
return obj.__module__ == 'typing' and any(obj.__name__ == name for name in names)


def _type_repr(obj):
"""Return the repr() of an object, special-casing types (internal helper).

Expand Down
45 changes: 42 additions & 3 deletions Lib/test/test_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,17 +581,33 @@ def test_paramspec(self):
Callable = self.Callable
fullname = f"{Callable.__module__}.Callable"
P = ParamSpec('P')
P2 = ParamSpec('P2')
C1 = Callable[P, T]
# substitution
self.assertEqual(C1[int, str], Callable[[int], str])
self.assertEqual(C1[[int], str], Callable[[int], str])
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

The PEP explicitly claim that only substituting ParamSpec with the parameters expression should be accepted. It will also simplify the future code.

Copy link
Member

Choose a reason for hiding this comment

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

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])
Copy link
Member

Choose a reason for hiding this comment

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

Good test! 👍

self.assertEqual(C1[..., str], Callable[..., str])
self.assertEqual(C1[P2, str], Callable[P2, str])
self.assertEqual(C1[Concatenate[int, P2], str],
Callable[Concatenate[int, P2], str])
self.assertEqual(repr(C1), f"{fullname}[~P, ~T]")
self.assertEqual(repr(C1[int, str]), f"{fullname}[[int], str]")
self.assertEqual(repr(C1[[int, str], str]), f"{fullname}[[int, str], str]")
with self.assertRaises(TypeError):
C1[int, str]

C2 = Callable[P, int]
self.assertEqual(C2[[int]], Callable[[int], int])
self.assertEqual(C2[[int, str]], Callable[[int, str], int])
self.assertEqual(C2[[]], Callable[[], int])
self.assertEqual(C2[...], Callable[..., int])
self.assertEqual(C2[P2], Callable[P2, int])
self.assertEqual(C2[Concatenate[int, P2]],
Callable[Concatenate[int, P2], int])
# special case in PEP 612 where
# X[int, str, float] == X[[int, str, float]]
self.assertEqual(C2[int, str, float], C2[[int, str, float]])
self.assertEqual(C2[int], Callable[[int], int])
self.assertEqual(C2[int, str], Callable[[int, str], int])
self.assertEqual(repr(C2), f"{fullname}[~P, int]")
self.assertEqual(repr(C2[int, str]), f"{fullname}[[int, str], int]")

Expand Down Expand Up @@ -4656,6 +4672,29 @@ class Z(Generic[P]):
self.assertEqual(G5.__parameters__, G6.__parameters__)
self.assertEqual(G5, G6)

G7 = Z[int]
self.assertEqual(G7.__args__, ((int,),))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Right, thanks for pointing that out! I forgot there was ambiguity with multiple ParamSpec. Hopefully some future PEP makes this clearer.

self.assertEqual(G7.__parameters__, ())

with self.assertRaisesRegex(TypeError, "many arguments for"):
Z[[int, str], bool]
with self.assertRaisesRegex(TypeError, "many arguments for"):
Z[P_2, bool]

def test_multiple_paramspecs_in_user_generics(self):
P = ParamSpec("P")
P2 = ParamSpec("P2")

class X(Generic[P, P2]):
f: Callable[P, int]
g: Callable[P2, str]

G1 = X[[int, str], [bytes]]
G2 = X[[int], [str, bytes]]
self.assertNotEqual(G1, G2)
self.assertEqual(G1.__args__, ((int, str), (bytes,)))
self.assertEqual(G2.__args__, ((int,), (str, bytes)))

def test_no_paramspec_in__parameters__(self):
# ParamSpec should not be found in __parameters__
# of generics. Usages outside Callable, Concatenate
Expand Down
26 changes: 18 additions & 8 deletions Lib/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ def _type_check(arg, msg, is_argument=True, module=None):
return arg


def _is_param_expr(arg):
return arg is ... or isinstance(arg,
(tuple, list, ParamSpec, _ConcatenateGenericAlias))


def _type_repr(obj):
"""Return the repr() of an object, special-casing types (internal helper).

Expand Down Expand Up @@ -236,7 +241,9 @@ def _prepare_paramspec_params(cls, params):
variables (internal helper).
"""
# Special case where Z[[int, str, bool]] == Z[int, str, bool] in PEP 612.
if len(cls.__parameters__) == 1 and len(params) > 1:
if (len(cls.__parameters__) == 1
and params and not _is_param_expr(params[0])):
assert isinstance(cls.__parameters__[0], ParamSpec)
return (params,)
else:
_check_generic(cls, params, len(cls.__parameters__))
Expand Down Expand Up @@ -1033,7 +1040,13 @@ def __getitem__(self, params):
new_args = []
for arg in self.__args__:
if isinstance(arg, self._typevar_types):
arg = subst[arg]
if isinstance(arg, ParamSpec):
arg = subst[arg]
if not _is_param_expr(arg):
raise TypeError(f"Expected a list of types, an ellipsis, "
f"ParamSpec, or Concatenate. Got {arg}")
else:
arg = subst[arg]
elif isinstance(arg, (_GenericAlias, GenericAlias, types.UnionType)):
subparams = arg.__parameters__
if subparams:
Expand Down Expand Up @@ -1131,17 +1144,15 @@ class _CallableGenericAlias(_GenericAlias, _root=True):
def __repr__(self):
assert self._name == 'Callable'
args = self.__args__
if len(args) == 2 and (args[0] is Ellipsis
or isinstance(args[0], (ParamSpec, _ConcatenateGenericAlias))):
if len(args) == 2 and _is_param_expr(args[0]):
return super().__repr__()
return (f'typing.Callable'
f'[[{", ".join([_type_repr(a) for a in args[:-1]])}], '
f'{_type_repr(args[-1])}]')

def __reduce__(self):
args = self.__args__
if not (len(args) == 2 and (args[0] is Ellipsis
or isinstance(args[0], (ParamSpec, _ConcatenateGenericAlias)))):
if not (len(args) == 2 and _is_param_expr(args[0])):
args = list(args[:-1]), args[-1]
return operator.getitem, (Callable, args)

Expand Down Expand Up @@ -1864,8 +1875,7 @@ def get_args(tp):
if isinstance(tp, (_GenericAlias, GenericAlias)):
res = tp.__args__
if (tp.__origin__ is collections.abc.Callable
and not (res[0] is Ellipsis
or isinstance(res[0], (ParamSpec, _ConcatenateGenericAlias)))):
and not (len(res) == 2 and _is_param_expr(res[0]))):
res = (list(res[:-1]), res[-1])
return res
if isinstance(tp, types.UnionType):
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2021-08-03-20-37-45.bpo-44801.i49Aug.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Ensure that the :class:`~typing.ParamSpec` variable in Callable
can only be substituted with a parameters expression (a list of types,
an ellipsis, ParamSpec or Concatenate).