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

Make sure ParamSpec suffix and arg kind do match #13468

Merged
merged 3 commits into from
Aug 21, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Aug 21, 2022

I am mostly worried about the error message here :)

Closes #13460

@sobolevn sobolevn requested a review from AlexWaygood August 21, 2022 11:03
*args: P.kwargs, # E: Use ".args" for "*" argument
**kwargs: P.args, # E: Use ".kwargs" for "**" argument
) -> str:
return "foo"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add some reveal_types here, as a sanity check to make sure that the typo doesn't cause mypy to make any incorrect inferences elsewhere?

Suggested change
return "foo"
reveal_type(args) # N: Revealed type is "builtins.tuple[builtins.object, ...]"
reveal_type(kwargs) # N: Revealed type is "builtins.dict[builtins.str, builtins.object]"
return "foo"

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 is not true:

main:11: note: Revealed type is "P1.args`-1"  (diff)
main:12: note: Revealed type is "P1.kwargs`-1" (diff)

Copy link
Member

Choose a reason for hiding this comment

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

This is not true:

main:11: note: Revealed type is "P1.args`-1"  (diff)
main:12: note: Revealed type is "P1.kwargs`-1" (diff)

Oh, right. Maybe this, in that case?

Suggested change
return "foo"
a = args[0]
reveal_type(a) # N: Revealed type is "builtins.object"
for b, c in kwargs.items():
reveal_type(b) # N: Revealed type is "builtins.str"
reveal_type(c) # N: Revealed type is "builtins.object"
return "foo"

Copy link
Member Author

@sobolevn sobolevn Aug 21, 2022

Choose a reason for hiding this comment

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

I think this is too much :)

No other tests do anything similar.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Maybe:

mypy/typeanal.py Outdated
@@ -847,8 +846,12 @@ def anal_star_arg_type(self, t: Type, kind: ArgKind, nested: bool) -> Type:
if isinstance(tvar_def, ParamSpecType):
if kind == ARG_STAR:
make_paramspec = paramspec_args
if components[-1] == "kwargs":
self.fail('Use ".args" for "*" argument', t)
Copy link
Member

@AlexWaygood AlexWaygood Aug 21, 2022

Choose a reason for hiding this comment

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

Suggested change
self.fail('Use ".args" for "*" argument', t)
self.fail(f'Use "{'.'.join(components[:-1])}.args" for variadic "*" parameter', t)

mypy/typeanal.py Outdated
elif kind == ARG_STAR2:
make_paramspec = paramspec_kwargs
if components[-1] == "args":
self.fail('Use ".kwargs" for "**" argument', t)
Copy link
Member

@AlexWaygood AlexWaygood Aug 21, 2022

Choose a reason for hiding this comment

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

Suggested change
self.fail('Use ".kwargs" for "**" argument', t)
self.fail(f'Use "{'.'.join(components[:-1])}.kwargs" for variadic "**" parameter', t)

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

sobolevn commented Aug 21, 2022

I will leave this open for a couple of days, maybe some other ideas about the error message will araise :)

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks, one nit.

Another weirdness I noticed: if you do def inner(*args: P.__bound__, **kwargs: P.kwargs):, you get main.py:6: error: Name "P.__bound__" is not defined (expected though the wording could be improved) but also main.py:6: error: Name "P.kwargs" is not defined (not expected). https://mypy-play.net/?mypy=latest&python=3.10&gist=17f2477b6410e476d73dc720b43b31b2

@sobolevn
Copy link
Member Author

Another weirdness I noticed: if you do def inner(*args: P.bound, **kwargs: P.kwargs):, you get main.py:6: error: Name "P.bound" is not defined (expected though the wording could be improved) but also main.py:6: error: Name "P.kwargs" is not defined (not expected).

Thanks! This is something else. I will take a look later.

Good catch about !=, refactored.

@github-actions
Copy link
Contributor

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

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.

ParamSpec: mypy is happy for args to be annotated with P.kwargs and kwargs to be annotated with P.args
3 participants