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.

Sign up for GitHub

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

Jump to bottom

Reject ParamSpec-typed callables calls with insufficient arguments #17323

Merged
merged 9 commits into from
Sep 24, 2024
Merged

Reject ParamSpec-typed callables calls with insufficient arguments #17323

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5cdb753
Reject ParamSpec-typed callables calls with insufficient arguments
sterliakov Jun 4, 2024
3b2297f
Reuse params preprocessing logic for generic functions
sterliakov Jun 4, 2024
a32ad3f
Only perform deep expansion on overloads when ParamSpec is present
sterliakov Jun 4, 2024
63995e3
Tidy up code a bit
sterliakov Jun 10, 2024
be2c49b
Merge remote-tracking branch 'upstream/master' into bugfix/st-paramsp…
sterliakov Aug 21, 2024
0a658a7
Always pick ParamSpec-containing overloads as plausible candidates
sterliakov Aug 21, 2024
63f9438
Remove parameters thaat are no longer used
sterliakov Aug 21, 2024
786fb55
Merge branch 'master' into bugfix/st-paramspec-missing-args
sterliakov Sep 12, 2024
1903402
Undo style change to make it easier to review, feel free to add in se…
hauntsaninja Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 56 additions & 29 deletions mypy/checkexpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1716,36 +1716,16 @@ def check_callable_call(
callee = callee.copy_modified(ret_type=fresh_ret_type)

if callee.is_generic():
need_refresh = any(
isinstance(v, (ParamSpecType, TypeVarTupleType)) for v in callee.variables
callee, formal_to_actual = self.adjust_generic_callable_params_mapping(
callee, args, arg_kinds, arg_names, formal_to_actual, context
)
callee = freshen_function_type_vars(callee)
callee = self.infer_function_type_arguments_using_context(callee, context)
if need_refresh:
# Argument kinds etc. may have changed due to
# ParamSpec or TypeVarTuple variables being replaced with an arbitrary
# number of arguments; recalculate actual-to-formal map
formal_to_actual = map_actuals_to_formals(
arg_kinds,
arg_names,
callee.arg_kinds,
callee.arg_names,
lambda i: self.accept(args[i]),
)
callee = self.infer_function_type_arguments(
callee, args, arg_kinds, arg_names, formal_to_actual, need_refresh, context
)
if need_refresh:
formal_to_actual = map_actuals_to_formals(
arg_kinds,
arg_names,
callee.arg_kinds,
callee.arg_names,
lambda i: self.accept(args[i]),
)

param_spec = callee.param_spec()
if param_spec is not None and arg_kinds == [ARG_STAR, ARG_STAR2]:
if (
param_spec is not None
and arg_kinds == [ARG_STAR, ARG_STAR2]
and len(formal_to_actual) == 2
):
arg1 = self.accept(args[0])
arg2 = self.accept(args[1])
if (
Expand Down Expand Up @@ -2351,6 +2331,9 @@ def check_argument_count(
# Positional argument when expecting a keyword argument.
self.msg.too_many_positional_arguments(callee, context)
ok = False
elif callee.param_spec() is not None and not formal_to_actual[i]:
self.msg.too_few_arguments(callee, context, actual_names)
ok = False
return ok

def check_for_extra_actual_arguments(
Expand Down Expand Up @@ -2625,7 +2608,7 @@ def check_overload_call(
arg_types = self.infer_arg_types_in_empty_context(args)
# Step 1: Filter call targets to remove ones where the argument counts don't match
plausible_targets = self.plausible_overload_call_targets(
arg_types, arg_kinds, arg_names, callee
args, arg_types, arg_kinds, arg_names, callee, context
)

# Step 2: If the arguments contain a union, we try performing union math first,
Expand Down Expand Up @@ -2743,12 +2726,52 @@ def check_overload_call(
self.chk.fail(message_registry.TOO_MANY_UNION_COMBINATIONS, context)
return result

def adjust_generic_callable_params_mapping(
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need to be deduplicated anymore because it's only called once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I'd rather keep this separated: that block is rather long and not absolutely trivial, it looks better as a separate method to my eyes. However, there was another problem not detected by linter for some reason (uhm, sorry, because ARG001 is not enabled?.. Why?..), pushed to remove arguments that are no longer necessary.

self,
callee: CallableType,
args: list[Expression],
arg_kinds: list[ArgKind],
arg_names: Sequence[str | None] | None,
formal_to_actual: list[list[int]],
context: Context,
) -> tuple[CallableType, list[list[int]]]:
need_refresh = any(
isinstance(v, (ParamSpecType, TypeVarTupleType)) for v in callee.variables
)
callee = freshen_function_type_vars(callee)
callee = self.infer_function_type_arguments_using_context(callee, context)
if need_refresh:
# Argument kinds etc. may have changed due to
# ParamSpec or TypeVarTuple variables being replaced with an arbitrary
# number of arguments; recalculate actual-to-formal map
formal_to_actual = map_actuals_to_formals(
arg_kinds,
arg_names,
callee.arg_kinds,
callee.arg_names,
lambda i: self.accept(args[i]),
)
callee = self.infer_function_type_arguments(
callee, args, arg_kinds, arg_names, formal_to_actual, need_refresh, context
)
if need_refresh:
formal_to_actual = map_actuals_to_formals(
arg_kinds,
arg_names,
callee.arg_kinds,
callee.arg_names,
lambda i: self.accept(args[i]),
)
return callee, formal_to_actual

def plausible_overload_call_targets(
self,
args: list[Expression],
arg_types: list[Type],
arg_kinds: list[ArgKind],
arg_names: Sequence[str | None] | None,
overload: Overloaded,
context: Context,
) -> list[CallableType]:
"""Returns all overload call targets that having matching argument counts.

Expand Down Expand Up @@ -2782,8 +2805,12 @@ def has_shape(typ: Type) -> bool:
formal_to_actual = map_actuals_to_formals(
arg_kinds, arg_names, typ.arg_kinds, typ.arg_names, lambda i: arg_types[i]
)

with self.msg.filter_errors():
if typ.is_generic() and typ.param_spec() is not None:
typ, formal_to_actual = self.adjust_generic_callable_params_mapping(
typ, args, arg_kinds, arg_names, formal_to_actual, context
)
Copy link
Contributor

@A5rocks A5rocks Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm confused about why this depends on typ.param_spec() when it doesn't in check_callable_call. Removing that makes sense to me but also fails a ton of test cases. Do you know why?

Just noticed you added this in a commit presumably after seeing those test case failures. Do you have any sort of deeper intuition for why this is right? (this is the only bit that doesn't make sense to me)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plain typevars are irrelevant here: this function looks for targets that have proper arguments count (signature shape, ignoring parameters types). Typevar resolution cannot affect this shape since it's a 1:1 mapping. ParamSpec, on the other hand, might be expanded into some fixed-arg form if we can solve this ParamSpec now.

Removing this constraint results in a bunch of overload resolution failures, like

from typing import Any, Callable, overload, TypeVar

F = TypeVar('F', bound=Callable[..., Any])

@overload
def dec(x: F) -> F: ...
@overload
def dec(x: str) -> Callable[[F], F]: ...
def dec(x) -> Any:
    pass

@dec
def f(name: str) -> int:
    return 0

@dec('abc')
def g(name: str) -> int:
    return 0

reveal_type(f)  # N: Revealed type is "def (name: builtins.str) -> builtins.int"
reveal_type(g)  # N: Revealed type is "def (name: builtins.str) -> builtins.int"

If we try to expand, dec("abc") begins to match the first overload, and then "str not callable". That's because infer_function_type_arguments is too lax and does not include some constraints - I don't know whether it's intended, but sounds difficult to fix. Such impossible resolutions are filtered out later in check_callable_call, so it doesn't cause any false negatives with regular calls - but plausible_overload_call_targets does not need such precision, we're just filtering out overloads that certainly can't match.

This piece supports scenarios similar to testRunParamSpecOverload testcase - without such check, ParamSpec overloads will be unexpectedly filtered out. As an alternative, we can avoid this expansion entirely and just report a match whenever paramspec is encountered in overload - that may be even more robust, actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, and that alternative also passes all testcases. I think it's a safer solution that won't harm: the most severe impact of adding an extra overload would be a performance overhead to check one. Overload+paramspec isn't a very popular combination, so the performance impact shouldn't be noticeable. I pushed a version with this update, let's see what CI says.


if self.check_argument_count(
typ, arg_types, arg_kinds, arg_names, formal_to_actual, None
):
Expand Down
111 changes: 111 additions & 0 deletions test-data/unit/check-parameter-specification.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2204,3 +2204,114 @@ parametrize(_test, Case(1, b=2), Case(3, b=4))
parametrize(_test, Case(1, 2), Case(3))
parametrize(_test, Case(1, 2), Case(3, b=4))
[builtins fixtures/paramspec.pyi]

[case testRunParamSpecInsufficientArgs]
from typing_extensions import ParamSpec, Concatenate
from typing import Callable

_P = ParamSpec("_P")

def run(predicate: Callable[_P, None], *args: _P.args, **kwargs: _P.kwargs) -> None: # N: "run" defined here
predicate() # E: Too few arguments
predicate(*args) # E: Too few arguments
predicate(**kwargs) # E: Too few arguments
predicate(*args, **kwargs)

def fn() -> None: ...
def fn_args(x: int) -> None: ...
def fn_posonly(x: int, /) -> None: ...

run(fn)
run(fn_args, 1)
run(fn_args, x=1)
run(fn_posonly, 1)
run(fn_posonly, x=1) # E: Unexpected keyword argument "x" for "run"

[builtins fixtures/paramspec.pyi]

[case testRunParamSpecConcatenateInsufficientArgs]
from typing_extensions import ParamSpec, Concatenate
from typing import Callable

_P = ParamSpec("_P")

def run(predicate: Callable[Concatenate[int, _P], None], *args: _P.args, **kwargs: _P.kwargs) -> None: # N: "run" defined here
predicate() # E: Too few arguments
predicate(1) # E: Too few arguments
predicate(1, *args) # E: Too few arguments
predicate(1, *args) # E: Too few arguments
predicate(1, **kwargs) # E: Too few arguments
predicate(*args, **kwargs) # E: Argument 1 has incompatible type "*_P.args"; expected "int"
predicate(1, *args, **kwargs)

def fn() -> None: ...
def fn_args(x: int, y: str) -> None: ...
def fn_posonly(x: int, /) -> None: ...
def fn_posonly_args(x: int, /, y: str) -> None: ...

run(fn) # E: Argument 1 to "run" has incompatible type "Callable[[], None]"; expected "Callable[[int], None]"
run(fn_args, 1, 'a') # E: Too many arguments for "run" \
# E: Argument 2 to "run" has incompatible type "int"; expected "str"
run(fn_args, y='a')
run(fn_args, 'a')
run(fn_posonly)
run(fn_posonly, x=1) # E: Unexpected keyword argument "x" for "run"
run(fn_posonly_args) # E: Missing positional argument "y" in call to "run"
run(fn_posonly_args, 'a')
run(fn_posonly_args, y='a')

[builtins fixtures/paramspec.pyi]

[case testRunParamSpecConcatenateInsufficientArgsInDecorator]
from typing_extensions import ParamSpec, Concatenate
from typing import Callable

P = ParamSpec("P")

def decorator(fn: Callable[Concatenate[str, P], None]) -> Callable[P, None]:
def inner(*args: P.args, **kwargs: P.kwargs) -> None:
fn("value") # E: Too few arguments
fn("value", *args) # E: Too few arguments
fn("value", **kwargs) # E: Too few arguments
fn(*args, **kwargs) # E: Argument 1 has incompatible type "*P.args"; expected "str"
fn("value", *args, **kwargs)
return inner

@decorator
def foo(s: str, s2: str) -> None: ...

[builtins fixtures/paramspec.pyi]

[case testRunParamSpecOverload]
from typing_extensions import ParamSpec
from typing import Callable, NoReturn, TypeVar, Union, overload

P = ParamSpec("P")
T = TypeVar("T")

@overload
def capture(
sync_fn: Callable[P, NoReturn],
*args: P.args,
**kwargs: P.kwargs,
) -> int: ...
@overload
def capture(
sync_fn: Callable[P, T],
*args: P.args,
**kwargs: P.kwargs,
) -> Union[T, int]: ...
def capture(
sync_fn: Callable[P, T],
*args: P.args,
**kwargs: P.kwargs,
) -> Union[T, int]:
return sync_fn(*args, **kwargs)

def fn() -> str: return ''
def err() -> NoReturn: ...

reveal_type(capture(fn)) # N: Revealed type is "Union[builtins.str, builtins.int]"
reveal_type(capture(err)) # N: Revealed type is "builtins.int"

[builtins fixtures/paramspec.pyi]
Loading