Fix partial mypy plugin: make positionals keyword-only after a keyword-bound positional (#2191)#2446
Conversation
When a positional parameter of a function is applied by keyword via `partial` (e.g. `partial(foo, x=1)` for `foo(x, y)`), the resulting callable's remaining positional parameters can no longer be filled positionally at runtime, because `functools.partial` prepends the bound arguments and any positional call argument would collide with the keyword-bound parameter (`TypeError: foo() got multiple values ...`). The mypy plugin previously copied those parameters unchanged, inferring `def (y)` instead of `def (*, y)`, so `bar(2)` type-checked but failed at runtime. Now, once a positional parameter is applied by keyword (outside the leading positional prefix), every subsequent positional parameter is converted to keyword-only, so passing it positionally is correctly reported as a type error. Fixes dry-python#2191.
|
|
||
| def diff(self) -> CallableType: | ||
| #: Kinds of arguments that can be passed positionally. | ||
| _positional_kinds: ClassVar[frozenset[ArgKind]] = frozenset(( |
There was a problem hiding this comment.
Please, move this before the __init__
| main: | | ||
| from returns.curry import partial | ||
|
|
||
| def two_args(first: int, second: float) -> str: |
There was a problem hiding this comment.
please, test the same with *, second: float :)
|
|
||
| - Add `mypy>=1.19,<1.22` support | ||
|
|
||
| ### Bugfixes |
There was a problem hiding this comment.
This should go under new ## WIP section, 0.28.0 was already released.
| main: | | ||
| from returns.curry import partial | ||
|
|
||
| def two_args(first: int, second: float) -> str: |
There was a problem hiding this comment.
Please, also add a test case with first: int, /, second: int, third: str, where you bind second as kw arg :)
| intermediate_names: list[str | None], | ||
| ) -> bool: | ||
| """Tells whether an original argument was already applied.""" | ||
| if arg.kind in {ARG_STAR, ARG_STAR2}: |
There was a problem hiding this comment.
we can move this to be a constant as well :)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2446 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 80 81 +1
Lines 2485 2565 +80
Branches 437 44 -393
==========================================
+ Hits 2485 2565 +80 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… kw-only tests
- Move the partial bugfix entry into a new '0.28.1 WIP' changelog section,
since 0.28.0 is already released.
- Move '_positional_kinds' class var in 'Functions' before '__init__'.
- Extract the '{ARG_STAR, ARG_STAR2}' set into a module-level
'_VARIADIC_KINDS' constant, reused in both '_keyword_hole_at' and
'_was_applied'.
- Add typesafety case for an already keyword-only param
('first: int, *, second: float') bound by keyword.
- Add typesafety case for a positional-only param
('first: int, /, second: int, third: str') with 'second' bound by keyword.
|
Thanks for the review @sobolevn! I've addressed all five points in
Verified locally: the full |
|
|
||
| #: Kinds of arguments that consume the leftover positional or keyword | ||
| #: arguments (``*args`` and ``**kwargs``) and therefore cannot be applied. | ||
| _VARIADIC_KINDS: frozenset[ArgKind] = frozenset((ARG_STAR, ARG_STAR2)) |
There was a problem hiding this comment.
| _VARIADIC_KINDS: frozenset[ArgKind] = frozenset((ARG_STAR, ARG_STAR2)) | |
| _VARIADIC_KINDS: Final = frozenset((ARG_STAR, ARG_STAR2)) |
| """ | ||
|
|
||
| #: Kinds of arguments that can be passed positionally. | ||
| _positional_kinds: ClassVar[frozenset[ArgKind]] = frozenset(( |
There was a problem hiding this comment.
So, maybe this should be a constant as well? It is strange that something a module level constant and something is a class level attr.
|
|
||
| # `second` is already keyword-only, so binding the positional `first` | ||
| # by keyword leaves it keyword-only just as before: | ||
| reveal_type(partial(two_args, first=1)) # N: Revealed type is "def (*, second: float) -> str" |
There was a problem hiding this comment.
Please, also add partial(two_args, second=2) case
…s; add kw-bound last-arg test
|
Thanks for the follow-up review! Addressed all three in
Verified locally with the pinned mypy: The earlier |
Summary
returns.curry.partial's mypy plugin produced an unsound signature when apositional parameter was applied by keyword. For example:
Because
partialis backed byfunctools.partial, bound arguments areprepended and bound keywords are merged. Once an earlier positional parameter
is bound by keyword, the remaining positional parameters can no longer be
reached positionally at runtime, they must be passed by keyword. The plugin
was copying them unchanged (still positional), so
bar(2)type-checked butfailed at runtime.
Fix
In
Functions.diff(returns/contrib/mypy/_typeops/transform_callable.py),after determining which parameters were applied, we now detect the first
positional parameter that was applied by keyword (i.e. outside the leading
positional prefix that
functools.partialfills). Every remaining positionalparameter after that "hole" is converted to keyword-only
(
ARG_POS/ARG_OPT->ARG_NAMED/ARG_NAMED_OPT). Parameters boundpositionally, and functions with no keyword hole, are unaffected.
The revealed type for the example above becomes
def (*, y: int), andbar(2)is now correctly reported asToo many positional arguments.Tests / Verification
Added three regression cases to
typesafety/test_curry/test_partial/test_partial.yml:partial_positional_arg_by_keyword_makes_rest_kwonly- the issue's casenow reveals
def (*, second: float) -> str.partial_positional_arg_by_keyword_rejects_positional_call- a positionalcall on the partial is now a type error.
partial_middle_positional_arg_by_keyword_makes_rest_kwonly- only theparameters after the keyword hole become keyword-only; earlier positionals
are preserved.
All three fail on
masterand pass with this change. The fulltypesafety/test_curryandtypesafety/test_pipelinesuites pass (120cases), and
flake8(wemake-python-styleguide) andruff/ruff formatareclean on the changed files. Also manually verified at runtime that the new
keyword-only signature matches actual
functools.partialbehaviour.Disclosure: prepared with AI assistance; reviewed and verified locally.