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

Fix TypedDict.get("missing_key") with string literal #9906

Merged

Conversation

freundTech
Copy link
Contributor

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.

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'
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@freundTech freundTech Jan 15, 2021

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].

Copy link
Collaborator

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.

Copy link
Member

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'
Copy link
Member

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?

Copy link
Contributor Author

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'
Copy link
Member

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.

Copy link
Collaborator

@JukkaL JukkaL left a 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.

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.

TypedDict does not allow .get("missing_key", ...) to pass
3 participants