Align codegen passes and opcode metadata with CPython#7987
Conversation
read_marshal_bytes, _str, _str_vec, _name_tuple, and _const_tuple now take a shared ref table and resolve TYPE_REF / register FLAG_REF entries. deserialize_code is split into a public wrapper and an inner function that receives the ref table; deserialize_value_depth opens a fresh inner ref space when it hits Type::Code, mirroring CPython's behaviour of putting the code object itself at ref slot 0. Nested code objects inside const tuples reuse the surrounding code's ref space via the new read_const_value helper.
… 3.14 PYC_MAGIC_NUMBER changes from 2994 to 3627, matching CPython 3.14's pyc_magic_number_token (0x0a0d0e2b). marshal FORMAT_VERSION drops from 5 to 4 (the encoder/marshal.version value; the decoder already accepts both). check_pyc_magic_number_bytes now compares all four magic bytes instead of the first two.
SourceFileLoader.get_code now also looks for .pyc files using
_RP_FALLBACK_CACHE_TAGS (currently ('cpython-314',)) in addition to
sys.implementation.cache_tag. The matched .pyc is only used for
reading; recompilation still writes to the RustPython-tagged path, so
CPython's .pyc is never overwritten. Source-stat / hash / timestamp
validation logic is unchanged.
CPython's marshal supports TYPE_SLICE from format version 4 onwards and that is the default version. Rejecting slice dumps below version 5 made marshal.dumps(slice(...)) fail with the default version and broke test.test_marshal.SliceTestCase.test_slice.
Lib/importlib/_bootstrap_external.py is CPython's own code copied verbatim; local patches here defeat compatibility tracking. The cpython-XX cache_tag fallback needs to live on the RustPython side (Rust code or sys.implementation.cache_tag policy), not as edits to the imported standard library. This reverts commit 1fc426d0fb5fcdb50d35cad13bbb43e8f6ce1c7f.
Py_MARSHAL_VERSION is 5 in CPython 3.14.5 (Include/marshal.h:16) and TYPE_SLICE serialization rejects version < 5 (Python/marshal.c:720). Restore the same threshold and constant so marshal.version and the slice-marshal gate match CPython.
Code objects embedded in const-tuples reset the depth budget on each recursion, so a hostile or pathological marshal stream of code-in-tuple- in-code can blow the stack despite MAX_MARSHAL_STACK_DEPTH. Pass the current depth through deserialize_code_inner and read_marshal_const_tuple and decrement at each code-object/tuple boundary. Also route dict keys through deserialize_value_after_header so TYPE_CODE keys decode instead of failing with BadType.
Rename CFG helpers and accessors to the names used in CPython's compile.c (basicblock_next_instr, basicblock_last_instr, basicblock_append_instructions, bb_has_fallthrough, is_jump, make_cfg_traversal_stack, mark_warm/mark_cold, etc.). Drop the unused boolop-folding gate, mark_cpython_cfg_label_block helper, and ComprehensionLoopControl::iter_range field. Track an is_coroutine flag on SymbolTable, set in async def, await, and async comprehensions, and propagate it through non-generator comprehensions per symtable_handle_comprehension(). Mark SetupCleanup/SetupFinally/SetupWith as has_arg pseudo-ops, mark ForIter as a terminator, and add has_arg/has_const on AnyInstruction. Fix Instruction::stack_effect_jump to delegate to the opcode's stack_effect_jump rather than stack_effect.
📝 WalkthroughWalkthroughThis PR enhances the RustPython compiler infrastructure across multiple layers: symbol analysis now tracks coroutine-related syntax; bytecode types gain comprehensive predicate methods; opcode metadata includes eval-break classification and corrected stack effects; error reporting improves for control-flow validation; marshal deserialization correctly preserves reference indices for nested code objects; and test coverage expands with validation of block expressions and instruction predicates. ChangesCompiler and Bytecode Infrastructure
Sequence Diagram(s)sequenceDiagram
participant SymbolAnalysis
participant BytecodeGen
participant InstructionPredicates
participant OpcodeMetadata
participant ControlFlowValidation
SymbolAnalysis->>BytecodeGen: emit async/coroutine info
BytecodeGen->>InstructionPredicates: query is_terminator, has_eval_break
InstructionPredicates->>OpcodeMetadata: delegate to opcode flags
OpcodeMetadata->>ControlFlowValidation: eval_break, stack_effect
ControlFlowValidation->>ControlFlowValidation: validate CFG, report InternalError variants
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add CNOTAB, LNOTAB, ialloc, ioffset, iused, nblocks, ncellsused, ncellvars, nextop, noffsets, nvars, swaptimize, untargeted to .cspell.dict/cpython.txt for the new CFG/assembler code in crates/codegen/src/ir.rs.
- clippy: drop redundant `test_` prefix on three test functions and remove an unnecessary `u32` cast in basicblock_clear_reuses_cpython_spare_slots_in_offset_order - insta: regenerate nested_double_async_with snapshot to match the new CFG output that drops unreferenced labels after the redundant-NOP pass - regrtest: drop `@expectedFailure` markers from test_func_args, test_meth_args (test_compile), test_disassemble_with, test_disassemble_try_finally (test_dis), and test_except_star (test_monitoring) which now pass
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_compile.py (TODO: 12) dependencies: dependent tests: (no tests depend on compile) [x] lib: cpython/Lib/dis.py dependencies:
dependent tests: (73 tests)
[ ] test: cpython/Lib/test/test_monitoring.py (TODO: 7) dependencies: dependent tests: (no tests depend on monitoring) Legend:
|
Empty conf.toml since WithExceptStart and Setup{Cleanup,Finally,With}
stack effects already match CPython, so the TODO override entries are
stale and only cause CI hook diffs.
Regenerate opcode_metadata.rs and drop the matching SetupCleanup/
SetupFinally/SetupWith assertions on PseudoOpcode::has_arg(); their
`HAS_ARG` flag comes from pseudo definitions in bytecodes.c that the
upstream analyzer does not propagate through PseudoInstruction.properties,
so the generated has_arg() excludes them. has_target() still covers
these block-push pseudos via is_block_push().
The CPython invariant `assert(OPCODE_HAS_ARG(op) || !IS_BLOCK_PUSH(op))`
relies on SETUP_{FINALLY,CLEANUP,WITH} carrying `HAS_ARG_FLAG` in
CPython's metadata. The autogen tool reads pseudo-opcode properties from
target instructions and does not propagate the pseudo's own
HAS_ARG flag, so PseudoOpcode::has_arg() omits these three opcodes.
Drop the debug_assert that fired inside py_freeze proc-macro expansion.
| self.has_jump() || self.is_block_push() | ||
| } | ||
|
|
||
| /// Does this opcode have CPython's `HAS_EVAL_BREAK_FLAG` set. |
There was a problem hiding this comment.
@youknowone can get auto-generate this?
https://github.com/python/cpython/blob/5607950ef232dad16d75c0cf53101d9649d89115/Tools/cases_generator/analyzer.py#L16-L39 has a eval_breaker attribute
| Self::Real(instr) => instr.as_opcode().has_const(), | ||
| Self::Pseudo(instr) => instr.as_opcode().has_const(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you use the either_real_pseudo! macro instead?
| ); | ||
|
|
||
| #[must_use] | ||
| pub const fn has_arg(&self) -> bool { |
…st via macro Add fn_has_eval_break to generate_rs_opcode_metadata.py using CPython's Properties.eval_breaker, removing the hand-written matches! body for Opcode::has_eval_break and PseudoOpcode::has_eval_break. Forward has_arg/has_const from Instruction and PseudoInstruction to their opcode, so AnyInstruction can use either_real_pseudo! like the other has_* accessors instead of an open-coded match.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/compiler-core/src/marshal.rs (1)
748-764:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPropagate new code-object refs back to the outer ref table.
Line 748 builds
inner_refsfromrefs, but refs created while decoding code fields are dropped when returning. That breaks global ref-space continuity for any later outerTYPE_REFthat points to entries first created inside the code object.Proposed direction
pub trait MarshalBag: Copy { type Value: Clone; type ConstantBag: ConstantBag; @@ fn constant_ref_from_value( &self, _value: &Self::Value, ) -> Option<<Self::ConstantBag as ConstantBag>::Constant> { None } + + fn value_ref_from_constant( + &self, + _value: &<Self::ConstantBag as ConstantBag>::Constant, + ) -> Option<Self::Value> { + None + } } @@ impl<Bag: ConstantBag> MarshalBag for Bag { @@ fn constant_ref_from_value( &self, value: &Self::Value, ) -> Option<<Self::ConstantBag as ConstantBag>::Constant> { Some(value.clone()) } + + fn value_ref_from_constant( + &self, + value: &<Self::ConstantBag as ConstantBag>::Constant, + ) -> Option<Self::Value> { + Some(value.clone()) + } } @@ - let mut inner_refs: Vec<Option<<Bag::ConstantBag as ConstantBag>::Constant>> = refs + let mut inner_refs: Vec<Option<<Bag::ConstantBag as ConstantBag>::Constant>> = refs .iter() .map(|value| { value .as_ref() .and_then(|value| bag.constant_ref_from_value(value)) }) .collect(); + let outer_len = inner_refs.len(); let code = deserialize_code_inner(rdr, bag.constant_bag(), depth - 1, &mut inner_refs)?; + refs.extend( + inner_refs + .into_iter() + .skip(outer_len) + .map(|v| v.and_then(|c| bag.value_ref_from_constant(&c))), + ); bag.make_code(code)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/compiler-core/src/marshal.rs` around lines 748 - 764, When decoding a code object we build inner_refs from refs (via bag.constant_ref_from_value) but never copy new constants created while deserializing the code back into the outer refs table; after calling deserialize_code_inner/make_code you should iterate the inner_refs (the Vec<Option<<Bag::ConstantBag as ConstantBag>::Constant>>) with their indices and for each Some(constant) convert it back into a Value (e.g. via a helper like bag.value_from_constant or equivalent) and set refs[index] = Some(converted_value) when refs[index] is currently None so newly-created code-local refs are propagated into the outer refs table before returning (update around the deserialize_code_inner / bag.make_code and the existing slot handling).
🧹 Nitpick comments (1)
crates/codegen/src/error.rs (1)
48-61: 💤 Low valueConsider standardizing error message formatting.
The error messages have minor formatting inconsistencies:
- Line 54 ends with a period while lines 51-53 don't
- Line 52 uses "stackdepth" (one word) instead of "stack depth" (two words)
- Line 54 starts with lowercase "malformed" while others use title case
Consider applying consistent formatting:
📝 Suggested consistency improvements
match self { Self::StackUnderflow => write!(f, "Invalid CFG, stack underflow"), - Self::InconsistentStackDepth => write!(f, "Invalid CFG, inconsistent stackdepth"), + Self::InconsistentStackDepth => write!(f, "Invalid CFG, inconsistent stack depth"), Self::InvalidStackEffect => write!(f, "Invalid stack effect"), - Self::MalformedControlFlowGraph => write!(f, "malformed control flow graph."), + Self::MalformedControlFlowGraph => write!(f, "Malformed control flow graph"), Self::MissingSymbol(s) => write!(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/codegen/src/error.rs` around lines 48 - 61, Standardize the error message strings in the Display impl for InternalError: make casing consistent (capitalize each message start), use "stack depth" (two words) for InconsistentStackDepth, and apply consistent punctuation (either add or remove periods uniformly) across all variants — update the messages for StackUnderflow, InconsistentStackDepth, InvalidStackEffect, MalformedControlFlowGraph, and MissingSymbol in the Display implementation to follow the chosen style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/compiler-core/src/bytecode/opcode_metadata.rs`:
- Line 474: The stack effect for the opcode variant Self::WithExceptStart is
wrong: it currently uses (6, 5) but CPython 3.13 semantics pop three values
(exit function, lasti, exception) and push one result, so change the tuple to
(1, 3) to represent (pushed, popped) and produce a net -2 effect (or, if your
tuple convention is reversed, update the code/comments to consistently document
that convention and then set the values to match CPython 3.13 semantics for
WITH_EXCEPT_START).
---
Outside diff comments:
In `@crates/compiler-core/src/marshal.rs`:
- Around line 748-764: When decoding a code object we build inner_refs from refs
(via bag.constant_ref_from_value) but never copy new constants created while
deserializing the code back into the outer refs table; after calling
deserialize_code_inner/make_code you should iterate the inner_refs (the
Vec<Option<<Bag::ConstantBag as ConstantBag>::Constant>>) with their indices and
for each Some(constant) convert it back into a Value (e.g. via a helper like
bag.value_from_constant or equivalent) and set refs[index] =
Some(converted_value) when refs[index] is currently None so newly-created
code-local refs are propagated into the outer refs table before returning
(update around the deserialize_code_inner / bag.make_code and the existing slot
handling).
---
Nitpick comments:
In `@crates/codegen/src/error.rs`:
- Around line 48-61: Standardize the error message strings in the Display impl
for InternalError: make casing consistent (capitalize each message start), use
"stack depth" (two words) for InconsistentStackDepth, and apply consistent
punctuation (either add or remove periods uniformly) across all variants —
update the messages for StackUnderflow, InconsistentStackDepth,
InvalidStackEffect, MalformedControlFlowGraph, and MissingSymbol in the Display
implementation to follow the chosen style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b93c9b50-b32c-4010-9297-5d9b16d5a699
⛔ Files ignored due to path filters (4)
Lib/test/test_compile.pyis excluded by!Lib/**Lib/test/test_dis.pyis excluded by!Lib/**Lib/test/test_monitoring.pyis excluded by!Lib/**crates/stdlib/src/snapshots/rustpython_stdlib___opcode__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
.cspell.dict/cpython.txtcrates/codegen/src/compile.rscrates/codegen/src/error.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/opcode_metadata.rscrates/compiler-core/src/marshal.rscrates/vm/src/vm/python_run.rstools/opcode_metadata/conf.tomltools/opcode_metadata/generate_rs_opcode_metadata.py
💤 Files with no reviewable changes (1)
- tools/opcode_metadata/conf.toml
| 7, // TODO: Differs from CPython `6` | ||
| 6, // TODO: Differs from CPython `5` | ||
| ), | ||
| Self::WithExceptStart => (6, 5), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython 3.13 WITH_EXCEPT_START opcode stack effect pushed popped values
💡 Result:
In CPython 3.13, the WITH_EXCEPT_START opcode is used to implement the exit method call for context managers when an exception has occurred within a with statement [1][2]. Regarding its stack effect: - It consumes three values from the stack: the exit function (exit), the last instruction pointer (lasti), and the current exception (exc) [2]. - It pushes one value onto the stack: the result of calling the exit method [2]. - Formally, its net stack effect is -2 (3 popped, 1 pushed) [2]. The opcode retrieves exception information (type, value, traceback) and calls the exit function with these arguments: exit_func(type(exc), exc, exc.traceback) [2]. If the exception is None, it uses Py_None for all three arguments [2]. The result of this call is then left on the stack for subsequent bytecode instructions, such as RERAISE or those handling exception suppression [2].
Citations:
- 1: https://docs.python.org/3/library/dis.html
- 2: https://gopy.tamnd.com/docs/annotations/python/python_ceval23_detail
Fix WithExceptStart stack effect to match CPython 3.13
crates/compiler-core/src/bytecode/opcode_metadata.rs:474setsSelf::WithExceptStart => (6, 5)(pushes=6, pops=5; net +1).- CPython 3.13
WITH_EXCEPT_STARTpops 3 values (exit function,lasti, exception) and pushes 1 value (the exit result), i.e. expected(1, 3)(net -2). - Update
WithExceptStart’s stack effect (or clarify the tuple’s(pushed, popped)convention if different) so it matches CPython 3.13.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/compiler-core/src/bytecode/opcode_metadata.rs` at line 474, The stack
effect for the opcode variant Self::WithExceptStart is wrong: it currently uses
(6, 5) but CPython 3.13 semantics pop three values (exit function, lasti,
exception) and push one result, so change the tuple to (1, 3) to represent
(pushed, popped) and produce a net -2 effect (or, if your tuple convention is
reversed, update the code/comments to consistently document that convention and
then set the values to match CPython 3.13 semantics for WITH_EXCEPT_START).
Summary by CodeRabbit
Bug Fixes
Refactor