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

Ambiguous **kwargs only map to unmapped arguments that have a matching type. #9705

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

esoma
Copy link
Contributor

@esoma esoma commented Nov 6, 2020

DO NOT MERGE

Just getting some eyes on this for now. There are some consequences to this change discussed in #9676 and that I'll continue here. In any case, should probably wait for #9629 to be merged as there will probably be some merge conflicts.

Description

Closes #9676

  • non-TypedDict **kwargs will only map to parameters which have a compatible type
  • introduces a new check for non-TypedDict **kwargs that are not able to map to at least 1 parameter

Test Plan

Added the testCallKwargsDifferentTypes test.

@esoma
Copy link
Contributor Author

esoma commented Nov 6, 2020

Continuing a line of conversation from #9676:

@hauntsaninja

In fact I'd go a little further than your example and say errors in the following cases are valuable:

f(a=A(), **ad)
f(a=A(), **ad, **bd)

Agreed, that was actually the first rule I tried. I changed it at some point, forgot I changed it and now I can't remember why 🙃. In any case, I've reverted it to have this behavior in my branch and its behaving well.

But we might be in the minority here (or at least, on the wrong side of the do-ocracy). It looks like mypy changed its behaviour for the *args case in #7392 and consistency between args and kwargs seems like a good thing.

Right, I'm not sure I'm convinced of the need for that change, but I can understand it and it's not my position to be convinced anyway. I've thought about this a bit more and I've probably misinterpreted why @JukkaL reasons Case A should be an error and Case B/C should not. Let's say the following:

d: Dict[str, str] = {}

def f0() -> None: pass
f0(**d) # no error

def f1(x: int = 0) -> None: pass
f1(**d) # no error

def f2(x: str = '') -> None: pass
f2(**d) # no error

def f3(x: str = '', **kwargs: int) -> None: pass
f3(**d) # no error

def f4(x: str = '', **kwargs: int) -> None: pass
f4(x='foo', **d) # error

def f5(x: int = 0, **kwargs: int) -> None: pass
f5(**d) # error

The rule here is that d can always be empty. However! If d is not compatible with any parameters and there is a **kwargs parameter it is an error. Since **kwargs will always consume the rest of the remaining arguments we can say that if d doesn't map anywhere else then the user probably means for it to be compatible with **kwargs.

I believe this would allow for Case A to raise an error, but B and C to not. This seems goofy to me, but I think it makes some practical sense, it aligns better with the *args behavior and doesn't step on #9629. If this is the way to go I'm happy to make the change.

@@ -485,7 +562,7 @@ def g(arg: int = 0, **kwargs: object) -> None:

d = {} # type: Dict[str, object]
f(**d)
g(**d) # E: Argument 1 to "g" has incompatible type "**Dict[str, object]"; expected "int"
g(**d)

m = {} # type: Mapping[str, object]
f(**m)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the line below there is a # TODO: should be an error
Presumably it shouldn't be anymore?

@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2020

Diff from mypy_primer, showing the effect of this PR on open source code:

+ pydantic/datetime_parse.py:241: error: unused 'type: ignore' comment

- poetry/inspection/info.py:121: error: Argument 2 to "PackageInfo" has incompatible type "**Dict[str, Union[str, List[str], None]]"; expected "Optional[List[str]]"
- poetry/inspection/info.py:121: error: Argument 2 to "PackageInfo" has incompatible type "**Dict[str, Union[str, List[str], None]]"; expected "Optional[str]"
+ poetry/inspection/info.py:121: error: Argument 2 in call to "PackageInfo" cannot unpack into any parameters; no parameter accepts type "Dict[str, Union[str, List[str], None]]"
+ poetry/inspection/info.py:121: error: Too many arguments for "PackageInfo"

+ homeassistant/runner.py:116: error: unused 'type: ignore' comment

- zerver/lib/markdown/tabbed_sections.py:163: error: Argument 1 to "TabbedSectionsGenerator" has incompatible type "**Dict[str, str]"; expected "Mapping[str, str]"  [arg-type]
+ zerver/lib/markdown/tabbed_sections.py:163: error: Argument 1 in call to "TabbedSectionsGenerator" cannot unpack into any parameters; no parameter accepts type "Dict[str, str]"  [call-arg]
+ zerver/lib/markdown/tabbed_sections.py:163: error: Too many arguments for "TabbedSectionsGenerator"  [call-arg]
+ zerver/lib/markdown/__init__.py:1892: error: Argument 2 in call to "__init__" of "Markdown" cannot unpack into any parameters; no parameter accepts type "Dict[str, Union[bool, int, List[Any]]]"  [call-arg]
+ zerver/lib/markdown/__init__.py:1892: error: Too many arguments for "__init__" of "Markdown"  [call-arg]
- zerver/lib/markdown/__init__.py:1892: error: Argument 2 to "__init__" of "Markdown" has incompatible type "**Dict[str, Union[bool, int, List[Any]]]"; expected "Optional[Mapping[str, Mapping[str, Any]]]"  [arg-type]
- zerver/lib/markdown/__init__.py:1892: error: Argument 2 to "__init__" of "Markdown" has incompatible type "**Dict[str, Union[bool, int, List[Any]]]"; expected "Optional[Sequence[Union[str, Extension]]]"  [arg-type]
- zerver/lib/markdown/__init__.py:1892: error: Argument 2 to "__init__" of "Markdown" has incompatible type "**Dict[str, Union[bool, int, List[Any]]]"; expected "Optional[int]"  [arg-type]
- zerver/lib/markdown/__init__.py:1892: error: Argument 2 to "__init__" of "Markdown" has incompatible type "**Dict[str, Union[bool, int, List[Any]]]"; expected "Union[Literal['xhtml'], Literal['html'], None]"  [arg-type]
- zerver/lib/markdown/nested_code_blocks.py:75: error: Argument 1 to "NestedCodeBlocksRenderer" has incompatible type "**Dict[str, str]"; expected "Mapping[str, str]"  [arg-type]
+ zerver/lib/markdown/nested_code_blocks.py:75: error: Argument 1 in call to "NestedCodeBlocksRenderer" cannot unpack into any parameters; no parameter accepts type "Dict[str, str]"  [call-arg]
+ zerver/lib/markdown/nested_code_blocks.py:75: error: Too many arguments for "NestedCodeBlocksRenderer"  [call-arg]

+ lib/streamlit/server/server.py:387: error: unused 'type: ignore' comment

- pyppeteer/launcher.py:147: error: Argument 2 to "Popen" has incompatible type "**Dict[str, Optional[Any]]"; expected "bool"
- pyppeteer/launcher.py:147: error: Argument 2 to "Popen" has incompatible type "**Dict[str, Optional[Any]]"; expected "int"
- pyppeteer/launcher.py:147: error: Argument 2 to "Popen" has incompatible type "**Dict[str, Optional[Any]]"; expected "str"

+ src/werkzeug/routing.py:751: error: unused 'type: ignore' comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spurious error with dynamic keyword arguments
1 participant