Skip to content

gh-148680: Replace internal names with type_reprs of objects in string representations of ForwardRef #148682

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.

Sign up for GitHub

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

Open
DavidCEllis wants to merge 4 commits into
base: main
Choose a base branch
from
+58 −2
Open

gh-148680: Replace internal names with type_reprs of objects in string representations of ForwardRef #148682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12d7a45
Add tests for replacement of extra names in repr and string evaluation
DavidCEllis Apr 17, 2026
e1e258e
Replace names from __extra_names__ when evaluating forward references…
DavidCEllis Apr 17, 2026
ade5f8e
Try to write a clearer comment
DavidCEllis Apr 17, 2026
f3b6293
Make the repr test use retrieved rather than 'artificial' forward ref…
DavidCEllis Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions Lib/annotationlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def evaluate(
"""
match format:
case Format.STRING:
return self.__forward_arg__
return self.__resolved_forward_str__
case Format.VALUE:
is_forwardref_format = False
case Format.FORWARDREF:
Expand Down Expand Up @@ -258,6 +258,26 @@ def __forward_arg__(self):
"Attempted to access '__forward_arg__' on an uninitialized ForwardRef"
)

@property
def __resolved_forward_str__(self):
# __forward_arg__ with any names from __extra_names__ replaced
# with the type_repr of the value they represent
resolved_str = self.__forward_arg__
names = self.__extra_names__

if names:
# identifiers can be replaced directly
if resolved_str.isidentifier():
if (name_obj := names.get(resolved_str), _sentinel) is not _sentinel:
resolved_str = type_repr(name_obj)
else:
visitor = _ExtraNameFixer(names)
ast_expr = ast.parse(resolved_str, mode="eval").body
Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra Apr 17, 2026

Choose a reason for hiding this comment

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

We might need to cache this, it's probably pretty slow. What do you think?

Copy link
Copy Markdown
Contributor Author

@DavidCEllis DavidCEllis Apr 17, 2026

Choose a reason for hiding this comment

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

This is in fact, not very fast. Definitely worth caching if it's going to be accessed frequently.

Copy link
Copy Markdown
Contributor Author

@DavidCEllis DavidCEllis Apr 17, 2026

Choose a reason for hiding this comment

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

>>> a_anno
ForwardRef('unknown | str | int | list[str] | tuple[int, ...]', is_class=True, owner=<class '__main__.Example'>)
>>> b_anno
ForwardRef('unknown', is_class=True, owner=<class '__main__.Example'>)

>>> a = timeit(lambda: a_anno.__resolved_forward_str__, number=10_000)
>>> b = timeit(lambda: b_anno.__resolved_forward_str__, number=10_000)
>>> a
0.18359258100008446
>>> b
0.0018204959997092374
>>> a / b
100.8475607907994

node = visitor.visit(ast_expr)
resolved_str = ast.unparse(node)

return resolved_str

@property
def __forward_code__(self):
if self.__code__ is not None:
Expand Down Expand Up @@ -321,7 +341,7 @@ def __repr__(self):
extra.append(", is_class=True")
if self.__owner__ is not None:
extra.append(f", owner={self.__owner__!r}")
return f"ForwardRef({self.__forward_arg__!r}{''.join(extra)})"
return f"ForwardRef({self.__resolved_forward_str__!r}{''.join(extra)})"


_Template = type(t"")
Expand Down Expand Up @@ -1163,3 +1183,16 @@ def _get_dunder_annotations(obj):
if not isinstance(ann, dict):
raise ValueError(f"{obj!r}.__annotations__ is neither a dict nor None")
return ann


class _ExtraNameFixer(ast.NodeTransformer):
"""Fixer for __extra_names__ items in ForwardRef __repr__ and string evaluation"""
def __init__(self, extra_names):
self.extra_names = extra_names

def visit_Name(self, node: ast.Name):
if (new_name := self.extra_names.get(node.id, _sentinel)) is not _sentinel:
new_node = ast.Name(id=type_repr(new_name))
ast.copy_location(node, new_node)
node = new_node
return node
23 changes: 23 additions & 0 deletions Lib/test/test_annotationlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1961,6 +1961,15 @@ def test_forward_repr(self):
"typing.List[ForwardRef('int', owner='class')]",
)

def test_forward_repr_extra_names(self):
Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra Apr 17, 2026

Choose a reason for hiding this comment

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

Can you make the tests rely less on direct assignment to __extra_names__? I don't care too much about the behavior if you manually construct ForwardRefs and set attributes on them, what matters is the buggy behavior with things you get from get_annotations.

Copy link
Copy Markdown
Contributor Author

@DavidCEllis DavidCEllis Apr 17, 2026

Choose a reason for hiding this comment

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

Yes, I can do that here. This was partly done to avoid the owner=... part of the repr.

The other example of this in test_evaluate_string_format_extra_names exists largely to exercise the logic for the single identifier case. I'm actually not sure you can get the single identifier case with standard get_annotations?

I need this in reannotate for splitting up unions within forward references but unless you're doing similar manipulations I'm not sure this is a case you can get?

Perhaps the isidentifier special case should be removed? The other logic should work for it anyway.

def f(a: undefined | str): ...

annos = get_annotations(f, format=Format.FORWARDREF)

self.assertRegex(
repr(annos['a']), r"ForwardRef\('undefined \| str'.*\)"
)

def test_forward_recursion_actually(self):
def namespace1():
a = ForwardRef("A")
Expand Down Expand Up @@ -2037,6 +2046,20 @@ def test_evaluate_string_format(self):
fr = ForwardRef("set[Any]")
self.assertEqual(fr.evaluate(format=Format.STRING), "set[Any]")

def test_evaluate_string_format_extra_names(self):
# Test that internal extra_names are replaced when evaluating as strings

# As identifier
fr = ForwardRef("__annotationlib_name_1__")
fr.__extra_names__ = {"__annotationlib_name_1__": str}
self.assertEqual(fr.evaluate(format=Format.STRING), "str")

# Via AST visitor
def f(a: ref | str): ...

fr = get_annotations(f, format=Format.FORWARDREF)['a']
self.assertEqual(fr.evaluate(format=Format.STRING), "ref | str")

def test_evaluate_forwardref_format(self):
fr = ForwardRef("undef")
evaluated = fr.evaluate(format=Format.FORWARDREF)
Expand Down
Loading