Warn on invalid *args and **kwargs with ParamSpec
#13892
Conversation
This comment has been minimized.
This comment has been minimized.
|
The case in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mypy/semanal.py
Outdated
| if isinstance(arg, CallableType) | ||
| ) | ||
| if not has_paramspec_callable: | ||
| return # Callable[ParamSpec, ...] was not found |
There was a problem hiding this comment.
Is this check necessary? I think that we should also reject things like this:
def f(*x: int, **y: P.kwargs) -> C[P]:
pass
mypy/semanal.py
Outdated
| assert isinstance(func, CallableType) | ||
|
|
||
| param_spec_var = next( | ||
| (var for var in func.variables if isinstance(var, ParamSpecType)), None |
There was a problem hiding this comment.
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).
mypy/semanal.py
Outdated
|
|
||
| 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.
What if these refer to different ParamSpec variables? E.g. *args: P1.args, **kwargs: P2.kwargs? Could we catch this error as well (or perhaps we are catching it)?
|
Ok, I broke |
|
@JukkaL thanks a lot for the review! This check is now quite complex to find all the corner cases you've mentioned. I want to highlight that this case is not supported on purpose: from typing import Generic, Callable, ParamSpec
P = ParamSpec('P')
# This must be allowed:
class Some(Generic[P]):
def call(self, *args: P.args, **kwargs: P.kwargs): ...
# TODO: this probably should be reported.
def call(*args: P.args, **kwargs: P.kwargs): ...We need to detected functions like this and raise an error: |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
Do you know what the code was that caused Black to crash? I'd like to know so we can fix it in Black. |
|
@JelleZijlstra no, I was not able to reproduce it. I've changed something and it is gone. |
There was a problem hiding this comment.
This is great, thanks for working on this!
I'm going to merge so we can get it into 0.990. If people have concrete suggestions, we'll address in follow up PRs. I think we can still improve some stuff here, e.g. there's the TODO in this PR, we should have a test case for something like #13966, I wouldn't be surprised if there are some cases missing or where we can have an even more helpful error, etc
|
|
Closes #13890