Rework exception compiler #6909
Conversation
📝 WalkthroughWalkthroughRefactors exception handling: removes per-block handler/stack/lasti fields from FBlockInfo and push_fblock APIs, adds post-codegen CFG passes to annotate blocks (except_handler/preserve_lasti/start_depth), and converts Setup* pseudo-instructions to carry explicit target labels. Changes
Sequence Diagram(s)sequenceDiagram
participant Codegen as Codegen (compile.rs)
participant IR as IR Layer (ir.rs)
participant CFG as CFG Passes
participant Bytecode as Bytecode Instrs
Codegen->>Codegen: Emit blocks (simplified fblocks)
Codegen->>IR: create CodeInfo / finalize_code
IR->>CFG: mark_except_handlers(blocks)
CFG->>CFG: label_exception_targets(blocks)
CFG->>CFG: convert_pseudo_ops(blocks, varnames_len)
CFG->>Bytecode: annotate blocks (except_handler, start_depth, preserve_lasti)
Bytecode->>Bytecode: emit SetupCleanup{target}, SetupFinally{target}, SetupWith{target}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin exceptions |
youknowone
force-pushed
the
exceptions
branch
from
92f8696 to
41538d0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/codegen/src/compile.rs`:
- Around line 3178-3186: The code emits a SetupFinally and calls
push_fblock(FBlockType::FinallyTry, ...) but stores FBlockDatum::None so the
finally body is not inlined on unwind; replace the push_fblock call in the try*
path (the block that emits PseudoInstruction::SetupFinally and uses
finally_block) with push_fblock_full and pass
FBlockDatum::FinallyBody(finalbody.to_vec()) so unwind_fblock_stack can inline
finalbody on return/break/continue; apply the same change at the other
occurrence around the lines noted (also where PseudoInstruction::SetupFinally
and FBlockType::FinallyTry are used).
🧹 Nitpick comments (3)
crates/codegen/src/ir.rs (3)
247-256: Consider consolidating NOP removal.NOPs are removed here after
convert_pseudo_ops, but there's also a separateremove_nops()call at Line 195. The early call handles NOPs from constant folding, but this creates a two-phase removal pattern that could be confusing.
1266-1269: POP_BLOCK is converted to NOP twice.
POP_BLOCKis converted toInstruction::Nophere inlabel_exception_targets(Line 1269) and again inconvert_pseudo_ops(Lines 1348-1350). The second conversion is redundant but harmless. The comment on Line 1347 acknowledges this.Consider removing the redundant case in
convert_pseudo_opssince it's already handled:♻️ Proposed simplification
// POP_BLOCK → NOP (should already be NOP from label_exception_targets) PseudoInstruction::PopBlock => { - info.instr = Instruction::Nop.into(); + // Already converted to NOP in label_exception_targets + debug_assert!(matches!(info.instr.real(), Some(Instruction::Nop))); }
1360-1367: Unimplemented panic for unexpected pseudo instructions.Using
unimplemented!will cause a panic if these pseudo-ops are encountered. If these represent compiler bugs rather than user errors, consider usingunreachable!with a clearer message, or returning an error.♻️ Proposed change
// These should have been resolved earlier PseudoInstruction::AnnotationsPlaceholder | PseudoInstruction::JumpIfFalse { .. } | PseudoInstruction::JumpIfTrue { .. } | PseudoInstruction::StoreFastMaybeNull(_) => { - unimplemented!( - "Unexpected pseudo instruction in convert_pseudo_ops: {pseudo:?}" - ) + unreachable!( + "Pseudo instruction should have been resolved earlier: {pseudo:?}" + ) }
| if !finalbody.is_empty() { | ||
| // No SetupFinally emit - exception table handles this | ||
| self.push_fblock_with_handler( | ||
| FBlockType::FinallyTry, | ||
| finally_block, | ||
| finally_block, | ||
| Some(finally_block), | ||
| current_depth, // stack depth for exception handler | ||
| true, // preserve lasti for finally | ||
| )?; | ||
| emit!( | ||
| self, | ||
| PseudoInstruction::SetupFinally { | ||
| target: finally_block | ||
| } | ||
| ); | ||
| self.push_fblock(FBlockType::FinallyTry, finally_block, finally_block)?; | ||
| } |
There was a problem hiding this comment.
FinallyTry fblocks in try path drop the finally body on unwind.*
push_fblock stores FBlockDatum::None, so unwind_fblock_stack won’t inline finalbody on return/break/continue from try/else paths in except* constructs. Use push_fblock_full with FBlockDatum::FinallyBody(finalbody.to_vec()) in both places.
✅ Suggested fix
- self.push_fblock(FBlockType::FinallyTry, finally_block, finally_block)?;
+ self.push_fblock_full(
+ FBlockType::FinallyTry,
+ finally_block,
+ finally_block,
+ FBlockDatum::FinallyBody(finalbody.to_vec()),
+ )?;
...
- self.push_fblock(FBlockType::FinallyTry, finally_block, finally_block)?;
+ self.push_fblock_full(
+ FBlockType::FinallyTry,
+ finally_block,
+ finally_block,
+ FBlockDatum::FinallyBody(finalbody.to_vec()),
+ )?;Also applies to: 3435-3442
🤖 Prompt for AI Agents
In `@crates/codegen/src/compile.rs` around lines 3178 - 3186, The code emits a
SetupFinally and calls push_fblock(FBlockType::FinallyTry, ...) but stores
FBlockDatum::None so the finally body is not inlined on unwind; replace the
push_fblock call in the try* path (the block that emits
PseudoInstruction::SetupFinally and uses finally_block) with push_fblock_full
and pass FBlockDatum::FinallyBody(finalbody.to_vec()) so unwind_fblock_stack can
inline finalbody on return/break/continue; apply the same change at the other
occurrence around the lines noted (also where PseudoInstruction::SetupFinally
and FBlockType::FinallyTry are used).
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.