Skip to content

Fix argument checking on empty dict with double stars #9629

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

Merged
merged 5 commits into from
Aug 26, 2021
Merged

Fix argument checking on empty dict with double stars #9629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5dcac8
Infer type of **kwargs with a context of Mapping[str, Any]
momohatt Oct 22, 2020
2a88bb8
Relax 'Too many arguments' check for **kwargs
momohatt Oct 23, 2020
13dc4d2
Rename test
momohatt Oct 23, 2020
ff90cf3
Relax check on kwarg key type
momohatt Oct 31, 2020
989512b
Add a test to prevent inferring Dict[str, Any]
momohatt Nov 6, 2020
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
32 changes: 14 additions & 18 deletions mypy/checkexpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1401,11 +1401,13 @@ def check_for_extra_actual_arguments(self,
ok = True # False if we've found any error

for i, kind in enumerate(actual_kinds):
if i not in all_actuals and (
kind != nodes.ARG_STAR or
if (i not in all_actuals and
# We accept the other iterables than tuple (including Any)
# as star arguments because they could be empty, resulting no arguments.
is_non_empty_tuple(actual_types[i])):
(kind != nodes.ARG_STAR or is_non_empty_tuple(actual_types[i])) and
# Accept all types for double-starred arguments, because they could be empty
# dictionaries and we can't tell it from their types
kind != nodes.ARG_STAR2):
# Extra actual: not matched by a formal argument.
ok = False
if kind != nodes.ARG_NAMED:
Expand Down Expand Up @@ -3934,21 +3936,15 @@ def is_valid_var_arg(self, typ: Type) -> bool:

def is_valid_keyword_var_arg(self, typ: Type) -> bool:
"""Is a type valid as a **kwargs argument?"""
if self.chk.options.python_version[0] >= 3:
return is_subtype(typ, self.chk.named_generic_type(
'typing.Mapping', [self.named_type('builtins.str'),
AnyType(TypeOfAny.special_form)]))
else:
return (
is_subtype(typ, self.chk.named_generic_type(
'typing.Mapping',
[self.named_type('builtins.str'),
AnyType(TypeOfAny.special_form)]))
or
is_subtype(typ, self.chk.named_generic_type(
'typing.Mapping',
[self.named_type('builtins.unicode'),
AnyType(TypeOfAny.special_form)])))
ret = (
is_subtype(typ, self.chk.named_generic_type('typing.Mapping',
[self.named_type('builtins.str'), AnyType(TypeOfAny.special_form)])) or
is_subtype(typ, self.chk.named_generic_type('typing.Mapping',
[UninhabitedType(), UninhabitedType()])))
if self.chk.options.python_version[0] < 3:
ret = ret or is_subtype(typ, self.chk.named_generic_type('typing.Mapping',
[self.named_type('builtins.unicode'), AnyType(TypeOfAny.special_form)]))
return ret

def has_member(self, typ: Type, member: str) -> bool:
"""Does type have member with the given name?"""
Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/check-ctypes.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,6 @@ import ctypes
intarr4 = ctypes.c_int * 4

x = {"a": 1, "b": 2}
intarr4(**x) # E: Too many arguments for "Array"
intarr4(**x)

[builtins fixtures/floatdict.pyi]
15 changes: 12 additions & 3 deletions test-data/unit/check-kwargs.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,9 @@ d = None # type: Dict[str, A]
f(**d)
f(x=A(), **d)
d2 = None # type: Dict[str, B]
f(**d2) # E: Argument 1 to "f" has incompatible type "**Dict[str, B]"; expected "A"
f(x=A(), **d2) # E: Argument 2 to "f" has incompatible type "**Dict[str, B]"; expected "A"
f(**d2) # E: Argument 1 to "f" has incompatible type "**Dict[str, B]"; expected "A"
f(x=A(), **d2) # E: Argument 2 to "f" has incompatible type "**Dict[str, B]"; expected "A"
f(**{'x': B()}) # E: Argument 1 to "f" has incompatible type "**Dict[str, B]"; expected "A"
class A: pass
class B: pass
[builtins fixtures/dict.pyi]
Expand Down Expand Up @@ -396,7 +397,7 @@ class A: pass
from typing import Any, Dict
def f(*args: 'A') -> None: pass
d = None # type: Dict[Any, Any]
f(**d) # E: Too many arguments for "f"
f(**d)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this no longer an error?

Copy link
Contributor

@lazytype lazytype Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is addressed by the author (not me) in the PR description.

Do not check for "too many arguments" error when there are any double-starred arguments. This will lead to some false-negavites, see my comment here: #4001

The rationale being that since a Dict[Any, Any] could be an empty dictionary, so it might not be the case that there are too many arguments. I think this is a similar rationale to how indexing a dict where the key may not exist is not a type error, but instead deferred to runtime.

Copy link
Member

Choose a reason for hiding this comment

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

I'm open to persuasion on this, but to me it seems totally reasonable to give an error for f(**kwargs) when f takes no keyword arguments at all. It's true that kwargs could be empty, but the code makes little sense anyway.

Copy link
Contributor

@NeilGirdhar NeilGirdhar Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@JelleZijlstra One problem with giving an error on that is that it's a common pattern in Python's cooperative multiple inheritance to do forwarding like so:

class SomeMixin:
    def __init__(self, **kwargs):
        # Do other things...
        super().__init__(**kwargs)

SomeMixin has no idea what class super is, so it should forward keyword arguments at least. Then, it can be used like so:

class SomeBase:
    def __init__(self, x, **kwargs):
        self.x = x
        super().__init__(**kwargs)

class C(SomeMixin, SomeBase):
    pass

Not everyone uses cooperative multiple inheritance, but I don't think this pattern should raise any type errors.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that's a good point! Let's just land this in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much. I've been looking forward to this 😄

class A: pass
[builtins fixtures/dict.pyi]

Expand Down Expand Up @@ -491,3 +492,11 @@ m = {} # type: Mapping[str, object]
f(**m)
g(**m) # TODO: Should be an error
[builtins fixtures/dict.pyi]

[case testPassingEmptyDictWithStars]
def f(): pass
def g(x=1): pass

f(**{})
g(**{})
[builtins fixtures/dict.pyi]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add a test case where we check the this continues to generate an error:

def f(**kwargs: int) -> None: pass

f(**{'x': 'y'})  # Here should be an error

4 changes: 2 additions & 2 deletions test-data/unit/check-typeddict.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1584,8 +1584,8 @@ d = None # type: Dict[Any, Any]

f1(**td, **d)
f1(**d, **td)
f2(**td, **d) # E: Too many arguments for "f2"
f2(**d, **td) # E: Too many arguments for "f2"
f2(**td, **d)
f2(**d, **td)
[builtins fixtures/dict.pyi]

[case testTypedDictNonMappingMethods]
Expand Down