diff --git a/src/mir/loop_pattern_detection/mutable_accumulator_analyzer.rs b/src/mir/loop_pattern_detection/mutable_accumulator_analyzer.rs index 252eac2f..e807a1f6 100644 --- a/src/mir/loop_pattern_detection/mutable_accumulator_analyzer.rs +++ b/src/mir/loop_pattern_detection/mutable_accumulator_analyzer.rs @@ -64,16 +64,21 @@ impl MutableAccumulatorAnalyzer { /// /// # Returns /// - /// * `Ok(Some(spec))` - Accumulator pattern detected - /// * `Ok(None)` - No accumulator pattern found - /// * `Err(String)` - Pattern violation (Fail-Fast) + /// * `Ok(Some(spec))` - Accumulator pattern detected (target = target + x) + /// * `Ok(None)` - No accumulator pattern found (other patterns may apply) + /// * `Err(String)` - Internal consistency error (should be rare) /// - /// # Fail-Fast Cases + /// # Detection Strategy (Phase 253) /// + /// Returns `Ok(None)` for non-accumulator patterns: /// - Multiple assignments to the same variable - /// - Assignment form not accumulator pattern (e.g., reversed operands) - /// - RHS contains function call (not supported) - /// - Operator not Add (other ops not supported yet) + /// - Operator not Add (e.g., i = i - 1) + /// - Left operand not matching target (e.g., x = y + x) + /// - RHS contains function call (e.g., x = x + f()) + /// - RHS is complex expression (e.g., x = x + (i + 1)) + /// - Value is not BinaryOp (e.g., i = s.length()) + /// + /// This allows other loop lowering patterns to handle non-accumulator cases. pub fn analyze(loop_body: &[ASTNode]) -> Result, String> { // Collect all assignments in loop body let assignments = collect_assignments(loop_body); @@ -109,12 +114,10 @@ impl MutableAccumulatorAnalyzer { .. } = value_node { - // Check operator is Add + // Phase 253: Check operator is Add (return Ok(None) if not) + // Only Add is supported as accumulator pattern if operator != &BinaryOperator::Add { - return Err(format!( - "[joinir/mutable-acc-spec] Operator '{}' not supported (only '+' allowed)", - operator - )); + return Ok(None); // Not our pattern (e.g., i = i - 1) } // Check left operand is Variable with same name as target @@ -149,39 +152,26 @@ impl MutableAccumulatorAnalyzer { })); } ASTNode::MethodCall { .. } | ASTNode::Call { .. } => { - return Err( - "[joinir/mutable-acc-spec] RHS contains function call (not supported)" - .to_string(), - ); + // Phase 253: RHS with function call is not accumulator pattern + return Ok(None); // Not our pattern (e.g., x = x + f()) } _ => { - return Err(format!( - "[joinir/mutable-acc-spec] RHS expression type not supported: {:?}", - right - )); + // Phase 253: Complex RHS expression is not accumulator pattern + return Ok(None); // Not our pattern (e.g., x = x + (i + 1)) } } } else { - // Left operand is different variable (reversed operands) - return Err( - "[joinir/mutable-acc-spec] Assignment form not accumulator pattern (required: target = target + x)" - .to_string(), - ); + // Phase 253: Left operand is different variable (reversed operands) + return Ok(None); // Not our pattern (e.g., x = y + x) } } else { - // Left operand is not Variable - return Err( - "[joinir/mutable-acc-spec] Assignment form not accumulator pattern (required: target = target + x)" - .to_string(), - ); + // Phase 253: Left operand is not Variable + return Ok(None); // Not our pattern } } else { - // Value is not BinaryOp, check if it's a potential mutation - // If the assignment is to a candidate variable, it's a non-accumulator mutation - return Err(format!( - "[joinir/mutable-acc-spec] Assignment to '{}' is not accumulator form (required: target = target + x)", - target_name - )); + // Phase 253: Value is not BinaryOp - not accumulator pattern + // e.g., i = s.length() - 1 (MethodCall result, not x = x + y) + return Ok(None); // Not our pattern } } @@ -278,7 +268,8 @@ mod tests { #[test] fn test_mutable_accumulator_spec_ng_reversed() { - // Build AST for: out = ch + out (reversed operands) + // Phase 253: Build AST for: out = ch + out (reversed operands) + // Expected: Ok(None) - not accumulator pattern let loop_body = vec![ASTNode::Assignment { target: Box::new(ASTNode::Variable { name: "out".to_string(), @@ -300,10 +291,8 @@ mod tests { }]; let result = MutableAccumulatorAnalyzer::analyze(&loop_body); - assert!(result.is_err()); - assert!(result - .unwrap_err() - .contains("Assignment form not accumulator pattern")); + assert!(result.is_ok()); + assert!(result.unwrap().is_none(), "Reversed operands should return None"); } #[test] @@ -358,7 +347,8 @@ mod tests { #[test] fn test_mutable_accumulator_spec_ng_method_call() { - // Build AST for: out = out + f() + // Phase 253: Build AST for: out = out + f() + // Expected: Ok(None) - RHS contains function call let loop_body = vec![ASTNode::Assignment { target: Box::new(ASTNode::Variable { name: "out".to_string(), @@ -385,10 +375,8 @@ mod tests { }]; let result = MutableAccumulatorAnalyzer::analyze(&loop_body); - assert!(result.is_err()); - assert!(result - .unwrap_err() - .contains("RHS contains function call (not supported)")); + assert!(result.is_ok()); + assert!(result.unwrap().is_none(), "RHS with function call should return None"); } #[test] @@ -512,4 +500,96 @@ mod tests { assert_eq!(spec.op, BinaryOperator::Add); assert_eq!(spec.kind, AccumulatorKind::Int); } + + #[test] + fn test_decrement_not_accumulator() { + // Phase 253: Build AST for: i = i - 1 + // Expected: Ok(None) - decrement is not accumulator pattern (only Add supported) + let loop_body = vec![ASTNode::Assignment { + target: Box::new(ASTNode::Variable { + name: "i".to_string(), + span: Span::unknown(), + }), + value: Box::new(ASTNode::BinaryOp { + operator: BinaryOperator::Subtract, + left: Box::new(ASTNode::Variable { + name: "i".to_string(), + span: Span::unknown(), + }), + right: Box::new(ASTNode::Literal { + value: LiteralValue::Integer(1), + span: Span::unknown(), + }), + span: Span::unknown(), + }), + span: Span::unknown(), + }]; + + let result = MutableAccumulatorAnalyzer::analyze(&loop_body); + assert!(result.is_ok()); + assert!(result.unwrap().is_none(), "Decrement (i = i - 1) should return None"); + } + + #[test] + fn test_complex_rhs_not_accumulator() { + // Phase 253: Build AST for: x = x + (i + 1) + // Expected: Ok(None) - complex RHS expression + let loop_body = vec![ASTNode::Assignment { + target: Box::new(ASTNode::Variable { + name: "x".to_string(), + span: Span::unknown(), + }), + value: Box::new(ASTNode::BinaryOp { + operator: BinaryOperator::Add, + left: Box::new(ASTNode::Variable { + name: "x".to_string(), + span: Span::unknown(), + }), + right: Box::new(ASTNode::BinaryOp { + operator: BinaryOperator::Add, + left: Box::new(ASTNode::Variable { + name: "i".to_string(), + span: Span::unknown(), + }), + right: Box::new(ASTNode::Literal { + value: LiteralValue::Integer(1), + span: Span::unknown(), + }), + span: Span::unknown(), + }), + span: Span::unknown(), + }), + span: Span::unknown(), + }]; + + let result = MutableAccumulatorAnalyzer::analyze(&loop_body); + assert!(result.is_ok()); + assert!(result.unwrap().is_none(), "Complex RHS (x = x + (i + 1)) should return None"); + } + + #[test] + fn test_non_binop_assignment_not_accumulator() { + // Phase 253: Build AST for: i = s.length() + // Expected: Ok(None) - not BinaryOp (MethodCall result) + let loop_body = vec![ASTNode::Assignment { + target: Box::new(ASTNode::Variable { + name: "i".to_string(), + span: Span::unknown(), + }), + value: Box::new(ASTNode::MethodCall { + object: Box::new(ASTNode::Variable { + name: "s".to_string(), + span: Span::unknown(), + }), + method: "length".to_string(), + arguments: vec![], + span: Span::unknown(), + }), + span: Span::unknown(), + }]; + + let result = MutableAccumulatorAnalyzer::analyze(&loop_body); + assert!(result.is_ok()); + assert!(result.unwrap().is_none(), "Non-BinaryOp assignment (i = s.length()) should return None"); + } }