-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[ty] Validate unpacked TypedDict **kwargs arguments
#24710
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
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3060,6 +3060,16 @@ func(v1=1, v3="ok", v4=1) | |
|
|
||
| # error: [invalid-argument-type] | ||
| func(v1=1, v3=1) | ||
|
|
||
| td2 = TD2(v1=1, v3="ok") | ||
| func(**td2) | ||
|
|
||
| untyped_dict: dict[str, str] = {} | ||
| # error: [invalid-argument-type] | ||
| func(**untyped_dict) | ||
|
|
||
| # error: [parameter-already-assigned] | ||
| func(v1=1, **td2) | ||
| ``` | ||
|
|
||
| ### Extra keyword arguments | ||
|
|
@@ -3305,6 +3315,144 @@ def stringified(**kwargs: "Unpack[StringifiedTD]") -> None: | |
| stringified(a=1) | ||
| ``` | ||
|
|
||
| ### Non-string-keyed mappings are rejected | ||
|
|
||
| Only string-keyed mappings can be unpacked into named keyword parameters. | ||
|
|
||
| ```py | ||
| from typing_extensions import TypedDict, Unpack | ||
|
|
||
| class HasNameKwargs(TypedDict): | ||
| name: str | ||
|
|
||
| def takes_name_kwargs(**kwargs: Unpack[HasNameKwargs]) -> None: ... | ||
| def _(int_key_dict: dict[int, str]) -> None: | ||
| # snapshot: invalid-argument-type | ||
| takes_name_kwargs(**int_key_dict) | ||
| ``` | ||
|
|
||
| ```snapshot | ||
| error[invalid-argument-type]: Argument expression after ** must be a mapping with `str` key type | ||
| --> src/mdtest_snippet.py:9:23 | ||
| | | ||
| 9 | takes_name_kwargs(**int_key_dict) | ||
| | ^^^^^^^^^^^^^^ Found `int` | ||
| | | ||
| ``` | ||
|
|
||
| ### Non-mapping values are rejected without missing-argument cascades | ||
|
|
||
| ```py | ||
| from typing_extensions import TypedDict, Unpack | ||
|
|
||
| class HasNameKwargs(TypedDict): | ||
| name: str | ||
|
|
||
| class NotAMapping: ... | ||
|
|
||
| def takes_name_kwargs(**kwargs: Unpack[HasNameKwargs]) -> None: ... | ||
| def _(bad: NotAMapping) -> None: | ||
| # snapshot: invalid-argument-type | ||
| takes_name_kwargs(**bad) | ||
| ``` | ||
|
|
||
| ```snapshot | ||
| error[invalid-argument-type]: Argument expression after ** must be a mapping type | ||
| --> src/mdtest_snippet.py:11:25 | ||
| | | ||
| 11 | takes_name_kwargs(**bad) | ||
| | ^^^ Found `NotAMapping` | ||
| | | ||
| ``` | ||
|
|
||
| ### Explicit keywords still conflict with maybe-present unpacked keys | ||
|
|
||
| If a partial `TypedDict` may provide a key, passing that key explicitly still counts as a duplicate. | ||
|
|
||
| ```py | ||
| from typing_extensions import TypedDict | ||
|
|
||
| class MaybeX(TypedDict, total=False): | ||
| x: int | ||
|
|
||
| def takes_x(*, x: int) -> None: ... | ||
| def _(maybe_x: MaybeX) -> None: | ||
| # error: [parameter-already-assigned] | ||
| takes_x(x=1, **maybe_x) | ||
| ``` | ||
|
|
||
| ### Partial `TypedDict`s do not satisfy required unpacked keys | ||
|
|
||
| When a `TypedDict` key is not required, unpacking it does not prove that the corresponding required | ||
| parameter is present. | ||
|
|
||
| ```py | ||
| from typing_extensions import TypedDict, Unpack | ||
|
|
||
| class MaybeX(TypedDict, total=False): | ||
| x: int | ||
|
|
||
| class HasX(TypedDict): | ||
| x: int | ||
|
|
||
| def takes_required_x(**kwargs: Unpack[HasX]) -> None: ... | ||
| def _(maybe_x: MaybeX, has_x: HasX) -> None: | ||
| # snapshot: missing-argument | ||
| takes_required_x(**maybe_x) | ||
|
|
||
| takes_required_x(**has_x) | ||
| ``` | ||
|
|
||
| ```snapshot | ||
| error[missing-argument]: No argument provided for required parameter `x` of function `takes_required_x` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we are going to do this, users will find this error message confusing; we will probably need an error message that explicitly says "yes I know you are unpacking a typed-dict with this key, but it's not guaranteed to be present" |
||
| --> src/mdtest_snippet.py:12:5 | ||
| | | ||
| 12 | takes_required_x(**maybe_x) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| info: Parameter declared here | ||
| --> src/mdtest_snippet.py:9:22 | ||
| | | ||
| 9 | def takes_required_x(**kwargs: Unpack[HasX]) -> None: ... | ||
| | ^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| ``` | ||
|
|
||
| ### Partial `TypedDict`s can still contribute unknown keys | ||
|
|
||
| If a partial `TypedDict` only offers unrelated keys, the call can fail both because a required key | ||
| is missing and because the provided key is unknown. | ||
|
|
||
| ```py | ||
| from typing_extensions import TypedDict | ||
|
|
||
| class MaybeExtra(TypedDict, total=False): | ||
| extra: int | ||
|
|
||
| def takes_y(*, y: int) -> None: ... | ||
| def _(maybe_extra: MaybeExtra) -> None: | ||
| # error: [missing-argument] | ||
| # error: [unknown-argument] | ||
| takes_y(**maybe_extra) | ||
| ``` | ||
|
|
||
| ### Legacy dunder-style positional-only parameters still coexist with unpacked keys | ||
|
|
||
| Legacy stub-style positional-only parameter names like `__x` should not conflict with unpacked | ||
| `TypedDict` keys of the same name. | ||
|
|
||
| ```py | ||
| from typing_extensions import TypedDict, Unpack | ||
|
|
||
| LegacyPositionalOnlyKwargs = TypedDict("LegacyPositionalOnlyKwargs", {"__x": int}) | ||
|
|
||
| def legacy(__x: int, **kwargs: Unpack[LegacyPositionalOnlyKwargs]) -> None: | ||
| reveal_type(kwargs) # revealed: LegacyPositionalOnlyKwargs | ||
|
|
||
| def _(legacy_kwargs: LegacyPositionalOnlyKwargs) -> None: | ||
| legacy(1, **legacy_kwargs) | ||
| ``` | ||
|
|
||
| ## Bare `TypedDict` annotations in `**kwargs` | ||
|
|
||
| A bare `TypedDict` annotation on `**kwargs` still means “arbitrary keyword names whose values have | ||
|
|
@@ -3340,6 +3488,52 @@ def unrelated_named_parameter(x: int, **kwargs: BareKwargs) -> None: | |
| reveal_type(kwargs) # revealed: dict[str, BareKwargs] | ||
| ``` | ||
|
|
||
| ## `dict[str, T]` remains permissive | ||
|
|
||
| When the unpacked mapping is a string-keyed mapping like `dict[str, T]`, ty should optimistically | ||
| assume that the right keys may be present. It should still require the mapping's value type `T` to | ||
| be assignable to each parameter exposed by the unpacked `TypedDict`. | ||
|
|
||
| ```py | ||
| from typing_extensions import TypedDict, Unpack | ||
|
|
||
| class NameKwargs(TypedDict, total=False): | ||
| name: int | ||
|
|
||
| class MixedKwargs(TypedDict, total=False): | ||
| name: int | ||
| label: str | ||
|
|
||
| def accepts_name_kwargs(**kwargs: Unpack[NameKwargs]) -> None: ... | ||
| def accepts_mixed_kwargs(**kwargs: Unpack[MixedKwargs]) -> None: ... | ||
|
|
||
| class AcceptsNameKwargs: | ||
| def __init__(self, **kwargs: Unpack[NameKwargs]) -> None: | ||
| pass | ||
|
|
||
| class AcceptsMixedKwargs: | ||
| def __init__(self, **kwargs: Unpack[MixedKwargs]) -> None: | ||
| pass | ||
|
|
||
| class ForwardingWrapper(AcceptsNameKwargs): | ||
| def __init__(self, **kwargs: int) -> None: | ||
| super().__init__(**kwargs) | ||
|
|
||
| def _(good_kwargs: dict[str, int], bad_kwargs: dict[str, str]) -> None: | ||
| accepts_name_kwargs(**good_kwargs) | ||
| AcceptsNameKwargs(**good_kwargs) | ||
| ForwardingWrapper(**good_kwargs) | ||
|
|
||
| # error: [invalid-argument-type] | ||
| accepts_name_kwargs(**bad_kwargs) | ||
|
|
||
| # error: [invalid-argument-type] | ||
| accepts_mixed_kwargs(**good_kwargs) | ||
|
|
||
| # error: [invalid-argument-type] | ||
| AcceptsMixedKwargs(**good_kwargs) | ||
| ``` | ||
|
|
||
| ## Recursive functional `TypedDict` (unstringified forward reference) | ||
|
|
||
| Forward references in functional `TypedDict` calls must be stringified, since the field types are | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
According to my tests, this is the only test assertion in this entire PR that does not already pass on main. So this (non-required typed-dict keys no longer being considered as reliably providing an argument) is really the behavior change in this PR.