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

Generic function can't make function call with mapping unpacking #11583

Open
NeilGirdhar opened this issue Nov 20, 2021 · 12 comments
Open

Generic function can't make function call with mapping unpacking #11583

NeilGirdhar opened this issue Nov 20, 2021 · 12 comments
Labels
bug mypy got something wrong topic-calls Function calls, *args, **kwargs, defaults

Comments

@NeilGirdhar
Copy link
Contributor

NeilGirdhar commented Nov 20, 2021

from typing import Any, Optional, TypeVar

_T = TypeVar('_T')

def f(b: bool = True, **kwargs: Any) -> None:
    pass

def g(x: _T) -> _T:
    f(**{'x': x})
    return x

gives

a.py:9: error: Argument 1 to "f" has incompatible type "**Dict[str, _T]"; expected "bool"
@NeilGirdhar NeilGirdhar added the bug mypy got something wrong label Nov 20, 2021
@sobolevn
Copy link
Member

Simplified sample:

def f(b: bool = True, **kwargs: str) -> None:
    pass

f(**{'x': 'x'})

This works in runtime, but mypy's output is not in sync. I will try to fix it!
Thanks for the report!

@NeilGirdhar
Copy link
Contributor Author

@sobolevn Wow, thanks for simplifying what I thought was a MWE! And double thanks for helping to fix it!

@sobolevn
Copy link
Member

sobolevn commented Nov 21, 2021

Related #1969

I am confused by this explicit test: https://github.com/python/mypy/blame/master/test-data/unit/check-kwargs.test#L478-L489

Maybe I am missing something 🤔

@NeilGirdhar
Copy link
Contributor Author

NeilGirdhar commented Nov 21, 2021

I am confused by this explicit test

Yeah, that test also seems just wrong to me 😄 .

@last-partizan
Copy link

Is there any progress on this issue?

PR #11589 by @sobolevn was closed, and @JukkaL suggested making strict **kwargs type checking optional, which is really good idea.

Recently pyright introduced strcit kwargs type checking (microsoft/pyright#5545), and they want to be consistent with other type-checkers, so they're doing it without option to disable it.

Maybe someone have workarounds for this? my best guess is to cast kwargs to Any before unpacking.

@erictraut
Copy link

erictraut commented Jul 24, 2023

Mypy's behavior here with dictionary unpacking is consistent with its behavior with iterable unpacking, so I assume it's intended behavior.

def f1(b: bool = True, **kwargs: str) -> None:
    pass

f1(**{"x": "x"}) # Mypy: incompatible type

def f2(b: bool = True, *args: str) -> None:
    pass

f2(*["a", "b", "c"]) # Mypy: incompatible type

I recently changed pyright's behavior to match mypy's in this regard because it affects overload matching behaviors, and it's important for overload matching to be consistent across type checkers.

If my assumption is incorrect and there is general agreement that this behavior is undesirable, then I'd recommend that it be changed for both dictionary unpacking and iterable unpacking for consistency.

Edit: Actually, the dictionary unpack and iterable unpack cases are not equivalent. In the iterable unpack case, the runtime will always map the first result of the unpacked iterable to parameter b. So ignore what I said above about consistency. There is still a question about whether mypy's current behavior with dictionary unpacking is intended. I assumed it was, so I matched the behavior in pyright, but I've received bug reports from several pyright users complaining about the change. I'm willing to revert the change in pyright if there's agreement that mypy's behavior is not desirable and there are plans to change it accordingly.

@last-partizan, here are a couple of workarounds:

  1. Modify the target function to specify that the first parameter (the one with a default argument) is positional-only:
def f(b: bool = True, /, **kwargs: str) -> None:
    pass
  1. Provide a first argument:
f(True, **{"x": "x"})

@last-partizan
Copy link

@hauntsaninja can we get your opinion on this?

Current behaviour is a bug and should be fixed, or it works like it should?

@ambv
Copy link
Contributor

ambv commented Jul 25, 2023

There's two things here.

First, the Mypy error message is clearly a bug. It claims the first argument to the f() call is the exploded dict expression but that's not true. The keyword arguments get exploded from the provided dict, and in effect, the first argument is actually never provided.

Second, while I can see how we got there, I can't think how the current Mypy behavior can be considered useful. It's not exactly a trivial fix:

  • the literal gets assigned the type dict[str, str] because maybe somewhere along the way we want to mutate it by adding more key-value pairs;
  • the signature is actually a TypedDict(total=False) with the "b" key being boolean and all other keys being strings. We cannot currently map this type exactly to what TypedDict allows us to express (see Support for default types in TypedDict #6131).

If you specify the kwargs dictionary literal as a TypedDict type, Mypy is happy with the result:

from typing import TypedDict, NotRequired

def f(b: bool = True, **kwargs: str) -> None:
    pass

class FArgs(TypedDict, total=False):
    b: bool
    x: str

some_kwargs: FArgs = {'x': 'x'}

f(**some_kwargs)

So solely on the fact that the variable is later used in dictionary unpacking on a signature, we would have to have Mypy infer {'x': 'x'} being an anonymously built TypedDict(total=False) type with b: bool and other keys being strings. This seems to go contrary to how inference works now, which is "infer early and report incompatible usage as errors".

I don't think it's worth only fixing the direct "dict literal into double star on a function call" as it's pretty artificial. But as soon as we name the dictionary and declare it somewhere else, it's easier to see that inferring its type based on the later double-star unpacking isn't an obvious proposition.

What did pyright do before it was made to behave like Mypy?

@erictraut
Copy link

What did pyright do before it was made to behave like Mypy?

It's nothing as complex as what you're suggesting above. In the general case, you can't know whether a dict[str, str] will contain a given key. A type checker needs to decide whether it wants to be more conservative (potentially producing a false positive) or less conservative (potentially producing a false negative). Mypy has opted in this case for the former, and a false positive error results. It's easy to switch the assumption to the latter and assume that if a parameter with a default argument is present, it doesn't need to be supplied by an unpacked dict argument.

In most cases, mypy opts for eliminating false positives at the expense of potential false negatives. I think that's a good philosophy in general. For example, neither mypy nor pyright produce an error here even though a runtime error will result.

def f(*, a: str, **kwargs: str) -> None:
    pass

f(**{"x": "x"}) # Runtime error: missing keyword argument 'a'

f(a="", **{"a": "x"}) # Runtime error: multiple values for keyword argument 'a'

So I think there's a good argument to be made that mypy is being inconsistent in opting for the more conservative approach and preferring potential false positives over potential false negatives.

@ambv
Copy link
Contributor

ambv commented Jul 25, 2023

I agree with you in general, but in the example on this issue dict[str, str] -- while it can have more keys -- cannot have a key "b" that holds a boolean value. So the only thing to do is to emit an error unless you do the shenanigans I talk about above.

@last-partizan
Copy link

last-partizan commented Jul 26, 2023

What do you think about making "immediately unpacked dict", a special case, and to infere it as TypedDict?

In my case, it's syntax sugar for unittest.mock.patch.

from unittest.mock import patch
# Instead of this
m = patch("requests.post")
m.configure_mock(**{"return_value.json.return_value": 1})
# I can write this:
m = patch("requests.post", **{"return_value.json.return_value": 1})

There is no alternative to this syntax, i cannot use keyword arguments, because . cannot be in normal kwargs.

@sobolevn
Copy link
Member

Yes, we can do some special analysis for both cases:

  1. When it is a TypedDict as @last-partizan suggested
  2. When value type part of dict[str, X] does not match some arguments types as @ambv suggested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-calls Function calls, *args, **kwargs, defaults
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants