bpo-45166: fixes `get_type_hints` failure on `Final` by sobolevn · Pull Request #28279 · python/cpython · GitHub

https://bugs.python.org/issue45166

@ambv @Fidget-Spinner any chance to get a review please? 🙂

Sorry about the delay and thanks for your fix! This looks mostly good to me. I just have a few concerns.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again

Thanks for making the requested changes!

@Fidget-Spinner: please review the changes made to this pull request.

Very good insight about the need for is_class=. I have one change here that we need to make (the __init__ signature) and two suggestions which I think make things clearer.

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

Review changes implemented.

Thanks for your changes. I only have one more nit about adding a docstring :).

Interestingly, this test fails when you remove this import. So it still allows top-level ClassVar if they aren't strings.

If you'd like to forbid this, you probably need to create another issue/and or PR. This behavior is what typing currently does (since 3.7-3.10). Even if the behavior is wrong, we don't usually backport bugfixes that add extra restrictions (unless it's a security fix) because that usually breaks backwards compatibility.

Yes, good catch! I've opened a new issue: https://bugs.python.org/issue45283
I will work on it after this one will be finished.

Thanks! 👍

Note to self: there's a subtle change in the logic compared to the old code, but it's ok because is_argument defaulted to True in both ForwardRef and _type_check. Which means by default we should always add ClassVar too.

LGTM. Thanks for your patience!

This test_asyncio is failing sometimes on Windows, reruning the CI.

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

Sorry, @sobolevn and @ambv, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 784905dbeff68cf788bbeefe0a675af1af04affc 3.9

GH-28560 is a backport of this pull request to the 3.10 branch.

Co-authored-by: Łukasz Langa <lukasz@langa.pl> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 784905d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

…28279) Co-authored-by: Łukasz Langa <lukasz@langa.pl> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>. (cherry picked from commit 784905d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

GH-28561 is a backport of this pull request to the 3.9 branch.

Thanks everyone! 🎉

Don't mention it, thank you for the fix!

…8560) Co-authored-by: Łukasz Langa <lukasz@langa.pl> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> (cherry picked from commit 784905d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

…GH-28561) Co-authored-by: Łukasz Langa <lukasz@langa.pl> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>. (cherry picked from commit 784905d) Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

Just a question: will this not be backported to Python 3.8 as well?

bpo-45166: fixes get_type_hints failure on Final

1d46e26

sobolevn requested review from Fidget-Spinner and gvanrossum as code owners September 10, 2021 18:36

the-knights-who-say-ni added the CLA signed label Sep 10, 2021

bedevere-bot added the awaiting review label Sep 10, 2021

sobolevn mentioned this pull request Sep 10, 2021

get_type_hints + Final + future annotations = TypeError python/typing#890

Closed

gvanrossum removed their request for review September 10, 2021 19:14

ambv added needs backport to 3.9 labels Sep 13, 2021

pawamoy mentioned this pull request Sep 21, 2021

fix: Support future annotations mkdocstrings/pytkdocs#113

Closed

Fidget-Spinner previously requested changes Sep 23, 2021

View reviewed changes

Comment thread Lib/typing.py Outdated

Comment thread Misc/NEWS.d/next/Library/2021-09-10-21-35-53.bpo-45166.UHipXF.rst Outdated

bedevere-bot added awaiting changes and removed awaiting review labels Sep 23, 2021

Adds is_class prototype to ForwardRef in typing

e121641

bedevere-bot removed the awaiting changes label Sep 23, 2021

bedevere-bot added the awaiting change review label Sep 23, 2021

bedevere-bot requested a review from Fidget-Spinner September 23, 2021 20:59

ambv approved these changes Sep 24, 2021

View reviewed changes

Comment thread Lib/typing.py Outdated

Comment thread Lib/typing.py

Comment thread Lib/typing.py Outdated

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 24, 2021

ambv reviewed Sep 24, 2021

View reviewed changes

Comment thread Lib/typing.py Outdated

sobolevn and others added 4 commits September 24, 2021 14:28

Update Lib/typing.py

9752a76

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

Update Lib/typing.py

ff0deb5

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

Update Lib/typing.py

7a34336

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

Update Lib/typing.py

3e033a8

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

Fidget-Spinner reviewed Sep 24, 2021

View reviewed changes

Update news entry

b66c8f6

Fidget-Spinner reviewed Sep 24, 2021

View reviewed changes

Comment thread Lib/test/ann_module5.py

Add comment/docstring to Lib/test/ann_module5.py

4f9e4c6

Fidget-Spinner approved these changes Sep 24, 2021

View reviewed changes

sobolevn closed this Sep 24, 2021

sobolevn reopened this Sep 24, 2021

ambv merged commit 784905d into python:main Sep 25, 2021

bedevere-bot removed the awaiting merge label Sep 25, 2021

bedevere-bot removed the needs backport to 3.10 label Sep 25, 2021

miss-islington assigned ambv Sep 25, 2021

bedevere-bot removed the needs backport to 3.9 label Sep 25, 2021

sobolevn mentioned this pull request Sep 25, 2021

bpo-45283: Run _type_check on get_type_hints() #28563

Closed

uriyyo mentioned this pull request Nov 18, 2021

Fix issue with ClassVar parsing pydantic/pydantic#3403

Merged

5 tasks done

+                  if not is_class:
+                      invalid_generic_forms += (ClassVar,)
+                      if is_argument:
+                          invalid_generic_forms += (Final,)

		@@ -0,0 +1,7 @@
		# Tests that top-level ClassVar is not allowed

		from __future__ import annotations

bpo-45166: fixes `get_type_hints` failure on `Final` by sobolevn · Pull Request #28279 · python/cpython · GitHub

Repository navigation

bpo-45166: fixes get_type_hints failure on Final - #28279

bpo-45166: fixes get_type_hints failure on Final#28279ambv merged 8 commits intopython:mainpython/cpython:mainfrom sobolevn:issue-45166sobolevn/cpython:issue-45166Copy head branch name to clipboard

Conversation

sobolevn commented Sep 10, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Sep 23, 2021

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Sep 23, 2021

Uh oh!

sobolevn commented Sep 23, 2021

Uh oh!

bedevere-bot commented Sep 23, 2021

Uh oh!

ambv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fidget-Spinner Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

sobolevn Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miss-islington commented Sep 25, 2021

Uh oh!

miss-islington commented Sep 25, 2021

Uh oh!

bedevere-bot commented Sep 25, 2021

Uh oh!

bedevere-bot commented Sep 25, 2021

Uh oh!

sobolevn commented Sep 25, 2021

Uh oh!

ambv commented Sep 25, 2021

Uh oh!

pawamoy commented Sep 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bpo-45166: fixes get_type_hints failure on Final#28279
ambv merged 8 commits into
python:mainfrom
sobolevn:issue-45166

sobolevn commented Sep 10, 2021 •
edited by bedevere-bot
Loading

sobolevn commented Sep 24, 2021 •
edited
Loading