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

Support ParamSpec mapping with functools.partial #17355

Merged

Conversation

sterliakov
Copy link
Contributor

@sterliakov sterliakov commented Jun 10, 2024

Follow-up for #17323, resolving a false positive discovered there.

Closes #17960.

This enables use of functools.partial to bind some *args or **kwargs on a callable typed with ParamSpec.

This comment has been minimized.

This comment has been minimized.

@sterliakov sterliakov force-pushed the bugfix/st-paramspec-with-functools-partial branch from fd79c7a to 01390c0 Compare September 25, 2024 16:46

This comment has been minimized.

@sterliakov sterliakov marked this pull request as ready for review September 26, 2024 00:31
@ilevkivskyi
Copy link
Member

FWIW I don't like this. partial should remain within a plugin. It is not special enough to make significant changes in core mypy logic.

@sterliakov
Copy link
Contributor Author

TBH, I tend to agree with @ilevkivskyi - there's a balance between "we want to support this case correctly" and "we don't want to embed too many library-specific details into the typechecker". As far as I see, there's no other way to track such partial applications to ParamSpec components in any other way, but maybe the right thing is to just say "we don't want to support this".

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@sterliakov
Copy link
Contributor Author

sterliakov commented Oct 18, 2024

@ilevkivskyi tagging you since you gave some feedback on the first revision. I managed to remove the scary part - error messages are less specific now, but only special_sig = "partial" escapes the plugin boundary. Are you comfortable with this amount of special-casing? (filtering error messages for "too few arguments" is way too fragile to consider here, IMO - those diagnostics can be issued by multiple checks, and we're only interested in one specific source of those)

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

LG, I don't really understand the bound start args logic, but it will be easy to tweak if it will cause problems.


def run_bad3(func: Callable[Concatenate[int, P], T], *args: P.args, **kwargs: P.kwargs) -> T:
func2 = partial(func, 1, *args)
return func2(1, **kwargs) # E: Argument 1 has incompatible type "int"; expected "P.args"
Copy link
Member

Choose a reason for hiding this comment

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

The bodies of run_bad2 and run_bad3 look identical. Did you want to test something else here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ough, thanks for spotting! Updated.

This comment has been minimized.

This comment has been minimized.

@sterliakov
Copy link
Contributor Author

LG, I don't really understand the bound start args logic, but it will be easy to tweak if it will cause problems.

I just invented a counterexample to that test, it's indeed not sufficient to check by arg kinds. Last commit should address that problem and make the logic more clear as well.

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

starlette (https://github.com/encode/starlette)
- starlette/concurrency.py:38: error: Too few arguments  [call-arg]

@ilevkivskyi ilevkivskyi merged commit a706914 into python:master Oct 26, 2024
19 checks passed
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.

[1.12 regression] functools.partial and ParamSpec
3 participants