Skip to content

Commit

Permalink
pythongh-119180: Fix bug where fwdrefs were evaluated in the annotati…
Browse files Browse the repository at this point in the history
…onlib module scope

We were sometimes passing None as the globals argument to eval(), which makes it
inherit the globals from the calling scope. Instead, ensure that globals is always
non-None. The test was passing accidentally because I passed "annotationlib" as a
module object; fix that. Also document the parameters to ForwardRef() and remove
two unused private ones.
  • Loading branch information
JelleZijlstra committed Sep 22, 2024
1 parent 453da53 commit 293fb0a
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 15 deletions.
32 changes: 19 additions & 13 deletions Lib/annotationlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,17 @@ class Format(enum.IntEnum):


class ForwardRef:
"""Wrapper that holds a forward reference."""
"""Wrapper that holds a forward reference.
Constructor arguments:
- arg: a string representing the code to be evaluated.
- module: the module where the forward reference was created. Must be a string,
not a module object.
- owner: The owning object (module, class, or function).
- is_argument: Does nothing, retained for compatibility.
- is_class: True if the forward reference was created in class scope.
"""

__slots__ = _SLOTS

Expand All @@ -57,8 +67,6 @@ def __init__(
owner=None,
is_argument=True,
is_class=False,
_globals=None,
_cell=None,
):
if not isinstance(arg, str):
raise TypeError(f"Forward reference must be a string -- got {arg!r}")
Expand All @@ -71,8 +79,8 @@ def __init__(
self.__forward_module__ = module
self.__code__ = None
self.__ast_node__ = None
self.__globals__ = _globals
self.__cell__ = _cell
self.__globals__ = None
self.__cell__ = None
self.__owner__ = owner

def __init_subclass__(cls, /, *args, **kwds):
Expand Down Expand Up @@ -115,6 +123,10 @@ def evaluate(self, *, globals=None, locals=None, type_params=None, owner=None):
elif callable(owner):
globals = getattr(owner, "__globals__", None)

# If we pass None to eval() below, the globals of this module are used.
if globals is None:
globals = {}

if locals is None:
locals = {}
if isinstance(owner, type):
Expand All @@ -134,14 +146,8 @@ def evaluate(self, *, globals=None, locals=None, type_params=None, owner=None):
# but should in turn be overridden by names in the class scope
# (which here are called `globalns`!)
if type_params is not None:
if globals is None:
globals = {}
else:
globals = dict(globals)
if locals is None:
locals = {}
else:
locals = dict(locals)
globals = dict(globals)
locals = dict(locals)
for param in type_params:
param_name = param.__name__
if not self.__forward_is_class__ or param_name not in globals:
Expand Down
6 changes: 4 additions & 2 deletions Lib/test/test_annotationlib.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Tests for the annotations module."""

import annotationlib
import collections
import functools
import itertools
import pickle
Expand Down Expand Up @@ -278,11 +279,12 @@ class Gen[T]:
)

def test_fwdref_with_module(self):
self.assertIs(ForwardRef("Format", module=annotationlib).evaluate(), Format)
self.assertIs(ForwardRef("Format", module="annotationlib").evaluate(), Format)
self.assertIs(ForwardRef("Counter", module="collections").evaluate(), collections.Counter)

with self.assertRaises(NameError):
# If globals are passed explicitly, we don't look at the module dict
ForwardRef("Format", module=annotationlib).evaluate(globals={})
ForwardRef("Format", module="annotationlib").evaluate(globals={})

def test_fwdref_value_is_cached(self):
fr = ForwardRef("hello")
Expand Down

0 comments on commit 293fb0a

Please sign in to comment.