-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix TypedDict.get("missing_key") with string literal #9906
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
Fix TypedDict.get("missing_key") with string literal #9906
Conversation
Previously this would throw an error, however according to PEP 589 .get() should be allowed for arbitrary expressions with type str, while only [] throws an error.
@@ -2342,12 +2342,12 @@ reveal_type(test.get(good_keys, 3)) # N: Revealed type is 'Union[_ | |||
reveal_type(test.pop(optional_keys)) # N: Revealed type is 'Union[__main__.D, __main__.E]' | |||
reveal_type(test.pop(optional_keys, 3)) # N: Revealed type is 'Union[__main__.D, __main__.E, Literal[3]?]' | |||
reveal_type(test.setdefault(good_keys, AAndB())) # N: Revealed type is 'Union[__main__.A, __main__.B]' | |||
reveal_type(test.get(bad_keys)) # N: Revealed type is 'builtins.object*' | |||
reveal_type(test.get(bad_keys, 3)) # N: Revealed type is 'builtins.object' |
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.
It would be great if a maintainer could have a look at this.
I don't know the difference between builtins.object
and builtins.object*
and couldn't find any reference to the asterisk in the documentation.
Therefor I just placed the needed asterisks by trial and error. This might or might not be wrong.
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.
The asterisk means that it's an inferred type; I think it can be safely ignored.
I'm not sure why mypy infers object
here though, and it doesn't seem very useful.
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 PEP 589 "The static type of d.get(e) should be object if the string value of e cannot be determined statically".
As far as I can tell PEP 589 doesn't say anything about d.get(e, default)
.
Also something else not related to my code seems to convert Union[..., builtins.object, ...]
to just builtins.object, which can bee seen when making defauly.py line 234 retun Any instead. In that case mypy infers Union[__main__.A, Literal[3]?, Any]
.
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.
The default doesn't change anything, since we can't infer the type of the value from the default (the value type could be an arbitrary union type, for example).
Since typed dicts are heterogeneous, we can't predict the type of arbitrary keys. In this case the only possible precise type is object
. Any
would also work, but the spirit of PEP 484 is that there shouldn't be Any
types in fully and precisely annotated programs. We don't want to silently produce an Any
type if a programmer mistypes a string literal.
A union that includes object
as an item is equivalent to object
(it can contain arbitrary objects). Mypy sometimes simplifies these unions and it's okay.
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 the correction!
@@ -2227,7 +2227,7 @@ d[c_key] # E: TypedDict "Outer" has no key 'c' | |||
|
|||
reveal_type(d.get(a_key, u)) # N: Revealed type is 'Union[builtins.int, __main__.Unrelated]' | |||
reveal_type(d.get(b_key, u)) # N: Revealed type is 'Union[builtins.str, __main__.Unrelated]' | |||
d.get(c_key, u) # E: TypedDict "Outer" has no key 'c' | |||
reveal_type(d.get(c_key, u)) # N: Revealed type is 'builtins.object' |
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.
Shouldn't this infer Unrelated
, or at least a Union of something with Unrelated
?
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.
Like in the other discussion, the type here would be Union[builtins.object, Unrelated]
, which mypy simplifies to builtins.object
@@ -2342,12 +2342,12 @@ reveal_type(test.get(good_keys, 3)) # N: Revealed type is 'Union[_ | |||
reveal_type(test.pop(optional_keys)) # N: Revealed type is 'Union[__main__.D, __main__.E]' | |||
reveal_type(test.pop(optional_keys, 3)) # N: Revealed type is 'Union[__main__.D, __main__.E, Literal[3]?]' | |||
reveal_type(test.setdefault(good_keys, AAndB())) # N: Revealed type is 'Union[__main__.A, __main__.B]' | |||
reveal_type(test.get(bad_keys)) # N: Revealed type is 'builtins.object*' | |||
reveal_type(test.get(bad_keys, 3)) # N: Revealed type is 'builtins.object' |
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.
The asterisk means that it's an inferred type; I think it can be safely ignored.
I'm not sure why mypy infers object
here though, and it doesn't seem very useful.
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.
Looks good. The original behavior was designed to catch some misspellings, but it was kind of inflexible. The new behavior is more flexible but the inferred object
types could be confusing. It will be interesting to see what users think about this.
Description
Fixes #9902
Previously this would throw an error, however according to PEP 589
.get() should be allowed for arbitrary expressions with type str, while
only [] throws an error.
Test Plan
All affected tests have been updated to the new, intended behavior. This is my first contribution to mypy, so it would be good to have an extra pair of eyes checking if everything is correct.