From 9ea599299737505e6eb2dc318c42ad25aa3223de Mon Sep 17 00:00:00 2001 From: harjoth Date: Thu, 11 Jun 2026 15:20:51 -0700 Subject: [PATCH] gh-151321: Fix opcode metadata flags for LOAD_DEREF and LOAD_CLOSURE LOAD_DEREF and LOAD_CLOSURE were missing HAS_FREE_FLAG in the generated opcode metadata, and both also carried HAS_LOCAL_FLAG, so dis.hasfree no longer reported them while dis.haslocal wrongly included them. This regressed the documented dis.hasfree / dis.haslocal contract: LOAD_CLOSURE broke in 3.13 (when it became a pseudo-op inheriting LOAD_FAST's flags) and LOAD_DEREF broke in 3.14 (when it started using _PyCell_GetStackRef, which the cases generator's has_free heuristic did not recognize). Teach the cases generator to treat _PyCell_GetStackRef as a free-variable access, and give the LOAD_CLOSURE pseudo an explicit HAS_FREE flag. A pseudo that explicitly accesses a free variable now suppresses the inherited HAS_LOCAL, mirroring the existing "uses_locals and not has_free" rule for real instructions, so HAS_FREE and HAS_LOCAL are never both set. The change is to introspection metadata only; the generated interpreter and optimizer code are unchanged. Co-Authored-By: Claude Fable 5 --- Include/internal/pycore_opcode_metadata.h | 4 ++-- Include/internal/pycore_uop_metadata.h | 2 +- Lib/test/test__opcode.py | 17 +++++++++++++++++ ...26-06-11-15-17-09.gh-issue-151321.Kf3Lp9.rst | 3 +++ Python/bytecodes.c | 2 +- Tools/cases_generator/analyzer.py | 11 ++++++++++- 6 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-06-11-15-17-09.gh-issue-151321.Kf3Lp9.rst diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index 80ad440bac8293c..3ab9d83296aeff7 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -1260,7 +1260,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[267] = { [LOAD_BUILD_CLASS] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [LOAD_COMMON_CONSTANT] = { true, INSTR_FMT_IB, HAS_ARG_FLAG }, [LOAD_CONST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_CONST_FLAG }, - [LOAD_DEREF] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, + [LOAD_DEREF] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [LOAD_FAST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_PURE_FLAG }, [LOAD_FAST_AND_CLEAR] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG }, [LOAD_FAST_BORROW] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_PURE_FLAG }, @@ -1351,7 +1351,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[267] = { [JUMP_IF_FALSE] = { true, -1, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [JUMP_IF_TRUE] = { true, -1, HAS_ARG_FLAG | HAS_JUMP_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [JUMP_NO_INTERRUPT] = { true, -1, HAS_ARG_FLAG | HAS_JUMP_FLAG }, - [LOAD_CLOSURE] = { true, -1, HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_PURE_FLAG }, + [LOAD_CLOSURE] = { true, -1, HAS_ARG_FLAG | HAS_PURE_FLAG | HAS_FREE_FLAG }, [POP_BLOCK] = { true, -1, HAS_PURE_FLAG }, [SETUP_CLEANUP] = { true, -1, HAS_PURE_FLAG | HAS_ARG_FLAG }, [SETUP_FINALLY] = { true, -1, HAS_PURE_FLAG | HAS_ARG_FLAG }, diff --git a/Include/internal/pycore_uop_metadata.h b/Include/internal/pycore_uop_metadata.h index 45f407dae3ddf57..dc81bc29fb049a3 100644 --- a/Include/internal/pycore_uop_metadata.h +++ b/Include/internal/pycore_uop_metadata.h @@ -202,7 +202,7 @@ const uint32_t _PyUop_Flags[MAX_UOP_ID+1] = { [_MAKE_CELL] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG | HAS_ESCAPES_FLAG, [_DELETE_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG | HAS_ESCAPES_FLAG, [_LOAD_FROM_DICT_OR_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG | HAS_ESCAPES_FLAG, - [_LOAD_DEREF] = HAS_ARG_FLAG | HAS_LOCAL_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, + [_LOAD_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_STORE_DEREF] = HAS_ARG_FLAG | HAS_FREE_FLAG | HAS_ESCAPES_FLAG, [_COPY_FREE_VARS] = HAS_ARG_FLAG, [_BUILD_STRING] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, diff --git a/Lib/test/test__opcode.py b/Lib/test/test__opcode.py index c253bc2be025a01..85a7b4159f5e1dd 100644 --- a/Lib/test/test__opcode.py +++ b/Lib/test/test__opcode.py @@ -61,6 +61,23 @@ def check_function(self, func, expected): check_function(self, _opcode.has_local, dis.haslocal) check_function(self, _opcode.has_exc, dis.hasexc) + def test_hasfree_membership(self): + # gh-151321: opcodes that access a free (closure) variable must be + # reported by dis.hasfree and not by dis.haslocal. + free_ops = { + 'DELETE_DEREF', + 'LOAD_CLOSURE', + 'LOAD_DEREF', + 'LOAD_FROM_DICT_OR_DEREF', + 'MAKE_CELL', + 'STORE_DEREF', + } + self.assertEqual(free_ops, {dis.opname[op] for op in dis.hasfree}) + self.assertNotIn(dis.opmap['LOAD_DEREF'], dis.haslocal) + self.assertNotIn(dis.opmap['LOAD_CLOSURE'], dis.haslocal) + # A free-variable access is never also a local access. + self.assertEqual(set(), set(dis.hasfree) & set(dis.haslocal)) + class StackEffectTests(unittest.TestCase): def test_stack_effect(self): diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-06-11-15-17-09.gh-issue-151321.Kf3Lp9.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-06-11-15-17-09.gh-issue-151321.Kf3Lp9.rst new file mode 100644 index 000000000000000..4f377445ea4bdac --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-06-11-15-17-09.gh-issue-151321.Kf3Lp9.rst @@ -0,0 +1,3 @@ +Fix the opcode metadata flags for :opcode:`LOAD_DEREF` and :opcode:`LOAD_CLOSURE` +so that they are reported by :data:`dis.hasfree` again, and are no longer +incorrectly reported by :data:`dis.haslocal`. diff --git a/Python/bytecodes.c b/Python/bytecodes.c index e368092b300f864..5909b4109edc160 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -264,7 +264,7 @@ dummy_func( _CHECK_PERIODIC_IF_NOT_YIELD_FROM + _MONITOR_RESUME; - pseudo(LOAD_CLOSURE, (-- unused)) = { + pseudo(LOAD_CLOSURE, (-- unused), (HAS_FREE)) = { LOAD_FAST, }; diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index 22a321b4953de7d..892d265c30d8478 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -313,7 +313,15 @@ def dump(self, indent: str) -> None: @property def properties(self) -> Properties: - return Properties.from_list([i.properties for i in self.targets]) + properties = Properties.from_list([i.properties for i in self.targets]) + if "HAS_FREE" in self.flags: + # A pseudo that explicitly accesses a free (closure) variable is + # not a local access, even though it may target a local-accessing + # instruction such as LOAD_FAST. This mirrors the + # ``uses_locals and not has_free`` rule for real instructions so + # that HAS_FREE and HAS_LOCAL are not both set (gh-151321). + properties.uses_locals = False + return properties @dataclass @@ -970,6 +978,7 @@ def compute_properties(op: parser.CodeDef) -> Properties: or variable_used(op, "PyCell_GetRef") or variable_used(op, "PyCell_SetTakeRef") or variable_used(op, "PyCell_SwapTakeRef") + or variable_used(op, "_PyCell_GetStackRef") ) deopts_if = variable_used(op, "DEOPT_IF") exits_if = variable_used(op, "EXIT_IF")