Skip to content

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

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

Closed
wants to merge 5 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 25, 2021

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

sobolevn commented Sep 25, 2021

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 python deleted a comment Sep 26, 2021
@python python deleted a comment Sep 26, 2021
Copy link
Contributor

@ambv ambv left a comment

Choose a reason for hiding this comment

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

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.

@ambv 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
Copy link
Member Author

sobolevn commented Oct 21, 2021

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

Measuring performance impact here would be important as well.

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

@gvanrossum
Copy link
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.

@JelleZijlstra
Copy link
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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants