annotationlib: `ref.evaluate(format=Format.FORWARDREF)` returns a ForwardRef with a copied `__globals__` that no longer updates

[DavidCEllis]

Bug report

Bug description:

This can happen internally in get_annotations and means subsequent attempts to evaluate it will fail even if the names have since been defined.

Underlying logic:

from annotationlib import get_annotations, Format

class Demo:
    x: Sequence[undefined]

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

x_anno = annos['x']

# Try to evaluate the reference, but just give a forwardref if it fails
x_repeat_anno = x_anno.evaluate(format=Format.FORWARDREF)

# The resulting reference no longer shares the globals namespace with the annotate function
print(f"{x_anno.__globals__ is Demo.__annotate__.__globals__ = }")  # True
print(f"{x_repeat_anno.__globals__ is Demo.__annotate__.__globals__ = }")  # False

# Define the previously undefined attributes
from collections.abc import Sequence
undefined = str

# This means evaluation fails in the second case
print(f"{x_anno.evaluate() = }")  # collections.abc.Sequence[str]
print(f"{x_repeat_anno.evaluate() = }")  # NameError

This evaluate call happens internally if get_annotations has to rely on the fallback behaviour for an unexpected exception, such as an AttributeError:

from annotationlib import get_annotations, Format
import typing

class Works:
    a: Sequence[undefined]
    b: unknowable

# Intentionally set up an annotation that will raise AttributeError on evaluation
class Fails:
    a: Sequence[undefined]
    b: typing.doesnotexist

a_works = get_annotations(Works, format=Format.FORWARDREF)['a']
a_fails = get_annotations(Fails, format=Format.FORWARDREF)['a']

# Realise the references
from collections.abc import Sequence
undefined = str

print(f"{a_works.evaluate() = }")  # collections.abc.Sequence[str]
print(f"{a_fails.evaluate() = }")  # NameError

This appears to be caused by the creation of a new globals dict here:

cpython/Lib/annotationlib.py

Lines 164 to 167 in e39255e

	if type_params is not None:
	globals = dict(globals)
	for param in type_params:
	globals[param.__name__] = param

Commenting this out makes these examples succeed, but obviously breaks type parameters.

CPython versions tested on:

CPython main branch, 3.14

Operating systems tested on:

No response

Linked PRs

• gh-137969: Fix evaluation of ref.evaluate(format=Format.FORWARDREF) objects #138075
• [3.14] gh-137969: Fix evaluation of ref.evaluate(format=Format.FORWARDREF) objects (GH-138075) #140929
• Revert "gh-137969: Fix evaluation of ref.evaluate(format=Format.FORWARDREF) objects (#138075)" #140930
• [3.14] Revert "gh-137969: Fix evaluation of ref.evaluate(format=Format.FORWARDREF) objects (GH-138075) (#140929)" #140931
• gh-137969: Fix double evaluation of ForwardRefs which rely on globals #140974
• [3.14] gh-137969: Fix double evaluation of ForwardRefs which rely on globals (GH-140974) #141527

[dr-carlos]

A simple (possibly naive?) fix for this is just to add a new dictionary to store type param values instead of copying the globals.
Then, a check against this dict is added where globals is accessed later in evaluate(), logically between the locals and globals.

See dr-carlos@f8ac2c0

This seemingly fixes the provided example and works with type parameters (as far as my simple tests have shown).
Happy to open a PR, just wanting to make sure I haven't missed the problem (or created a new one).

[DavidCEllis]

Make sure you run the annotationlib tests with:

./python -m test -v test_annotationlib

It looks like your commit breaks the tests as you shadow the types module with your new variable which breaks the check a few lines up for isinstance(owner, types.ModuleType).

I thought I'd tried putting the type_params at the start of the locals and had test failures I didn't have time to investigate, but I think I had some logic in the wrong order.

[dr-carlos]

Thanks for the heads up re the test command!

I've renamed the variable to something much more sensible (forgot I was going to do that before posting earlier 🤦 )
All tests are now passing. A new test would possibly be warranted for this use case, but that should probably be discussed in some future PR.

[JelleZijlstra]

That sounds reasonable, please send a PR and I'll take a look.