From 293fb0aad9504ddccc68423a5648f5d456ee1068 Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Sun, 22 Sep 2024 14:25:04 -0700 Subject: [PATCH 1/3] gh-119180: Fix bug where fwdrefs were evaluated in the annotationlib 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. --- Lib/annotationlib.py | 32 +++++++++++++++++++------------- Lib/test/test_annotationlib.py | 6 ++++-- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/Lib/annotationlib.py b/Lib/annotationlib.py index 9d1943b27e8e9c..ffd47497056289 100644 --- a/Lib/annotationlib.py +++ b/Lib/annotationlib.py @@ -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 @@ -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}") @@ -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): @@ -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): @@ -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: diff --git a/Lib/test/test_annotationlib.py b/Lib/test/test_annotationlib.py index ce4f92624d9036..321cd38a9a7fff 100644 --- a/Lib/test/test_annotationlib.py +++ b/Lib/test/test_annotationlib.py @@ -1,6 +1,7 @@ """Tests for the annotations module.""" import annotationlib +import collections import functools import itertools import pickle @@ -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") From 4f08276e95337d4d4505ae50ff94b53f25dab060 Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Sun, 22 Sep 2024 14:50:04 -0700 Subject: [PATCH 2/3] More tests --- Lib/test/test_annotationlib.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Lib/test/test_annotationlib.py b/Lib/test/test_annotationlib.py index 321cd38a9a7fff..8867fcec9c753e 100644 --- a/Lib/test/test_annotationlib.py +++ b/Lib/test/test_annotationlib.py @@ -286,6 +286,18 @@ def test_fwdref_with_module(self): # If globals are passed explicitly, we don't look at the module dict ForwardRef("Format", module="annotationlib").evaluate(globals={}) + def test_fwdref_to_builtin(self): + self.assertIs(ForwardRef("int").evaluate(), int) + self.assertIs(ForwardRef("int", module="collections").evaluate(), int) + self.assertIs(ForwardRef("int", owner=str).evaluate(), int) + + # builtins are still searched with explicit globals + self.assertIs(ForwardRef("int").evaluate(globals={}), int) + + # explicit values in globals have precedence + obj = object() + self.assertIs(ForwardRef("int").evaluate(globals={"int": obj}), obj) + def test_fwdref_value_is_cached(self): fr = ForwardRef("hello") with self.assertRaises(NameError): From fca05d572cdfaad74360b3a97f6eb17e5b08992c Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Mon, 23 Sep 2024 11:20:54 -0700 Subject: [PATCH 3/3] Update Lib/annotationlib.py Co-authored-by: Alex Waygood --- Lib/annotationlib.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/annotationlib.py b/Lib/annotationlib.py index ffd47497056289..cb86f9c682b5f6 100644 --- a/Lib/annotationlib.py +++ b/Lib/annotationlib.py @@ -48,12 +48,12 @@ class ForwardRef: """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. + * 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. """