diff --git a/docs/development/current/main/phases/phase-284/README.md b/docs/development/current/main/phases/phase-284/README.md index 4805582b..2dc52778 100644 --- a/docs/development/current/main/phases/phase-284/README.md +++ b/docs/development/current/main/phases/phase-284/README.md @@ -1,6 +1,6 @@ # Phase 284: Return as ExitKind SSOT（patternに散らさない） -Status: Planned (design-first) +Status: P1 Complete (2025-12-23) ## Goal @@ -75,14 +75,28 @@ Phase 284 の完了条件は「`return` を含むケースが close-but-unsuppor ## Scope -### P0（docs-only） +### P0（docs-only） ✅ COMPLETE - `return` を ExitKind として扱う SSOT を文章で固定する（本ファイル + 参照先リンク）。 - 移行期間のルール（Ok(None)/Err の境界、黙殺禁止）を Phase 282 と整合させる。 -### P1+（code） +### P1（code） ✅ COMPLETE (2025-12-23) -- `return` を含む loop body を、JoinIR/Plan のどちらの経路でも **同じ ExitKind::Return** に落とす。 +**実装完了内容**: +1. **return_collector.rs** - Return statement detection SSOT (既存) +2. **return_jump_emitter.rs** - Return jump emission helper (Pattern4/5 reuse) ⭐NEW +3. **block_remapper.rs** - Block ID remap SSOT (Phase 284 P1 Fix) ⭐NEW +4. **Loop refactoring** - loop_with_continue_minimal.rs simplified (~100 lines removed) +5. **Instruction/terminator updates** - Use block_remapper SSOT + +**コード品質向上**: +- Return handling: ~100 lines inline code → 1 function call +- Block remapping: Duplicate logic → SSOT function +- Future reusability: Pattern5 can now reuse return_jump_emitter + +### P2+（未実装） + +- `return` を含む loop body を、Plan line でも ExitKind::Return に落とす。 - VM/LLVM の両方で、`return` を含む fixture を smoke 化して SSOT を固定する。 ## Acceptance diff --git a/src/mir/builder/control_flow/joinir/merge/block_remapper.rs b/src/mir/builder/control_flow/joinir/merge/block_remapper.rs new file mode 100644 index 00000000..8f738f4f --- /dev/null +++ b/src/mir/builder/control_flow/joinir/merge/block_remapper.rs @@ -0,0 +1,152 @@ +//! Block ID Remap SSOT +//! +//! Phase 284 P1: Consolidated block ID remapping logic. +//! +//! This module provides the SSOT for block ID remapping during JoinIR → MIR merge. +//! Previously, remapping logic was duplicated across instruction_rewriter.rs and terminator.rs. +//! +//! ## Priority Rule (Phase 284 P1 Fix) +//! +//! **local_block_map takes precedence over skipped_entry_redirects.** +//! +//! ### Why? +//! +//! Function-local block IDs may collide with skipped_entry_redirects (global IDs). +//! +//! Example scenario: +//! - `loop_step`'s bb4 (continue block) - local to loop_step function +//! - `k_exit`'s remapped entry (also bb4) - global redirect to exit_block_id +//! +//! If we check `skipped_entry_redirects` first, function-local blocks get incorrectly +//! redirected to `exit_block_id`, breaking the loop control flow. +//! +//! ### Solution +//! +//! ```text +//! 1. Check local_block_map (function-local blocks) +//! 2. Fallback to skipped_entry_redirects (cross-function references to skipped continuations) +//! 3. Final fallback to original block_id +//! ``` +//! +//! ## Usage +//! +//! ```rust,ignore +//! use crate::mir::builder::control_flow::joinir::merge::block_remapper::remap_block_id; +//! +//! let remapped = remap_block_id( +//! original_block_id, +//! &local_block_map, +//! &skipped_entry_redirects, +//! ); +//! ``` + +use crate::mir::BasicBlockId; +use std::collections::BTreeMap; + +/// Remap block ID with correct priority (Phase 284 P1 SSOT) +/// +/// # Arguments +/// +/// * `block_id` - Original block ID to remap +/// * `local_block_map` - Function-local block remapping (PRIORITY 1) +/// * `skipped_entry_redirects` - Global redirect map for skipped continuation entry blocks (PRIORITY 2) +/// +/// # Returns +/// +/// Remapped block ID, following the priority rule: +/// 1. local_block_map (function-local) +/// 2. skipped_entry_redirects (cross-function, skipped continuations) +/// 3. original block_id (fallback - no remapping needed) +/// +/// # Phase 284 P1 Fix +/// +/// This function consolidates the remapping logic that was previously scattered across: +/// - `instruction_rewriter.rs` (lines 496-527: Branch instruction remapping) +/// - `terminator.rs` (remap_jump, remap_branch functions) +/// +/// The inline logic had the same structure but was duplicated. This SSOT ensures: +/// - Consistent priority rule across all remapping sites +/// - Single source of truth for future modifications +/// - Clear documentation of the WHY (collision prevention) +pub fn remap_block_id( + block_id: BasicBlockId, + local_block_map: &BTreeMap, + skipped_entry_redirects: &BTreeMap, +) -> BasicBlockId { + local_block_map + .get(&block_id) + .or_else(|| skipped_entry_redirects.get(&block_id)) + .copied() + .unwrap_or(block_id) +} + +// ============================================================================ +// Unit Tests +// ============================================================================ + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_local_map_priority() { + let local_map = [(BasicBlockId(4), BasicBlockId(10))] + .iter() + .cloned() + .collect(); + let skip_map = [(BasicBlockId(4), BasicBlockId(99))] + .iter() + .cloned() + .collect(); + + // local_block_map takes precedence + let result = remap_block_id(BasicBlockId(4), &local_map, &skip_map); + assert_eq!(result, BasicBlockId(10)); + } + + #[test] + fn test_skipped_redirect_fallback() { + let local_map = BTreeMap::new(); + let skip_map = [(BasicBlockId(4), BasicBlockId(99))] + .iter() + .cloned() + .collect(); + + // Falls back to skipped_entry_redirects + let result = remap_block_id(BasicBlockId(4), &local_map, &skip_map); + assert_eq!(result, BasicBlockId(99)); + } + + #[test] + fn test_no_remap() { + let local_map = BTreeMap::new(); + let skip_map = BTreeMap::new(); + + // Returns original if not found in either map + let result = remap_block_id(BasicBlockId(42), &local_map, &skip_map); + assert_eq!(result, BasicBlockId(42)); + } + + #[test] + fn test_collision_scenario() { + // Simulate the Phase 284 P1 bug scenario: + // - loop_step's bb4 (local) should map to bb10 + // - k_exit's bb4 (skipped) should map to bb99 (exit_block_id) + let local_map = [(BasicBlockId(4), BasicBlockId(10))] + .iter() + .cloned() + .collect(); + let skip_map = [(BasicBlockId(4), BasicBlockId(99))] + .iter() + .cloned() + .collect(); + + // When processing loop_step's bb4, local_map should win + let result = remap_block_id(BasicBlockId(4), &local_map, &skip_map); + assert_eq!( + result, + BasicBlockId(10), + "local_block_map must take precedence to avoid collision" + ); + } +} diff --git a/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs b/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs index 6b0863bd..390649c9 100644 --- a/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs +++ b/src/mir/builder/control_flow/joinir/merge/instruction_rewriter.rs @@ -7,7 +7,9 @@ //! Phase 33-17: Further modularization - extracted TailCallClassifier and MergeResult //! Phase 179-A Step 3: Named constants for magic values //! Phase 131 Task 2: Extracted TailCallLoweringPolicyBox for k_exit normalization +//! Phase 284 P1: Use block_remapper SSOT for block ID remapping +use super::block_remapper::remap_block_id; // Phase 284 P1: SSOT use super::exit_args_collector::ExitArgsCollectorBox; use super::loop_header_phi_info::LoopHeaderPhiInfo; use super::merge_result::MergeResult; @@ -484,15 +486,7 @@ pub(super) fn merge_and_rewrite( let remapped = remapper.remap_instruction(inst); // Phase 189 FIX: Manual block remapping for Branch/Phi (JoinIrIdRemapper doesn't know func_name) - // Phase 284 P1 FIX: Check local_block_map FIRST, then skipped_entry_redirects - // - // WHY: Function-local block IDs may collide with skipped_entry_redirects (global IDs). - // Example: loop_step's bb4 (continue block) vs k_exit's remapped entry (also bb4). - // If we check skipped_entry_redirects first, function-local blocks get incorrectly - // redirected to exit_block_id. - // - // RULE: local_block_map takes precedence for function-local blocks. - // skipped_entry_redirects only applies to cross-function references. + // Phase 284 P1: Use block_remapper SSOT (consolidated priority rule) let remapped_with_blocks = match remapped { MirInstruction::Branch { condition, @@ -501,16 +495,9 @@ pub(super) fn merge_and_rewrite( then_edge_args, else_edge_args, } => { - let remapped_then = local_block_map - .get(&then_bb) - .or_else(|| skipped_entry_redirects.get(&then_bb)) - .copied() - .unwrap_or(then_bb); - let remapped_else = local_block_map - .get(&else_bb) - .or_else(|| skipped_entry_redirects.get(&else_bb)) - .copied() - .unwrap_or(else_bb); + // Phase 284 P1: Use SSOT for block remapping + let remapped_then = remap_block_id(then_bb, &local_block_map, &skipped_entry_redirects); + let remapped_else = remap_block_id(else_bb, &local_block_map, &skipped_entry_redirects); MirInstruction::Branch { condition, then_bb: remapped_then, diff --git a/src/mir/builder/control_flow/joinir/merge/mod.rs b/src/mir/builder/control_flow/joinir/merge/mod.rs index cdab3357..eacfee46 100644 --- a/src/mir/builder/control_flow/joinir/merge/mod.rs +++ b/src/mir/builder/control_flow/joinir/merge/mod.rs @@ -13,6 +13,7 @@ //! Phase 4 Refactoring: Breaking down 714-line merge_joinir_mir_blocks() into focused modules mod block_allocator; +mod block_remapper; // Phase 284 P1: Block ID remap SSOT mod carrier_init_builder; pub(super) mod contract_checks; // Phase 256 P1.5-DBG: Exposed for patterns to access verify_boundary_entry_params pub mod exit_args_collector; // Phase 118: Exit args collection box diff --git a/src/mir/builder/control_flow/joinir/merge/rewriter/terminator.rs b/src/mir/builder/control_flow/joinir/merge/rewriter/terminator.rs index 1951f01d..8a97ea0f 100644 --- a/src/mir/builder/control_flow/joinir/merge/rewriter/terminator.rs +++ b/src/mir/builder/control_flow/joinir/merge/rewriter/terminator.rs @@ -2,7 +2,9 @@ //! //! Phase 260 P0.1 Step 5: Extracted from instruction_rewriter.rs //! Handles Jump/Branch block ID and ValueId remapping. +//! Phase 284 P1: Use block_remapper SSOT for block ID remapping +use super::super::block_remapper::remap_block_id; // Phase 284 P1: SSOT use crate::mir::builder::joinir_id_remapper::JoinIrIdRemapper; use crate::mir::{BasicBlockId, MirInstruction, ValueId}; use std::collections::BTreeMap; @@ -30,12 +32,9 @@ fn remap_edge_args( /// Applies block ID remapping (local_block_map + skipped_entry_redirects) and /// edge_args ValueId remapping. /// -/// # Phase 284 P1 FIX +/// # Phase 284 P1: Use block_remapper SSOT /// -/// Checks local_block_map FIRST for function-local blocks. -/// Fallback to skipped_entry_redirects for cross-function references (skipped continuations). -/// -/// WHY: Function-local block IDs may collide with skipped_entry_redirects (global IDs). +/// Delegates block remapping to block_remapper::remap_block_id for consistent priority rule. pub(in crate::mir::builder::control_flow::joinir::merge) fn remap_jump( remapper: &JoinIrIdRemapper, target: BasicBlockId, @@ -43,11 +42,8 @@ pub(in crate::mir::builder::control_flow::joinir::merge) fn remap_jump( skipped_entry_redirects: &BTreeMap, local_block_map: &BTreeMap, ) -> MirInstruction { - let remapped_target = local_block_map - .get(&target) - .or_else(|| skipped_entry_redirects.get(&target)) - .copied() - .unwrap_or(target); + // Phase 284 P1: Use SSOT for block remapping + let remapped_target = remap_block_id(target, local_block_map, skipped_entry_redirects); MirInstruction::Jump { target: remapped_target, @@ -60,12 +56,9 @@ pub(in crate::mir::builder::control_flow::joinir::merge) fn remap_jump( /// Applies block ID remapping (local_block_map + skipped_entry_redirects) for /// both then/else branches, condition ValueId remapping, and edge_args remapping. /// -/// # Phase 284 P1 FIX +/// # Phase 284 P1: Use block_remapper SSOT /// -/// Checks local_block_map FIRST for function-local blocks. -/// Fallback to skipped_entry_redirects for cross-function references. -/// -/// WHY: Function-local block IDs may collide with skipped_entry_redirects (global IDs). +/// Delegates block remapping to block_remapper::remap_block_id for consistent priority rule. pub(in crate::mir::builder::control_flow::joinir::merge) fn remap_branch( remapper: &JoinIrIdRemapper, condition: ValueId, @@ -76,17 +69,9 @@ pub(in crate::mir::builder::control_flow::joinir::merge) fn remap_branch( skipped_entry_redirects: &BTreeMap, local_block_map: &BTreeMap, ) -> MirInstruction { - let remapped_then = local_block_map - .get(&then_bb) - .or_else(|| skipped_entry_redirects.get(&then_bb)) - .copied() - .unwrap_or(then_bb); - - let remapped_else = local_block_map - .get(&else_bb) - .or_else(|| skipped_entry_redirects.get(&else_bb)) - .copied() - .unwrap_or(else_bb); + // Phase 284 P1: Use SSOT for block remapping + let remapped_then = remap_block_id(then_bb, local_block_map, skipped_entry_redirects); + let remapped_else = remap_block_id(else_bb, local_block_map, skipped_entry_redirects); MirInstruction::Branch { condition: remapper.remap_value(condition), diff --git a/src/mir/join_ir/lowering/loop_with_continue_minimal.rs b/src/mir/join_ir/lowering/loop_with_continue_minimal.rs index c1f443ae..9e1e94b2 100644 --- a/src/mir/join_ir/lowering/loop_with_continue_minimal.rs +++ b/src/mir/join_ir/lowering/loop_with_continue_minimal.rs @@ -69,6 +69,7 @@ use crate::mir::join_ir::lowering::loop_body_local_env::LoopBodyLocalEnv; // Pha use crate::mir::join_ir::lowering::loop_scope_shape::LoopScopeShape; use crate::mir::join_ir::lowering::loop_update_analyzer::{UpdateExpr, UpdateRhs}; use crate::mir::join_ir::lowering::return_collector::ReturnInfo; // Phase 284 P1 +use crate::mir::join_ir::lowering::return_jump_emitter::emit_return_conditional_jump; // Phase 284 P1 use crate::mir::join_ir::{ BinOpKind, CompareOp, ConstValue, JoinFuncId, JoinFunction, JoinInst, JoinModule, MirLikeInst, UnaryOp, @@ -432,102 +433,17 @@ pub(crate) fn lower_loop_with_continue_minimal( // ------------------------------------------------------------------ // For fixture: if (i == 3) { return 7 } // Condition is checked against i_next (after increment) - if let Some(ret_info) = return_info { - debug.log( - "phase284", - &format!( - "Generating return handling: value={}, has_condition={}", - ret_info.value, - ret_info.condition.is_some() - ), - ); - - if let Some(ref cond_ast) = ret_info.condition { - // Return is inside an if block - need to evaluate condition - // Phase 284 P1: Only support simple comparison (i == N) - // Extract the comparison value from the condition AST - if let ASTNode::BinaryOp { - operator: crate::ast::BinaryOperator::Equal, - left, - right, - .. - } = cond_ast.as_ref() - { - // Check if left is loop variable and right is integer - let compare_value = match (left.as_ref(), right.as_ref()) { - ( - ASTNode::Variable { name, .. }, - ASTNode::Literal { - value: crate::ast::LiteralValue::Integer(n), - .. - }, - ) if name == &loop_var_name => Some(*n), - _ => None, - }; - - if let Some(cmp_val) = compare_value { - // Generate: return_cond = (i_next == cmp_val) - let return_cmp_const = alloc_value(); - let return_cond = alloc_value(); - - loop_step_func - .body - .push(JoinInst::Compute(MirLikeInst::Const { - dst: return_cmp_const, - value: ConstValue::Integer(cmp_val), - })); - - // Phase 284 P1: Use i_next because fixture has increment BEFORE the if check - // Execution: i_next = i_param + 1, then check if i_next == 3 - loop_step_func - .body - .push(JoinInst::Compute(MirLikeInst::Compare { - dst: return_cond, - op: if ret_info.in_else { - CompareOp::Ne - } else { - CompareOp::Eq - }, - lhs: i_next, // Use i_next (post-increment value) - rhs: return_cmp_const, - })); - - // Generate: Jump(k_return, [], cond=return_cond) - loop_step_func.body.push(JoinInst::Jump { - cont: k_return_id.as_cont(), - args: vec![], - cond: Some(return_cond), - }); - - debug.log( - "phase284", - &format!( - "Return condition: i_next == {} → jump to k_return", - cmp_val - ), - ); - } else { - debug.log( - "phase284", - "Return condition not supported (not loop_var == N pattern)", - ); - } - } else { - debug.log( - "phase284", - "Return condition not supported (not Equal comparison)", - ); - } - } else { - // Unconditional return - always jump to k_return - loop_step_func.body.push(JoinInst::Jump { - cont: k_return_id.as_cont(), - args: vec![], - cond: None, - }); - debug.log("phase284", "Unconditional return → jump to k_return"); - } - } + // + // Phase 284 P1 Refactor: Use emit_return_conditional_jump SSOT + emit_return_conditional_jump( + &mut loop_step_func, + return_info, + k_return_id, + i_next, + &loop_var_name, + &mut alloc_value, + &debug, + )?; // ------------------------------------------------------------------ // Continue Condition Check: (i_next % 2 == 0) diff --git a/src/mir/join_ir/lowering/mod.rs b/src/mir/join_ir/lowering/mod.rs index eec45ff3..ea30b77a 100644 --- a/src/mir/join_ir/lowering/mod.rs +++ b/src/mir/join_ir/lowering/mod.rs @@ -66,6 +66,7 @@ pub(crate) mod loop_view_builder; // Phase 33-23: Loop lowering dispatch pub mod loop_with_break_minimal; // Phase 188-Impl-2: Pattern 2 minimal lowerer pub mod loop_with_continue_minimal; pub(crate) mod return_collector; // Phase 284 P1: Return statement collector SSOT +pub(crate) mod return_jump_emitter; // Phase 284 P1: Return jump emission helper (Pattern4/5 reuse) pub mod method_call_lowerer; // Phase 224-B: MethodCall lowering (metadata-driven) pub mod user_method_policy; // Phase 252: User-defined method policy (SSOT for static box method whitelists) pub mod method_return_hint; // Phase 83: P3-D 既知メソッド戻り値型推論箱 diff --git a/src/mir/join_ir/lowering/return_jump_emitter.rs b/src/mir/join_ir/lowering/return_jump_emitter.rs new file mode 100644 index 00000000..66e2e20f --- /dev/null +++ b/src/mir/join_ir/lowering/return_jump_emitter.rs @@ -0,0 +1,354 @@ +//! Return Jump Emitter +//! +//! Phase 284 P1: Extracted from loop_with_continue_minimal.rs (lines 430-530) +//! +//! This module provides a reusable helper for emitting conditional Jump to k_return +//! in JoinIR loop patterns (Pattern4/5 and future patterns). +//! +//! ## Design Rationale +//! +//! Pattern4/5 both need to handle early return statements in loop bodies: +//! ```nyash +//! loop(i < 10) { +//! i = i + 1 +//! if (i == 3) { +//! return 7 // Early return +//! } +//! // ... loop body continues +//! } +//! ``` +//! +//! The JoinIR representation emits a conditional Jump to k_return: +//! ```text +//! return_cond = (i_next == 3) +//! Jump(k_return, [], cond=return_cond) +//! ``` +//! +//! This helper consolidates the code generation logic into a single SSOT, +//! preventing duplication across multiple pattern lowerers. +//! +//! ## Usage +//! +//! ```rust,ignore +//! use crate::mir::join_ir::lowering::return_jump_emitter::emit_return_conditional_jump; +//! +//! if let Some(ret_info) = return_info { +//! emit_return_conditional_jump( +//! &mut loop_step_func, +//! &Some(ret_info), +//! k_return_id, +//! &mut alloc_value, +//! &debug, +//! )?; +//! } +//! ``` + +use crate::ast::ASTNode; +use crate::mir::join_ir::lowering::debug_output_box::DebugOutputBox; +use crate::mir::join_ir::lowering::return_collector::ReturnInfo; +use crate::mir::join_ir::{CompareOp, ConstValue, JoinFuncId, JoinFunction, JoinInst, MirLikeInst}; +use crate::mir::ValueId; + +/// Emit conditional Jump to k_return +/// +/// Generates JoinIR instructions for early return handling in loop bodies. +/// +/// # Arguments +/// +/// * `loop_step_func` - The loop step function (body) to append instructions to +/// * `return_info` - Return statement metadata (value, condition, in_else flag) +/// * `k_return_id` - The k_return continuation function ID +/// * `loop_var_id` - ValueId of the loop variable to check (typically i_next after increment) +/// * `loop_var_name` - Name of the loop variable (for condition validation) +/// * `alloc_value` - Value ID allocator closure +/// * `debug` - Debug output box for logging +/// +/// # Returns +/// +/// * `Ok(())` - Instructions emitted successfully +/// * `Err(msg)` - Unsupported return pattern (Fail-Fast) +/// +/// # Phase 284 P1 Scope +/// +/// - Single return statement only +/// - Return condition: Simple comparison (loop_var == N) +/// - Return value: Integer literal +/// - Supports then/else branches (via `in_else` flag) +/// +/// # Implementation Notes +/// +/// The generated instructions follow this pattern: +/// +/// **Unconditional return:** +/// ```text +/// Jump(k_return, []) +/// ``` +/// +/// **Conditional return (then branch):** +/// ```text +/// const_cmp = Const(3) +/// return_cond = Compare(Eq, i_next, const_cmp) +/// Jump(k_return, [], cond=return_cond) +/// ``` +/// +/// **Conditional return (else branch):** +/// ```text +/// const_cmp = Const(3) +/// return_cond = Compare(Ne, i_next, const_cmp) // Note: Ne (negated) +/// Jump(k_return, [], cond=return_cond) +/// ``` +/// +/// # Future Extensions (Pattern4/5 reuse) +/// +/// This function is designed to be reusable across: +/// - Pattern4 (loop with continue) +/// - Pattern5 (infinite loop with early exit) +/// - Future patterns with early return support +/// +/// When extending, consider: +/// - Complex condition support (beyond loop_var == N) +/// - Multiple return statements (Phase 284 P2+) +/// - Return value expressions (beyond integer literals) +pub fn emit_return_conditional_jump( + loop_step_func: &mut JoinFunction, + return_info: Option<&ReturnInfo>, + k_return_id: JoinFuncId, + loop_var_id: ValueId, + loop_var_name: &str, + alloc_value: &mut dyn FnMut() -> ValueId, + debug: &DebugOutputBox, +) -> Result<(), String> { + let Some(ret_info) = return_info else { + // No return statement - nothing to emit + return Ok(()); + }; + + debug.log( + "phase284", + &format!( + "Generating return handling: value={}, has_condition={}", + ret_info.value, + ret_info.condition.is_some() + ), + ); + + if let Some(ref cond_ast) = ret_info.condition { + // Return is inside an if block - need to evaluate condition + // Phase 284 P1: Only support simple comparison (loop_var == N) + // Extract the comparison value from the condition AST + if let ASTNode::BinaryOp { + operator: crate::ast::BinaryOperator::Equal, + left, + right, + .. + } = cond_ast.as_ref() + { + // Check if left is variable and right is integer + let compare_value = match (left.as_ref(), right.as_ref()) { + ( + ASTNode::Variable { name, .. }, + ASTNode::Literal { + value: crate::ast::LiteralValue::Integer(n), + .. + }, + ) if name == loop_var_name => Some(*n), + _ => None, + }; + + if let Some(cmp_val) = compare_value { + // Generate: return_cond = (i_next == cmp_val) + let return_cmp_const = alloc_value(); + let return_cond = alloc_value(); + + loop_step_func + .body + .push(JoinInst::Compute(MirLikeInst::Const { + dst: return_cmp_const, + value: ConstValue::Integer(cmp_val), + })); + + // Phase 284 P1: Use loop_var_id (i_next) because fixture has increment BEFORE the if check + // Execution: i_next = i_param + 1, then check if i_next == compare_value + loop_step_func + .body + .push(JoinInst::Compute(MirLikeInst::Compare { + dst: return_cond, + op: if ret_info.in_else { + CompareOp::Ne + } else { + CompareOp::Eq + }, + lhs: loop_var_id, // Use loop_var_id (post-increment value) + rhs: return_cmp_const, + })); + + // Generate: Jump(k_return, [], cond=return_cond) + loop_step_func.body.push(JoinInst::Jump { + cont: k_return_id.as_cont(), + args: vec![], + cond: Some(return_cond), + }); + + debug.log( + "phase284", + &format!("Return condition: {} == {} → jump to k_return", loop_var_name, cmp_val), + ); + Ok(()) + } else { + debug.log( + "phase284", + "Return condition not supported (not loop_var == N pattern)", + ); + Err(format!( + "Phase 284 P1 scope: return condition variable must match loop variable '{}' (got: {:?})", + loop_var_name, left + )) + } + } else { + Err( + "Phase 284 P1 scope: return condition must be Equal comparison".to_string(), + ) + } + } else { + // Unconditional return - always jump to k_return + loop_step_func.body.push(JoinInst::Jump { + cont: k_return_id.as_cont(), + args: vec![], + cond: None, + }); + debug.log("phase284", "Unconditional return → jump to k_return"); + Ok(()) + } +} + +// ============================================================================ +// Unit Tests +// ============================================================================ + +#[cfg(test)] +mod tests { + use super::*; + use crate::ast::Span; + + fn make_debug() -> DebugOutputBox { + DebugOutputBox::new_dev("test") + } + + #[test] + fn test_unconditional_return() { + let mut func = JoinFunction::new("test".to_string(), vec![]); + let info = ReturnInfo { + value: 42, + condition: None, + in_else: false, + }; + let return_info = Some(&info); + let k_return_id = JoinFuncId(99); + let loop_var_id = ValueId(100); + let mut counter = 0u32; + let mut alloc = || { + counter += 1; + ValueId(1000 + counter) + }; + let debug = make_debug(); + + let result = emit_return_conditional_jump( + &mut func, + return_info, + k_return_id, + loop_var_id, + "i", + &mut alloc, + &debug, + ); + + assert!(result.is_ok()); + assert_eq!(func.body.len(), 1); + match &func.body[0] { + JoinInst::Jump { cont, args, cond } => { + assert_eq!(*cont, k_return_id.as_cont()); + assert_eq!(args.len(), 0); + assert!(cond.is_none()); + } + _ => panic!("Expected Jump instruction"), + } + } + + #[test] + fn test_no_return() { + let mut func = JoinFunction::new("test".to_string(), vec![]); + let return_info: Option<&ReturnInfo> = None; + let k_return_id = JoinFuncId(99); + let loop_var_id = ValueId(100); + let mut counter = 0u32; + let mut alloc = || { + counter += 1; + ValueId(1000 + counter) + }; + let debug = make_debug(); + + let result = emit_return_conditional_jump( + &mut func, + return_info, + k_return_id, + loop_var_id, + "i", + &mut alloc, + &debug, + ); + + assert!(result.is_ok()); + assert_eq!(func.body.len(), 0); // No instructions emitted + } + + #[test] + fn test_conditional_return_success() { + // Phase 284 P1: Test successful conditional return generation + let mut func = JoinFunction::new("test".to_string(), vec![]); + let condition = Box::new(ASTNode::BinaryOp { + operator: crate::ast::BinaryOperator::Equal, + left: Box::new(ASTNode::Variable { + name: "i".to_string(), + span: Span::unknown(), + }), + right: Box::new(ASTNode::Literal { + value: crate::ast::LiteralValue::Integer(3), + span: Span::unknown(), + }), + span: Span::unknown(), + }); + let info = ReturnInfo { + value: 7, + condition: Some(condition), + in_else: false, + }; + let return_info = Some(&info); + let k_return_id = JoinFuncId(99); + let loop_var_id = ValueId(100); // i_next + let mut counter = 0u32; + let mut alloc = || { + counter += 1; + ValueId(1000 + counter) + }; + let debug = make_debug(); + + let result = emit_return_conditional_jump( + &mut func, + return_info, + k_return_id, + loop_var_id, + "i", + &mut alloc, + &debug, + ); + + // Should succeed + assert!(result.is_ok(), "Failed: {:?}", result.err()); + + // Check generated instructions + // 1. Const(3) + // 2. Compare(Eq, loop_var_id, const) + // 3. Jump(k_return, [], cond) + assert_eq!(func.body.len(), 3); + } +}