274 lines
9.7 KiB
Markdown
274 lines
9.7 KiB
Markdown
|
|
# Phase 225: LoopBodyLocalInit MethodCall Meta-Driven Lowering - Complete Summary
|
||
|
|
|
||
|
|
## Overview
|
||
|
|
|
||
|
|
Phase 225 successfully eliminated ALL hardcoding from `loop_body_local_init.rs` by delegating MethodCall lowering to `MethodCallLowerer` and using `CoreMethodId` metadata exclusively.
|
||
|
|
|
||
|
|
## Problem Statement
|
||
|
|
|
||
|
|
Phase 193 introduced MethodCall support in body-local init expressions (e.g., `local digit_pos = digits.indexOf(ch)`), but the implementation contained hardcoded method names and box name mappings:
|
||
|
|
|
||
|
|
```rust
|
||
|
|
// Line 387: Hardcoded whitelist
|
||
|
|
const SUPPORTED_INIT_METHODS: &[&str] = &["indexOf", "get", "toString"];
|
||
|
|
|
||
|
|
// Lines 433-438: Hardcoded box name mapping
|
||
|
|
let box_name = match method {
|
||
|
|
"indexOf" => "StringBox".to_string(),
|
||
|
|
"get" => "ArrayBox".to_string(),
|
||
|
|
"toString" => "IntegerBox".to_string(),
|
||
|
|
_ => unreachable!("Whitelist check should have caught this"),
|
||
|
|
};
|
||
|
|
```
|
||
|
|
|
||
|
|
This caused errors like:
|
||
|
|
```
|
||
|
|
Method 'substring' not supported in body-local init (Phase 193 limitation - only indexOf, get, toString supported)
|
||
|
|
```
|
||
|
|
|
||
|
|
## Solution: Meta-Driven Architecture
|
||
|
|
|
||
|
|
### Architecture Change
|
||
|
|
|
||
|
|
**Before (Phase 193 - Hardcoded)**:
|
||
|
|
```
|
||
|
|
LoopBodyLocalInitLowerer
|
||
|
|
└─ emit_method_call_init (static method)
|
||
|
|
├─ SUPPORTED_INIT_METHODS whitelist ❌
|
||
|
|
├─ match method { "indexOf" => "StringBox" } ❌
|
||
|
|
└─ Emit BoxCall instruction
|
||
|
|
```
|
||
|
|
|
||
|
|
**After (Phase 225 - Meta-Driven)**:
|
||
|
|
```
|
||
|
|
LoopBodyLocalInitLowerer
|
||
|
|
└─ emit_method_call_init (static method)
|
||
|
|
└─ Delegates to MethodCallLowerer::lower_for_init
|
||
|
|
├─ Resolve method_name → CoreMethodId ✅
|
||
|
|
├─ Check allowed_in_init() ✅
|
||
|
|
├─ Get box_name from CoreMethodId ✅
|
||
|
|
├─ Check arity ✅
|
||
|
|
└─ Emit BoxCall instruction
|
||
|
|
```
|
||
|
|
|
||
|
|
### Key Changes
|
||
|
|
|
||
|
|
1. **Deleted hardcoded whitelist** (`SUPPORTED_INIT_METHODS` constant)
|
||
|
|
2. **Deleted hardcoded box name match** (`indexOf → StringBox` mapping)
|
||
|
|
3. **Delegated to MethodCallLowerer** (single responsibility principle)
|
||
|
|
4. **All decisions driven by `CoreMethodId` metadata** (SSOT)
|
||
|
|
|
||
|
|
## Implementation Details
|
||
|
|
|
||
|
|
### Files Modified
|
||
|
|
|
||
|
|
1. **`src/mir/join_ir/lowering/loop_body_local_init.rs`** (main refactoring)
|
||
|
|
- Import `MethodCallLowerer`
|
||
|
|
- Refactor `emit_method_call_init` to delegate
|
||
|
|
- Delete `lower_init_arg` helper (no longer needed)
|
||
|
|
- Update module documentation
|
||
|
|
- **Net change**: -82 lines (158 deleted - 76 added)
|
||
|
|
|
||
|
|
2. **`src/mir/builder/control_flow/joinir/patterns/pattern_pipeline.rs`** (test fixes)
|
||
|
|
- Add `condition_aliases: Vec::new()` to CarrierInfo test structs (2 occurrences)
|
||
|
|
|
||
|
|
3. **`src/mir/builder/control_flow/joinir/patterns/pattern4_carrier_analyzer.rs`** (test fixes)
|
||
|
|
- Add `condition_aliases: Vec::new()` to CarrierInfo test struct (1 occurrence)
|
||
|
|
|
||
|
|
4. **`docs/development/current/main/joinir-architecture-overview.md`** (documentation)
|
||
|
|
- Added Phase 225 section to LoopBodyLocal init history
|
||
|
|
|
||
|
|
5. **`CURRENT_TASK.md`** (status update)
|
||
|
|
- Added Phase 225 completion summary
|
||
|
|
|
||
|
|
### CoreMethodId Metadata
|
||
|
|
|
||
|
|
The `allowed_in_init()` method already included the methods we needed:
|
||
|
|
|
||
|
|
```rust
|
||
|
|
pub fn allowed_in_init(&self) -> bool {
|
||
|
|
use CoreMethodId::*;
|
||
|
|
match self {
|
||
|
|
// String operations - allowed
|
||
|
|
StringLength | StringSubstring | StringIndexOf => true, // ✅
|
||
|
|
|
||
|
|
// String transformations - allowed for init
|
||
|
|
StringUpper | StringLower | StringTrim => true,
|
||
|
|
|
||
|
|
// Array operations - allowed
|
||
|
|
ArrayLength | ArrayGet => true,
|
||
|
|
|
||
|
|
// Map operations - allowed
|
||
|
|
MapGet | MapHas | MapKeys => true,
|
||
|
|
|
||
|
|
// ...
|
||
|
|
}
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
No changes were needed to metadata - it was already correct!
|
||
|
|
|
||
|
|
## Results
|
||
|
|
|
||
|
|
### Code Quality
|
||
|
|
|
||
|
|
- **-82 net lines** (158 deletions - 76 additions)
|
||
|
|
- **Zero hardcoded method names** (all resolved via `CoreMethodId::iter()`)
|
||
|
|
- **Zero hardcoded box names** (all from `method_id.box_id().name()`)
|
||
|
|
- **Single Responsibility**: MethodCallLowerer is the only place that handles MethodCall → JoinIR
|
||
|
|
|
||
|
|
### Test Results
|
||
|
|
|
||
|
|
#### Unit Tests
|
||
|
|
- **MethodCallLowerer**: 8/8 tests PASS
|
||
|
|
- **loop_body_local_init**: 3/3 tests PASS
|
||
|
|
|
||
|
|
#### Integration Tests
|
||
|
|
- **877/884 tests PASS** (99.2% pass rate)
|
||
|
|
- **7 failures**: Pre-existing Pattern 3 accumulator variable issues (not related to Phase 225)
|
||
|
|
|
||
|
|
#### E2E Verification
|
||
|
|
- **substring now works in body-local init** ✅
|
||
|
|
- Simple test case: `local ch = s.substring(p, p+1)` compiles without error
|
||
|
|
- Complex test case: `phase2235_p2_digit_pos_min.hako` progresses past substring error to cascading dependency issue (which is a pre-existing limitation from Phase 193)
|
||
|
|
|
||
|
|
### New Capabilities
|
||
|
|
|
||
|
|
- **substring** method now usable in loop body-local init
|
||
|
|
- **Any method with `allowed_in_init() == true`** automatically supported
|
||
|
|
- **Easy to extend**: Add method to `CoreMethodId`, set `allowed_in_init()`, done!
|
||
|
|
|
||
|
|
## Known Limitations
|
||
|
|
|
||
|
|
### Cascading LoopBodyLocal Dependencies
|
||
|
|
|
||
|
|
The test `apps/tests/phase2235_p2_digit_pos_min.hako` reveals a **pre-existing limitation** (not introduced by Phase 225):
|
||
|
|
|
||
|
|
```nyash
|
||
|
|
loop(p < s.length()) {
|
||
|
|
local ch = s.substring(p, p+1) // ✅ Now works (Phase 225)
|
||
|
|
local digit_pos = digits.indexOf(ch) // ❌ Error: 'ch' not found in ConditionEnv
|
||
|
|
...
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**Root cause**: When lowering `digit_pos = digits.indexOf(ch)`, the argument `ch` is looked up in `ConditionEnv` only. However, `ch` is a LoopBodyLocal variable, so it should be looked up in `LoopBodyLocalEnv`.
|
||
|
|
|
||
|
|
**Status**: This limitation existed in Phase 193 - the original `lower_init_arg` also only checked `ConditionEnv`.
|
||
|
|
|
||
|
|
**Future work**: Phase 226+ could extend argument lowering to check `LoopBodyLocalEnv` for cascading dependencies.
|
||
|
|
|
||
|
|
## Architecture Benefits
|
||
|
|
|
||
|
|
### 1. Metadata-Driven
|
||
|
|
- **Single Source of Truth**: `CoreMethodId` defines all method metadata
|
||
|
|
- **No duplication**: Method name, box name, arity, whitelist all in one place
|
||
|
|
- **Easy to extend**: Add new methods by updating `CoreMethodId` only
|
||
|
|
|
||
|
|
### 2. Single Responsibility
|
||
|
|
- **MethodCallLowerer**: "MethodCall → JoinIR" conversion (Phase 224-B)
|
||
|
|
- **LoopBodyLocalInitLowerer**: Loop body-local init coordination (Phase 186)
|
||
|
|
- **Clear boundary**: Init lowerer delegates, doesn't duplicate logic
|
||
|
|
|
||
|
|
### 3. Fail-Fast
|
||
|
|
- **Unknown methods** → immediate error (not silent fallback)
|
||
|
|
- **Arity mismatch** → immediate error
|
||
|
|
- **Not whitelisted** → immediate error with clear message
|
||
|
|
|
||
|
|
### 4. Type Safety
|
||
|
|
- **No string matching** → use enum (`CoreMethodId`)
|
||
|
|
- **Compile-time checks** → catch errors early
|
||
|
|
- **Refactoring-safe** → rename detection
|
||
|
|
|
||
|
|
### 5. Maintainability
|
||
|
|
- **Add new method**: Update `CoreMethodId` only (one place)
|
||
|
|
- **Change whitelist**: Update `allowed_in_init()` only
|
||
|
|
- **No scattered hardcoding** across files
|
||
|
|
|
||
|
|
## Comparison: Phase 193 vs Phase 225
|
||
|
|
|
||
|
|
| Aspect | Phase 193 | Phase 225 |
|
||
|
|
|--------|-----------|-----------|
|
||
|
|
| Method whitelist | Hardcoded constant | CoreMethodId metadata |
|
||
|
|
| Box name mapping | Match statement | `method_id.box_id().name()` |
|
||
|
|
| Supported methods | 3 (indexOf, get, toString) | 15+ (all with `allowed_in_init() == true`) |
|
||
|
|
| Code lines | 158 | 76 (-82 lines) |
|
||
|
|
| Extensibility | Add to 2 places | Add to `CoreMethodId` only |
|
||
|
|
| Type safety | String matching | Enum-based |
|
||
|
|
| Single responsibility | Violated | Achieved |
|
||
|
|
|
||
|
|
## Future Work (Not in Phase 225)
|
||
|
|
|
||
|
|
### Phase 226+: Potential Improvements
|
||
|
|
|
||
|
|
1. **Cascading LoopBodyLocal support**
|
||
|
|
- Extend argument lowering to check `LoopBodyLocalEnv`
|
||
|
|
- Enable `ch` → `digit_pos` → condition chains
|
||
|
|
|
||
|
|
2. **Type inference**
|
||
|
|
- Use actual receiver type instead of heuristics
|
||
|
|
- More accurate box name resolution
|
||
|
|
|
||
|
|
3. **Custom method support**
|
||
|
|
- User-defined box methods in init expressions
|
||
|
|
|
||
|
|
4. **Optimization**
|
||
|
|
- Dead code elimination for unused method calls
|
||
|
|
- Common subexpression elimination
|
||
|
|
|
||
|
|
5. **Error messages**
|
||
|
|
- Better diagnostics with suggestions
|
||
|
|
- "Did you mean...?" for typos
|
||
|
|
|
||
|
|
## Commit Message
|
||
|
|
|
||
|
|
```
|
||
|
|
refactor(joinir): Phase 225 - LoopBodyLocalInit MethodCall meta-driven
|
||
|
|
|
||
|
|
Eliminate ALL hardcoding from loop_body_local_init.rs by delegating
|
||
|
|
to MethodCallLowerer and using CoreMethodId metadata exclusively.
|
||
|
|
|
||
|
|
Changes:
|
||
|
|
- Delete SUPPORTED_INIT_METHODS whitelist constant
|
||
|
|
- Delete hardcoded box name match (indexOf→StringBox, etc.)
|
||
|
|
- Delegate emit_method_call_init to MethodCallLowerer::lower_for_init
|
||
|
|
- Use CoreMethodId metadata for allowed_in_init() whitelist
|
||
|
|
- Delete lower_init_arg helper (no longer needed)
|
||
|
|
- Fix test structs to include condition_aliases field
|
||
|
|
|
||
|
|
Results:
|
||
|
|
- substring method now works in body-local init
|
||
|
|
- Net change: -82 lines (158 deleted - 76 added)
|
||
|
|
- 877/884 tests PASS (7 pre-existing P3 failures)
|
||
|
|
- Zero hardcoded method/box names remaining
|
||
|
|
|
||
|
|
Architecture:
|
||
|
|
- Single Source of Truth: CoreMethodId metadata
|
||
|
|
- Single Responsibility: MethodCallLowerer handles all MethodCall lowering
|
||
|
|
- Fail-Fast: Unknown methods → immediate error
|
||
|
|
- Type Safety: Enum-based instead of string matching
|
||
|
|
|
||
|
|
Phase 225 complete - meta-driven architecture achieved ✅
|
||
|
|
```
|
||
|
|
|
||
|
|
## Success Criteria ✅
|
||
|
|
|
||
|
|
All success criteria met:
|
||
|
|
|
||
|
|
1. ✅ `cargo build --release` succeeds
|
||
|
|
2. ✅ All unit tests in `method_call_lowerer.rs` pass (8/8)
|
||
|
|
3. ✅ All unit tests in `loop_body_local_init.rs` pass (3/3)
|
||
|
|
4. ✅ `substring` method now works in body-local init
|
||
|
|
5. ✅ **Zero hardcoded method names or box names** in `emit_method_call_init`
|
||
|
|
6. ✅ No regressions in existing tests (877/884 pass, 7 pre-existing failures)
|
||
|
|
|
||
|
|
## References
|
||
|
|
|
||
|
|
- **Design Document**: [phase225-bodylocal-init-methodcall-design.md](phase225-bodylocal-init-methodcall-design.md)
|
||
|
|
- **Phase 186**: Loop Body-Local Variable Initialization (initial implementation)
|
||
|
|
- **Phase 193**: MethodCall support in body-local init (hardcoded version)
|
||
|
|
- **Phase 224-B**: MethodCallLowerer Box creation (metadata-driven)
|
||
|
|
- **Phase 224-C**: MethodCallLowerer argument support
|
||
|
|
- **Phase 224-D**: ConditionAlias variable resolution
|
||
|
|
- **Architecture Overview**: [joinir-architecture-overview.md](joinir-architecture-overview.md)
|