diff --git a/docs/development/current/main/exit_binding_design_review.md b/docs/development/current/main/exit_binding_design_review.md new file mode 100644 index 00000000..0d75662b --- /dev/null +++ b/docs/development/current/main/exit_binding_design_review.md @@ -0,0 +1,128 @@ +# exit_binding.rs Design Review + +**File**: `src/mir/builder/control_flow/joinir/patterns/exit_binding.rs` +**Review Date**: 2025-12-08 +**Status**: Production-ready with one technical debt item + +## Current State + +### Functionality +- ✅ **Core Purpose**: Connects JoinIR exit values back to host function's variable_map +- ✅ **Abstraction Level**: Fully boxified, eliminates hardcoded variable names +- ✅ **Test Coverage**: 8 comprehensive tests covering success and error paths +- ✅ **Error Handling**: Robust validation with clear error messages + +### Code Quality +- **Lines**: 416 lines (includes extensive tests) +- **Modularity**: Well-structured with clear separation of concerns +- **Documentation**: Excellent inline documentation and comments +- **Test Quality**: 8 tests covering: + - Single/multi-carrier bindings + - Error cases (missing carriers, name mismatches, loop var in exit_meta) + - Boundary application + +### TODO Items Found + +**Line 179-181**: One TODO identified: +```rust +/// TODO: This should be delegated to a proper ValueId allocator +/// For now, we use a placeholder strategy +fn allocate_new_value_id(&self) -> ValueId { + // Find the maximum ValueId in current variable_map + let max_id = self.variable_map.values() + .map(|v| v.0) + .max() + .unwrap_or(0); + + // Allocate next sequential ID + ValueId(max_id + 1) +} +``` + +## Technical Debt Analysis + +### Issue: Temporary ValueId Allocation Strategy + +**Current Approach**: +- Finds max ValueId in variable_map +- Allocates next sequential ID +- **Risk**: Potential conflicts with builder's ValueIdGenerator + +**Why It Works Now**: +- Variable_map is updated before JoinIR merge +- Merge process respects existing allocations +- Sequential allocation is deterministic +- No observed conflicts in current patterns (1-4) + +**Why It's Technical Debt**: +1. **No Central Authority**: Builder has `value_gen`, but this bypasses it +2. **Implicit Contract**: Relies on merge process behavior +3. **Scalability**: May break with concurrent pattern lowering +4. **Maintainability**: Hard to track ValueId allocation sources + +## Recommendations + +### Short Term (Current Phase) +✅ **Accept as-is**: The current strategy works reliably for all existing patterns +✅ **Document the contract**: Already well-documented in comments +✅ **Keep monitoring**: No action needed unless conflicts appear + +### Medium Term (Next Refactoring Phase) +**Proposed Solution**: Delegate to builder's ValueIdGenerator + +```rust +// Instead of self.allocate_new_value_id() +// Pass value_gen to ExitBindingBuilder + +pub struct ExitBindingBuilder<'a> { + carrier_info: &'a CarrierInfo, + exit_meta: &'a ExitMeta, + variable_map: &'a mut HashMap, + value_gen: &'a mut ValueIdGenerator, // NEW +} + +fn allocate_new_value_id(&mut self) -> ValueId { + self.value_gen.next() // Centralized allocation +} +``` + +**Benefits**: +- Centralized allocation tracking +- No risk of ID conflicts +- Easier to debug ValueId leaks +- Standard pattern across codebase + +**Migration Path**: +1. Add `value_gen` parameter to `ExitBindingBuilder::new()` +2. Update all 3 call sites (pattern2, pattern3, pattern4) +3. Replace `allocate_new_value_id()` implementation +4. Run full test suite to verify + +**Estimated Effort**: 1-2 hours (low risk, mechanical changes) + +### Long Term (Architecture Evolution) +**No action needed**: Current design is sound for foreseeable patterns + +## Recommendations Summary + +| Action | Priority | Effort | Risk | +|--------|----------|--------|------| +| Keep current implementation | ✅ Now | 0h | None | +| Document in TODO | ✅ Done | 0h | None | +| Add value_gen parameter | ⏰ Next refactor | 1-2h | Low | +| Full ValueId allocation audit | ⏰ If issues arise | 4-8h | Low | + +## Conclusion + +**exit_binding.rs is production-ready** with one minor technical debt item that: +- ✅ Works correctly in all current use cases +- ✅ Is well-documented +- ✅ Has clear migration path if needed +- ⏰ Can be addressed in next refactoring phase + +**No blocking issues for Stage 3 completion.** + +--- + +**Reviewer**: Claude Code +**Approval**: Recommend proceeding with current implementation diff --git a/src/mir/builder/control_flow/joinir/patterns/mod.rs b/src/mir/builder/control_flow/joinir/patterns/mod.rs index 4e07b0cc..54d9715d 100644 --- a/src/mir/builder/control_flow/joinir/patterns/mod.rs +++ b/src/mir/builder/control_flow/joinir/patterns/mod.rs @@ -30,6 +30,10 @@ //! //! Phase 33-23: Pattern-Specific Analyzers (Stage 2) //! - pattern4_carrier_analyzer.rs: Pattern 4 carrier analysis and normalization (Issue 2) +//! +//! Stage 3 + Issue 1: Trim Pattern Extraction +//! - trim_pattern_validator.rs: Trim pattern validation and whitespace check generation +//! - trim_pattern_lowerer.rs: Trim-specific JoinIR lowering pub(in crate::mir::builder) mod ast_feature_extractor; pub(in crate::mir::builder) mod common_init; @@ -43,6 +47,8 @@ pub(in crate::mir::builder) mod pattern3_with_if_phi; pub(in crate::mir::builder) mod pattern4_carrier_analyzer; pub(in crate::mir::builder) mod pattern4_with_continue; pub(in crate::mir::builder) mod router; +pub(in crate::mir::builder) mod trim_pattern_validator; +pub(in crate::mir::builder) mod trim_pattern_lowerer; // Re-export router for convenience pub(in crate::mir::builder) use router::{route_loop_pattern, LoopPatternContext}; diff --git a/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs b/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs index 2684fcd5..58bc504f 100644 --- a/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs +++ b/src/mir/builder/control_flow/joinir/patterns/pattern2_with_break.rs @@ -4,99 +4,8 @@ use crate::ast::ASTNode; use crate::mir::builder::MirBuilder; use crate::mir::ValueId; use super::super::trace; - -// Phase 172: Helper function for Trim pattern whitespace checking -/// Generate MIR for OR chain of whitespace character comparisons -/// -/// Creates: ch == " " || ch == "\t" || ch == "\n" || ch == "\r" ... -/// -/// # Arguments -/// -/// * `builder` - MirBuilder for emitting instructions -/// * `ch_value` - ValueId of character to check -/// * `whitespace_chars` - List of whitespace characters to compare against -/// -/// # Returns -/// -/// ValueId of bool result (true if ch matches any whitespace char) -fn emit_whitespace_check( - builder: &mut MirBuilder, - ch_value: ValueId, - whitespace_chars: &[String], -) -> Result { - use crate::mir::builder::emission::constant::emit_string; - use crate::mir::builder::emission::compare::emit_eq_to; - use crate::mir::types::BinaryOp; - use crate::mir::instruction::MirInstruction; - - if whitespace_chars.is_empty() { - return Err("[emit_whitespace_check] Empty whitespace_chars".to_string()); - } - - let mut result_opt: Option = None; - - for ws_char in whitespace_chars { - // ws_const = const " " (or "\t", etc.) - let ws_const = emit_string(builder, ws_char.clone()); - - // eq_check = ch == ws_const - let eq_dst = builder.value_gen.next(); - emit_eq_to(builder, eq_dst, ch_value, ws_const)?; - - result_opt = Some(if let Some(prev_result) = result_opt { - // result = prev_result || eq_check - let dst = builder.value_gen.next(); - builder.emit_instruction(MirInstruction::BinOp { - dst, - op: BinaryOp::Or, - lhs: prev_result, - rhs: eq_dst, - })?; - dst - } else { - // First comparison, no OR needed yet - eq_dst - }); - } - - result_opt.ok_or_else(|| { - "[emit_whitespace_check] Internal error: result should be Some".to_string() - }) -} - -// Phase 172-2: Extract substring arguments from Trim pattern -/// Extract the substring method call arguments from loop body -/// -/// Looks for pattern: local ch = s.substring(start, start+1) -/// -/// # Returns -/// -/// (object_name, start_expr) tuple if found -fn extract_substring_args(loop_body: &[ASTNode], var_name: &str) -> Option<(String, Box)> { - for stmt in loop_body { - // Look for: local ch = ... - if let ASTNode::Local { variables, initial_values, .. } = stmt { - for (i, var) in variables.iter().enumerate() { - if var == var_name { - if let Some(Some(init_expr)) = initial_values.get(i) { - // Check if it's a substring method call - if let ASTNode::MethodCall { object, method, arguments, .. } = init_expr.as_ref() { - if method == "substring" && arguments.len() == 2 { - // Extract object name - if let ASTNode::Variable { name, .. } = object.as_ref() { - // Return object name and start expression - // (We assume second arg is start+1, first arg is start) - return Some((name.clone(), Box::new(arguments[0].clone()))); - } - } - } - } - } - } - } - } - None -} +use super::trim_pattern_validator::TrimPatternValidator; +use super::trim_pattern_lowerer::TrimPatternLowerer; /// Phase 194: Detection function for Pattern 2 /// @@ -287,7 +196,7 @@ impl MirBuilder { carrier_name, original_var, whitespace_chars); // Phase 172-2: Extract substring pattern and generate initial check - let (s_name, start_expr) = extract_substring_args(_body, &original_var) + let (s_name, start_expr) = TrimPatternValidator::extract_substring_args(_body, &original_var) .ok_or_else(|| { format!( "[cf_loop/pattern2] Failed to extract substring pattern for Trim carrier '{}'", @@ -330,7 +239,7 @@ impl MirBuilder { eprintln!("[pattern2/trim] Generated initial substring call: ch0 = {:?}", ch0); // Generate: is_ch_match0 = (ch0 == " " || ch0 == "\t" || ...) - let is_ch_match0 = emit_whitespace_check(self, ch0, &whitespace_chars)?; + let is_ch_match0 = TrimPatternValidator::emit_whitespace_check(self, ch0, &whitespace_chars)?; eprintln!("[pattern2/trim] Generated initial whitespace check: is_ch_match0 = {:?}", is_ch_match0); @@ -376,36 +285,21 @@ impl MirBuilder { // Phase 172-4: Replace break condition for Trim pattern and add carrier to ConditionEnv let effective_break_condition = if let Some(helper) = carrier_info.trim_helper() { if helper.is_safe_trim() { - // Add carrier to ConditionEnv - let carrier_host_id = self.variable_map.get(&helper.carrier_name) - .copied() - .ok_or_else(|| format!("[pattern2/trim] Carrier '{}' not in variable_map", helper.carrier_name))?; - - let carrier_join_id = alloc_join_value(); // Allocate JoinIR-local ValueId - - env.insert(helper.carrier_name.clone(), carrier_join_id); - condition_bindings.push(ConditionBinding { - name: helper.carrier_name.clone(), - host_value: carrier_host_id, - join_value: carrier_join_id, - }); + // Add carrier to ConditionEnv using TrimPatternLowerer + let get_value = |name: &str| self.variable_map.get(name).copied(); + let binding = TrimPatternLowerer::add_to_condition_env( + helper, + get_value, + |name, value| { env.insert(name, value); }, + &mut alloc_join_value, + )?; + condition_bindings.push(binding); eprintln!("[pattern2/trim] Added carrier '{}' to ConditionEnv: HOST {:?} → JoinIR {:?}", - helper.carrier_name, carrier_host_id, carrier_join_id); + helper.carrier_name, condition_bindings.last().unwrap().host_value, condition_bindings.last().unwrap().join_value); - // Create negated carrier check: !is_ch_match - use crate::ast::{Span, UnaryOperator}; - - let carrier_var_node = ASTNode::Variable { - name: helper.carrier_name.clone(), - span: Span::unknown(), - }; - - let negated_carrier = ASTNode::UnaryOp { - operator: UnaryOperator::Not, - operand: Box::new(carrier_var_node), - span: Span::unknown(), - }; + // Generate negated carrier check: !is_ch_match + let negated_carrier = TrimPatternLowerer::generate_trim_break_condition(helper); eprintln!("[pattern2/trim] Replaced break condition with !{}", helper.carrier_name); negated_carrier diff --git a/src/mir/builder/control_flow/joinir/patterns/trim_pattern_lowerer.rs b/src/mir/builder/control_flow/joinir/patterns/trim_pattern_lowerer.rs new file mode 100644 index 00000000..d929194e --- /dev/null +++ b/src/mir/builder/control_flow/joinir/patterns/trim_pattern_lowerer.rs @@ -0,0 +1,231 @@ +//! Trim Pattern JoinIR Lowering +//! +//! Handles JoinIR-specific lowering for Trim patterns. +//! Responsible for: +//! - Trim break condition generation +//! - Carrier binding setup in ConditionEnv + +use crate::mir::loop_pattern_detection::trim_loop_helper::TrimLoopHelper; +use crate::mir::join_ir::lowering::condition_env::ConditionBinding; +use crate::mir::ValueId; +use crate::ast::{ASTNode, UnaryOperator, Span}; + +pub(in crate::mir::builder::control_flow::joinir::patterns) struct TrimPatternLowerer; + +impl TrimPatternLowerer { + /// Generate Trim-specific JoinIR break condition + /// + /// Replaces: break on (ch == " " || ...) + /// With: break on !is_carrier + /// + /// # Arguments + /// + /// * `trim_helper` - The TrimLoopHelper containing pattern info + /// + /// # Returns + /// + /// AST node representing the negated carrier check + pub(in crate::mir::builder::control_flow::joinir::patterns) fn generate_trim_break_condition( + trim_helper: &TrimLoopHelper, + ) -> ASTNode { + let carrier_var_node = ASTNode::Variable { + name: trim_helper.carrier_name.clone(), + span: Span::unknown(), + }; + + ASTNode::UnaryOp { + operator: UnaryOperator::Not, + operand: Box::new(carrier_var_node), + span: Span::unknown(), + } + } + + /// Setup Trim carrier in ConditionEnv + /// + /// Creates binding for bool carrier in JoinIR condition space + /// + /// # Arguments + /// + /// * `trim_helper` - The TrimLoopHelper containing pattern info + /// * `get_host_value` - Closure to get host ValueId for carrier name + /// * `alloc_join_value` - Closure to allocate new JoinIR ValueId + /// + /// # Returns + /// + /// ConditionBinding for the carrier + pub(in crate::mir::builder::control_flow::joinir::patterns) fn setup_trim_carrier_binding( + trim_helper: &TrimLoopHelper, + get_host_value: impl Fn(&str) -> Option, + alloc_join_value: &mut dyn FnMut() -> ValueId, + ) -> Result { + let carrier_name = &trim_helper.carrier_name; + + // Get host ValueId for carrier + let host_value_id = get_host_value(carrier_name) + .ok_or_else(|| format!("[TrimPatternLowerer] Carrier '{}' not in variable_map", carrier_name))?; + + // Allocate JoinIR ValueId + let joinir_value_id = alloc_join_value(); + + Ok(ConditionBinding { + name: carrier_name.clone(), + host_value: host_value_id, + join_value: joinir_value_id, + }) + } + + /// Add Trim carrier to ConditionEnv + /// + /// Inserts the carrier into the environment and returns its binding + /// + /// # Arguments + /// + /// * `trim_helper` - The TrimLoopHelper containing pattern info + /// * `get_host_value` - Closure to get host ValueId for carrier name + /// * `insert_to_env` - Closure to insert binding into env + /// * `alloc_join_value` - Closure to allocate new JoinIR ValueId + /// + /// # Returns + /// + /// ConditionBinding for the carrier + pub(in crate::mir::builder::control_flow::joinir::patterns) fn add_to_condition_env( + trim_helper: &TrimLoopHelper, + get_host_value: impl Fn(&str) -> Option, + insert_to_env: impl FnOnce(String, ValueId), + alloc_join_value: &mut dyn FnMut() -> ValueId, + ) -> Result { + let binding = Self::setup_trim_carrier_binding(trim_helper, get_host_value, alloc_join_value)?; + + // Insert into env + insert_to_env(binding.name.clone(), binding.join_value); + + Ok(binding) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn create_test_helper() -> TrimLoopHelper { + TrimLoopHelper { + original_var: "ch".to_string(), + carrier_name: "is_ws".to_string(), + whitespace_chars: vec![" ".to_string()], + } + } + + #[test] + fn test_generate_trim_break_condition() { + let helper = create_test_helper(); + let result = TrimPatternLowerer::generate_trim_break_condition(&helper); + + // Should be UnaryOp::Not + match result { + ASTNode::UnaryOp { operator, operand, .. } => { + assert_eq!(operator, UnaryOperator::Not); + // Operand should be Variable with carrier name + match *operand { + ASTNode::Variable { name, .. } => { + assert_eq!(name, "is_ws"); + } + _ => panic!("Expected Variable node"), + } + } + _ => panic!("Expected UnaryOp node"), + } + } + + #[test] + fn test_setup_trim_carrier_binding() { + use std::collections::HashMap; + let helper = create_test_helper(); + let mut variable_map = HashMap::new(); + variable_map.insert("is_ws".to_string(), ValueId(42)); + + let mut counter = 100u32; + let mut alloc = || { + let id = ValueId(counter); + counter += 1; + id + }; + + let get_value = |name: &str| variable_map.get(name).copied(); + let result = TrimPatternLowerer::setup_trim_carrier_binding( + &helper, + get_value, + &mut alloc, + ); + + assert!(result.is_ok()); + let binding = result.unwrap(); + assert_eq!(binding.name, "is_ws"); + assert_eq!(binding.host_value, ValueId(42)); + assert_eq!(binding.join_value, ValueId(100)); + } + + #[test] + fn test_setup_trim_carrier_binding_missing_carrier() { + use std::collections::HashMap; + let helper = create_test_helper(); + let variable_map: HashMap = HashMap::new(); // Empty! + + let mut counter = 100u32; + let mut alloc = || { + let id = ValueId(counter); + counter += 1; + id + }; + + let get_value = |name: &str| variable_map.get(name).copied(); + let result = TrimPatternLowerer::setup_trim_carrier_binding( + &helper, + get_value, + &mut alloc, + ); + + assert!(result.is_err()); + assert!(result.unwrap_err().contains("not in variable_map")); + } + + #[test] + fn test_add_to_condition_env() { + use std::collections::HashMap; + let helper = create_test_helper(); + let mut variable_map = HashMap::new(); + variable_map.insert("is_ws".to_string(), ValueId(42)); + + let mut env = HashMap::new(); + + let mut counter = 100u32; + let mut alloc = || { + let id = ValueId(counter); + counter += 1; + id + }; + + let get_value = |name: &str| variable_map.get(name).copied(); + let insert = |name: String, value: ValueId| { + env.insert(name, value); + }; + + let result = TrimPatternLowerer::add_to_condition_env( + &helper, + get_value, + insert, + &mut alloc, + ); + + assert!(result.is_ok()); + let binding = result.unwrap(); + + // Check that env was updated + assert!(env.contains_key("is_ws")); + assert_eq!(env["is_ws"], ValueId(100)); + + // Check binding + assert_eq!(binding.name, "is_ws"); + assert_eq!(binding.host_value, ValueId(42)); + assert_eq!(binding.join_value, ValueId(100)); + } +} diff --git a/src/mir/builder/control_flow/joinir/patterns/trim_pattern_validator.rs b/src/mir/builder/control_flow/joinir/patterns/trim_pattern_validator.rs new file mode 100644 index 00000000..57a18f87 --- /dev/null +++ b/src/mir/builder/control_flow/joinir/patterns/trim_pattern_validator.rs @@ -0,0 +1,236 @@ +//! Trim Pattern Validation +//! +//! Validates and prepares Trim pattern information for JoinIR lowering. +//! Responsible for: +//! - Safety checks on Trim pattern structure +//! - Whitespace check generation +//! - Substring argument extraction + +use crate::ast::ASTNode; +use crate::mir::builder::MirBuilder; +use crate::mir::ValueId; + +pub(in crate::mir::builder::control_flow::joinir::patterns) struct TrimPatternValidator; + +impl TrimPatternValidator { + /// Generate MIR for OR chain of whitespace character comparisons + /// + /// Creates: ch == " " || ch == "\t" || ch == "\n" || ch == "\r" ... + /// + /// # Arguments + /// + /// * `builder` - MirBuilder for emitting instructions + /// * `ch_value` - ValueId of character to check + /// * `whitespace_chars` - List of whitespace characters to compare against + /// + /// # Returns + /// + /// ValueId of bool result (true if ch matches any whitespace char) + pub(in crate::mir::builder::control_flow::joinir::patterns) fn emit_whitespace_check( + builder: &mut MirBuilder, + ch_value: ValueId, + whitespace_chars: &[String], + ) -> Result { + use crate::mir::builder::emission::constant::emit_string; + use crate::mir::builder::emission::compare::emit_eq_to; + use crate::mir::types::BinaryOp; + use crate::mir::instruction::MirInstruction; + + if whitespace_chars.is_empty() { + return Err("[emit_whitespace_check] Empty whitespace_chars".to_string()); + } + + let mut result_opt: Option = None; + + for ws_char in whitespace_chars { + // ws_const = const " " (or "\t", etc.) + let ws_const = emit_string(builder, ws_char.clone()); + + // eq_check = ch == ws_const + let eq_dst = builder.value_gen.next(); + emit_eq_to(builder, eq_dst, ch_value, ws_const)?; + + result_opt = Some(if let Some(prev_result) = result_opt { + // result = prev_result || eq_check + let dst = builder.value_gen.next(); + builder.emit_instruction(MirInstruction::BinOp { + dst, + op: BinaryOp::Or, + lhs: prev_result, + rhs: eq_dst, + })?; + dst + } else { + // First comparison, no OR needed yet + eq_dst + }); + } + + result_opt.ok_or_else(|| { + "[emit_whitespace_check] Internal error: result should be Some".to_string() + }) + } + + /// Extract the substring method call arguments from loop body + /// + /// Looks for pattern: local ch = s.substring(start, start+1) + /// + /// # Arguments + /// + /// * `loop_body` - Loop body AST nodes + /// * `var_name` - Variable name to search for (e.g., "ch") + /// + /// # Returns + /// + /// (object_name, start_expr) tuple if found + pub(in crate::mir::builder::control_flow::joinir::patterns) fn extract_substring_args( + loop_body: &[ASTNode], + var_name: &str, + ) -> Option<(String, Box)> { + for stmt in loop_body { + // Look for: local ch = ... + if let ASTNode::Local { variables, initial_values, .. } = stmt { + for (i, var) in variables.iter().enumerate() { + if var == var_name { + if let Some(Some(init_expr_box)) = initial_values.get(i) { + // Check if it's a substring method call + if let ASTNode::MethodCall { object, method, arguments, .. } = init_expr_box.as_ref() { + if method == "substring" && arguments.len() == 2 { + // Extract object name + if let ASTNode::Variable { name, .. } = object.as_ref() { + // Return object name and start expression + // (We assume second arg is start+1, first arg is start) + return Some((name.clone(), Box::new(arguments[0].clone()))); + } + } + } + } + } + } + } + } + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::ast::{LiteralValue, Span, BinaryOperator}; + + #[test] + fn test_extract_substring_args_valid() { + // Create: local ch = s.substring(start, start+1) + let body = vec![ + ASTNode::Local { + variables: vec!["ch".to_string()], + initial_values: vec![ + Some(Box::new(ASTNode::MethodCall { + object: Box::new(ASTNode::Variable { + name: "s".to_string(), + span: Span::unknown(), + }), + method: "substring".to_string(), + arguments: vec![ + ASTNode::Variable { + name: "start".to_string(), + span: Span::unknown(), + }, + ASTNode::BinaryOp { + operator: BinaryOperator::Add, + left: Box::new(ASTNode::Variable { + name: "start".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 = TrimPatternValidator::extract_substring_args(&body, "ch"); + assert!(result.is_some()); + let (s_name, _) = result.unwrap(); + assert_eq!(s_name, "s"); + } + + #[test] + fn test_extract_substring_args_not_found() { + // Empty body + let body = vec![]; + let result = TrimPatternValidator::extract_substring_args(&body, "ch"); + assert!(result.is_none()); + } + + #[test] + fn test_extract_substring_args_wrong_var() { + // local other_var = s.substring(0, 1) + let body = vec![ + ASTNode::Local { + variables: vec!["other_var".to_string()], + initial_values: vec![ + Some(Box::new(ASTNode::MethodCall { + object: Box::new(ASTNode::Variable { + name: "s".to_string(), + span: Span::unknown(), + }), + method: "substring".to_string(), + arguments: vec![ + ASTNode::Literal { + value: LiteralValue::Integer(0), + span: Span::unknown(), + }, + ASTNode::Literal { + value: LiteralValue::Integer(1), + span: Span::unknown(), + }, + ], + span: Span::unknown(), + })), + ], + span: Span::unknown(), + }, + ]; + + let result = TrimPatternValidator::extract_substring_args(&body, "ch"); + assert!(result.is_none()); + } + + #[test] + fn test_extract_substring_args_wrong_method() { + // local ch = s.charAt(0) + let body = vec![ + ASTNode::Local { + variables: vec!["ch".to_string()], + initial_values: vec![ + Some(Box::new(ASTNode::MethodCall { + object: Box::new(ASTNode::Variable { + name: "s".to_string(), + span: Span::unknown(), + }), + method: "charAt".to_string(), + arguments: vec![ + ASTNode::Literal { + value: LiteralValue::Integer(0), + span: Span::unknown(), + }, + ], + span: Span::unknown(), + })), + ], + span: Span::unknown(), + }, + ]; + + let result = TrimPatternValidator::extract_substring_args(&body, "ch"); + assert!(result.is_none()); + } +}