refactor(joinir): Extract common carrier update validation to reduce Pattern2/4 duplication
## Summary - **130 lines of duplicate code eliminated** (66 from Pattern2 + 64 from Pattern4) - **40 lines net reduction** (130 duplicates - 90 new common code) - **New utility function**: `CommonPatternInitializer::check_carrier_updates_allowed()` ## Changes ### Added - `common_init.rs`: New `check_carrier_updates_allowed()` function (90 lines) - Validates carrier updates allow only simple expressions (Const, Variable, StringLiteral) - Rejects complex expressions (method calls, nested BinOp) - Shared by Pattern2 and Pattern4 ### Refactored - `pattern2_with_break.rs`: Simplified `can_lower()` (-66 lines) - Removed 60-line duplicate carrier validation logic - Now calls `CommonPatternInitializer::check_carrier_updates_allowed()` - `pattern4_with_continue.rs`: Simplified `can_lower()` (-64 lines) - Removed 60-line duplicate carrier validation logic - Now calls `CommonPatternInitializer::check_carrier_updates_allowed()` ## Benefits 1. **Single source of truth**: All Pattern2/4 carrier validation uses same logic 2. **Maintainability**: Updates to carrier validation only need to happen once 3. **Consistency**: Uniform error messages and validation behavior 4. **Testability**: Common function can be tested independently ## Tests ✅ All tests passing (788 passed, 0 failed, 64 ignored) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This commit is contained in:
@ -114,4 +114,94 @@ impl CommonPatternInitializer {
|
|||||||
|
|
||||||
Ok((loop_var_name, loop_var_id, carrier_info))
|
Ok((loop_var_name, loop_var_id, carrier_info))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Check if carrier updates are allowed in current pattern
|
||||||
|
///
|
||||||
|
/// Phase 188: Validates that all carrier updates use simple expressions.
|
||||||
|
///
|
||||||
|
/// Accepts:
|
||||||
|
/// - Const (numeric) - e.g., `i = i + 1`
|
||||||
|
/// - Variable (numeric/string) - e.g., `sum = sum + i`
|
||||||
|
/// - StringLiteral (Phase 188) - e.g., `s = s + "x"`
|
||||||
|
///
|
||||||
|
/// Rejects:
|
||||||
|
/// - Other (method calls, complex expressions) - e.g., `x = x + foo.bar()`
|
||||||
|
///
|
||||||
|
/// # Arguments
|
||||||
|
///
|
||||||
|
/// - `body`: Loop body AST nodes to analyze
|
||||||
|
/// - `loop_var_name`: Loop variable name (for creating dummy carriers)
|
||||||
|
/// - `variable_map`: Current variable mappings (for creating dummy carriers)
|
||||||
|
///
|
||||||
|
/// # Returns
|
||||||
|
///
|
||||||
|
/// - `true` if all carrier updates are allowed
|
||||||
|
/// - `false` if any update uses complex expressions (UpdateRhs::Other)
|
||||||
|
///
|
||||||
|
/// # Example
|
||||||
|
///
|
||||||
|
/// ```rust
|
||||||
|
/// if !CommonPatternInitializer::check_carrier_updates_allowed(
|
||||||
|
/// body,
|
||||||
|
/// &loop_var_name,
|
||||||
|
/// &builder.variable_map,
|
||||||
|
/// ) {
|
||||||
|
/// return false; // Pattern cannot lower - reject
|
||||||
|
/// }
|
||||||
|
/// ```
|
||||||
|
pub fn check_carrier_updates_allowed(
|
||||||
|
body: &[ASTNode],
|
||||||
|
loop_var_name: &str,
|
||||||
|
variable_map: &BTreeMap<String, ValueId>,
|
||||||
|
) -> bool {
|
||||||
|
use crate::mir::join_ir::lowering::loop_update_analyzer::{LoopUpdateAnalyzer, UpdateExpr, UpdateRhs};
|
||||||
|
use crate::mir::join_ir::lowering::carrier_info::CarrierVar;
|
||||||
|
|
||||||
|
// Create dummy carriers from body assignment targets for analysis
|
||||||
|
let dummy_carriers: Vec<CarrierVar> = body.iter().filter_map(|node| {
|
||||||
|
match node {
|
||||||
|
ASTNode::Assignment { target, .. } => {
|
||||||
|
if let ASTNode::Variable { name, .. } = target.as_ref() {
|
||||||
|
Some(CarrierVar {
|
||||||
|
name: name.clone(),
|
||||||
|
host_id: ValueId(0), // Dummy
|
||||||
|
join_id: None,
|
||||||
|
})
|
||||||
|
} else {
|
||||||
|
None
|
||||||
|
}
|
||||||
|
}
|
||||||
|
_ => None,
|
||||||
|
}
|
||||||
|
}).collect();
|
||||||
|
|
||||||
|
let updates = LoopUpdateAnalyzer::analyze_carrier_updates(body, &dummy_carriers);
|
||||||
|
|
||||||
|
// Phase 188: Check if any update is complex (reject only UpdateRhs::Other)
|
||||||
|
// Allow: Const (numeric), Variable (numeric/string), StringLiteral
|
||||||
|
// Reject: Other (method calls, nested BinOp)
|
||||||
|
for update in updates.values() {
|
||||||
|
if let UpdateExpr::BinOp { rhs, .. } = update {
|
||||||
|
match rhs {
|
||||||
|
UpdateRhs::Const(_) => {
|
||||||
|
// Numeric: i = i + 1 (allowed)
|
||||||
|
}
|
||||||
|
UpdateRhs::Variable(_) => {
|
||||||
|
// Phase 188: StringAppendChar: s = s + ch (allowed)
|
||||||
|
// Or numeric: sum = sum + i (allowed)
|
||||||
|
}
|
||||||
|
UpdateRhs::StringLiteral(_) => {
|
||||||
|
// Phase 188: StringAppendLiteral: s = s + "x" (allowed)
|
||||||
|
}
|
||||||
|
UpdateRhs::Other => {
|
||||||
|
// Phase 188: Complex update (method call, nested BinOp) - reject
|
||||||
|
eprintln!("[common_init/check_carriers] Phase 188: Complex update detected (UpdateRhs::Other), rejecting pattern");
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
true
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@ -46,65 +46,27 @@ fn collect_body_local_variables(
|
|||||||
/// Pattern 2 matches:
|
/// Pattern 2 matches:
|
||||||
/// - Pattern kind is Pattern2Break (has break, no continue)
|
/// - Pattern kind is Pattern2Break (has break, no continue)
|
||||||
/// - No string/complex carrier updates (JoinIR doesn't support string concat)
|
/// - No string/complex carrier updates (JoinIR doesn't support string concat)
|
||||||
pub fn can_lower(_builder: &MirBuilder, ctx: &super::router::LoopPatternContext) -> bool {
|
pub fn can_lower(builder: &MirBuilder, ctx: &super::router::LoopPatternContext) -> bool {
|
||||||
use crate::mir::loop_pattern_detection::LoopPatternKind;
|
use crate::mir::loop_pattern_detection::LoopPatternKind;
|
||||||
use crate::mir::join_ir::lowering::loop_update_analyzer::{LoopUpdateAnalyzer, UpdateExpr, UpdateRhs};
|
use super::common_init::CommonPatternInitializer;
|
||||||
use crate::mir::join_ir::lowering::carrier_info::CarrierVar;
|
|
||||||
use crate::mir::ValueId;
|
|
||||||
|
|
||||||
// Basic pattern check
|
// Basic pattern check
|
||||||
if ctx.pattern_kind != LoopPatternKind::Pattern2Break {
|
if ctx.pattern_kind != LoopPatternKind::Pattern2Break {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Phase 178/188: Check for complex carrier updates
|
// Phase 188/Refactor: Use common carrier update validation
|
||||||
// Phase 188: StringAppendChar/StringAppendLiteral are now allowed
|
// Extracts loop variable for dummy carrier creation (not used but required by API)
|
||||||
// Create dummy carriers from body assignment targets for analysis
|
let loop_var_name = match builder.extract_loop_variable_from_condition(ctx.condition) {
|
||||||
let dummy_carriers: Vec<CarrierVar> = ctx.body.iter().filter_map(|node| {
|
Ok(name) => name,
|
||||||
match node {
|
Err(_) => return false,
|
||||||
crate::ast::ASTNode::Assignment { target, .. } => {
|
};
|
||||||
if let crate::ast::ASTNode::Variable { name, .. } = target.as_ref() {
|
|
||||||
Some(CarrierVar {
|
|
||||||
name: name.clone(),
|
|
||||||
host_id: ValueId(0), // Dummy
|
|
||||||
join_id: None,
|
|
||||||
})
|
|
||||||
} else {
|
|
||||||
None
|
|
||||||
}
|
|
||||||
}
|
|
||||||
_ => None,
|
|
||||||
}
|
|
||||||
}).collect();
|
|
||||||
|
|
||||||
let updates = LoopUpdateAnalyzer::analyze_carrier_updates(ctx.body, &dummy_carriers);
|
CommonPatternInitializer::check_carrier_updates_allowed(
|
||||||
|
ctx.body,
|
||||||
// Phase 188: Check if any update is complex (reject only UpdateRhs::Other)
|
&loop_var_name,
|
||||||
// Allow: Const (numeric), Variable (numeric/string), StringLiteral
|
&builder.variable_map,
|
||||||
// Reject: Other (method calls, nested BinOp)
|
)
|
||||||
for update in updates.values() {
|
|
||||||
if let UpdateExpr::BinOp { rhs, .. } = update {
|
|
||||||
match rhs {
|
|
||||||
UpdateRhs::Const(_) => {
|
|
||||||
// Numeric: i = i + 1 (allowed)
|
|
||||||
}
|
|
||||||
UpdateRhs::Variable(_) => {
|
|
||||||
// Phase 188: StringAppendChar: s = s + ch (allowed)
|
|
||||||
// Or numeric: sum = sum + i (allowed)
|
|
||||||
}
|
|
||||||
UpdateRhs::StringLiteral(_) => {
|
|
||||||
// Phase 188: StringAppendLiteral: s = s + "x" (allowed)
|
|
||||||
}
|
|
||||||
UpdateRhs::Other => {
|
|
||||||
// Phase 188: Complex update (method call, nested BinOp) - reject
|
|
||||||
eprintln!("[pattern2/can_lower] Phase 188: Complex update detected (UpdateRhs::Other), rejecting Pattern 2");
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
true
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Phase 194: Lowering function for Pattern 2
|
/// Phase 194: Lowering function for Pattern 2
|
||||||
|
|||||||
@ -59,65 +59,27 @@ use super::super::trace;
|
|||||||
/// 3. **Phase 178**: No string/complex carrier updates (JoinIR doesn't support string concat)
|
/// 3. **Phase 178**: No string/complex carrier updates (JoinIR doesn't support string concat)
|
||||||
///
|
///
|
||||||
/// If all conditions are met, Pattern 4 is detected.
|
/// If all conditions are met, Pattern 4 is detected.
|
||||||
pub fn can_lower(_builder: &MirBuilder, ctx: &super::router::LoopPatternContext) -> bool {
|
pub fn can_lower(builder: &MirBuilder, ctx: &super::router::LoopPatternContext) -> bool {
|
||||||
use crate::mir::loop_pattern_detection::LoopPatternKind;
|
use crate::mir::loop_pattern_detection::LoopPatternKind;
|
||||||
use crate::mir::join_ir::lowering::loop_update_analyzer::{LoopUpdateAnalyzer, UpdateExpr, UpdateRhs};
|
use super::common_init::CommonPatternInitializer;
|
||||||
use crate::mir::join_ir::lowering::carrier_info::CarrierVar;
|
|
||||||
use crate::mir::ValueId;
|
|
||||||
|
|
||||||
// Basic pattern check
|
// Basic pattern check
|
||||||
if ctx.pattern_kind != LoopPatternKind::Pattern4Continue {
|
if ctx.pattern_kind != LoopPatternKind::Pattern4Continue {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Phase 178/188: Check for complex carrier updates
|
// Phase 188/Refactor: Use common carrier update validation
|
||||||
// Phase 188: StringAppendChar/StringAppendLiteral are now allowed
|
// Extracts loop variable for dummy carrier creation (not used but required by API)
|
||||||
// Create dummy carriers from body assignment targets for analysis
|
let loop_var_name = match builder.extract_loop_variable_from_condition(ctx.condition) {
|
||||||
let dummy_carriers: Vec<CarrierVar> = ctx.body.iter().filter_map(|node| {
|
Ok(name) => name,
|
||||||
match node {
|
Err(_) => return false,
|
||||||
crate::ast::ASTNode::Assignment { target, .. } => {
|
};
|
||||||
if let crate::ast::ASTNode::Variable { name, .. } = target.as_ref() {
|
|
||||||
Some(CarrierVar {
|
|
||||||
name: name.clone(),
|
|
||||||
host_id: ValueId(0), // Dummy
|
|
||||||
join_id: None,
|
|
||||||
})
|
|
||||||
} else {
|
|
||||||
None
|
|
||||||
}
|
|
||||||
}
|
|
||||||
_ => None,
|
|
||||||
}
|
|
||||||
}).collect();
|
|
||||||
|
|
||||||
let updates = LoopUpdateAnalyzer::analyze_carrier_updates(ctx.body, &dummy_carriers);
|
CommonPatternInitializer::check_carrier_updates_allowed(
|
||||||
|
ctx.body,
|
||||||
// Phase 188: Check if any update is complex (reject only UpdateRhs::Other)
|
&loop_var_name,
|
||||||
// Allow: Const (numeric), Variable (numeric/string), StringLiteral
|
&builder.variable_map,
|
||||||
// Reject: Other (method calls, nested BinOp)
|
)
|
||||||
for update in updates.values() {
|
|
||||||
if let UpdateExpr::BinOp { rhs, .. } = update {
|
|
||||||
match rhs {
|
|
||||||
UpdateRhs::Const(_) => {
|
|
||||||
// Numeric: i = i + 1 (allowed)
|
|
||||||
}
|
|
||||||
UpdateRhs::Variable(_) => {
|
|
||||||
// Phase 188: StringAppendChar: s = s + ch (allowed)
|
|
||||||
// Or numeric: sum = sum + i (allowed)
|
|
||||||
}
|
|
||||||
UpdateRhs::StringLiteral(_) => {
|
|
||||||
// Phase 188: StringAppendLiteral: s = s + "x" (allowed)
|
|
||||||
}
|
|
||||||
UpdateRhs::Other => {
|
|
||||||
// Phase 188: Complex update (method call, nested BinOp) - reject
|
|
||||||
eprintln!("[pattern4/can_lower] Phase 188: Complex update detected (UpdateRhs::Other), rejecting Pattern 4");
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
true
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Phase 33-19: Lowering function for Pattern 4
|
/// Phase 33-19: Lowering function for Pattern 4
|
||||||
|
|||||||
Reference in New Issue
Block a user