Skip to content

Improve ambiguous **kwarg checking #9573

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

Merged
merged 6 commits into from
Oct 18, 2020
Merged

Improve ambiguous **kwarg checking #9573

merged 6 commits into from
Oct 18, 2020

Conversation

esoma
Copy link
Contributor

@esoma esoma commented Oct 10, 2020

Description

Fixes #4708
Allows for multiple ambiguous **kwarg unpacking in a call -- all ambiguous **kwargs will map to all formal args that do not have a certain actual arg.

Fixes #9395
Defers ambiguous **kwarg mapping until all other unambiguous formal args have been mapped -- order of **kwarg unpacking no longer affects the arg map.

Test Plan

Additional tests included in PR, no existing tests were changed. Below are new tests that would exhibit different behaviors if they're run without code changes in the PR:


[case testPassingMultipleKeywordVarArgs]
from typing import Any, Dict
def f1(a: 'A', b: 'A') -> None: pass
def f2(a: 'A') -> None: pass
def f3(a: 'A', **kwargs: 'A') -> None: pass
def f4(**kwargs: 'A') -> None: pass
d = None # type: Dict[Any, Any]
d2 = None # type: Dict[Any, Any]
f1(**d, **d2)
f2(**d, **d2)
f3(**d, **d2)
f4(**d, **d2)
class A: pass
[builtins fixtures/dict.pyi]

f1(**d, **d2) produces no error in PR. Current produces Too many arguments for "f1" (#4708)
f2(**d, **d2) produces no error in PR. Current produces Too many arguments for "f2" (#4708)


[case testTypedDictAsStarStarAndDictAsStarStar]
from mypy_extensions import TypedDict
from typing import Any, Dict

TD = TypedDict('TD', {'x': int, 'y': str})

def f1(x: int, y: str, z: bytes) -> None: ...
def f2(x: int, y: str) -> None: ...

td: TD
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"
[builtins fixtures/dict.pyi]

f1(**d, **td) produces no error in PR. Current produces "f1" gets multiple values for keyword argument "x" and error: "f1" gets multiple values for keyword argument "y" (#9395)
f2(**d, **td) produces Too many arguments for "f2". Current produces "f2" gets multiple values for keyword argument "x" and "f2" gets multiple values for keyword argument "y" (#9395)

# **kwargs which cannot be mapped with certainty (non-TypedDict
# **kwargs).
and not all(actual_kinds[m] == nodes.ARG_STAR2 and
not isinstance(actual_types[m], TypedDictType)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mypy/checkexpr.py:4131: error: Never apply isinstance() to unexpanded types;

I don't see a terribly clean way to fix this. # type: ignore okay here? Or other suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
not isinstance(actual_types[m], TypedDictType)
not isinstance(get_proper_type(actual_types[m]), TypedDictType)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks. I totally misunderstood the error.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@msullivan msullivan merged commit 6077dc8 into python:master Oct 18, 2020
JelleZijlstra pushed a commit that referenced this pull request Aug 26, 2021
### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants