| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughThis 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)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3a0c4118-c74c-4b6a-beaa-4402e66577f3
📥 CommitsReviewing files that changed from the base of the PR and between ae3804f and 8d1b959.
📒 Files selected for processing (2)
Sorry, something went wrong.
There was a problem hiding this comment.
crates/codegen/src/ir.rs (1)🤖 Prompt for all review comments with AI agents10931-10936: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Check 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🤖 Prompt for AI Agents- 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 });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.
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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e7ad65d4-609a-468e-88b2-90f7b2e590d2
📥 CommitsReviewing files that changed from the base of the PR and between 8d1b959 and aa50896.
📒 Files selected for processing (2)
Sorry, something went wrong.
* Align cleanup bytecode layout with CPython * Align bytecode anchors with CPython * Fix bytecode CFG review regressions
| Back | FazBrowse Home | New Git URL |
Summary
Align exception, finally, and with cleanup 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:
Validation
All grouped-scope compare reports are at 0 differences.
Summary by CodeRabbit
Refactor
Tests