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 3 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
14 changes: 10 additions & 4 deletions mypy/checkexpr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,11 @@ def infer_arg_types_in_context(
# Fill in the rest of the argument types.
for i, t in enumerate(res):
if not t:
res[i] = self.accept(args[i])
if arg_kinds[i] == ARG_STAR2:
res[i] = self.accept(args[i], self.chk.named_generic_type('typing.Mapping',
[self.named_type('builtins.str'), AnyType(TypeOfAny.special_form)]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately the Any type here may result in false negatives, since it's used for type inference:

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

f(**{'x': 'y'})  # No error here after this change

More generally, it would be great if we can avoid inferring Any types in fully annotated code.

Random idea: Use Mapping[str, <nothing>] as the context, potentially only if the kwargs expression is an empty dictionary expression (check for it here). This would certainly be a hack, but at least it's a pretty localized hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see... Another idea can be to relax the check here to allow the case where typ is dict[<nothing>, <nothing>]. At first I didn't like this solution as I thought it was less elegant, but if trying to change the inferred type will result in a hacky code like you said, maybe this one is better?

mypy/mypy/checkexpr.py

Lines 3933 to 3949 in 985a20d

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)])))

else:
res[i] = self.accept(args[i])
assert all(tp is not None for tp in res)
return cast(List[Type], res)

Expand Down Expand Up @@ -1401,11 +1405,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
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]
8 changes: 7 additions & 1 deletion test-data/unit/check-kwargs.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,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 +491,9 @@ m = {} # type: Mapping[str, object]
f(**m)
g(**m) # TODO: Should be an error
[builtins fixtures/dict.pyi]

[case testPassingEmptyDictWithStars]
def f(): pass

f(**{})
[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