Skip to content
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

Merged
merged 9 commits into from Oct 30, 2022

Conversation

sobolevn
Copy link
Member

Closes #13890

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

The case in xarray-dataclasses is not correct. Let me fix it.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sobolevn sobolevn requested review from hauntsaninja and JukkaL Oct 14, 2022
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a few questions/suggestions.

mypy/semanal.py Outdated
if isinstance(arg, CallableType)
)
if not has_paramspec_callable:
return # Callable[ParamSpec, ...] was not found
Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

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
Copy link
Collaborator

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

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"',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@sobolevn
Copy link
Member Author

Ok, I broke black :)

» black mypy/semanal.py
error: cannot format mypy/semanal.py: INTERNAL ERROR: Black produced code that is not equivalent to the source.  Please report a bug on https://github.com/psf/black/issues.  This diff might be helpful: /var/folders/qn/2gssw9hx48g81chw0398hlrr0000gn/T/blk_7_ir_fua.log

Oh no! 💥 💔 💥
1 file failed to reformat.

@sobolevn
Copy link
Member Author

@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: P is not bound.
But, this is a different story. I will work on it in a different PR.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra
Copy link
Member

Do you know what the code was that caused Black to crash? I'd like to know so we can fix it in Black.

@sobolevn
Copy link
Member Author

@JelleZijlstra no, I was not able to reproduce it. I've changed something and it is gone.

@sobolevn sobolevn requested a review from JukkaL Oct 22, 2022
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

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

@hauntsaninja hauntsaninja merged commit 41c1602 into python:master Oct 30, 2022
17 checks passed
hauntsaninja pushed a commit to hauntsaninja/mypy that referenced this pull request Oct 30, 2022
@sobolevn
Copy link
Member Author

LiteralString is the next thing to get merged. I need to polish it a bit: today / tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False negative with ParamSpec
4 participants