diff --git a/Doc/library/dis.rst b/Doc/library/dis.rst index 73e77f4707cf92..6424e4a8295997 100644 --- a/Doc/library/dis.rst +++ b/Doc/library/dis.rst @@ -2003,6 +2003,16 @@ but are replaced by real opcodes or removed before bytecode is generated. .. versionchanged:: 3.13 This opcode is now a pseudo-instruction. +.. opcode:: STORE_CLOSURE (i) + + Stores the TOS into the cell slot ``i`` of the "fast locals" storage. + Used to restore a cell saved by ``LOAD_CLOSURE`` when isolating an + inlined comprehension that reuses an enclosing free variable. + + Note that ``STORE_CLOSURE`` is replaced with ``STORE_FAST`` in the assembler. + + .. versionadded:: next + .. _opcode_collections: diff --git a/Include/internal/pycore_interpframe.h b/Include/internal/pycore_interpframe.h index a85fa9bd32853c..3f049429ddb9a6 100644 --- a/Include/internal/pycore_interpframe.h +++ b/Include/internal/pycore_interpframe.h @@ -376,6 +376,12 @@ _PyFrame_Traverse(_PyInterpreterFrame *frame, visitproc visit, void *arg); bool _PyFrame_HasHiddenLocals(_PyInterpreterFrame *frame); +/* True when localsplus[oparg] is a free cell that currently differs from + * the function's func_closure cell — i.e. an inlined comprehension has + * temporarily replaced it. */ +PyAPI_FUNC(bool) +_PyFrame_IsInlinedCompTempFree(_PyInterpreterFrame *frame, int oparg); + PyObject * _PyFrame_GetLocals(_PyInterpreterFrame *frame); diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index d3546119bc9ee0..b3882bf1fd4f64 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -19,6 +19,7 @@ extern "C" { #define IS_PSEUDO_INSTR(OP) ( \ ((OP) == LOAD_CLOSURE) || \ + ((OP) == STORE_CLOSURE) || \ ((OP) == STORE_FAST_MAYBE_NULL) || \ ((OP) == ANNOTATIONS_PLACEHOLDER) || \ ((OP) == JUMP) || \ @@ -460,6 +461,8 @@ int _PyOpcode_num_popped(int opcode, int oparg) { return 2; case STORE_ATTR_WITH_HINT: return 2; + case STORE_CLOSURE: + return 1; case STORE_DEREF: return 1; case STORE_FAST: @@ -955,6 +958,8 @@ int _PyOpcode_num_pushed(int opcode, int oparg) { return 0; case STORE_ATTR_WITH_HINT: return 0; + case STORE_CLOSURE: + return 0; case STORE_DEREF: return 0; case STORE_FAST: @@ -1037,7 +1042,7 @@ enum InstructionFormat { }; #define IS_VALID_OPCODE(OP) \ - (((OP) >= 0) && ((OP) < 267) && \ + (((OP) >= 0) && ((OP) < 268) && \ (_PyOpcode_opcode_metadata[(OP)].valid_entry)) #define HAS_ARG_FLAG (1) @@ -1097,9 +1102,9 @@ struct opcode_metadata { uint32_t flags; }; -PyAPI_DATA(const struct opcode_metadata) _PyOpcode_opcode_metadata[267]; +PyAPI_DATA(const struct opcode_metadata) _PyOpcode_opcode_metadata[268]; #ifdef NEED_OPCODE_METADATA -const struct opcode_metadata _PyOpcode_opcode_metadata[267] = { +const struct opcode_metadata _PyOpcode_opcode_metadata[268] = { [BINARY_OP] = { true, INSTR_FMT_IBC0000, HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG | HAS_ESCAPES_FLAG | HAS_RECORDS_VALUE_FLAG }, [BINARY_OP_ADD_FLOAT] = { true, INSTR_FMT_IXC0000, HAS_EXIT_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG }, [BINARY_OP_ADD_INT] = { true, INSTR_FMT_IXC0000, HAS_EXIT_FLAG }, @@ -1341,6 +1346,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[267] = { [SETUP_CLEANUP] = { true, -1, HAS_PURE_FLAG | HAS_ARG_FLAG }, [SETUP_FINALLY] = { true, -1, HAS_PURE_FLAG | HAS_ARG_FLAG }, [SETUP_WITH] = { true, -1, HAS_PURE_FLAG | HAS_ARG_FLAG }, + [STORE_CLOSURE] = { true, -1, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ESCAPES_FLAG }, [STORE_FAST_MAYBE_NULL] = { true, -1, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ESCAPES_FLAG }, }; #endif @@ -1549,9 +1555,9 @@ _PyOpcode_macro_expansion[256] = { }; #endif // NEED_OPCODE_METADATA -PyAPI_DATA(const char) *_PyOpcode_OpName[267]; +PyAPI_DATA(const char) *_PyOpcode_OpName[268]; #ifdef NEED_OPCODE_METADATA -const char *_PyOpcode_OpName[267] = { +const char *_PyOpcode_OpName[268] = { [ANNOTATIONS_PLACEHOLDER] = "ANNOTATIONS_PLACEHOLDER", [BINARY_OP] = "BINARY_OP", [BINARY_OP_ADD_FLOAT] = "BINARY_OP_ADD_FLOAT", @@ -1764,6 +1770,7 @@ const char *_PyOpcode_OpName[267] = { [STORE_ATTR_INSTANCE_VALUE] = "STORE_ATTR_INSTANCE_VALUE", [STORE_ATTR_SLOT] = "STORE_ATTR_SLOT", [STORE_ATTR_WITH_HINT] = "STORE_ATTR_WITH_HINT", + [STORE_CLOSURE] = "STORE_CLOSURE", [STORE_DEREF] = "STORE_DEREF", [STORE_FAST] = "STORE_FAST", [STORE_FAST_LOAD_FAST] = "STORE_FAST_LOAD_FAST", @@ -2119,10 +2126,11 @@ struct pseudo_targets { uint8_t as_sequence; uint8_t targets[4]; }; -extern const struct pseudo_targets _PyOpcode_PseudoTargets[11]; +extern const struct pseudo_targets _PyOpcode_PseudoTargets[12]; #ifdef NEED_OPCODE_METADATA -const struct pseudo_targets _PyOpcode_PseudoTargets[11] = { +const struct pseudo_targets _PyOpcode_PseudoTargets[12] = { [LOAD_CLOSURE-256] = { 0, { LOAD_FAST, 0, 0, 0 } }, + [STORE_CLOSURE-256] = { 0, { STORE_FAST, 0, 0, 0 } }, [STORE_FAST_MAYBE_NULL-256] = { 0, { STORE_FAST, 0, 0, 0 } }, [ANNOTATIONS_PLACEHOLDER-256] = { 0, { NOP, 0, 0, 0 } }, [JUMP-256] = { 0, { JUMP_FORWARD, JUMP_BACKWARD, 0, 0 } }, @@ -2138,7 +2146,7 @@ const struct pseudo_targets _PyOpcode_PseudoTargets[11] = { #endif // NEED_OPCODE_METADATA static inline bool is_pseudo_target(int pseudo, int target) { - if (pseudo < 256 || pseudo >= 267) { + if (pseudo < 256 || pseudo >= 268) { return false; } for (int i = 0; _PyOpcode_PseudoTargets[pseudo-256].targets[i]; i++) { diff --git a/Include/opcode_ids.h b/Include/opcode_ids.h index 11342ae451b9f6..b7df17eb74490f 100644 --- a/Include/opcode_ids.h +++ b/Include/opcode_ids.h @@ -251,7 +251,8 @@ extern "C" { #define SETUP_CLEANUP 263 #define SETUP_FINALLY 264 #define SETUP_WITH 265 -#define STORE_FAST_MAYBE_NULL 266 +#define STORE_CLOSURE 266 +#define STORE_FAST_MAYBE_NULL 267 #define HAVE_ARGUMENT 41 #define MIN_SPECIALIZED_OPCODE 129 diff --git a/InternalDocs/inlined_comprehensions.md b/InternalDocs/inlined_comprehensions.md index e1ccd485c165b1..3a285db71270ee 100644 --- a/InternalDocs/inlined_comprehensions.md +++ b/InternalDocs/inlined_comprehensions.md @@ -86,15 +86,35 @@ The walk stops at a class: nested scopes do not see class locals. Class-closure names that would otherwise be free through a class become `GLOBAL_IMPLICIT`. +If the inlined name is `LOCAL` or `CELL` but the nearest non-inlined +enclosing table has it as `FREE` (or `DEF_FREE_CLASS`), resolve it as +`FREE` so the comprehension reuses that localsplus slot. +`compiler_cellvars()` also skips adding those child cells, which would +otherwise create a second same-named entry. Class-closure names +(`__class__` and friends) are never reused: zero-arg `super()` and +class-cell bookkeeping need the real free cell. + ### Isolating iteration variables `codegen_push_inlined_comprehension_locals()` in -[`Python/codegen.c`](../Python/codegen.c) isolates names bound in the -comprehension: +[`Python/codegen.c`](../Python/codegen.c) isolates each name bound in +the comprehension on one of two paths: + +Reuse an enclosing free (`_PyCompile_GetRefType()` is `FREE`): + +* `LOAD_CLOSURE` saves the enclosing cell without clearing the slot + (so free-slot introspection never sees a NULL). +* `MAKE_CELL` on a `CO_FAST_FREE` slot always installs a fresh empty + cell, replacing the saved one. The comprehension then uses `DEREF`. +* Restore uses `STORE_CLOSURE` (a cell/free-index pseudo that becomes + `STORE_FAST` after `fix_cell_offsets`). -* `LOAD_FAST_AND_CLEAR` saves the enclosing value (possibly `NULL`) and - clears the slot. -* `MAKE_CELL` runs if the name is a cell for this comprehension. +Own fast-local slot (everything else): + +* `LOAD_FAST_AND_CLEAR` saves the enclosing value (possibly `NULL`) + and clears the slot. +* `MAKE_CELL` runs only if the name is a cell for this comprehension. +* Restore uses `STORE_FAST_MAYBE_NULL`. * In module and class units the name is added to `u_fasthidden` so assemble can set `CO_FAST_HIDDEN`. @@ -105,10 +125,19 @@ or `finally` sees the original values. Runtime ------- -An inlined comprehension cell can share a localsplus name with an -enclosing free variable (for example `[lambda: x for x in x]` inside a -nested function). `FrameLocalsProxy` keys, values, items, and `len` -keep the first slot of each name so they agree with `getitem`. +An inlined comprehension local that collides with an enclosing free +(for example `[x for x in x]` or `[lambda: x for x in x]` inside a +nested function) reuses the free slot. Isolation saves that cell and +installs a temporary one so `STORE_DEREF` does not change the value +seen by existing closures; lambdas that capture the iteration variable +share the temporary cell. After the comprehension, the original cell +is restored. + +While the temporary cell is installed, `_PyFrame_IsInlinedCompTempFree()` +is true (frame cell differs from `func_closure`). That drives +class/module `locals()` to use `FrameLocalsProxy` even without a +`CO_FAST_HIDDEN` slot, and makes an empty temporary cell raise +`UnboundLocalError` rather than `NameError`. Source ------ @@ -128,5 +157,3 @@ Source `InlinedComprehensionBlock` * [`Include/internal/pycore_compile.h`](../Include/internal/pycore_compile.h): `_PyCompile_InlinedComprehensionState` -* [`Objects/frameobject.c`](../Objects/frameobject.c): - `FrameLocalsProxy` duplicate-name handling diff --git a/Lib/_opcode_metadata.py b/Lib/_opcode_metadata.py index df92eae151d248..8de01b19fbcaaf 100644 --- a/Lib/_opcode_metadata.py +++ b/Lib/_opcode_metadata.py @@ -377,7 +377,8 @@ SETUP_CLEANUP=263, SETUP_FINALLY=264, SETUP_WITH=265, - STORE_FAST_MAYBE_NULL=266, + STORE_CLOSURE=266, + STORE_FAST_MAYBE_NULL=267, ) HAVE_ARGUMENT = 41 diff --git a/Lib/test/test_listcomps.py b/Lib/test/test_listcomps.py index f02f3223a513d2..a2f8ba4d40fe79 100644 --- a/Lib/test/test_listcomps.py +++ b/Lib/test/test_listcomps.py @@ -1,11 +1,12 @@ import doctest +import sys import textwrap import traceback import types import unittest from test import support -from test.support import BrokenIter +from test.support import BrokenIter, import_helper doctests = """ @@ -316,6 +317,205 @@ def inner(): outputs = {"z": [2, 2], "w": 99} self._check_in_scopes(code, outputs) + def test_inlined_comp_reuses_enclosing_free_slot(self): + # An inlined local that collides with an enclosing free reuses that + # free slot instead of adding a second same-named localsplus entry. + def outer(x): + def inner(): + return [x for x in x] + return inner + code = outer([1]).__code__ + self.assertEqual(code.co_varnames, ()) + self.assertEqual(code.co_cellvars, ()) + self.assertEqual(code.co_freevars, ('x',)) + + def test_inlined_comp_cell_reuses_enclosing_free_slot(self): + def outer(x): + def inner(): + return [lambda: x for x in x] + return inner + code = outer([1]).__code__ + self.assertEqual(code.co_varnames, ()) + self.assertEqual(code.co_cellvars, ()) + self.assertEqual(code.co_freevars, ('x',)) + + def test_nested_inlined_comp_reuses_enclosing_free_slot(self): + def outer(x): + def inner(): + return [[x for _ in (0,)] for x in x] + return inner + code = outer([1]).__code__ + self.assertNotIn('x', code.co_varnames) + self.assertNotIn('x', code.co_cellvars) + self.assertEqual(code.co_freevars, ('x',)) + + def test_inlined_comp_exception_restores_enclosing_free(self): + def outer(x): + def inner(): + try: + [1 / 0 for x in x] + except ZeroDivisionError: + pass + return x + return inner() + self.assertEqual(outer([1, 2]), [1, 2]) + + def test_reuse_class_closure_name_as_param_with_lambda(self): + # Class-closure names are not reused. Lambdas share the last + # iteration value; the enclosing binding is unchanged. + def outer(__class__): + class C: + result = [lambda: __class__ for __class__ in __class__] + return [f() for f in C.result], __class__ + self.assertEqual(outer([1, 2]), ([2, 2], [1, 2])) + + def outer(__classdict__): + class C: + result = [lambda: __classdict__ for __classdict__ in __classdict__] + return [f() for f in C.result], __classdict__ + self.assertEqual(outer([1, 2]), ([2, 2], [1, 2])) + + def test_reuse_class_closure_name_as_param_preserves_enclosing(self): + def outer(__class__): + class C: + result = [__class__ for __class__ in __class__] + return C.result, __class__ + self.assertEqual(outer([1, 2]), ([1, 2], [1, 2])) + + def outer(__classdict__): + class C: + result = [__classdict__ for __classdict__ in __classdict__] + return C.result, __classdict__ + self.assertEqual(outer([1, 2]), ([1, 2], [1, 2])) + + def test_reuse_free_slot_visible_to_opcode_trace_getvar(self): + # Isolation must not leave a free slot temporarily NULL where + # PyFrame_GetVar() (and similar) assume a cell is always present. + _testcapi = import_helper.import_module("_testcapi") + + def outer(x): + def inner(): + return [x for x in x] + return inner + + f = outer([1]) + + def trace(frame, event, arg): + if frame.f_code is f.__code__: + frame.f_trace_opcodes = True + if event == "opcode": + try: + _testcapi.frame_getvar(frame, "x") + except NameError: + pass + return trace + + sys.settrace(trace) + try: + self.assertEqual(f(), [1]) + finally: + sys.settrace(None) + + def test_reuse_does_not_break_zero_arg_super(self): + # Temporary reuse of the __class__ free must not change what + # zero-argument super() observes during the comprehension. + class C: + def method(self): + __class__ + return [super() for __class__ in (int,)] + + self.assertIs(C().method()[0].__thisclass__, C) + + def test_class_closure_name_not_duplicated_in_f_locals(self): + # Class-closure names are not slot-reused, so localsplus can hold + # both a hidden comprehension local and the free. FrameLocalsProxy + # must still present a unique key (first wins). + class C: + def method(self): + __class__ + return [( + dict(**sys._getframe().f_locals), + len(sys._getframe().f_locals), + list(sys._getframe().f_locals.keys()), + list(sys._getframe().f_locals.values()), + list(sys._getframe().f_locals.items()), + ) for __class__ in (int,)] + + d, n, ks, vs, it = C().method()[0] + self.assertEqual(d["__class__"], int) + self.assertEqual(ks.count("__class__"), 1) + self.assertEqual(n, len(ks)) + self.assertEqual(n, len(vs)) + self.assertEqual(n, len(it)) + self.assertEqual(d, dict(zip(ks, vs))) + self.assertEqual(d, dict(it)) + # Duplicate slots are still present in the code object. + code = C.method.__code__ + self.assertEqual(code.co_varnames.count("__class__") + + code.co_cellvars.count("__class__") + + code.co_freevars.count("__class__"), 2) + + def test_reuse_class_body_locals_sees_iteration_var(self): + # Class-body locals() must still expose the comprehension target + # when that name reuses an enclosing free (no separate hidden slot). + def outer(x): + class C: + values = [locals()["x"] for x in x] + return C.values + self.assertEqual(outer([1, 2]), [1, 2]) + + def outer_eval(x): + class C: + values = [eval("x") for x in x] + return C.values + self.assertEqual(outer_eval([1, 2]), [1, 2]) + + def test_reuse_uninitialized_comp_local_is_unbound_local(self): + # Reading the comprehension target before it is assigned must remain + # UnboundLocalError, not NameError from LOAD_DEREF on a free slot. + def outer(x): + def inner(): + return [x for y in x for x in x] + return inner() + + with self.assertRaises(UnboundLocalError): + outer([1, 2]) + + def outer_lambda(x): + def inner(): + return [lambda: x for y in x for x in x] + return inner() + + with self.assertRaises(UnboundLocalError): + outer_lambda([1, 2]) + + def test_reuse_def_free_class_avoids_duplicate_f_locals(self): + # Class-local names with DEF_FREE_CLASS are in freevars but scoped + # LOCAL; reuse must still apply so f_locals has a unique 'x'. + def outer(): + x = 1 + class C: + x = 2 + vals = [( + dict(**sys._getframe().f_locals), + len(sys._getframe().f_locals), + list(sys._getframe().f_locals.keys()), + list(sys._getframe().f_locals.values()), + list(sys._getframe().f_locals.items()), + ) for x in [3]] + def m(): + return x + return C + + d, n, ks, vs, it = outer().vals[0] + self.assertEqual(d["x"], 3) + self.assertEqual(n, len(ks)) + self.assertEqual(n, len(vs)) + self.assertEqual(n, len(it)) + self.assertEqual(ks.count("x"), 1) + self.assertEqual(d, dict(zip(ks, vs))) + self.assertEqual(d, dict(it)) + def test_free_inner_cell_outer(self): code = """ g = 2 @@ -944,6 +1144,36 @@ def inner(): {"snaps": [1, 2], "vals": [2, 2], "consistent": [True, True]}, ns={"sys": sys}, scopes=["module", "function"]) + def test_frame_locals_comp_local_and_enclosing_free(self): + # Same-name collision without a lambda: the inlined local reuses the + # enclosing free slot. f_locals keys must still be unique. + code = """ + def outer(x): + def inner(): + return [(dict(**sys._getframe().f_locals), + len(sys._getframe().f_locals), + list(sys._getframe().f_locals.keys()), + list(sys._getframe().f_locals.values()), + list(sys._getframe().f_locals.items()), + dict(sys._getframe().f_locals.items())) + for x in x] + return inner() + result = outer([1, 2]) + snaps = [d['x'] for d, *_ in result] + consistent = [] + for d, n, ks, vs, it, d_items in result: + consistent.append( + n == len(ks) == len(vs) == len(it) + and ks.count('x') == 1 + and d == d_items == dict(zip(ks, vs)) + ) + """ + import sys + self._check_in_scopes( + code, + {"snaps": [1, 2], "consistent": [True, True]}, + ns={"sys": sys}, scopes=["module", "function"]) + def test_frame_locals_nested_comp_cell_and_enclosing_free(self): # Stress a nested inlined shape where a comp cell and enclosing free # share a name; all f_locals views must stay consistent. diff --git a/Modules/_testinternalcapi/test_cases.c.h b/Modules/_testinternalcapi/test_cases.c.h index 3bdc16437e2bc6..a5a537b8e4cfed 100644 --- a/Modules/_testinternalcapi/test_cases.c.h +++ b/Modules/_testinternalcapi/test_cases.c.h @@ -9744,12 +9744,21 @@ value = _PyCell_GetStackRef(cell); _PyFrame_StackPointerInvalidate(frame); if (PyStackRef_IsNull(value)) { + PyCodeObject *co = _PyFrame_GetCode(frame); stack_pointer[0] = value; stack_pointer += 1; ASSERT_WITHIN_STACK_BOUNDS(__FILE__, __LINE__); _PyFrame_SetStackPointer(frame, stack_pointer); _PyFrame_StackPointerValidate(frame); - _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); + int unbound_local = (oparg < PyUnstable_Code_GetFirstFree(co) || + _PyFrame_IsInlinedCompTempFree(frame, oparg)); + _PyFrame_StackPointerInvalidate(frame); + assert(stack_pointer == _PyFrame_GetStackPointer(frame)); + _PyFrame_StackPointerValidate(frame); + _PyEval_FormatExcCheckArg(tstate, + unbound_local ? PyExc_UnboundLocalError : PyExc_NameError, + unbound_local ? UNBOUNDLOCAL_ERROR_MSG : UNBOUNDFREE_ERROR_MSG, + PyTuple_GET_ITEM(co->co_localsplusnames, oparg)); _PyFrame_StackPointerInvalidate(frame); JUMP_TO_LABEL(error); } @@ -10652,7 +10661,11 @@ frame->instr_ptr = next_instr; next_instr += 1; INSTRUCTION_STATS(MAKE_CELL); - PyObject *initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + PyObject *initial = NULL; + PyCodeObject *co = _PyFrame_GetCode(frame); + if (!(_PyLocals_GetKind(co->co_localspluskinds, oparg) & CO_FAST_FREE)) { + initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + } PyObject *cell = PyCell_New(initial); if (cell == NULL) { JUMP_TO_LABEL(error); diff --git a/Objects/frameobject.c b/Objects/frameobject.c index a4cc14a6eaad45..93a1116a2f5f0b 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -14,6 +14,7 @@ #include "pycore_object.h" // _PyObject_GC_UNTRACK() #include "pycore_opcode_metadata.h" // _PyOpcode_Caches #include "pycore_optimizer.h" // _Py_Executors_InvalidateDependency() +#include "pycore_symtable.h" // _PyST_IsClassClosureName() #include "pycore_tuple.h" // _PyTuple_FromPair #include "pycore_unicodeobject.h" // _PyUnicode_Equal() #include "pycore_weakref.h" // FT_CLEAR_WEAKREFS() @@ -94,9 +95,15 @@ framelocalsproxy_hasval(_PyInterpreterFrame *frame, PyCodeObject *co, int i) return true; } +/* 1 = include, 0 = skip duplicate, -1 = error. + * Class-closure names are not slot-reused, so localsplus can hold both a + * comprehension local and a free; track only those names in seen. */ static int -framelocalsproxy_is_first_occurrence(PyObject *seen, PyObject *name) +framelocalsproxy_include_name(PyObject *seen, PyObject *name) { + if (!_PyST_IsClassClosureName(name)) { + return 1; + } int found = PySet_Contains(seen, name); if (found < 0) { return -1; @@ -396,7 +403,6 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) if (names == NULL) { return NULL; } - // An inlined comprehension cell can share a name with a free var. PyObject *seen = PySet_New(NULL); if (seen == NULL) { Py_DECREF(names); @@ -406,18 +412,18 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) 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); - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first < 0) { + int include = framelocalsproxy_include_name(seen, name); + if (include < 0) { goto error; } - if (first) { - if (PyList_Append(names, name) < 0) { - goto error; - } + if (!include) { + continue; + } + if (PyList_Append(names, name) < 0) { + goto error; } } } - Py_DECREF(seen); // Iterate through the extra locals if (frame->f_extra_locals) { @@ -429,12 +435,12 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) while (PyDict_Next(frame->f_extra_locals, &i, &key, &value)) { if (PyList_Append(names, key) < 0) { - Py_DECREF(names); - return NULL; + goto error; } } } + Py_DECREF(seen); return names; error: @@ -443,6 +449,7 @@ framelocalsproxy_keys(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } + static void framelocalsproxy_dealloc(PyObject *self) { @@ -632,20 +639,22 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); if (value) { PyObject *name = PyTuple_GET_ITEM(co->co_localsplusnames, i); - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first == 1) { - if (PyList_Append(values, value) < 0) { - Py_DECREF(value); - goto error; - } + int include = framelocalsproxy_include_name(seen, name); + if (include < 0) { + Py_DECREF(value); + goto error; } - Py_DECREF(value); - if (first < 0) { + if (!include) { + Py_DECREF(value); + continue; + } + if (PyList_Append(values, value) < 0) { + Py_DECREF(value); goto error; } + Py_DECREF(value); } } - Py_DECREF(seen); // Iterate through the extra locals if (frame->f_extra_locals) { @@ -654,12 +663,12 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) PyObject *value = NULL; while (PyDict_Next(frame->f_extra_locals, &j, &key, &value)) { if (PyList_Append(values, value) < 0) { - Py_DECREF(values); - return NULL; + goto error; } } } + Py_DECREF(seen); return values; error: @@ -668,6 +677,7 @@ framelocalsproxy_values(PyObject *self, PyObject *Py_UNUSED(ignored)) return NULL; } + static PyObject * framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) { @@ -688,26 +698,24 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) PyObject *value = framelocalsproxy_getval(frame->f_frame, co, i); if (value) { - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first == 1) { - PyObject *pair = _PyTuple_FromPairSteal(Py_NewRef(name), value); - if (pair == NULL) { - goto error; - } - if (_PyList_AppendTakeRef((PyListObject *)items, pair) < 0) { - goto error; - } + int include = framelocalsproxy_include_name(seen, name); + if (include < 0) { + Py_DECREF(value); + goto error; } - else { + if (!include) { Py_DECREF(value); - if (first < 0) { - goto error; - } + continue; + } + PyObject *pair = _PyTuple_FromPairSteal(Py_NewRef(name), value); + if (pair == NULL) { + goto error; + } + if (_PyList_AppendTakeRef((PyListObject *)items, pair) < 0) { + goto error; } } } - Py_DECREF(seen); - seen = NULL; // Iterate through the extra locals if (frame->f_extra_locals) { @@ -726,14 +734,16 @@ framelocalsproxy_items(PyObject *self, PyObject *Py_UNUSED(ignored)) } } + Py_DECREF(seen); return items; error: - Py_XDECREF(seen); + Py_DECREF(seen); Py_DECREF(items); return NULL; } + static Py_ssize_t framelocalsproxy_length(PyObject *self) { @@ -753,12 +763,12 @@ framelocalsproxy_length(PyObject *self) 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); - int first = framelocalsproxy_is_first_occurrence(seen, name); - if (first < 0) { + int include = framelocalsproxy_include_name(seen, name); + if (include < 0) { Py_DECREF(seen); return -1; } - else if (first) { + if (include) { size++; } } @@ -767,6 +777,7 @@ framelocalsproxy_length(PyObject *self) return size; } + static int framelocalsproxy_contains(PyObject *self, PyObject *key) { @@ -2326,6 +2337,35 @@ frame_get_var(_PyInterpreterFrame *frame, PyCodeObject *co, int i, } +bool +_PyFrame_IsInlinedCompTempFree(_PyInterpreterFrame *frame, int oparg) +{ + PyCodeObject *co = _PyFrame_GetCode(frame); + if (oparg < 0 || oparg >= co->co_nlocalsplus) { + return false; + } + if (!(_PyLocals_GetKind(co->co_localspluskinds, oparg) & CO_FAST_FREE)) { + return false; + } + if (!PyStackRef_FunctionCheck(frame->f_funcobj)) { + return false; + } + PyFunctionObject *func = + (PyFunctionObject *)PyStackRef_AsPyObjectBorrow(frame->f_funcobj); + PyObject *closure = func->func_closure; + if (closure == NULL) { + return false; + } + int free_index = oparg - (co->co_nlocalsplus - co->co_nfreevars); + if (free_index < 0 || free_index >= co->co_nfreevars) { + return false; + } + assert(free_index < PyTuple_GET_SIZE(closure)); + PyObject *closure_cell = PyTuple_GET_ITEM(closure, free_index); + PyObject *frame_cell = PyStackRef_AsPyObjectBorrow(frame->localsplus[oparg]); + return frame_cell != NULL && frame_cell != closure_cell; +} + bool _PyFrame_HasHiddenLocals(_PyInterpreterFrame *frame) { @@ -2343,6 +2383,14 @@ _PyFrame_HasHiddenLocals(_PyInterpreterFrame *frame) return true; } } + else if (kind & CO_FAST_FREE) { + /* A free slot whose cell was swapped for an inlined + * comprehension temporary must also force FrameLocalsProxy + * in class/module scopes (no separate HIDDEN slot). */ + if (_PyFrame_IsInlinedCompTempFree(frame, i)) { + return true; + } + } } return false; diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 31eaeab0d67841..5dcecc0d348ea8 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -270,6 +270,13 @@ dummy_func( LOAD_FAST, }; + /* Like STORE_FAST, but the oparg is a cell/free index remapped + * in fix_cell_offsets. Restores the cell saved by LOAD_CLOSURE + * when isolating an inlined comprehension that reuses a free. */ + pseudo(STORE_CLOSURE, (unused --)) = { + STORE_FAST, + }; + inst(LOAD_FAST_CHECK, (-- value)) { _PyStackRef value_s = GETLOCAL(oparg); if (PyStackRef_IsNull(value_s)) { @@ -2339,7 +2346,14 @@ dummy_func( inst(MAKE_CELL, (--)) { // "initial" is probably NULL but not if it's an arg (or set // via the f_locals proxy before MAKE_CELL has run). - PyObject *initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + // For CO_FAST_FREE, always start empty: used to isolate an + // inlined comprehension that reuses a free slot after + // LOAD_CLOSURE saved the enclosing cell on the stack. + PyObject *initial = NULL; + PyCodeObject *co = _PyFrame_GetCode(frame); + if (!(_PyLocals_GetKind(co->co_localspluskinds, oparg) & CO_FAST_FREE)) { + initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + } PyObject *cell = PyCell_New(initial); if (cell == NULL) { ERROR_NO_POP(); @@ -2396,7 +2410,15 @@ dummy_func( PyCellObject *cell = (PyCellObject *)PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); value = _PyCell_GetStackRef(cell); if (PyStackRef_IsNull(value)) { - _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); + /* Same choice as _PyEval_FormatExcUnbound, plus: a free slot + * reused as an inlined-comprehension temporary is unbound local. */ + PyCodeObject *co = _PyFrame_GetCode(frame); + int unbound_local = (oparg < PyUnstable_Code_GetFirstFree(co) || + _PyFrame_IsInlinedCompTempFree(frame, oparg)); + _PyEval_FormatExcCheckArg(tstate, + unbound_local ? PyExc_UnboundLocalError : PyExc_NameError, + unbound_local ? UNBOUNDLOCAL_ERROR_MSG : UNBOUNDFREE_ERROR_MSG, + PyTuple_GET_ITEM(co->co_localsplusnames, oparg)); ERROR_IF(true); } } diff --git a/Python/codegen.c b/Python/codegen.c index 7143d9abc4255e..84e8d3c5fded8a 100644 --- a/Python/codegen.c +++ b/Python/codegen.c @@ -4915,22 +4915,34 @@ codegen_push_inlined_comprehension_locals(compiler *c, location loc, return ERROR; } } - // in the case of a cell, this will actually push the cell - // itself to the stack, then we'll create a new one for the - // comprehension and restore the original one after - ADDOP_NAME(c, loc, LOAD_FAST_AND_CLEAR, k, varnames); - if (scope == CELL) { - ADDOP_NAME(c, loc, MAKE_CELL, k, cellvars); + int reftype = _PyCompile_GetRefType(c, k); + RETURN_IF_ERROR(reftype); + if (reftype == FREE) { + // Reuse the enclosing free slot. Save that cell without + // clearing (avoids a NULL free slot), then MAKE_CELL + // replaces it with a fresh empty cell (see MAKE_CELL on + // CO_FAST_FREE). + ADDOP_NAME(c, loc, LOAD_CLOSURE, k, freevars); + ADDOP_NAME(c, loc, MAKE_CELL, k, freevars); + } + else { + // in the case of a cell, this will actually push the cell + // itself to the stack, then we'll create a new one for the + // comprehension and restore the original one after + ADDOP_NAME(c, loc, LOAD_FAST_AND_CLEAR, k, varnames); + if (scope == CELL) { + ADDOP_NAME(c, loc, MAKE_CELL, k, cellvars); + } + if (METADATA(c)->u_fasthidden != NULL) { + /* For Module/Class scopes, assemble needs to set CO_FAST_HIDDEN on these names */ + if (PySet_Add(METADATA(c)->u_fasthidden, k) < 0) { + return ERROR; + } + } } if (PyList_Append(state->pushed_locals, k) < 0) { return ERROR; } - if (METADATA(c)->u_fasthidden != NULL) { - /* For Module/Class scopes, assemble needs to set CO_FAST_HIDDEN on these names */ - if (PySet_Add(METADATA(c)->u_fasthidden, k) < 0) { - return ERROR; - } - } } } if (state->pushed_locals) { @@ -4987,7 +4999,14 @@ restore_inlined_comprehension_locals(compiler *c, location loc, if (k == NULL) { return ERROR; } - ADDOP_NAME(c, loc, STORE_FAST_MAYBE_NULL, k, varnames); + int reftype = _PyCompile_GetRefType(c, k); + RETURN_IF_ERROR(reftype); + if (reftype == FREE) { + ADDOP_NAME(c, loc, STORE_CLOSURE, k, freevars); + } + else { + ADDOP_NAME(c, loc, STORE_FAST_MAYBE_NULL, k, varnames); + } } return SUCCESS; } diff --git a/Python/compile.c b/Python/compile.c index ee29f7a9a5d589..cc29cdd094bd1f 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -596,7 +596,11 @@ dictbytype(PyObject *src, int scope_type, int flag, Py_ssize_t offset) } static int -add_cell_names_from_symbols(PyObject *symbols, PyObject *names) +compiler_should_reuse_enclosing_free(PySTEntryObject *enclosing, PyObject *name); + +static int +add_cell_names_from_symbols(PyObject *symbols, PyObject *names, + PySTEntryObject *skip_if_free) { Py_ssize_t pos = 0; PyObject *k, *v; @@ -605,17 +609,28 @@ add_cell_names_from_symbols(PyObject *symbols, PyObject *names) if (flags == -1 && PyErr_Occurred()) { return ERROR; } - if (SYMBOL_TO_SCOPE(flags) == CELL) { - if (PySet_Add(names, k) < 0) { - return ERROR; + if (SYMBOL_TO_SCOPE(flags) != CELL) { + continue; + } + /* Inlined cells that collide with an enclosing free reuse that + * slot instead of adding a second same-named localsplus entry. */ + if (skip_if_free != NULL) { + int reuse = compiler_should_reuse_enclosing_free(skip_if_free, k); + RETURN_IF_ERROR(reuse); + if (reuse) { + continue; } } + if (PySet_Add(names, k) < 0) { + return ERROR; + } } return SUCCESS; } static int -add_inlined_comprehension_cell_names(PySTEntryObject *ste, PyObject *names) +add_inlined_comprehension_cell_names(PySTEntryObject *unit, + PySTEntryObject *ste, PyObject *names) { for (Py_ssize_t i = 0; i < PyList_GET_SIZE(ste->ste_children); i++) { PySTEntryObject *child = @@ -623,10 +638,10 @@ add_inlined_comprehension_cell_names(PySTEntryObject *ste, PyObject *names) if (child->ste_type != InlinedComprehensionBlock) { continue; } - if (add_cell_names_from_symbols(child->ste_symbols, names) < 0) { + if (add_cell_names_from_symbols(child->ste_symbols, names, unit) < 0) { return ERROR; } - if (add_inlined_comprehension_cell_names(child, names) < 0) { + if (add_inlined_comprehension_cell_names(unit, child, names) < 0) { return ERROR; } } @@ -642,11 +657,11 @@ compiler_cellvars(PySTEntryObject *ste) if (names == NULL) { return NULL; } - if (add_cell_names_from_symbols(ste->ste_symbols, names) < 0) { + if (add_cell_names_from_symbols(ste->ste_symbols, names, NULL) < 0) { Py_DECREF(names); return NULL; } - if (add_inlined_comprehension_cell_names(ste, names) < 0) { + if (add_inlined_comprehension_cell_names(ste, ste, names) < 0) { Py_DECREF(names); return NULL; } @@ -979,6 +994,39 @@ compiler_mod(compiler *c, mod_ty mod) return co; } +static PySTEntryObject * +enclosing_non_inlined_ste(PySTEntryObject *ste) +{ + while (ste != NULL && ste->ste_type == InlinedComprehensionBlock) { + ste = ste->ste_parent; + } + return ste; +} + +/* True if an inlined LOCAL/CELL should reuse enclosing's freevars slot. + * Class-closure names are excluded: zero-arg super() and class-cell + * bookkeeping depend on the real free cell. DEF_FREE_CLASS counts as a + * free slot even though _PyST_GetScope() returns LOCAL. */ +static int +compiler_should_reuse_enclosing_free(PySTEntryObject *enclosing, PyObject *name) +{ + if (_PyST_IsClassClosureName(name)) { + return 0; + } + PyObject *v = PyDict_GetItemWithError(enclosing->ste_symbols, name); + if (v == NULL) { + return PyErr_Occurred() ? ERROR : 0; + } + long flags = PyLong_AsLong(v); + if (flags == -1 && PyErr_Occurred()) { + return ERROR; + } + if (SYMBOL_TO_SCOPE(flags) == FREE || (flags & DEF_FREE_CLASS)) { + return 1; + } + return 0; +} + /* Inlined comprehensions are compiled in the enclosing unit. If a name is * FREE in the comprehension, or is absent from its table (scope 0), resolve * it in enclosing tables until it is bound. Stop if the next table is a class: @@ -986,6 +1034,10 @@ compiler_mod(compiler *c, mod_ty mod) * the name stays FREE. __class__ and friends are not allowed to be free * through a class; treat those loads as implicit globals. * + * A LOCAL or CELL on the inlined table that collides with an enclosing free + * (or DEF_FREE_CLASS) reuses that free slot, so the compilation unit does not + * grow a second same-named localsplus entry. + * * Names with no entry (scope 0) include loads synthesized by codegen, such as * the implicit receiver for zero-arg super(). */ static int @@ -1007,6 +1059,22 @@ compiler_resolve_inlined_free(PySTEntryObject **ste, PyObject *name) scope = _PyST_GetScope(*ste, name); RETURN_IF_ERROR(scope); } + /* After the walk we may be on an inlined LOCAL/CELL that collides + * with an enclosing free. Reuse that free slot (including when a + * nested inlined load walked here). */ + if ((*ste)->ste_type == InlinedComprehensionBlock && + (scope == LOCAL || scope == CELL)) + { + PySTEntryObject *enclosing = enclosing_non_inlined_ste(*ste); + if (enclosing != NULL) { + int reuse = compiler_should_reuse_enclosing_free(enclosing, name); + RETURN_IF_ERROR(reuse); + if (reuse) { + *ste = enclosing; + return FREE; + } + } + } return scope; } diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index c5b2dfcf5f618f..7fff8697ff0e9f 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -10608,7 +10608,11 @@ CHECK_CURRENT_CACHED_VALUES(0); ASSERT_WITHIN_STACK_BOUNDS_IGNORING_CACHE(__FILE__, __LINE__); oparg = CURRENT_OPARG(); - PyObject *initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + PyObject *initial = NULL; + PyCodeObject *co = _PyFrame_GetCode(frame); + if (!(_PyLocals_GetKind(co->co_localspluskinds, oparg) & CO_FAST_FREE)) { + initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + } PyObject *cell = PyCell_New(initial); if (cell == NULL) { SET_CURRENT_CACHED_VALUES(0); @@ -10728,12 +10732,21 @@ value = _PyCell_GetStackRef(cell); _PyFrame_StackPointerInvalidate(frame); if (PyStackRef_IsNull(value)) { + PyCodeObject *co = _PyFrame_GetCode(frame); stack_pointer[0] = value; stack_pointer += 1; ASSERT_WITHIN_STACK_BOUNDS(__FILE__, __LINE__); _PyFrame_SetStackPointer(frame, stack_pointer); _PyFrame_StackPointerValidate(frame); - _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); + int unbound_local = (oparg < PyUnstable_Code_GetFirstFree(co) || + _PyFrame_IsInlinedCompTempFree(frame, oparg)); + _PyFrame_StackPointerInvalidate(frame); + assert(stack_pointer == _PyFrame_GetStackPointer(frame)); + _PyFrame_StackPointerValidate(frame); + _PyEval_FormatExcCheckArg(tstate, + unbound_local ? PyExc_UnboundLocalError : PyExc_NameError, + unbound_local ? UNBOUNDLOCAL_ERROR_MSG : UNBOUNDFREE_ERROR_MSG, + PyTuple_GET_ITEM(co->co_localsplusnames, oparg)); _PyFrame_StackPointerInvalidate(frame); SET_CURRENT_CACHED_VALUES(0); JUMP_TO_ERROR(); diff --git a/Python/flowgraph.c b/Python/flowgraph.c index a5138d1a1fa284..e8d8a13055dd53 100644 --- a/Python/flowgraph.c +++ b/Python/flowgraph.c @@ -3654,6 +3654,10 @@ convert_pseudo_ops(cfg_builder *g) assert(is_pseudo_target(LOAD_CLOSURE, LOAD_FAST)); instr->i_opcode = LOAD_FAST; } + else if (instr->i_opcode == STORE_CLOSURE) { + assert(is_pseudo_target(STORE_CLOSURE, STORE_FAST)); + instr->i_opcode = STORE_FAST; + } else if (instr->i_opcode == STORE_FAST_MAYBE_NULL) { assert(is_pseudo_target(STORE_FAST_MAYBE_NULL, STORE_FAST)); instr->i_opcode = STORE_FAST; @@ -3975,6 +3979,7 @@ fix_cell_offsets(_PyCompile_CodeUnitMetadata *umd, basicblock *entryblock, int * switch(inst->i_opcode) { case MAKE_CELL: case LOAD_CLOSURE: + case STORE_CLOSURE: case LOAD_DEREF: case STORE_DEREF: case DELETE_DEREF: diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index dd0ce41e4b06b4..eac9a96cc3fece 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -9742,12 +9742,21 @@ value = _PyCell_GetStackRef(cell); _PyFrame_StackPointerInvalidate(frame); if (PyStackRef_IsNull(value)) { + PyCodeObject *co = _PyFrame_GetCode(frame); stack_pointer[0] = value; stack_pointer += 1; ASSERT_WITHIN_STACK_BOUNDS(__FILE__, __LINE__); _PyFrame_SetStackPointer(frame, stack_pointer); _PyFrame_StackPointerValidate(frame); - _PyEval_FormatExcUnbound(tstate, _PyFrame_GetCode(frame), oparg); + int unbound_local = (oparg < PyUnstable_Code_GetFirstFree(co) || + _PyFrame_IsInlinedCompTempFree(frame, oparg)); + _PyFrame_StackPointerInvalidate(frame); + assert(stack_pointer == _PyFrame_GetStackPointer(frame)); + _PyFrame_StackPointerValidate(frame); + _PyEval_FormatExcCheckArg(tstate, + unbound_local ? PyExc_UnboundLocalError : PyExc_NameError, + unbound_local ? UNBOUNDLOCAL_ERROR_MSG : UNBOUNDFREE_ERROR_MSG, + PyTuple_GET_ITEM(co->co_localsplusnames, oparg)); _PyFrame_StackPointerInvalidate(frame); JUMP_TO_LABEL(error); } @@ -10650,7 +10659,11 @@ frame->instr_ptr = next_instr; next_instr += 1; INSTRUCTION_STATS(MAKE_CELL); - PyObject *initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + PyObject *initial = NULL; + PyCodeObject *co = _PyFrame_GetCode(frame); + if (!(_PyLocals_GetKind(co->co_localspluskinds, oparg) & CO_FAST_FREE)) { + initial = PyStackRef_AsPyObjectBorrow(GETLOCAL(oparg)); + } PyObject *cell = PyCell_New(initial); if (cell == NULL) { JUMP_TO_LABEL(error);