FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-45283: Run `_type_check` on `get_type_hints()` by sobolevn · Pull Request #28563 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Closed
sobolevn wants to merge 5 commits into
python:mainfrom
sobolevn:issue-45283
ConversationCommits (5)ChecksFiles changed
Closed

bpo-45283: Run _type_check on get_type_hints()#28563
sobolevn wants to merge 5 commits into
python:mainfrom
sobolevn:issue-45283

Conversation

sobolevn commented Sep 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This PR contains quite a lot of changes, but they are all related.

  1. First of all, I've fixed the core issue: ClassVar is now forbidden on module level during get_type_hints()
  2. But, I had to use _type_check() for that
  3. After running tests, I realised that several old (commit) annotations are not valid, for example:__annotations__['123'] = 1, so I changed them -> __annotations__['123'] = int
  4. Then, I realized that Final and ClassVar are not tested together with get_type_hints() and functions
  5. After that I wrote several other test cases for classes to check that the same feature works for them as well
  6. I also moved tests from bpo-45166: fixes get_type_hints failure on Final #28279, because I accidentally created them in the wrong class

I am pretty sure that this change can cause multiple problems for people with wrong annotations, because get_type_hints() was much more forgiving before this change. But, as a heavy typing user, I believe that this is a good thing, because correctness is very important in this area.

https://bugs.python.org/issue45283

sobolevn commented Sep 25, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

CC @ambv, because you was the reviewer together with @Fidget-Spinner the last time.
CC @Zac-HD, because you're using this feature quite often and might be affected 🙂

python deleted a comment Sep 26, 2021
python deleted a comment Sep 26, 2021
ambv reviewed Sep 26, 2021
View reviewed changes

ambv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I like the idea of making get_type_hints() stricter for runtime objects, which is already the case for forward references (e.g. either string annotations or from __future__ import annotations). It's a breaking change though so it can only go in for Python 3.11.

Measuring performance impact here would be important as well.

Comment thread Misc/NEWS.d/next/Library/2021-09-25-13-28-44.bpo-45283.ooE1jy.rst Outdated
Comment thread Lib/test/test_typing.py
Comment thread Lib/typing.py Outdated
Comment thread Lib/test/test_typing.py Outdated
Comment thread Lib/typing.py Outdated
ambv changed the title bpo-45283: runs _type_check on get_type_hints() bpo-45283: Run _type_check on get_type_hints() Sep 26, 2021

sobolevn commented Oct 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Thanks a lot, @ambv! I've implemented requested changes.

Sorry for the long wait, I am changing cities, so I cannot work full time on Python right now 🙂

sobolevn added a commit to sobolevn/cpython that referenced this pull request Oct 21, 2021
Move several typing tests to a proper class, refs python#28563
1150ce5
sobolevn mentioned this pull request Oct 21, 2021
Merged

sobolevn commented Oct 21, 2021

Copy link
Copy Markdown
Member Author

Measuring performance impact here would be important as well.

How can I do that? 🤔
Is there any specific benchmark?

gvanrossum commented Oct 21, 2021

Copy link
Copy Markdown
Member

Measuring performance impact here would be important as well.

How can I do that? 🤔 Is there any specific benchmark?

You probably have to write a small throwaway microbenchmark. Something that declares a whole bunch of annotations that happen to call _type_check() (for example, a large number of different Tuple types, defeating @_tp_cache).

I'm more concerned about annotations than about get_type_hints() here.

ambv pushed a commit that referenced this pull request Oct 21, 2021
Move several typing tests to a proper class, refs GH-28563 (GH-29126)
0c4c2e6

JelleZijlstra commented Jan 21, 2022

Copy link
Copy Markdown
Member

Marking as "do not merge" given the concerns from @Fidget-Spinner on the bug (which I agree with). We should do less runtime type checking in typing.py, not more.

sobolevn commented Jan 26, 2022

Copy link
Copy Markdown
Member Author

Yes, I now understand that the long-time goal is to make less runtime checks, not more 🙂
Closing. Thanks everyone!

sobolevn closed this Jan 26, 2022
sobolevn mentioned this pull request Apr 14, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ambv ambv left review comments
Fidget-Spinner Awaiting requested review from Fidget-Spinner
gvanrossum Awaiting requested review from gvanrossum

Assignees

No one assigned

Labels

awaiting review DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL