Update unpack TypedDict kwargs forwarding spec + add conformance tests - #2338
Update unpack TypedDict kwargs forwarding spec + add conformance tests#2338yangdanny97 wants to merge 6 commits into
Conversation
|
This issue is nuanced, because many unpacking behaviors in Python are unsafe. If you unpack a In ty we made the decision not to error on the first case here, because we saw large mypy-primer fallout if we enforced this rule, and we felt it was analogous to the case of We decided to be stricter with I think ty's compromise is reasonable, but I'm not totally convinced it's the best option, open to other resolutions here. But enforcing this rule on all implicitly open TypedDicts will have a significant impact on existing real-world code. |
|
For Pyrefly my current thought is that this would be off-by-default and enabled in strict mode. |
|
That seems reasonable. I'm not sure if behavior that we wouldn't turn on by default in our own type checkers should be encoded as a conformance suite requirement, though? It seems to me that a strict mode which requires safety in unpacking an open TypedDict should probably also require safety in unpacking a list/Sequence? |
|
So do you think that we should just merge #1960 and delete this section from the typing spec entirely? Or weaken the first assertion to "may" and "E?" |
|
I would probably land this with the first assertion weakened (and discussion of that in the spec also), but that's not a strong preference, it just matches what we decided to do in ty, so naturally it already makes sense to me :) Very open to other opinions here. |
…ypedDicts optional
|
Does it make sense to differentiate between implicitly open and explicitly open ( |
It might. Currently in ty we only apply the extra strictness with |
|
The current TypedDict spec says that In the abstract I'd prefer to treat open TypedDicts similar to ones with |
| would not cause errors at runtime during function invocation. Otherwise, the | ||
| type checker should generate an error. | ||
| Therefore, it is only safe to pass ``kwargs`` hinted with an unpacked, non-:term:`closed` ``TypedDict`` | ||
| to another function if that function has ``**kwargs`` in its signature as well. Type checkers should |
There was a problem hiding this comment.
Should we also explicitly require possible extra-item values to be assignable to the destination's **kwargs annotation? Having **kwargs alone isn't sufficient:
class Open(TypedDict):
name: str
def target(name: str, **kwargs: int) -> None: ...
def forward(**kwargs: Unpack[Open]) -> None:
target(**kwargs) # Hidden extra items have type object, not int.Both ty and pyright reject this, while zuban currently accepts it. I think the optional-error compromise should apply to unexpected keyword names, not to values that are incompatible with an explicitly annotated parameter. Could we spell out that distinction and cover forwarding an open TypedDict into typed **kwargs?
| def func10(**kwargs: Unpack[TD5]) -> None: | ||
| takes_name(**kwargs) # E: extra items may be present | ||
| takes_name_str_kwargs(**kwargs) # E: extra items type is not compatible | ||
| takes_name_int_kwargs(**kwargs) |
There was a problem hiding this comment.
What happens when an extra item binds an explicitly declared optional parameter instead of the variadic parameter? For example:
def target(
name: str, *, label: str = "", **kwargs: int
) -> None: ...
def forward(**kwargs: Unpack[TD5]) -> None:
target(**kwargs) # TD5 could supply label=1.The variadic parameter accepts int, but label does not. ty and zuban reject this; pyright currently accepts it. Could we cover both incompatible and compatible optional parameters, and maybe the corresponding ReadOnly[int] extra-items case?
| conformant = "Pass" | ||
| conformant = "Partial" | ||
| notes = """ | ||
| Does not reject passing unpacked kwargs typed with a non-closed TypedDict to a callable that has no `**kwargs`. |
There was a problem hiding this comment.
Should we remove this note here and from the other checker results? The updated spec explicitly allows an open TypedDict to be forwarded without an error, and the corresponding test is marked E?, so accepting that case isn't a conformance failure. (The separate note about explicitly declared extra_items still makes sense.)
| In cases similar to the ``bar`` function above the problem could be worked | ||
| around by explicitly dereferencing desired fields and using them as arguments | ||
| to perform the function call:: | ||
| around by marking ``Animal`` with ``closed=True``, or by explicitly dereferencing desired |
There was a problem hiding this comment.
I think the same closed-TypedDict distinction also affects the earlier callable-assignment rule in this chapter, which currently says a callable without **kwargs can never be assigned to a destination with **kwargs: Unpack[TypedDict]. For example:
class Closed(TypedDict, closed=True):
name: str
class AcceptsClosed(Protocol):
def __call__(self, **kwargs: Unpack[Closed]) -> None: ...
def takes_name(*, name: str) -> None: ...
callback: AcceptsClosed = takes_nameBoth ty and pyright accept this assignment, and the hidden-key rationale doesn't apply because Closed cannot contain extra keys. Would it make sense to update that earlier rule and add closed-assignment coverage while we're clarifying this? extra_items=Never should behave the same way.
There was a problem hiding this comment.
Yep, I can add that and add some tests
| # > should error if the ``TypedDict`` has ``extra_items``, and may error if the ``TypedDict`` is open. | ||
|
|
||
| def func8(**kwargs: Unpack[TD3]) -> None: | ||
| takes_name(**kwargs) # E?: a subtype may contain unknown keys |
There was a problem hiding this comment.
Should we also cover directly unpacking ordinary TypedDict-annotated parameters or locals, not just a function's own **kwargs? The existing spec already demonstrates this with baz(animal: Animal), and the same rules should apply regardless of where the TypedDict came from:
def forward_direct(value: TD5) -> None:
takes_name(**value) # E: extra items may be present
takes_name_str_kwargs(**value) # E: int is incompatible with strIt would be useful to include the corresponding open and closed cases too, so a checker can't pass by special-casing **kwargs provenance.
| notes = """ | ||
| Allows callable without kwargs to be assigned to callable with unpacked kwargs. | ||
| Does not support the `closed` and `extra_items` TypedDict class arguments. | ||
| Does not reject passing unpacked kwargs typed with a non-closed TypedDict to a callable that has no `**kwargs`. |
There was a problem hiding this comment.
Remove this note? Accepting an open TypedDict here is explicitly permitted by the updated spec, and the corresponding test is marked E?. The separate extra_items note is the actual conformance issue.
| conformance_automated = "Pass" | ||
| conformant = "Partial" | ||
| notes = """ | ||
| Does not reject passing unpacked kwargs typed with a non-closed TypedDict to a callable that has no `**kwargs`. |
| conformance_automated = "Pass" | ||
| conformant = "Partial" | ||
| notes = """ | ||
| Does not reject passing unpacked kwargs typed with a non-closed TypedDict to a callable that has no `**kwargs`. |
| conformance_automated = "Pass" | ||
| conformant = "Partial" | ||
| notes = """ | ||
| Does not reject passing unpacked kwargs typed with a non-closed TypedDict to a callable that has no `**kwargs`. |
| to has ``**kwargs`` in its signature as well, because then additional keywords | ||
| would not cause errors at runtime during function invocation. Otherwise, the | ||
| type checker should generate an error. | ||
| Therefore, it is only safe to pass ``kwargs`` hinted with an unpacked, non-:term:`closed` ``TypedDict`` |
There was a problem hiding this comment.
This rule shouldn't be specific to forwarding kwargs -- but I guess that would imply a larger rewrite of this whole section. (Which I think we should do, but maybe not in this PR.) The existing text is way too focused on specific cases rather than outlining the general principles.
There was a problem hiding this comment.
I can reword this part a little bit in this PR, but let's save a bigger rewrite for later
Previously, this section of the spec was not exercised at all in the conformance tests, and also was not implemented by any type checker.
This PR updates the spec to account for closed & extra_items TypedDicts, and adds conformance tests.
There are 3 asserted errors:
This would supersede #1960, which proposed that we delete the section of the spec entirely.
@rchen152's comment in https://discuss.python.org/t/typing-spec-inconsistency-for-unpacking-typed-dict-kwargs/79640 favors deletion.