Align cleanup bytecode layout with CPython#7935
Conversation
📝 WalkthroughWalkthroughThis PR refines RustPython's bytecode codegen to achieve closer CPython-parity by improving control-flow handling in try/finally, while-true loops, match statements, and jump assembly. Changes span both high-level AST compilation and low-level IR finalization, introducing new statement-shape predicates, refined NOP preservation logic, location-override propagation, and conditional jump retargeting. ChangesCPython-parity bytecode codegen refinements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
032a6cb to
03a9594
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/codegen/src/compile.rs`:
- Around line 8728-8731: The current check only sees root-level OR patterns by
testing has_match_or_case over m.pattern with matches!(...,
ast::Pattern::MatchOr(_)); implement a recursive detector (e.g.,
contains_match_or(pattern: &ast::Pattern) -> bool) that walks all pattern
variants and returns true if any nested ast::Pattern::MatchOr is found, then
replace the .any(...) calls (including the similar check at the later block
around the 8788-8793 area) to use contains_match_or(&m.pattern) so nested forms
like [1 | 2, x] or Foo(bar=1 | 2) are correctly detected.
- Around line 1096-1099: The helper statements_end_with_conditional currently
only checks for a top-level ast::Stmt::If; broaden it to treat other conditional
shapes (e.g., ast::Stmt::Match) and container statements whose inner body ends
with a conditional (e.g., ast::Stmt::With whose body.last() is an If, nested
blocks, etc.) by making statements_end_with_conditional recursively inspect the
last statement: return true for If or Match, and for With (and other block-like
stmts) call the same helper on their inner body(s); update any call sites
(notably the final-body layout logic that uses statements_end_with_conditional)
to rely on this expanded predicate so trailing match/with-nested-if forms are
recognized as conditional finalizers.
In `@crates/codegen/src/ir.rs`:
- Around line 10882-10898: The current booleans
has_nointerrupt_jump_to_last_block and
has_lineful_return_fallthrough_to_last_block are global to the function and
incorrectly suppress duplication for all predecessors of last_block; change them
to be edge-specific by computing these predicates per predecessor (or per (pred,
last_block) edge) instead of once for the whole function. Concretely, replace
the top-level uses of has_nointerrupt_jump_to_last_block and
has_lineful_return_fallthrough_to_last_block with code that for each predecessor
block computes: scan that predecessor's instructions with jump_thread_kind(...)
== Some(JumpThreadKind::NoInterrupt) and target==last_block (using
next_nonempty_block to compare), and check whether the specific predecessor's
fallthrough (block.next) ends with a lineful ReturnValue (use
instruction_lineno(...) and matches!(instr.instr.real(),
Some(Instruction::ReturnValue))). Do the same edge-specific refactor at the
other occurrence referenced (lines ~10924-10948), so suppression decisions are
made per-edge rather than function-wide.
- Around line 10787-10798: The current pattern in
ends_with_line_marker_implicit_return(block: &Block) uses [.., marker, load,
ret] which will match blocks that have real instructions before the suffix;
change the match so we only consider truly empty epilogues by requiring the
block be exactly [marker, load, ret] or, if you want to allow only padding nops,
add a guard that verifies all instructions before marker are non-real padding
(e.g. instr.real() is None or Instruction::Nop and/or have the appropriate
no-location flags) before retargeting; update the match and/or add that extra
guard around marker, load, ret to avoid skipping earlier real instructions.
🪄 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: 3a0c4118-c74c-4b6a-beaa-4402e66577f3
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/codegen/src/ir.rs (1)
10931-10936:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck the fallthrough successor for the lineful return.
This predicate still inspects the current block instead of the fallthrough block, so it never becomes true for the jump cases this branch is trying to suppress. That means
duplicate_end_returns()can still clone the final epilogue even when the specific fallthrough edge already reaches a lineful return.Suggested fix
- let has_lineful_return_fallthrough_to_last_block = block.next != BlockIdx::NULL - && next_nonempty_block(blocks, block.next) == last_block - && block.instructions.last().is_some_and(|instr| { + let has_lineful_return_fallthrough_to_last_block = next != BlockIdx::NULL + && next == last_block + && blocks[next.idx()].instructions.last().is_some_and(|instr| { matches!(instr.instr.real(), Some(Instruction::ReturnValue)) && instruction_lineno(instr) > 0 });🤖 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/ir.rs` around lines 10931 - 10936, The predicate has_lineful_return_fallthrough_to_last_block is checking the current block's last instruction instead of the fallthrough block; update it to inspect the fallthrough block returned by next_nonempty_block(blocks, block.next) (e.g., let fallthrough = next_nonempty_block(blocks, block.next)) and then test blocks[fallthrough].instructions.last().is_some_and(|instr| matches!(instr.instr.real(), Some(Instruction::ReturnValue)) && instruction_lineno(instr) > 0); this prevents duplicate_end_returns() from cloning the final epilogue when the fallthrough already ends with a lineful return.
🤖 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.
Duplicate comments:
In `@crates/codegen/src/ir.rs`:
- Around line 10931-10936: The predicate
has_lineful_return_fallthrough_to_last_block is checking the current block's
last instruction instead of the fallthrough block; update it to inspect the
fallthrough block returned by next_nonempty_block(blocks, block.next) (e.g., let
fallthrough = next_nonempty_block(blocks, block.next)) and then test
blocks[fallthrough].instructions.last().is_some_and(|instr|
matches!(instr.instr.real(), Some(Instruction::ReturnValue)) &&
instruction_lineno(instr) > 0); this prevents duplicate_end_returns() from
cloning the final epilogue when the fallthrough already ends with a lineful
return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e7ad65d4-609a-468e-88b2-90f7b2e590d2
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
Summary
Align exception, finally, and
withcleanup bytecode layout with CPython 3.14 compiler behavior for issue youknowone#32.This updates shared codegen/CFG assembly behavior rather than editing
Lib/, using the local CPython compiler sources as reference:Python/codegen.cfortry/finally,with,while, break/continue, and named-except cleanup layoutPython/flowgraph.cfor late CFG cleanup and load-fast borrow behaviorPython/assemble.cfor jump offset fixed-point behavior aroundEXTENDED_ARGValidation
cargo fmt --checkcargo test -p rustpython-codegen --libcargo build --release -p rustpythoncargo clippy -p rustpython-codegen --libgit diff --checkscripts/compare_bytecode.pywith release RustPython for the issue Add proper support for kwargs #32 grouped scope:multiprocessing/forkserver.pytest/test_compile.pytest/test_faulthandler.pytest/test_hashlib.pytest/test_importlib/extension/test_loader.pytest/test_poplib.pytest/test_socket.pytest/test_sys_settrace.pytest/test_unittest/testmock/testsealable.pyAll grouped-scope compare reports are at 0 differences.
Summary by CodeRabbit
Refactor
Tests