Skip to content

WIP: Enable Unpack for kwargs #15612

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

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

WIP: Enable Unpack for kwargs #15612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4066eff
Add more tests cases based on PEP 692
franekmagiera May 20, 2023
762f50d
Implement function assignment check when only destination contains kw…
franekmagiera Jun 1, 2023
b622dfb
Update docstring for UnpackType
franekmagiera Jul 6, 2023
bf2510a
Add function assignment test when both functions are typed with Unpack
franekmagiera Jul 6, 2023
2cd2bf8
Add test for passing kwargs inside a function to another function
franekmagiera Jul 6, 2023
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
16 changes: 16 additions & 0 deletions mypy/checker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3935,8 +3935,24 @@ def check_simple_assignment(
f"{lvalue_name} has type",
notes=notes,
)

if isinstance(rvalue_type, CallableType) and isinstance(lvalue_type, CallableType):
self.check_compliance_with_pep692_assignment_rules(
rvalue_type, lvalue_type, context
)
Copy link
Member

Choose a reason for hiding this comment

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

Checking this only for assignments is a bad idea. What about other situations involving subtyping like passing argument to a function, overload selection, type variable bound checks etc.


return rvalue_type

def check_compliance_with_pep692_assignment_rules(
self, source: CallableType, destination: CallableType, context: Context
) -> None:
# Destination contains kwargs and source doesn't.
if destination.unpack_kwargs and not source.is_kw_arg:
self.fail(
"Incompatible function signatures - variable contains **kwargs and the expression does not. See PEP 692 for more details.",
Copy link
Member

Choose a reason for hiding this comment

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

Introducing a subtle difference between two ways to define function with keyword-only args looks like a bandaid for an unrelated problem (see below). I think TypedDict form should really be handled as a syntactic sugar for its expanded form.

context,
)

def check_member_assignment(
self, instance_type: Type, attribute_type: Type, rvalue: Expression, context: Context
) -> tuple[Type, Type, bool]:
Expand Down
5 changes: 3 additions & 2 deletions mypy/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,10 +994,11 @@ def __eq__(self, other: object) -> bool:

class UnpackType(ProperType):
"""Type operator Unpack from PEP646. Can be either with Unpack[]
or unpacking * syntax.
or unpacking * syntax. It can also be used for typing **kwargs (PEP692).

The inner type should be either a TypeVarTuple, a constant size
tuple, or a variable length tuple, or a union of one of those.
tuple, or a variable length tuple, or a union of one of those OR a
TypedDict.
"""

__slots__ = ["type"]
Expand Down
214 changes: 214 additions & 0 deletions test-data/unit/check-varargs.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,19 @@ def foo(arg: bool, **kwargs: Unpack[Person]) -> None: ...
reveal_type(foo) # N: Revealed type is "def (arg: builtins.bool, **kwargs: Unpack[TypedDict('__main__.Person', {'name': builtins.str, 'age': builtins.int})])"
[builtins fixtures/dict.pyi]

[case testUnpackKwargsTypeInsideOfTheFunction]
from typing import assert_type
from typing_extensions import TypedDict, Unpack

class Person(TypedDict):
name: str
age: int

def foo(**kwargs: Unpack[Person]):
assert_type(kwargs, Person)

[builtins fixtures/dict.pyi]

[case testUnpackOutsideOfKwargs]
from typing_extensions import Unpack, TypedDict
class Person(TypedDict):
Expand Down Expand Up @@ -785,6 +798,8 @@ class Person(TypedDict):
age: int
def foo(name: str, **kwargs: Unpack[Person]) -> None: # E: Overlap between argument names and ** TypedDict items: "name"
...
def bar(name:str, /, **kwargs: Unpack[Person]) -> None: # OK
...
[builtins fixtures/dict.pyi]

[case testUnpackWithDuplicateKeywordKwargs]
Expand Down Expand Up @@ -843,6 +858,42 @@ def bar(**kwargs: Unpack[Square]):
bar(side=12)
[builtins fixtures/dict.pyi]

[case testUnpackTypedDictRequired]
from typing import Required, TypedDict
from typing_extensions import Unpack

class Movie(TypedDict, total=False):
name: Required[str]
year: Required[int]
based_on: str

def foo(**kwargs: Unpack[Movie]): ...

foo(name="Life of Brian", year=1979) # OK
foo(name="Harry Potter and the Philosopher's Stone", year=2001, based_on="book") # OK

foo(name="Harry Potter and the Philosopher's Stone", based_on="book") # E: Missing named argument "year" for "foo"
[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]

[case testUnpackTypedDictNotRequired]
from typing import NotRequired, TypedDict
from typing_extensions import Unpack

class Movie(TypedDict, total=True):
name: str
year: int
based_on: NotRequired[str]

def foo(**kwargs: Unpack[Movie]): ...

foo(name="Life of Brian", year=1979) # OK
foo(name="Harry Potter and the Philosopher's Stone", year=2001, based_on="book") # OK

foo(name="Harry Potter and the Philosopher's Stone", based_on="book") # E: Missing named argument "year" for "foo"
[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]

[case testUnpackUnexpectedKeyword]
from typing_extensions import Unpack, TypedDict

Expand Down Expand Up @@ -1066,3 +1117,166 @@ class C:
class D:
def __init__(self, **kwds: Unpack[int, str]) -> None: ... # E: Unpack[...] requires exactly one type argument
[builtins fixtures/dict.pyi]

[case testUnpackKwargsAssignmentWhenSourceIsUntyped]
from typing_extensions import Unpack, TypedDict

class Movie(TypedDict):
name: str
year: int

def src(**kwargs): ...
def dest(**kwargs: Unpack[Movie]): ...

dest = src # OK
[builtins fixtures/dict.pyi]

[case testUnpackKwargsAssignmentWhenSourceHasTraditionallyTypedKwargs]
from typing_extensions import Unpack, TypedDict

class Vehicle:
...

class Car(Vehicle):
...

class Motorcycle(Vehicle):
...

def src(**kwargs: Vehicle): ...

class Vehicles(TypedDict):
car: Car
moto: Motorcycle

def dest(**kwargs: Unpack[Vehicles]): ...

dest = src # OK
[builtins fixtures/dict.pyi]

[case testUnpackKwargsAssignmentWhenDestinationHasTraditionallyTypedKwargs]
from typing_extensions import Unpack, TypedDict

class Vehicle:
...

class Car(Vehicle):
...

class Motorcycle(Vehicle):
...

def dest(**kwargs: Vehicle): ...

class Vehicles(TypedDict):
car: Car
moto: Motorcycle

def src(**kwargs: Unpack[Vehicles]): ...

dest = src # E: Incompatible types in assignment (expression has type "Callable[[KwArg(Vehicles)], Any]", variable has type "Callable[[KwArg(Vehicle)], Any]")
[builtins fixtures/dict.pyi]

[case testUnpackKwargsAssignmentWhenOnlySourceContainsKwargs]
from typing import NotRequired
from typing_extensions import Unpack, TypedDict

class Animal(TypedDict):
name: str

class Dog(Animal):
breed: str

class Example(TypedDict):
animal: Animal
string: str
number: NotRequired[int]

def src(**kwargs: Unpack[Example]): ...
def dest(*, animal: Dog, string: str, number: int = ...): ...

dest = src # OK

def dest_with_positional_args(animal: Dog, string: str, number: int = ...): ...

dest_with_positional_args = src # E: Incompatible types in assignment (expression has type "Callable[[KwArg(Example)], Any]", variable has type "Callable[[Dog, str, int], Any]")
[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]

[case testUnpackKwargsAssignmentWhenOnlyDestinationContainsKwargs]
from typing_extensions import Unpack, TypedDict

class Animal(TypedDict):
name: str

class Dog(Animal):
breed: str

def dest(**kwargs: Unpack[Animal]): ...
def src(name: str): ...

dog: Dog = {"name": "Daisy", "breed": "Labrador"}
animal: Animal = dog

dest = src # E: Incompatible function signatures - variable contains **kwargs and the expression does not. See PEP 692 for more details.

[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]

[case testUnpackKwargsAssignmentWhenBothSidesContainKwargsTypedWithUnpack]
from typing_extensions import Unpack, TypedDict

class Animal(TypedDict):
name: str

class Dog(Animal):
breed: str

def accept_animal(**kwargs: Unpack[Animal]): ...
def accept_dog(**kwargs: Unpack[Dog]): ...

accept_dog = accept_animal # OK
Copy link
Member

Choose a reason for hiding this comment

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

This is obviously not OK, accept_animal requires exactly one argument, while accept_dog requires exactly two arguments. Neither can be a subtype of other one.

Copy link
Author

Choose a reason for hiding this comment

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

You are right, when I was writing this I was thinking more in terms of "will this cause a runtime error or not?" - but in terms of "is this a subtype of that?" it's not correct. Something I'll need to revisit.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that it is true, but only if we look at it from the normalized signature point of view. My thinking is that what matters is what is happening later in the function itself. If the function contains **kwargs, then it means inside the function we can use a variable kwargs - wouldn't in that case thinking of **kwargs as just another parameter that should just adhere to the contravariance of parameters rule make sense?

It becomes very cumbersome in case of "normal" function signatures (without **kwargs) interacting with the functions that use **kwargs...

Copy link
Member

Choose a reason for hiding this comment

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

There is nothing cumbersome in reducing the PEP to just one paragraph that would explain that Unpack[SomeTD] is a syntactic sugar for (and is considered equivalent to) the expanded signature. This has a number of benefits:

  • This will not add any new unsafety that is not already present for existing uses of TypedDicts in ** contexts. (And type checkers may handle this unsafety in a uniform way, say in mypy we may use existing --extra-checks flag to prohibit some techincally unsafe calls as I mentioned before.)
  • This is actually easy to remember and to reason about.
  • This will allow people who want subtyping between callables to easily achieve this using total=False, which follows from existing rules for expanded callables.

Btw a comment on the last point, subtyping relations are quite counter-intuitive when you think about them in terms of packed TypedDicts. Consider this example:

class A(TypedDict, total=False):
    x: int
class B(TypedDict, total=False):
    x: int
    y: int

def takes_a(**kwargs: Unpack[A]) -> None: ...
def takes_b(**kwargs: Unpack[B]) -> None: ...

if bool():
    takes_a = takes_b  # mypy now says it is OK
else:
    takes_b = takes_a  # mypy now gives an error for this

If you would think in terms of subtyping between TypedDicts, you might conclude the opposite relations, because B is subtype of A, and functions should be contravariant in argument types, right? But obviously takes_a is not a subtype of takes_b because takes_b(x=42, y=0) is a valid call, while takes_a(x=42, y=0) is not a valid one. (You can say this is because mappings are contravariant in keys, but covariant in values etc., but yet again, simple logic of always expanding makes all this unnecessary).

Finally, the simple logic of always expanding is more future-proof. For example, if we would decide to add default types for TypedDicts, it would naturally translate to trailing homogeneous **kwargs in expanded form. While if you treat packed and expanded forms differently (as PEP currently does), it would add many new corner cases.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for a detailed response! There's a lot to unpack here (pun intended), but I think what you're saying makes sense.


def also_accept_animal(**kwargs: Unpack[Animal]): ...
def also_accept_dog(**kwargs: Unpack[Dog]): ...

also_accept_animal = also_accept_dog # E: Incompatible types in assignment (expression has type "Callable[[KwArg(Dog)], Any]", variable has type "Callable[[KwArg(Animal)], Any]")

[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]

[case testPassingKwargsFromFunctionToFunction]
from typing_extensions import Unpack, TypedDict

class Animal(TypedDict):
name: str

class Dog(Animal):
breed: str

def takes_name(name: str):
...

dog: Dog = {"name": "Daisy", "breed": "Labrador"}
animal: Animal = dog

def foo(**kwargs: Unpack[Animal]):
return kwargs["name"]

def bar(**kwargs: Unpack[Animal]):
takes_name(**kwargs)

def baz(animal: Animal):
takes_name(**animal)

def spam(**kwargs: Unpack[Animal]):
baz(kwargs)

foo(**animal) # OK! foo only expects and uses keywords of 'Animal'.

bar(**animal) # E: This will fail at runtime because 'breed' keyword will be passed to 'takes_name' as well.
Copy link
Member

Choose a reason for hiding this comment

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

Trying to support this is futile (not just in mypy, in any reasonable type checker), since foo and bar have identical signatures, you can't make one fail and other pass for an identical call.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, makes sense - probably another thing that PEP has to be amended for.

Copy link
Author

Choose a reason for hiding this comment

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

Would it make sense in your opinion to report an error here instead:

def bar(**kwargs: Unpack[Animal]):
    takes_name(**kwargs) # E: Potentially passing more arguments than expected, because a subtype of Animal could have been passed

?

Copy link
Member

@ilevkivskyi ilevkivskyi Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This will break a ton of existing code, unless you will make this check apply only when x in **x comes from Unpack in a function, but even then it will likely break some code. This feels like sacrificing a useful feature in favor of something no one asked for.


spam(**animal) # E: Again, 'breed' keyword will be eventually passed to 'takes_name'.
Copy link
Member

Choose a reason for hiding this comment

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

There core issue here has nothing to do with Unpack syntax. Mypy allows this code:

from typing import TypedDict

class Point(TypedDict):
    x: int
    y: int

def foo(*, x: int, y: int) -> None: ...

p: Point
foo(**p)

which is unsafe because p can be a subtype at runtime. But FWIW I don't think anyone complained about this false negative in ~5 years this feature is available. On the contrary, many people complained about similar checks for other cases where TypedDict subtypes at runtime would cause problems, this is why I recently moved them behind new --extra-checks flag.


[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]