99d329096f
Task 176-1: Pattern2 limitation investigation - Identified 10 limitation points where only position carrier was handled - Added TODO markers for Phase 176-2/3 implementation - Created phase176-pattern2-limitations.md documentation Task 176-2: CarrierUpdateLowerer helper implementation - Implemented emit_carrier_update() helper function - Supports CounterLike and AccumulationLike UpdateExpr patterns - Added 6 unit tests (all passing) - Fail-Fast error handling for carrier/variable not found Task 176-3: Pattern2 lowerer multi-carrier extension - Extended header PHI generation for all carriers - Implemented loop update for all carriers using emit_carrier_update() - Extended ExitLine/ExitMeta construction for all carriers - Updated function call/jump args to include all carriers - 9/10 tests passing (1 pre-existing test issue) Task 176-4: E2E testing and bug fixes - Fixed Trim pattern loop_var_name overwrite bug (pattern2_with_break.rs) - Fixed InstructionRewriter latch_incoming mapping bug - All E2E tests passing (RC=0): pos + result dual-carrier loops work - test_jsonparser_parse_string_min2.hako verified Task 176-5: Documentation updates - Created phase176-completion-report.md - Updated phase175-multicarrier-design.md with completion status - Updated joinir-architecture-overview.md roadmap - Updated CURRENT_TASK.md with Phase 176 completion + Phase 177 TODO - Updated loop_pattern_space.md F-axis (multi-carrier support complete) Technical achievements: - Pattern2 now handles single/multiple carriers uniformly - CarrierInfo architecture proven to work end-to-end - Two critical bugs fixed (loop_var overwrite, latch_incoming mapping) - No regressions in existing tests Next: Phase 177 - Apply to JsonParser _parse_string full implementation
117 lines
4.4 KiB
Markdown
117 lines
4.4 KiB
Markdown
# Task 176-4: Trim Pattern Loop Variable Overwrite Bug Fix
|
|
|
|
**Status**: ✅ COMPLETED
|
|
|
|
**Date**: 2025-12-08
|
|
|
|
## Problem
|
|
|
|
In `src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs`, lines 271-272 were incorrectly overwriting the loop variable information during Trim pattern processing:
|
|
|
|
```rust
|
|
// ❌ Incorrect code (lines 271-272)
|
|
carrier_info.loop_var_id = is_ch_match0;
|
|
carrier_info.loop_var_name = carrier_name.clone(); // Overwrites "pos" with "is_ch_match"
|
|
```
|
|
|
|
### Root Cause
|
|
|
|
The Trim pattern implementation (Phase 171-172) was confusing two distinct concepts:
|
|
|
|
1. **Loop variable**: The actual loop counter (e.g., `pos`) that increments through the string
|
|
2. **Promoted carrier**: The derived variable (e.g., `is_ch_match`) that tracks whether the current character matches whitespace
|
|
|
|
The code was incorrectly treating the promoted carrier as a replacement for the loop variable, when it should only be an additional carrier alongside the loop variable.
|
|
|
|
### Impact
|
|
|
|
- **ExitMeta generation**: Would use wrong variable name ("is_ch_match" instead of "pos")
|
|
- **PHI node construction**: Would create PHI nodes for the wrong variable
|
|
- **Variable mapping**: Would lose track of the actual loop counter
|
|
|
|
## Solution
|
|
|
|
Removed lines 271-272 and replaced with explanatory comment:
|
|
|
|
```rust
|
|
// Note: DO NOT overwrite carrier_info.loop_var_id/loop_var_name here!
|
|
// The loop variable is 'pos' (counter), not 'is_ch_match' (carrier).
|
|
// carrier_info.loop_var_name should remain as the original loop variable.
|
|
|
|
eprintln!("[pattern2/trim] Carrier registered. Loop var='{}' remains unchanged",
|
|
carrier_info.loop_var_name);
|
|
```
|
|
|
|
### Design Principle
|
|
|
|
In Trim patterns:
|
|
- `loop_var_name` = "pos" (the counter variable)
|
|
- `carriers` = ["result", "is_ch_match"] (accumulated values)
|
|
- `is_ch_match` is a **promoted carrier**, not a loop variable replacement
|
|
|
|
## Verification
|
|
|
|
### 1. Build Status
|
|
✅ Build succeeded with no errors
|
|
|
|
### 2. Log Output Verification
|
|
Both E2E tests show correct behavior:
|
|
|
|
```
|
|
[pattern2/promoter] Phase 171-C-4 DEBUG: BEFORE merge - carrier_info.loop_var_name='pos'
|
|
[pattern2/promoter] Phase 171-C-4 DEBUG: promoted_carrier.loop_var_name='is_ch_match'
|
|
[pattern2/promoter] Phase 171-C-4 DEBUG: AFTER merge - carrier_info.loop_var_name='pos' ← CORRECT!
|
|
[pattern2/trim] Carrier registered. Loop var='pos' remains unchanged ← NEW LOG MESSAGE
|
|
[pattern2/before_lowerer] About to call lower_loop_with_break_minimal with carrier_info.loop_var_name='pos'
|
|
```
|
|
|
|
**Key observation**: The loop_var_name correctly remains as 'pos' throughout the entire process.
|
|
|
|
### 3. Regression Tests
|
|
✅ All carrier update tests pass (6/6):
|
|
- `test_emit_binop_update_with_const`
|
|
- `test_emit_binop_update_with_variable`
|
|
- `test_emit_const_update`
|
|
- `test_emit_update_lhs_mismatch`
|
|
- `test_emit_update_carrier_not_in_env`
|
|
- `test_emit_update_rhs_variable_not_found`
|
|
|
|
❌ One pre-existing test failure (unrelated to this fix):
|
|
- `test_pattern2_accepts_loop_param_only` - Failed on both HEAD and with our changes
|
|
|
|
### 4. E2E Test Results
|
|
|
|
**test_jsonparser_parse_string_min2.hako**:
|
|
- ✅ Loop variable name correctly preserved as 'pos'
|
|
- ⚠️ Execution fails with: "Phase 33-16: Carrier 'result' has no latch incoming set"
|
|
- **Note**: This is a separate issue unrelated to our fix (carrier PHI initialization)
|
|
|
|
**test_trim_simple.hako** (new test):
|
|
- ✅ Loop variable name correctly preserved as 'pos'
|
|
- ⚠️ Same carrier PHI error as above
|
|
|
|
## Remaining Issues (Out of Scope)
|
|
|
|
The following error is NOT caused by this fix and exists independently:
|
|
```
|
|
[ERROR] ❌ MIR compilation error: Phase 33-16: Carrier 'result' has no latch incoming set
|
|
```
|
|
|
|
This appears to be related to PHI node initialization for carriers in Pattern 2, and will need to be addressed separately.
|
|
|
|
## Files Changed
|
|
|
|
1. `src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs`
|
|
- Lines 271-272: Removed incorrect loop_var overwrite
|
|
- Lines 270-275: Added explanatory comment and new debug log
|
|
|
|
## Conclusion
|
|
|
|
✅ **Bug successfully fixed**
|
|
- Loop variable name no longer gets overwritten by carrier name
|
|
- Design clarification: promoted carriers are additions, not replacements
|
|
- No regressions introduced in carrier update logic
|
|
- Remaining errors are pre-existing and unrelated to this fix
|
|
|
|
The fix correctly implements the design intention: in Trim patterns, `is_ch_match` should be an additional carrier alongside the original loop variable `pos`, not a replacement for it.
|