Skip to content

Commit df243ab

Browse files
gh-124697: fix slot-reuse edge cases for inlined comprehensions
Skip reuse for class-closure names; isolate reused frees with LOAD_CLOSURE + empty MAKE_CELL; treat DEF_FREE_CLASS as reusable; and handle temp frees in FrameLocalsProxy and LOAD_DEREF. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 3c5c710 commit df243ab

15 files changed

Lines changed: 317 additions & 89 deletions

File tree

‎Doc/library/dis.rst‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2003,21 +2003,11 @@ but are replaced by real opcodes or removed before bytecode is generated.
20032003
.. versionchanged:: 3.13
20042004
This opcode is now a pseudo-instruction.
20052005

2006-
.. opcode:: LOAD_CLOSURE_AND_CLEAR (i)
2007-
2008-
Pushes a reference to the cell contained in slot ``i`` of the "fast locals"
2009-
storage and clears that slot. Used to isolate an inlined comprehension
2010-
local that reuses an enclosing free variable.
2011-
2012-
Note that ``LOAD_CLOSURE_AND_CLEAR`` is replaced with
2013-
``LOAD_FAST_AND_CLEAR`` in the assembler.
2014-
2015-
.. versionadded:: next
2016-
20172006
.. opcode:: STORE_CLOSURE (i)
20182007

20192008
Stores the TOS into the cell slot ``i`` of the "fast locals" storage.
2020-
Used to restore a cell saved by ``LOAD_CLOSURE_AND_CLEAR``.
2009+
Used to restore a cell saved by ``LOAD_CLOSURE`` when isolating an
2010+
inlined comprehension that reuses an enclosing free variable.
20212011

20222012
Note that ``STORE_CLOSURE`` is replaced with ``STORE_FAST`` in the assembler.
20232013

‎Include/internal/pycore_interpframe.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,12 @@ _PyFrame_Traverse(_PyInterpreterFrame *frame, visitproc visit, void *arg);
376376
bool
377377
_PyFrame_HasHiddenLocals(_PyInterpreterFrame *frame);
378378

379+
/* True when localsplus[oparg] is a free cell that currently differs from
380+
* the function's func_closure cell — i.e. an inlined comprehension has
381+
* temporarily replaced it. */
382+
PyAPI_FUNC(bool)
383+
_PyFrame_IsInlinedCompTempFree(_PyInterpreterFrame *frame, int oparg);
384+
379385
PyObject *
380386
_PyFrame_GetLocals(_PyInterpreterFrame *frame);
381387

‎Include/internal/pycore_opcode_metadata.h‎

Lines changed: 8 additions & 16 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Include/opcode_ids.h‎

Lines changed: 6 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎InternalDocs/inlined_comprehensions.md‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,12 @@ Class-closure names that would otherwise be free through a class become
8787
`GLOBAL_IMPLICIT`.
8888

8989
If the inlined name is `LOCAL` or `CELL` but the nearest non-inlined
90-
enclosing table has it as `FREE`, resolve it as `FREE` so the
91-
comprehension reuses that localsplus slot. `compiler_cellvars()` also
92-
skips adding those child cells, which would otherwise create a second
93-
same-named entry.
90+
enclosing table has it as `FREE` (or `DEF_FREE_CLASS`), resolve it as
91+
`FREE` so the comprehension reuses that localsplus slot.
92+
`compiler_cellvars()` also skips adding those child cells, which would
93+
otherwise create a second same-named entry. Class-closure names
94+
(`__class__` and friends) are never reused: zero-arg `super()` and
95+
class-cell bookkeeping need the real free cell.
9496

9597
### Isolating iteration variables
9698

@@ -100,14 +102,12 @@ the comprehension on one of two paths:
100102

101103
Reuse an enclosing free (`_PyCompile_GetRefType()` is `FREE`):
102104

103-
* `LOAD_CLOSURE_AND_CLEAR` saves the enclosing cell.
104-
* `MAKE_CELL` always runs, so the slot holds a fresh empty cell.
105-
The comprehension then uses `DEREF`; it must not store into the
106-
enclosing cell.
107-
* Restore uses `STORE_CLOSURE`.
108-
* Those pseudo instructions carry a cell/free index so
109-
`fix_cell_offsets` can remap them; they become `LOAD_FAST_AND_CLEAR`
110-
/ `STORE_FAST` in the assembler.
105+
* `LOAD_CLOSURE` saves the enclosing cell without clearing the slot
106+
(so free-slot introspection never sees a NULL).
107+
* `MAKE_CELL` on a `CO_FAST_FREE` slot always installs a fresh empty
108+
cell, replacing the saved one. The comprehension then uses `DEREF`.
109+
* Restore uses `STORE_CLOSURE` (a cell/free-index pseudo that becomes
110+
`STORE_FAST` after `fix_cell_offsets`).
111111

112112
Own fast-local slot (everything else):
113113

@@ -133,6 +133,12 @@ seen by existing closures; lambdas that capture the iteration variable
133133
share the temporary cell. After the comprehension, the original cell
134134
is restored.
135135

136+
While the temporary cell is installed, `_PyFrame_IsInlinedCompTempFree()`
137+
is true (frame cell differs from `func_closure`). That drives
138+
class/module `locals()` to use `FrameLocalsProxy` even without a
139+
`CO_FAST_HIDDEN` slot, and makes an empty temporary cell raise
140+
`UnboundLocalError` rather than `NameError`.
141+
136142
Source
137143
------
138144

‎Lib/_opcode_metadata.py‎

Lines changed: 6 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Lib/test/test_listcomps.py‎

Lines changed: 129 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import doctest
2+
import sys
23
import textwrap
34
import traceback
45
import types
56
import unittest
67

78
from test import support
8-
from test.support import BrokenIter
9+
from test.support import BrokenIter, import_helper
910

1011

1112
doctests = """
@@ -359,6 +360,133 @@ def inner():
359360
return inner()
360361
self.assertEqual(outer([1, 2]), [1, 2])
361362

363+
def test_reuse_class_closure_name_as_param_with_lambda(self):
364+
# Class-closure names are not reused. Lambdas share the last
365+
# iteration value; the enclosing binding is unchanged.
366+
def outer(__class__):
367+
class C:
368+
result = [lambda: __class__ for __class__ in __class__]
369+
return [f() for f in C.result], __class__
370+
self.assertEqual(outer([1, 2]), ([2, 2], [1, 2]))
371+
372+
def outer(__classdict__):
373+
class C:
374+
result = [lambda: __classdict__ for __classdict__ in __classdict__]
375+
return [f() for f in C.result], __classdict__
376+
self.assertEqual(outer([1, 2]), ([2, 2], [1, 2]))
377+
378+
def test_reuse_class_closure_name_as_param_preserves_enclosing(self):
379+
def outer(__class__):
380+
class C:
381+
result = [__class__ for __class__ in __class__]
382+
return C.result, __class__
383+
self.assertEqual(outer([1, 2]), ([1, 2], [1, 2]))
384+
385+
def outer(__classdict__):
386+
class C:
387+
result = [__classdict__ for __classdict__ in __classdict__]
388+
return C.result, __classdict__
389+
self.assertEqual(outer([1, 2]), ([1, 2], [1, 2]))
390+
391+
def test_reuse_free_slot_visible_to_opcode_trace_getvar(self):
392+
# Isolation must not leave a free slot temporarily NULL where
393+
# PyFrame_GetVar() (and similar) assume a cell is always present.
394+
_testcapi = import_helper.import_module("_testcapi")
395+
396+
def outer(x):
397+
def inner():
398+
return [x for x in x]
399+
return inner
400+
401+
f = outer([1])
402+
403+
def trace(frame, event, arg):
404+
if frame.f_code is f.__code__:
405+
frame.f_trace_opcodes = True
406+
if event == "opcode":
407+
try:
408+
_testcapi.frame_getvar(frame, "x")
409+
except NameError:
410+
pass
411+
return trace
412+
413+
sys.settrace(trace)
414+
try:
415+
self.assertEqual(f(), [1])
416+
finally:
417+
sys.settrace(None)
418+
419+
def test_reuse_does_not_break_zero_arg_super(self):
420+
# Temporary reuse of the __class__ free must not change what
421+
# zero-argument super() observes during the comprehension.
422+
class C:
423+
def method(self):
424+
__class__
425+
return [super() for __class__ in (int,)]
426+
427+
self.assertIs(C().method()[0].__thisclass__, C)
428+
429+
def test_reuse_class_body_locals_sees_iteration_var(self):
430+
# Class-body locals() must still expose the comprehension target
431+
# when that name reuses an enclosing free (no separate hidden slot).
432+
def outer(x):
433+
class C:
434+
values = [locals()["x"] for x in x]
435+
return C.values
436+
self.assertEqual(outer([1, 2]), [1, 2])
437+
438+
def outer_eval(x):
439+
class C:
440+
values = [eval("x") for x in x]
441+
return C.values
442+
self.assertEqual(outer_eval([1, 2]), [1, 2])
443+
444+
def test_reuse_uninitialized_comp_local_is_unbound_local(self):
445+
# Reading the comprehension target before it is assigned must remain
446+
# UnboundLocalError, not NameError from LOAD_DEREF on a free slot.
447+
def outer(x):
448+
def inner():
449+
return [x for y in x for x in x]
450+
return inner()
451+
452+
with self.assertRaises(UnboundLocalError):
453+
outer([1, 2])
454+
455+
def outer_lambda(x):
456+
def inner():
457+
return [lambda: x for y in x for x in x]
458+
return inner()
459+
460+
with self.assertRaises(UnboundLocalError):
461+
outer_lambda([1, 2])
462+
463+
def test_reuse_def_free_class_avoids_duplicate_f_locals(self):
464+
# Class-local names with DEF_FREE_CLASS are in freevars but scoped
465+
# LOCAL; reuse must still apply so f_locals has a unique 'x'.
466+
def outer():
467+
x = 1
468+
class C:
469+
x = 2
470+
vals = [(
471+
dict(**sys._getframe().f_locals),
472+
len(sys._getframe().f_locals),
473+
list(sys._getframe().f_locals.keys()),
474+
list(sys._getframe().f_locals.values()),
475+
list(sys._getframe().f_locals.items()),
476+
) for x in [3]]
477+
def m():
478+
return x
479+
return C
480+
481+
d, n, ks, vs, it = outer().vals[0]
482+
self.assertEqual(d["x"], 3)
483+
self.assertEqual(n, len(ks))
484+
self.assertEqual(n, len(vs))
485+
self.assertEqual(n, len(it))
486+
self.assertEqual(ks.count("x"), 1)
487+
self.assertEqual(d, dict(zip(ks, vs)))
488+
self.assertEqual(d, dict(it))
489+
362490
def test_free_inner_cell_outer(self):
363491
code = """
364492
g = 2

0 commit comments

Comments
 (0)