From 09b968256ffaedad63c9895b4989c80b745efd42 Mon Sep 17 00:00:00 2001 From: tomoaki Date: Fri, 19 Dec 2025 20:38:57 +0900 Subject: [PATCH] feat(normalization): Phase 253 - mutable_accumulator_analyzer detector pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Core Change**: Refactor from "validator" to "detector" pattern **Before** (Fail-Fast): - Err for non-accumulator assignments (i = i - 1, i = s.length(), etc.) - JoinIR pipeline aborted on detection **After** (Detector): - Ok(None) for non-accumulator patterns (other lowering paths can handle) - Only Add operator supported (i = i + 1) - Other operators/patterns return None gracefully **Modified Behavior** (6 locations): 1. L112-116: operator != Add → Ok(None) 2. L149-152: RHS MethodCall/Call → Ok(None) 3. L153-156: RHS complex expression → Ok(None) 4. L158-161: Left operand reversed → Ok(None) 5. L162-165: Left operand not Variable → Ok(None) 6. L166-170: Value not BinaryOp → Ok(None) **Tests** (11/11 PASS): - 2 existing tests updated (Err → Ok(None)) - 3 new tests added: - test_decrement_not_accumulator (i = i - 1) - test_complex_rhs_not_accumulator (x = x + (i + 1)) - test_non_binop_assignment_not_accumulator (i = s.length()) **Documentation**: - Updated docstring to reflect detector strategy - Detection Strategy section added with all Ok(None) cases **Status**: - ✅ mutable-acc-spec errors eliminated - ⚠️ quick profile still FAIL: StringUtils.index_of/2 not JoinIR-supported (Phase 254 scope) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- .../mutable_accumulator_analyzer.rs | 172 +++++++++++++----- 1 file changed, 126 insertions(+), 46 deletions(-) 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"); + } }