-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
WIP: Enable Unpack for kwargs #15612
base: master
Are you sure you want to change the base?
WIP: Enable Unpack for kwargs #15612
Conversation
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this, sorry. The most I can maybe accept is prohibiting foo(**p)
calls I explained, under --extra-checks
flag.
if isinstance(rvalue_type, CallableType) and isinstance(lvalue_type, CallableType): | ||
self.check_compliance_with_pep692_assignment_rules( | ||
rvalue_type, lvalue_type, context | ||
) |
There was a problem hiding this comment.
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.
# 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.", |
There was a problem hiding this comment.
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.
def accept_animal(**kwargs: Unpack[Animal]): ... | ||
def accept_dog(**kwargs: Unpack[Dog]): ... | ||
|
||
accept_dog = accept_animal # OK |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
bar(**animal) # E: This will fail at runtime because 'breed' keyword will be passed to 'takes_name' as well. | ||
|
||
spam(**animal) # E: Again, 'breed' keyword will be eventually passed to 'takes_name'. |
There was a problem hiding this comment.
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.
|
||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
Thanks for the comments and sorry for a long time to respond! |
Thanks for the feedback @ilevkivskyi! FYI, I proposed changes to the PEP https://discuss.python.org/t/update-to-pep-692s-specification/32401. |
This is WIP.
Adding tests from PEP 692 - still the last two (added in last two commits) are not passing, trying to figure out how to approach this.