Skip to content
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

Allow unpacking of TypedDict into TypedDict #13353

Closed
wants to merge 13 commits into from

Conversation

eflorico
Copy link

@eflorico eflorico commented Aug 7, 2022

Description

Closes #9408 (and duplicates, e.g. #10585, #11108). Related tracking issue: #11753

Allows unpacking of TypedDicts into other TypedDicts:

class Foo(TypedDict):
    a: int

class Bar(TypedDict):
    a: int
    b: int

foo1: Foo = {'a': 1}
foo2: Foo = {**foo1}
bar: Bar = {**foo1, 'b': 2}

Should handle NotRequired fields correctly (they are counted as "maybe" present, we don't rely on them being present, but they should never be extraneous or have an incompatible type).

Also handles overwriting of fields correctly, for example:

class Foo(TypedDict):
    a: int
    b: str

class Bar(TypedDict):
    a: int
    b: int

foo: Foo = {'a': 1, 'b': '...'}
bar: Bar = {**foo} # Error: b is str instead of int
bar: Bar = {**foo, 'b': 2} # No error: b is overwritten

Limitations

  • Unpacking only works if the type of the unpacked expression is correctly inferred as a TypedDict. This is the case for variables or function return values, but not for dict expressions such as in foo: Foo = {**{'a': 1}}
  • Unpacking into unions does not work (foo_or_bar: Union[Foo, Bar] = {**foo1}). This is because find_typeddict_context can currently only handle one TypedDict in the context
  • Unpacking from unions does not work (foo: Bar = {**foo_or_bar}). I'm not sure yet how that could be added, I feel like it might take a bit more work

Test Plan

I have added quite detailed tests in check-typeddict.test.

@@ -1653,9 +1653,9 @@ a.update({'x': 1})
a.update({'x': ''}) # E: Incompatible types (expression has type "str", TypedDict item "x" has type "int")
a.update({'x': 1, 'y': []})
a.update({'x': 1, 'y': [1]})
a.update({'z': 1}) # E: Unexpected TypedDict key "z"
a.update({'z': 1, 'zz': 1}) # E: Unexpected TypedDict keys ("z", "zz")
a.update({'z': 1, 'x': 1}) # E: Expected TypedDict key "x" but found keys ("z", "x")
Copy link
Author

Choose a reason for hiding this comment

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

I had to change TypedDict error messages to correctly handle keys that may be there (coming from an unpacked TypedDict with NotRequired fields). I found the resulting new error messages to be clearer, but I'm open to feedback on that

context=item_value,
if item_name in items:
item_value_expr, item_actual_type = items[item_name]
self.chk.check_subtype(
Copy link
Author

Choose a reason for hiding this comment

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

I'm calling check_subtype instead of check_simple_assignment because that allows consistent handling of both immediate and unpacked items. Let me know if check_simple_assignment does any important additional checks that might be important

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


Copy link
Author

Choose a reason for hiding this comment

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

Both of the following test cases are for things that are not working yet, but that I hope will work at some point in the future. Is this the right way to test for that?

# Unpack operator (**) was used; unpack all items of the type of this expression into items list
value_type = self.accept(value_expr, callee)
proper_type = get_proper_type(value_type)
if isinstance(proper_type, TypedDictType):
Copy link
Author

Choose a reason for hiding this comment

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

This will not work for unions of TypedDicts (or other more complex types). I'm open to suggestions to improve this

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks, this is not a full review. Just a couple of minor comments 🙂

mypy/checkexpr.py Outdated Show resolved Hide resolved
return callee.required_keys <= set(validated_kwargs.keys()) <= set(callee.items.keys())
return (
callee.required_keys
<= set(dict(validated_kwargs).keys())
Copy link
Member

Choose a reason for hiding this comment

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

I think that keys() might be enough here 🤔

>>> set() <= {'a': 1}.keys()
True

Copy link
Author

Choose a reason for hiding this comment

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

You're right! Interesting to see that dict_keys behaves like a set. Changed!

@github-actions

This comment has been minimized.

@eflorico
Copy link
Author

eflorico commented Aug 8, 2022

Thanks, this is not a full review. Just a couple of minor comments 🙂

Thanks for your comments! I have updated the PR accordingly

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

steam.py (https://github.com/Gobot1234/steam.py)
- steam/profile.py:217: error: Expected TypedDict key to be string literal  [misc]
+ steam/profile.py:217: error: Any cannot be unpacked into TypedDict (must be TypedDict)  [misc]
- steam/profile.py:302: error: Expected TypedDict key to be string literal  [misc]
+ steam/profile.py:302: error: Any cannot be unpacked into TypedDict (must be TypedDict)  [misc]

@HansAarneLiblik
Copy link

Any progress on this PR ?

@eflorico
Copy link
Author

@HansAarneLiblik I have recently tried to merge master, but my changes were clashing with these recent changes. I haven't had the chance to figure out yet how to best solve this. If you have time to merge the current master and figure out how to unite both changes, I'd appreciate your help! I will only get around to look into this MR again in about two weeks

ilevkivskyi added a commit that referenced this pull request Jun 26, 2023
Fixes #9408
Fixes #4122
Fixes #6462
Supersedes #13353

This PR enables two similar technically unsafe behaviors for TypedDicts,
as @JukkaL explained in
#6462 (comment)
allowing an "incomplete" TypedDict as an argument to `.update()` is
technically unsafe (and a similar argument applies to `**` syntax in
TypedDict literals). These are however very common patterns (judging
from number of duplicates to above issues), so I think we should support
them. Here is what I propose:
* Always support cases that are safe (like passing the type itself to
`update`)
* Allow popular but technically unsafe cases _by default_
* Have a new flag (as part of `--strict`) to fall back to current
behavior

Note that unfortunately we can't use just a custom new error code, since
we need to conditionally tweak some types in a plugin. Btw there are
couple TODOs I add here:
* First is for unsafe behavior for repeated TypedDict keys. This is not
new, I just noticed it when working on this
* Second is for tricky corner case involving multiple `**` items where
we may have false-negatives in strict mode.

Note that I don't test all the possible combinations here (since the
phase space is huge), but I think I am testing all main ingredients (and
I will be glad to add more if needed):
* All syntax variants for TypedDicts creation are handled
* Various shadowing/overrides scenarios
* Required vs non-required keys handling
* Union types (both as item and target types)
* Inference for generic TypedDicts
* New strictness flag

More than half of the tests I took from the original PR #13353
@ilevkivskyi
Copy link
Member

This was superseded by #15425

Thank you for your efforts anyway, the test cases were in particular helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Star-star syntax not allowed with TypedDict
4 participants