Skip to content

gh-156664: Fix interaction between free variables and comprehensions #156691

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
JelleZijlstra wants to merge 5 commits into
base: main
Choose a base branch
from
Open

gh-156664: Fix interaction between free variables and comprehensions #156691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
829258e
gh-156664: Fix free variables in sibling comprehensions
JelleZijlstra Aug 30, 2026
65116d5
gh-156664: Keep comprehension cells separate from free variables
JelleZijlstra Aug 30, 2026
9c49003
gh-156664: Split comprehension scope regression tests
JelleZijlstra Aug 30, 2026
c93c1d8
gh-156664: Preserve enclosing cells in nested comprehensions
JelleZijlstra Sep 1, 2026
f2c0db4
gh-156664: Deduplicate comprehension frame locals
JelleZijlstra Sep 1, 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
47 changes: 47 additions & 0 deletions Lib/test/test_frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,53 @@ def test_closure_with_inline_comprehension(self):
lst = [locals() for k in [0]]
self.assertEqual(lst[0]['k'], 0)

def test_closure_with_inline_comprehension_proxy(self):
def snapshot():
proxy = sys._getframe(1).f_locals
return (
dict(**proxy),
proxy.copy(),
proxy.keys(),
proxy.values(),
proxy.items(),
len(proxy),
)

x = [1]

def inner():
return [(lambda: x, snapshot()) for x in x]

func, proxy_views = inner()[0]
self.assertEqual(func(), 1)
expected = {'x': 1, 'snapshot': snapshot}
self.assertEqual(
proxy_views,
(
expected,
expected,
['x', 'snapshot'],
[1, snapshot],
[('x', 1), ('snapshot', snapshot)],
2,
),
)

def test_closure_with_inline_comprehension_proxy_write(self):
def write_x(value):
proxy = sys._getframe(1).f_locals
proxy['x'] = value
return proxy['x']

x = 3

def inner():
proxy_saw = write_x(4)
funcs = [lambda: x for x in [1]]
return proxy_saw, x, funcs[0]()

self.assertEqual(inner(), (4, 4, 1))

def test_as_dict(self):
x = 1
y = 2
Expand Down
83 changes: 83 additions & 0 deletions Lib/test/test_listcomps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,89 @@ 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_comprehension_name_reuse_with_free_variable(self):
x = 3

def sibling_comprehension():
[x for x in [1]]
return [x for _ in [1]]

self.assertEqual(sibling_comprehension(), [3])

def nested_function():
[x for x in [1]]

def inner():
return x

return inner()

self.assertEqual(nested_function(), 3)

def test_comprehension_cell_and_free_variable(self):
x = 3

def captured_then_sibling():
funcs = [lambda: x for x in [1]]
return funcs[0](), [x for _ in [1]]

self.assertEqual(captured_then_sibling(), (1, [3]))

def captured_then_nested_function():
funcs = [lambda: x for x in [1]]

def inner():
return x

return funcs[0](), inner()

self.assertEqual(captured_then_nested_function(), (1, 3))

def captured_then_generator_expression():
funcs = [lambda: x for x in [1]]
return funcs[0](), list(x for _ in [1])

self.assertEqual(captured_then_generator_expression(), (1, [3]))

def test_nested_comprehension_cell_and_free_variable(self):
def local_cell(x):
result = [
([lambda: x for x in [1]], lambda: x, [x for _ in [0]])
for _ in [0]
]
captured, sibling, sibling_comprehension = result[0]
return captured[0](), sibling(), sibling_comprehension

self.assertEqual(local_cell(7), (1, 7, [7]))

x = 7

def free_variable():
result = [
([lambda: x for x in [1]], lambda: x, [x for _ in [0]])
for _ in [0]
]
captured, sibling, sibling_comprehension = result[0]
return captured[0](), sibling(), sibling_comprehension

self.assertEqual(free_variable(), (1, 7, [7]))

def test_comprehension_cell_exception_cleanup(self):
x = 3

def raises_after_one():
yield 1
raise RuntimeError

def captured_then_exception():
funcs = []
try:
[funcs.append(lambda: x) for x in raises_after_one()]
except RuntimeError:
return funcs[0](), [x for _ in [1]]

self.assertEqual(captured_then_exception(), (1, [3]))

def test_exception_locations(self):
# The location of an exception raised from __init__ or
# __next__ should be the iterator expression
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2026-08-30-07-36-57.gh-issue-156664.Ef7PHC.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix scope analysis for a name bound in one inlined comprehension and used as
a free variable by a sibling comprehension or nested function.
182 changes: 66 additions & 116 deletions Objects/frameobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject *key, bool read, PyO
}

bool found = false;
int write_fallback = -1;

// We do 2 loops here because it's highly possible the key is interned
// and we can do a pointer comparison.
Expand All @@ -139,7 +140,12 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject *key, bool read, PyO
}
} else {
if (!(_PyLocals_GetKind(co->co_localspluskinds, i) & CO_FAST_HIDDEN)) {
return i;
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {

@carljm carljm Sep 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comprehension's cell can be active but still empty before the first target assignment. In that state, this hasval() check skips it and directs a proxy write into the enclosing free-variable slot:

import sys

def values():
    sys._getframe(1).f_locals["x"] = 42
    yield 1

def outer():
    x = 7
    def inner():
        [lambda: x for x in (x, values())[1]]
    inner()
    return x

print(outer())

Main and 9c49003 print 7, while f2c0db42 prints 42. The write during iterator advancement escapes the comprehension and changes the enclosing variable.

return i;
}
if (write_fallback < 0) {
write_fallback = i;
}
}
}
found = true;
Expand All @@ -148,7 +154,7 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject *key, bool read, PyO
if (found) {
// This is an attempt to read an unset local variable or
// write to a variable that is hidden from regular write operations
return -1;
return read ? -1 : write_fallback;
}
// This is unlikely, but we need to make sure. This means the key
// is not interned.
Expand Down Expand Up @@ -177,13 +183,52 @@ framelocalsproxy_getkeyindex(PyFrameObject *frame, PyObject *key, bool read, PyO
}
} else {
if (!(_PyLocals_GetKind(co->co_localspluskinds, i) & CO_FAST_HIDDEN)) {
return i;
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {
return i;
}
if (write_fallback < 0) {
write_fallback = i;
}
}
}
}
}

return -1;
return read ? -1 : write_fallback;
}

static PyObject *
framelocalsproxy_snapshot(PyFrameObject *frame)
{
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *snapshot = PyDict_New();
if (snapshot == NULL) {
return NULL;
}

for (int i = 0; i < co->co_nlocalsplus; i++) {
PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i);
if (value == NULL) {
continue;
}
PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i);
// Match direct lookup by keeping the first live slot when a
// comprehension cell and a free variable have the same name.
if (PyDict_SetDefaultRef(snapshot, name, value, NULL) < 0) {
Py_DECREF(value);
Py_DECREF(snapshot);
return NULL;
}
Py_DECREF(value);
}

if (frame->f_extra_locals != NULL &&
PyDict_Merge(snapshot, frame->f_extra_locals, 0) < 0)
{
Py_DECREF(snapshot);
return NULL;
}
return snapshot;
}

static PyObject *
Expand Down Expand Up @@ -375,38 +420,12 @@ static PyObject *
framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored))
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *names = PyList_New(0);
if (names == NULL) {
PyObject *snapshot = framelocalsproxy_snapshot(frame);
if (snapshot == NULL) {
return NULL;
}

for (int i = 0; i < co->co_nlocalsplus; i++) {
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {
PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i);
if (PyList_Append(names, name) < 0) {
Py_DECREF(names);
return NULL;
}
}
}

// Iterate through the extra locals
if (frame->f_extra_locals) {
assert(PyDict_Check(frame->f_extra_locals));

Py_ssize_t i = 0;
PyObject *key = NULL;
PyObject *value = NULL;

while (PyDict_Next(frame->f_extra_locals, &i, &key, &value)) {
if (PyList_Append(names, key) < 0) {
Py_DECREF(names);
return NULL;
}
}
}

PyObject *names = PyDict_Keys(snapshot);
Py_DECREF(snapshot);
return names;
}

Expand Down Expand Up @@ -584,107 +603,38 @@ static PyObject *
framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored))
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *values = PyList_New(0);
if (values == NULL) {
PyObject *snapshot = framelocalsproxy_snapshot(frame);
if (snapshot == NULL) {
return NULL;
}

for (int i = 0; i < co->co_nlocalsplus; i++) {
PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i);
if (value) {
if (PyList_Append(values, value) < 0) {
Py_DECREF(values);
Py_DECREF(value);
return NULL;
}
Py_DECREF(value);
}
}

// Iterate through the extra locals
if (frame->f_extra_locals) {
Py_ssize_t j = 0;
PyObject *key = NULL;
PyObject *value = NULL;
while (PyDict_Next(frame->f_extra_locals, &j, &key, &value)) {
if (PyList_Append(values, value) < 0) {
Py_DECREF(values);
return NULL;
}
}
}

PyObject *values = PyDict_Values(snapshot);
Py_DECREF(snapshot);
return values;
}

static PyObject *
framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored))
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
PyObject *items = PyList_New(0);
if (items == NULL) {
PyObject *snapshot = framelocalsproxy_snapshot(frame);
if (snapshot == NULL) {
return NULL;
}

for (int i = 0; i < co->co_nlocalsplus; i++) {
PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i);
PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i);

if (value) {
PyObject *pair = _PyTuple_FromPairSteal(Py_NewRef(name), value);
if (pair == NULL) {
goto error;
}

if (_PyList_AppendTakeRef((PyListObject *)items, pair) < 0) {
goto error;
}
}
}

// Iterate through the extra locals
if (frame->f_extra_locals) {
Py_ssize_t j = 0;
PyObject *key = NULL;
PyObject *value = NULL;
while (PyDict_Next(frame->f_extra_locals, &j, &key, &value)) {
PyObject *pair = _PyTuple_FromPair(key, value);
if (pair == NULL) {
goto error;
}

if (_PyList_AppendTakeRef((PyListObject *)items, pair) < 0) {
goto error;
}
}
}

PyObject *items = PyDict_Items(snapshot);
Py_DECREF(snapshot);
return items;

error:
Py_DECREF(items);
return NULL;
}

static Py_ssize_t
framelocalsproxy_length(PyObject *self)
{
PyFrameObject *frame = PyFrameLocalsProxyObject_CAST(self)->frame;
PyCodeObject *co = _PyFrame_GetCode(frame->f_frame);
Py_ssize_t size = 0;

if (frame->f_extra_locals != NULL) {
assert(PyDict_Check(frame->f_extra_locals));
size += PyDict_Size(frame->f_extra_locals);
}

for (int i = 0; i < co->co_nlocalsplus; i++) {
if (framelocalsproxy_hasval(frame->f_frame, co, i)) {
size++;
}
PyObject *snapshot = framelocalsproxy_snapshot(frame);

@carljm carljm Sep 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure how much performance of framelocalsproxy matters in practice, but now even frames without comprehensions or duplicate names construct and destroy a full dictionary for len(proxy). The same snapshot cost also applies to key enumeration and the other views.

In matching --with-pydebug / -Og builds, Codex measured len(proxy) with 32 ordinary locals increasing from about 0.16 µs on main to 1.10 µs here; with 128 locals, it went from 0.52 µs to 4.39 µs. Key enumeration was roughly three times slower. These are microbenchmarks taking the minimum of three runs of 20,000 operations, on a frame with no comprehensions.

If this matters, we could maintain a fast path when duplicate bindings are impossible, especially for length, which previously required no container allocation?

if (snapshot == NULL) {
return -1;
}
Py_ssize_t size = PyDict_GET_SIZE(snapshot);
Py_DECREF(snapshot);
return size;
}

Expand Down
Loading
Loading