[ty] Validate unpacked TypedDict **kwargs arguments#24710
[ty] Validate unpacked TypedDict **kwargs arguments#24710charliermarsh wants to merge 2 commits intomainfrom
TypedDict **kwargs arguments#24710Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 88.59%. The percentage of expected errors that received a diagnostic held steady at 84.44%. The number of fully passing files held steady at 83/134. |
Memory usage reportMemory usage unchanged ✅ |
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
missing-argument |
33 | 0 | 0 |
unknown-argument |
11 | 0 | 0 |
invalid-argument-type |
0 | 4 | 0 |
| Total | 44 | 4 | 0 |
Raw diff (48 changes)
bokeh (https://github.com/bokeh/bokeh)
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `attr` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `cols` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `data` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `kind` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `model` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `msg_data` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `msg_type` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `new` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `patches` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `rollover` does not match any known parameter of `DocumentChangedEvent.__init__`
+ src/bokeh/document/events.py:244:56 error[unknown-argument] Argument `title` does not match any known parameter of `DocumentChangedEvent.__init__`
core (https://github.com/home-assistant/core)
- homeassistant/components/aprilaire/coordinator.py:106:21 error[invalid-argument-type] Argument to bound method `DeviceRegistry.async_update_device` is incorrect: Expected `DeviceEntryType | None | UndefinedType`, found `set[tuple[str, str]]`
- homeassistant/components/aprilaire/coordinator.py:106:21 error[invalid-argument-type] Argument to bound method `DeviceRegistry.async_update_device` is incorrect: Expected `str | None | UndefinedType`, found `set[tuple[str, str]]`
graphql-core (https://github.com/graphql-python/graphql-core)
+ src/graphql/type/directives.py:133:16 error[missing-argument] No arguments provided for required parameters `name`, `locations` of `GraphQLDirective.__init__`
+ src/graphql/utilities/extend_schema.py:295:16 error[missing-argument] No arguments provided for required parameters `name`, `locations` of `GraphQLDirective.__init__`
+ src/graphql/utilities/extend_schema.py:333:23 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLInputField.__init__`
+ src/graphql/utilities/extend_schema.py:352:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/extend_schema.py:367:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/extend_schema.py:384:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/extend_schema.py:418:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/extend_schema.py:457:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/extend_schema.py:482:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/extend_schema.py:492:16 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLField.__init__`
+ src/graphql/utilities/extend_schema.py:502:16 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLArgument.__init__`
+ src/graphql/type/definition.py:291:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/type/definition.py:443:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/type/definition.py:547:16 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLField.__init__`
+ src/graphql/type/definition.py:693:16 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLArgument.__init__`
+ src/graphql/type/definition.py:773:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/type/definition.py:877:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/type/definition.py:980:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/type/definition.py:1115:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/type/definition.py:1347:16 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/type/definition.py:1444:16 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLInputField.__init__`
+ src/graphql/utilities/lexicographic_sort_schema.py:67:16 error[missing-argument] No arguments provided for required parameters `name`, `locations` of `GraphQLDirective.__init__`
+ src/graphql/utilities/lexicographic_sort_schema.py:78:26 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLArgument.__init__`
+ src/graphql/utilities/lexicographic_sort_schema.py:89:28 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLField.__init__`
+ src/graphql/utilities/lexicographic_sort_schema.py:102:19 error[missing-argument] No argument provided for required parameter `type_` of `GraphQLInputField.__init__`
+ src/graphql/utilities/lexicographic_sort_schema.py:123:20 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/lexicographic_sort_schema.py:131:20 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/lexicographic_sort_schema.py:139:20 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/lexicographic_sort_schema.py:143:20 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ src/graphql/utilities/lexicographic_sort_schema.py:159:20 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
+ tests/type/test_definition.py:1280:17 error[missing-argument] No argument provided for required parameter `name` of constructor `GraphQLNamedType.__new__`
prefect (https://github.com/PrefectHQ/prefect)
+ src/prefect/cli/deployment.py:753:20 error[missing-argument] No argument provided for required parameter `interval` of `IntervalSchedule.__init__`
pylint (https://github.com/pycqa/pylint)
+ pylint/checkers/base_checker.py:207:16 error[missing-argument] No argument provided for required parameter `scope` of `MessageDefinition.__init__`
rotki (https://github.com/rotki/rotki)
- rotkehlchen/history/events/structures/solana_swap.py:111:17 error[invalid-argument-type] Argument to `SolanaSwapEvent.__init__` is incorrect: Expected `TimestampMS`, found `str | None`
- rotkehlchen/history/events/structures/solana_swap.py:111:17 error[invalid-argument-type] Argument to `SolanaSwapEvent.__init__` is incorrect: Expected `str | None`, found `Location`a7096ef to
6b6ef50
Compare
c94f28c to
9e30cce
Compare
6b6ef50 to
0b571b6
Compare
9e30cce to
42d1dc6
Compare
0b571b6 to
f3253e8
Compare
abcbf72 to
532079f
Compare
f3253e8 to
a407396
Compare
532079f to
9bd701b
Compare
a407396 to
cd6d0b6
Compare
9bd701b to
f09541b
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
69559e9 to
b23d0ca
Compare
|
Putting this back in draft for now to move it out of my review queue; re-open it when it's rebased on top of an updated version of the stacked PR, I'll review it then. |
16c9d74 to
23def22
Compare
ce0b91e to
d00bb4b
Compare
d00bb4b to
c78249b
Compare
There was a problem hiding this comment.
against
Unpack[TypedDict]at call sites
I don't think this PR actually has anything to do with Unpack[TypedDict] at this point. It changes how we treat non-required fields when doing a *td unpacking of a TypedDict at the call site. It doesn't matter at all whether the parameters of the function we are calling were defined normally or via Unpack.
And the change we are making here is not general "validation": most of the tests here already pass on main, so we are already doing most of this validation. We are just adding one specific new validation: to error if a required parameter is provided by a non-required item of an unpacked TypedDict at the call site. If we go ahead with this PR, we should re-title it and update the description to clearly describe the specific change that it makes.
But now that I'm looking at this PR in isolation, I'm reconsidering whether we should do this. It adds a strict check that no other type checker has, that no user (AFAIK) has requested, that the spec / conformance suite don't require, and that does show up as new diagnostics in the ecosystem. Many of them look like false positives (even though they are expected and correct, given this behavior change), in the sense that the code actually does ensure the right keys are present in the TypedDict; the type itself just doesn't carry that guarantee.
In general, unpacking call arguments in the Python type system is handled forgivingly. You can unpack a list[int] and the type-checker trusts that you're providing the right number of arguments, it just checks the type. You can unpack a dict[str, object]; same. TypedDicts give us more information -- but maybe we should apply a similar leniency here.
Being lenient in this way is not "unsound" in the usual sense. It does mean that the type checker is not fully protecting you against a certain class of runtime error. But it doesn't cause symbols to potentially take on values at runtime that do not inhabit the static type we infer for them; it doesn't falsify type annotations.
Open to other takes, and sorry I didn't reach this conclusion sooner. But my call would be to not do this.
If we did go ahead with this, I think it should be a distinct error code; probably warning instead of error severity, and probably disabled by default.
|
|
||
| def takes_required_x(**kwargs: Unpack[HasX]) -> None: ... | ||
| def _(maybe_x: MaybeX, has_x: HasX) -> None: | ||
| # snapshot: missing-argument |
There was a problem hiding this comment.
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.
| ``` | ||
|
|
||
| ```snapshot | ||
| error[missing-argument]: No argument provided for required parameter `x` of function `takes_required_x` |
There was a problem hiding this comment.
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"
|
Thanks for the thoughtful review. Personally, I view these as straightforwardly type errors and would want them to be flagged on my project. I view them as (subjectively) different from
However... given that it's not an obvious choice, and that other type checkers are permissive here, I'm inclined to close in favor of deprioritizing for now. |
Summary
This PR improves validation of
**kwargsunpacking againstUnpack[TypedDict]at call sites.In particular, we now distinguish definitely-present keys from maybe-present keys, so required parameters are no
longer treated as satisfied by optional
TypedDictentries, duplicate explicit keywords are diagnosed correctly, etc.Comparing our behavior before and after: