Skip to content
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

Add typing_extensions.get_annotations #423

Merged
merged 9 commits into from
Jun 10, 2024

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Jun 5, 2024

Fixes #409

This is based on the implementation in python/cpython#119891. The version that eventually lands in CPython may look a bit different, but should be compatible with what's in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

In #151, the consensus seemed to be that we shouldn't add new "data files" to the src directory for usage in tests, because these will end up in the site-packages directory for people who've installed typing_extensions. Should we just store these as strings in test_typing_extensions.py, like we did for the data files in that PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might also be time to split the test file, as it's approaching 10k lines.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could have an src/test_typing_extensons directory: then it's only a single importable module in site-packages, but we'd be able to split it up so that it doesn't get too huge.

Copy link
Member Author

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 a test directory. I'll send a separate PR for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this and I think it's not worth it. I like to simply run python test_typing_extensions.py to run the test suite; that would no longer work with a package. I'll move the test files from this PR into exec() calls instead.

src/typing_extensions.py Outdated Show resolved Hide resolved
Comment on lines +3741 to +3743
return_value = {key:
value if not isinstance(value, str) else eval(value, globals, locals)
for key, value in ann.items() }
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this isn't going to work for type parameters, which exist in neither the global nor local namespaces. Possibly the fix I made to the typing.py internals in python/cpython@1e3e7ce should also have been applied to inspect.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. In my PEP 649 implementation branch (python/cpython#119891) I handle this by getting obj's __type_params__, but this may also be worth backporting separately.

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +3725 to +3732
while True:
if hasattr(unwrap, '__wrapped__'):
unwrap = unwrap.__wrapped__
continue
if isinstance(unwrap, functools.partial):
unwrap = unwrap.func
continue
break
Copy link
Member

Choose a reason for hiding this comment

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

any idea why inspect.get_annotations doesn't use inspect.unwrap() here? Though I suppose we won't be able to use inspect.unwrap in the CPython version anyway after moving the implementation out of inspect.py, since we won't want the new annotations module to depend on inspect

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. Looks like unwrap() doesn't handle functools.partial specially, though unlike this code it deals with cycles.

Copy link
Member

Choose a reason for hiding this comment

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

It's pretty odd; no CPython tests fail if I make this change:

diff --git a/Lib/inspect.py b/Lib/inspect.py
index 2b7f8bec482..4f9c43d9fc7 100644
--- a/Lib/inspect.py
+++ b/Lib/inspect.py
@@ -235,13 +235,11 @@ def get_annotations(obj, *, globals=None, locals=None, eval_str=False):
             if module:
                 obj_globals = getattr(module, '__dict__', None)
         obj_locals = dict(vars(obj))
-        unwrap = obj
     elif isinstance(obj, types.ModuleType):
         # module
         ann = getattr(obj, '__annotations__', None)
         obj_globals = getattr(obj, '__dict__')
         obj_locals = None
-        unwrap = None
     elif callable(obj):
         # this includes types.Function, types.BuiltinFunctionType,
         # types.BuiltinMethodType, functools.partial, functools.singledispatch,
@@ -249,7 +247,6 @@ def get_annotations(obj, *, globals=None, locals=None, eval_str=False):
         ann = getattr(obj, '__annotations__', None)
         obj_globals = getattr(obj, '__globals__', None)
         obj_locals = None
-        unwrap = obj
     else:
         raise TypeError(f"{obj!r} is not a module, class, or callable.")
 
@@ -265,17 +262,8 @@ def get_annotations(obj, *, globals=None, locals=None, eval_str=False):
     if not eval_str:
         return dict(ann)
 
-    if unwrap is not None:
-        while True:
-            if hasattr(unwrap, '__wrapped__'):
-                unwrap = unwrap.__wrapped__
-                continue
-            if isinstance(unwrap, functools.partial):
-                unwrap = unwrap.func
-                continue
-            break
-        if hasattr(unwrap, "__globals__"):
-            obj_globals = unwrap.__globals__
+    if not isinstance(obj, types.ModuleType):
+        obj_globals = getattr(unwrap(obj), "__globals__", None) or obj_globals

You could consider that simplification here since we already import inspect in typing_extensions, though it obviously can't be applied to CPython

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like we're missing test coverage for functools.partial. I think we should mirror the CPython implementation as much as possible.

src/typing_extensions.py Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

src/typing_extensions.py Outdated Show resolved Hide resolved
src/typing_extensions.py Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM. #423 (comment) can be fixed in a followup once we've sorted it out at cpython

@JelleZijlstra JelleZijlstra merged commit 2d33f1f into python:main Jun 10, 2024
21 checks passed
@JelleZijlstra JelleZijlstra deleted the get_annotations branch June 10, 2024 03:03
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.

Add inspect.get_annotations backport
3 participants