gh-146553: Fix infinite loop in typing.get_type_hints() on circular __wrapped__#146554
gh-146553: Fix infinite loop in typing.get_type_hints() on circular __wrapped__#146554raminfp wants to merge 2 commits intopython:mainfrom
Conversation
brianschubert
left a comment
brianschubert
left a comment
There was a problem hiding this comment.
I wonder if it would make sense to lazily import inspect here instead of having two implementations of essentially the same thing? inspect is a heavy import, but if you're calling get_type_hints you're already doing some sort of runtime signature introspection, in which case the import cost may be less of a concern.
@raminfp Please be mindful of https://devguide.python.org/getting-started/generative-ai/. I see you have been warned about relying on LLM generated PRs in the past. If you do decide to use LLMs, please make sure you take the time to scrutinize the output and compare it with the existing code before submitting a PR. In particular, you should try to match the style of the existing code (e.g. don't use _private_names for local variables).
|
@brianschubert Thank you for the review and for pointing this out. I do use LLM assistance when working on contributions (testing, security check, design and etc), but I wasn't sufficiently familiar with the relevant |
|
|
||
| _cleanups.append(_lazy_load_getattr_static.cache_clear) | ||
|
|
||
|
|
There was a problem hiding this comment.
Please revert this change. Diffs should not change unrelated parts of the code as they are then less readable.
| import inspect | ||
| nsobj = inspect.unwrap(obj) |
There was a problem hiding this comment.
Everytime I see local imports for inspect, I feel that we need to have a lower-level inspect utililty that can have less functions but more common to avoid having to import inspect.
What I can suggest for this specific PR is to use lazy import inspect instead at the top-level to make it a bit cleaner. For backports, we'll however need to use a local import. Note that we have other places with a local import to inspect, which you can eventually remove since we would have both a lazy and a local import (which would be redundant). I'll let @JelleZijlstra decide whether to want lazy imports.
If we switch to lazy imports, we'll also need to update the lazy import tests somewhere to check that inspect is lazily imported.
3 participants
#146553
typing.get_type_hints()via Circular__wrapped__#146553