From fbe0f41f714082876edf3a7246c40521d984717c Mon Sep 17 00:00:00 2001 From: Kyle Cutler Date: Mon, 3 Mar 2025 16:56:27 -0800 Subject: [PATCH 1/8] gh-130809: Fix `PyFrame_LocalsToFast` copying the wrong value --- Objects/frameobject.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index d33c3cde526e9f..1087dd56c839b5 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1399,14 +1399,14 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) PyObject *exc = PyErr_GetRaisedException(); for (int i = 0; i < co->co_nlocalsplus; i++) { - _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); - - /* Same test as in PyFrame_FastToLocals() above. */ - if (kind & CO_FAST_FREE && !(co->co_flags & CO_OPTIMIZED)) { + /* Same test as in _PyFrame_GetLocals() above. */ + PyObject *value; // borrowed reference + if (!frame_get_var(frame, co, i, &value)) { continue; } + PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - PyObject *value = PyObject_GetItem(locals, name); + _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); /* We only care about NULLs if clear is true. */ if (value == NULL) { PyErr_Clear(); @@ -1452,7 +1452,6 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } Py_XSETREF(fast[i], Py_NewRef(value)); } - Py_XDECREF(value); } PyErr_SetRaisedException(exc); } From 665263a807940ca99d2ef00029171b40bf01091f Mon Sep 17 00:00:00 2001 From: kycutler Date: Tue, 4 Mar 2025 11:44:26 -0800 Subject: [PATCH 2/8] Skip hidden locals --- Objects/frameobject.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Objects/frameobject.c b/Objects/frameobject.c index 1087dd56c839b5..44f2726f2ccaf1 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -1399,14 +1399,17 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) PyObject *exc = PyErr_GetRaisedException(); for (int i = 0; i < co->co_nlocalsplus; i++) { - /* Same test as in _PyFrame_GetLocals() above. */ - PyObject *value; // borrowed reference - if (!frame_get_var(frame, co, i, &value)) { + _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); + + /* Same test as in PyFrame_FastToLocals() above. */ + if (kind & CO_FAST_FREE && !(co->co_flags & CO_OPTIMIZED)) { + continue; + } + if (kind & CO_FAST_HIDDEN) { continue; } - PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); + PyObject *value = PyObject_GetItem(locals, name); /* We only care about NULLs if clear is true. */ if (value == NULL) { PyErr_Clear(); @@ -1452,6 +1455,7 @@ _PyFrame_LocalsToFast(_PyInterpreterFrame *frame, int clear) } Py_XSETREF(fast[i], Py_NewRef(value)); } + Py_XDECREF(value); } PyErr_SetRaisedException(exc); } From 71140299cb0486a9b5e9d412eaf5156b76e14b3b Mon Sep 17 00:00:00 2001 From: kycutler Date: Tue, 4 Mar 2025 12:52:43 -0800 Subject: [PATCH 3/8] test, blurb --- Lib/test/test_listcomps.py | 9 +++++++++ .../2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst | 2 ++ 2 files changed, 11 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index 2065afd455de5c..22f0741e86bb71 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -709,6 +709,15 @@ def test_multiple_comprehension_name_reuse(self): self._check_in_scopes(code, {"x": 2, "y": [3]}, ns={"x": 3}, scopes=["class"]) self._check_in_scopes(code, {"x": 2, "y": [2]}, ns={"x": 3}, scopes=["function", "module"]) + def test_name_collision_locals(self): + code = """ + x = 1 + [x for x in [0]] + from abc import * + exec("b = 2") + """ + self._check_in_scopes(code, {"b": 2, "x": 1}, scopes=["module"]) + def test_exception_locations(self): # The location of an exception raised from __init__ or # __next__ should should be the iterator expression diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst new file mode 100644 index 00000000000000..4130705fe151f5 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst @@ -0,0 +1,2 @@ +Fix an issue where modifications to locals could fail to persist when there +is an inlined comprehension with colliding variable name. From e65279dcbe88e4ac22f7c0b4f96e1b127da3ecc2 Mon Sep 17 00:00:00 2001 From: Kyle Cutler <67761731+kycutler@users.noreply.github.com> Date: Tue, 4 Mar 2025 15:24:30 -0800 Subject: [PATCH 4/8] Update Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst Co-authored-by: Tian Gao --- .../2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst index 4130705fe151f5..419b2a3eb85043 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-03-04-12-52-21.gh-issue-130809.fSXq60.rst @@ -1,2 +1,2 @@ -Fix an issue where modifications to locals could fail to persist when there -is an inlined comprehension with colliding variable name. +Fixed an issue where ``_PyFrame_LocalsToFast`` tries to write module level +values to hidden fasts. From 2f1e4aa36a0ce330636409cc462449497bd7564f Mon Sep 17 00:00:00 2001 From: kycutler Date: Wed, 5 Mar 2025 08:12:49 -0800 Subject: [PATCH 5/8] Update test --- Lib/test/test_listcomps.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index 22f0741e86bb71..3e180a866c03b8 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -711,12 +711,15 @@ def test_multiple_comprehension_name_reuse(self): def test_name_collision_locals(self): code = """ - x = 1 - [x for x in [0]] + import sys + frame = sys._getframe() + f_locals = frame.f_locals + foo = 1 + [foo for foo in [0]] from abc import * - exec("b = 2") + assert frame.f_locals is f_locals """ - self._check_in_scopes(code, {"b": 2, "x": 1}, scopes=["module"]) + self._check_in_scopes(code, {"foo": 1}, scopes=["module"]) def test_exception_locations(self): # The location of an exception raised from __init__ or From 10e8035c1f3c734ab4967681201643dcc999e5e3 Mon Sep 17 00:00:00 2001 From: kycutler Date: Wed, 5 Mar 2025 09:54:41 -0800 Subject: [PATCH 6/8] PR feedback --- Lib/test/test_listcomps.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index 3e180a866c03b8..0bedf8d0ad6fcc 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -710,6 +710,9 @@ def test_multiple_comprehension_name_reuse(self): self._check_in_scopes(code, {"x": 2, "y": [2]}, ns={"x": 3}, scopes=["function", "module"]) def test_name_collision_locals(self): + # GH-130809: The existence of a hidden fast from list comprehension should not cause + # frame.f_locals on module level to return a new dict every time it is accessed. + code = """ import sys frame = sys._getframe() @@ -717,9 +720,9 @@ def test_name_collision_locals(self): foo = 1 [foo for foo in [0]] from abc import * - assert frame.f_locals is f_locals + same_f_locals = frame.f_locals is f_locals """ - self._check_in_scopes(code, {"foo": 1}, scopes=["module"]) + self._check_in_scopes(code, {"foo": 1, "same_f_locals": True}, scopes=["module"]) def test_exception_locations(self): # The location of an exception raised from __init__ or From 9a49729642f0efb49a05ac4ce682a391e7b91bd5 Mon Sep 17 00:00:00 2001 From: kycutler Date: Wed, 5 Mar 2025 10:10:59 -0800 Subject: [PATCH 7/8] formatting --- Lib/test/test_listcomps.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index 0bedf8d0ad6fcc..55c73c1b0676ff 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -710,8 +710,9 @@ def test_multiple_comprehension_name_reuse(self): self._check_in_scopes(code, {"x": 2, "y": [2]}, ns={"x": 3}, scopes=["function", "module"]) def test_name_collision_locals(self): - # GH-130809: The existence of a hidden fast from list comprehension should not cause - # frame.f_locals on module level to return a new dict every time it is accessed. + # GH-130809: The existence of a hidden fast from list comprehension + # should not cause frame.f_locals on module level to return a new dict + # every time it is accessed. code = """ import sys From 07451af06aa706ffb23b0a365053ef51ef7db543 Mon Sep 17 00:00:00 2001 From: kycutler Date: Wed, 5 Mar 2025 11:05:40 -0800 Subject: [PATCH 8/8] comment --- Lib/test/test_listcomps.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index 55c73c1b0676ff..3e2ef1809779c3 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -720,6 +720,7 @@ def test_name_collision_locals(self): f_locals = frame.f_locals foo = 1 [foo for foo in [0]] + # calls _PyFrame_LocalsToFast which triggers the issue from abc import * same_f_locals = frame.f_locals is f_locals """