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
Warn on invalid *args
and **kwargs
with ParamSpec
#13892
Changes from 7 commits
bc829f8
1430c35
7779d27
65dfc86
c921844
94ad912
d25447a
7540268
d888f0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,8 @@ | |
from mypy.nodes import ( | ||
ARG_NAMED, | ||
ARG_POS, | ||
ARG_STAR, | ||
ARG_STAR2, | ||
CONTRAVARIANT, | ||
COVARIANT, | ||
GDEF, | ||
|
@@ -843,6 +845,7 @@ def analyze_func_def(self, defn: FuncDef) -> None: | |
defn.type = result | ||
self.add_type_alias_deps(analyzer.aliases_used) | ||
self.check_function_signature(defn) | ||
self.check_paramspec_definition(defn) | ||
if isinstance(defn, FuncDef): | ||
assert isinstance(defn.type, CallableType) | ||
defn.type = set_callable_name(defn.type, defn) | ||
|
@@ -1282,6 +1285,71 @@ def check_function_signature(self, fdef: FuncItem) -> None: | |
elif len(sig.arg_types) > len(fdef.arguments): | ||
self.fail("Type signature has too many arguments", fdef, blocker=True) | ||
|
||
def check_paramspec_definition(self, defn: FuncDef) -> None: | ||
func = defn.type | ||
assert isinstance(func, CallableType) | ||
|
||
param_spec_var = next( | ||
(var for var in func.variables if isinstance(var, ParamSpecType)), None | ||
) | ||
if param_spec_var is None: | ||
return | ||
|
||
args = func.var_arg() | ||
kwargs = func.kw_arg() | ||
if args is None and kwargs is None: | ||
return # Looks like this function does not have starred args | ||
|
||
has_paramspec_callable = any( | ||
arg.from_concatenate | ||
or (arg.arg_types and isinstance(get_proper_type(arg.arg_types[0]), ParamSpecType)) | ||
for arg in get_proper_types(func.arg_types) | ||
if isinstance(arg, CallableType) | ||
) | ||
if not has_paramspec_callable: | ||
return # Callable[ParamSpec, ...] was not found | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this check necessary? I think that we should also reject things like this:
|
||
|
||
args_type = args.typ if args is not None else None | ||
kwargs_type = kwargs.typ if kwargs is not None else None | ||
|
||
args_defn = next( | ||
( | ||
arg_def | ||
for arg_def, arg_kind in zip(defn.arguments, defn.arg_kinds) | ||
if arg_kind == ARG_STAR | ||
), | ||
None, | ||
) | ||
kwargs_defn = next( | ||
( | ||
arg_def | ||
for arg_def, arg_kind in zip(defn.arguments, defn.arg_kinds) | ||
if arg_kind == ARG_STAR2 | ||
), | ||
None, | ||
) | ||
|
||
args_defn_type = args_defn.type_annotation if args_defn is not None else None | ||
kwargs_defn_type = kwargs_defn.type_annotation if kwargs_defn is not None else None | ||
|
||
# This may happen on invalid `ParamSpec` args / kwargs definition: | ||
if not ( | ||
isinstance(args_defn_type, UnboundType) | ||
and args_defn_type.name.endswith(".args") | ||
or isinstance(kwargs_defn_type, UnboundType) | ||
and kwargs_defn_type.name.endswith(".kwargs") | ||
): | ||
# Looks like both `*args` and `**kwargs` are not `ParamSpec` | ||
# It might be something else, skipping. | ||
return | ||
|
||
if not isinstance(args_type, ParamSpecType) or not isinstance(kwargs_type, ParamSpecType): | ||
self.fail( | ||
f'ParamSpec must have "*args" typed as "{param_spec_var.name}.args" and "**kwargs" typed as "{param_spec_var.name}.kwargs"', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if these refer to different ParamSpec variables? E.g. |
||
func, | ||
code=codes.VALID_TYPE, | ||
) | ||
|
||
def visit_decorator(self, dec: Decorator) -> None: | ||
self.statement = dec | ||
# TODO: better don't modify them at all. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A function could have multiple ParamSpec type variables. This only picks the first one. Maybe it's enough to look at the
*args
and**kwargs
arguments only, and ignore the variables? I.e., if either*args
or**kwargs
has a ParamSpec type, then we'd require that both of them are defined and they both refer to a ParamSpec (and the same ParamSpec).