Skip to content

bpo-41249: Fix postponed annotations for TypedDict #27017

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

Merged
merged 6 commits into from
Jul 17, 2021
Merged

Conversation

Kronuz
Copy link
Contributor

@Kronuz Kronuz commented Jul 4, 2021

Summary:

This fixes TypedDict to work with get_type_hints() and postponed evaluation of annotations across modules.

This is done by adding the module name to ForwardRef at the time the object is created and using that to resolve the globals during the evaluation.

Test Plan:

Having these two file:

# foo.py
from __future__ import annotations

from typing import Optional, TypedDict

class Foo(TypedDict):
    a: Optional[int]
# bar.py
from __future__ import annotations

from typing import get_type_hints
from foo import Foo

class Bar(Foo, total=False):
    b: int

print(get_type_hints(Bar))

Before:

$ python bar.py
Traceback (most recent call last):
  File "/Users/kronuz/Development/cpython/bar.py", line 9, in <module>
    print(get_type_hints(Bar))
  File "/Users/kronuz/Development/cpython/Lib/typing.py", line 1724, in get_type_hints
    value = _eval_type(value, base_globals, base_locals)
  File "/Users/kronuz/Development/cpython/Lib/typing.py", line 317, in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
  File "/Users/kronuz/Development/cpython/Lib/typing.py", line 660, in _evaluate
    eval(self.__forward_code__, globalns, localns),
  File "<string>", line 1, in <module>
NameError: name 'Optional' is not defined

After:

$ python bar.py
{'a': typing.Optional[int], 'b': <class 'int'>}

https://bugs.python.org/issue41249

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this fix! Some general comments:

I'll try to do a full review later.

@Kronuz
Copy link
Contributor Author

Kronuz commented Jul 4, 2021

@Fidget-Spinner, I added the tests, the news entry and fixed the KeyError as suggested. Thank you!

@Kronuz Kronuz requested a review from Fidget-Spinner July 6, 2021 21:04
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

LG in general! I just have a few non-code comments. Thanks :).

This fixes TypedDict to work with get_type_hints and postponed evaluation of annotations across modules.
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the quick response!

Please don't force push in the future. It overwrites valuable history, and besides, the core devs will squash and merge your PR as a single commit :).

I can't merge your PR, so I'll wait for Guido or some other core dev to review. If no one tends to this in a week or two, please gently ping the issue tracker.

@Kronuz
Copy link
Contributor Author

Kronuz commented Jul 7, 2021

@Fidget-Spinner, I made the suggested changes. Thank you for reviewing! 😃

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LG except I'd like the helper module renamed. Thanks!

Copy link

@septatrix septatrix left a comment

Choose a reason for hiding this comment

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

I still think it would be beneficial to figure out why TypedDicts have no MRO as this would trivially solve this problem. That being said I think this is a rather clean workaround for the time being

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Just one nit, I’ll commit the fix and then land.

@gvanrossum gvanrossum merged commit 889036f into python:main Jul 17, 2021
@Fidget-Spinner
Copy link
Member

@Kronuz congrats on landing your first PR in CPython :)! Great work!

@gvanrossum
Copy link
Member

Yes, good work!

@gvanrossum gvanrossum added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Jul 17, 2021
@miss-islington
Copy link
Contributor

Thanks @Kronuz for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Thanks @Kronuz for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @Kronuz and @gvanrossum, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 889036f7ef7290ef15b6c3373023f6a35387af0c 3.9

@miss-islington
Copy link
Contributor

Sorry @Kronuz and @gvanrossum, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 889036f7ef7290ef15b6c3373023f6a35387af0c 3.10

@Fidget-Spinner
Copy link
Member

I'll look into it.

@Fidget-Spinner Fidget-Spinner added needs backport to 3.10 only security fixes and removed needs backport to 3.10 only security fixes needs backport to 3.9 only security fixes labels Jul 17, 2021
@miss-islington
Copy link
Contributor

Thanks @Kronuz for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 17, 2021
This fixes TypedDict to work with get_type_hints and postponed evaluation of annotations across modules.

This is done by adding the module name to ForwardRef at the time the object is created and using that to resolve the globals during the evaluation.

Co-authored-by: Ken Jin <[email protected]>
(cherry picked from commit 889036f)

Co-authored-by: Germán Méndez Bravo <[email protected]>
@Fidget-Spinner
Copy link
Member

Seems like bedevere was just feeling a little unwell ;).

Fidget-Spinner added a commit to Fidget-Spinner/cpython that referenced this pull request Jul 17, 2021
This fixes TypedDict to work with get_type_hints and postponed evaluation of annotations across modules.

This is done by adding the module name to ForwardRef at the time the object is created and using that to resolve the globals during the evaluation.

Co-authored-by: Ken Jin <[email protected]>
ambv pushed a commit that referenced this pull request Jul 17, 2021
This fixes TypedDict to work with get_type_hints and postponed evaluation of annotations across modules.

This is done by adding the module name to ForwardRef at the time the object is created and using that to resolve the globals during the evaluation.

Co-authored-by: Ken Jin <[email protected]>
(cherry picked from commit 889036f)

Co-authored-by: Germán Méndez Bravo <[email protected]>
ambv added a commit that referenced this pull request Jul 17, 2021
…H-27205)

This fixes TypedDict to work with get_type_hints and postponed evaluation of annotations across modules.

This is done by adding the module name to ForwardRef at the time the object is created and using that to resolve the globals during the evaluation.

Co-authored-by: Ken Jin <[email protected]>
Co-authored-by: Germán Méndez Bravo <[email protected]>
Co-authored-by: Łukasz Langa <[email protected]>
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.

6 participants