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

Fix argument checking on empty dict with double stars #9629

Merged
merged 5 commits into from
Aug 26, 2021

Conversation

momohatt
Copy link
Contributor

@momohatt momohatt commented Oct 23, 2020

Description

Closes #5580
Previously, the type of empty dicts are inferred as dict[<nothing>, <nothing>], which is not a subtype of Mapping[str, Any], and this has caused a false-positive error saying Keywords must be strings. This PR fixes it by inferring the types of double-starred arguments with a context of Mapping[str, Any].

Closes #4001 and closes #9007 (duplicate)
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 (comment)

Test Plan

Added a simple test testPassingEmptyDictWithStars. This single test can cover both of the issues above.

I also modified some existing tests that were added in #9573 and #6213.

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

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

I like your solution! Looks good, just one request for a test case to prevent regressions similar to what I commented about earlier.


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

@lazytype
Copy link
Contributor

@JukkaL looks like @momohatt addressed your last piece of feedback. can this be merged?

@@ -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

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

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 😄

@JelleZijlstra JelleZijlstra merged commit ea7fed1 into python:master Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants