From 4a5f498fe065085750f6c70f983696a826a8a524 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 18 Mar 2026 03:11:31 +0000 Subject: [PATCH] refactor(builtins): replace magic variable hack with BuiltinSideEffect enum Introduce BuiltinSideEffect enum on ExecResult for builtins to communicate state changes to the interpreter via a typed channel instead of smuggling data through magic-prefixed variables in the HashMap. Migrated: Set (_SET_POSITIONAL), Shift (_SHIFT_COUNT), Read -a (_ARRAY_READ_*). Remaining magic vars (_NAMEREF_, _READONLY_, etc.) are still in use and will be migrated in follow-up work. Closes #728 --- crates/bashkit/src/builtins/mod.rs | 3 + crates/bashkit/src/builtins/read.rs | 31 +++++--- crates/bashkit/src/builtins/vars.rs | 36 ++++------ crates/bashkit/src/interpreter/mod.rs | 96 +++++++++++-------------- crates/bashkit/src/interpreter/state.rs | 19 +++++ 5 files changed, 99 insertions(+), 86 deletions(-) diff --git a/crates/bashkit/src/builtins/mod.rs b/crates/bashkit/src/builtins/mod.rs index bcbca490..8862b136 100644 --- a/crates/bashkit/src/builtins/mod.rs +++ b/crates/bashkit/src/builtins/mod.rs @@ -198,6 +198,9 @@ use crate::error::Result; use crate::fs::FileSystem; use crate::interpreter::ExecResult; +// Re-export for use by builtins +pub use crate::interpreter::BuiltinSideEffect; + /// Resolve a path relative to the current working directory. /// /// If the path is absolute, returns it unchanged. diff --git a/crates/bashkit/src/builtins/read.rs b/crates/bashkit/src/builtins/read.rs index 3b58aceb..1a141075 100644 --- a/crates/bashkit/src/builtins/read.rs +++ b/crates/bashkit/src/builtins/read.rs @@ -2,7 +2,7 @@ use async_trait::async_trait; -use super::{Builtin, Context}; +use super::{Builtin, BuiltinSideEffect, Context}; use crate::error::Result; use crate::interpreter::{ExecResult, is_internal_variable}; @@ -143,12 +143,12 @@ impl Builtin for Read { if is_internal_variable(arr_name) { return Ok(ExecResult::ok(String::new())); } - // Store as _ARRAY__ for the interpreter to pick up - ctx.variables.insert( - format!("_ARRAY_READ_{}", arr_name), - words.join("\x1F"), // unit separator as delimiter - ); - return Ok(ExecResult::ok(String::new())); + let mut result = ExecResult::ok(String::new()); + result.side_effects.push(BuiltinSideEffect::SetArray { + name: arr_name.to_string(), + elements: words.iter().map(|w| w.to_string()).collect(), + }); + return Ok(result); } // If no variable names given, use REPLY @@ -409,9 +409,14 @@ mod tests { ); let result = Read.execute(ctx).await.unwrap(); assert_eq!(result.exit_code, 0); - let stored = variables.get("_ARRAY_READ_ARR").unwrap(); - let parts: Vec<&str> = stored.split('\x1F').collect(); - assert_eq!(parts, vec!["one", "two", "three"]); + assert_eq!(result.side_effects.len(), 1); + match &result.side_effects[0] { + BuiltinSideEffect::SetArray { name, elements } => { + assert_eq!(name, "ARR"); + assert_eq!(elements, &["one", "two", "three"]); + } + _ => panic!("Expected SetArray side effect"), + } } #[tokio::test] @@ -429,7 +434,11 @@ mod tests { ); let result = Read.execute(ctx).await.unwrap(); assert_eq!(result.exit_code, 0); - assert!(variables.contains_key("_ARRAY_READ_REPLY")); + assert_eq!(result.side_effects.len(), 1); + match &result.side_effects[0] { + BuiltinSideEffect::SetArray { name, .. } => assert_eq!(name, "REPLY"), + _ => panic!("Expected SetArray side effect"), + } } // ==================== combined flags ==================== diff --git a/crates/bashkit/src/builtins/vars.rs b/crates/bashkit/src/builtins/vars.rs index 6dbab8b7..cc2b01b2 100644 --- a/crates/bashkit/src/builtins/vars.rs +++ b/crates/bashkit/src/builtins/vars.rs @@ -4,7 +4,7 @@ use async_trait::async_trait; -use super::{Builtin, Context}; +use super::{Builtin, BuiltinSideEffect, Context}; use crate::error::Result; use crate::interpreter::{ExecResult, is_internal_variable}; @@ -93,17 +93,9 @@ fn format_set_plus_o(variables: &std::collections::HashMap) -> S } impl Set { - /// Encode positional parameters as count\x1Farg1\x1Farg2... for the interpreter. - fn encode_positional( - variables: &mut std::collections::HashMap, - positional: &[&str], - ) { - let mut encoded = positional.len().to_string(); - for p in positional { - encoded.push('\x1F'); - encoded.push_str(p); - } - variables.insert("_SET_POSITIONAL".to_string(), encoded); + /// Create a SetPositional side effect. + fn positional_effect(positional: &[&str]) -> BuiltinSideEffect { + BuiltinSideEffect::SetPositional(positional.iter().map(|s| s.to_string()).collect()) } } @@ -121,13 +113,14 @@ impl Builtin for Set { return Ok(ExecResult::ok(output)); } + let mut side_effects = Vec::new(); let mut i = 0; while i < ctx.args.len() { let arg = &ctx.args[i]; if arg == "--" { // Everything after `--` becomes positional parameters. let positional: Vec<&str> = ctx.args[i + 1..].iter().map(|s| s.as_str()).collect(); - Self::encode_positional(ctx.variables, &positional); + side_effects.push(Self::positional_effect(&positional)); break; } else if (arg.starts_with('-') || arg.starts_with('+')) && arg.len() > 1 @@ -174,13 +167,15 @@ impl Builtin for Set { } else { // Non-flag arg: this and everything after become positional params let positional: Vec<&str> = ctx.args[i..].iter().map(|s| s.as_str()).collect(); - Self::encode_positional(ctx.variables, &positional); + side_effects.push(Self::positional_effect(&positional)); break; } i += 1; } - Ok(ExecResult::ok(String::new())) + let mut result = ExecResult::ok(String::new()); + result.side_effects = side_effects; + Ok(result) } } @@ -193,12 +188,11 @@ impl Builtin for Shift { // Number of positions to shift (default 1) let n: usize = ctx.args.first().and_then(|s| s.parse().ok()).unwrap_or(1); - // In real bash, this shifts the positional parameters - // For now, we store the shift count for the interpreter to handle - ctx.variables - .insert("_SHIFT_COUNT".to_string(), n.to_string()); - - Ok(ExecResult::ok(String::new())) + let mut result = ExecResult::ok(String::new()); + result + .side_effects + .push(BuiltinSideEffect::ShiftPositional(n)); + Ok(result) } } diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index 04355697..38f8033b 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -16,7 +16,7 @@ mod state; #[allow(unused_imports)] pub use jobs::{JobTable, SharedJobTable}; -pub use state::{ControlFlow, ExecResult}; +pub use state::{BuiltinSideEffect, ControlFlow, ExecResult}; // Re-export snapshot type for public API use std::collections::{HashMap, HashSet}; @@ -1054,6 +1054,7 @@ impl Interpreter { stderr_truncated, final_env, events, + ..Default::default() }) } @@ -4529,59 +4530,8 @@ impl Interpreter { } }; - // Post-process: read -a populates array from marker variable - let markers: Vec<(String, String)> = self - .variables - .iter() - .filter(|(k, _)| k.starts_with("_ARRAY_READ_")) - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); - for (marker, value) in markers { - let arr_name = marker.strip_prefix("_ARRAY_READ_").unwrap(); - let mut arr = HashMap::new(); - for (i, word) in value.split('\x1F').enumerate() { - if !word.is_empty() { - arr.insert(i, word.to_string()); - } - } - self.insert_array_checked(arr_name.to_string(), arr); - self.variables.remove(&marker); - } - - // Post-process: shift builtin updates positional parameters - if let Some(shift_str) = self.variables.remove("_SHIFT_COUNT") { - let n: usize = shift_str.parse().unwrap_or(1); - if let Some(frame) = self.call_stack.last_mut() { - if n <= frame.positional.len() { - frame.positional.drain(..n); - } else { - frame.positional.clear(); - } - } - } - - // Post-process: `set --` replaces positional parameters - // Encoded as count\x1Farg1\x1Farg2... to preserve empty args. - if let Some(encoded) = self.variables.remove("_SET_POSITIONAL") { - let parts: Vec<&str> = encoded.splitn(2, '\x1F').collect(); - let count: usize = parts[0].parse().unwrap_or(0); - let new_positional: Vec = if count == 0 { - Vec::new() - } else if parts.len() > 1 { - parts[1].split('\x1F').map(|s| s.to_string()).collect() - } else { - Vec::new() - }; - if let Some(frame) = self.call_stack.last_mut() { - frame.positional = new_positional; - } else { - self.call_stack.push(CallFrame { - name: String::new(), - locals: HashMap::new(), - positional: new_positional, - }); - } - } + // Process structured side effects from builtins + self.apply_builtin_side_effects(&result); // Handle output redirections return self.apply_redirections(result, &command.redirects).await; @@ -6051,6 +6001,7 @@ impl Interpreter { git_client: self.git_client.as_ref(), }; let mut result = builtin.execute(ctx).await?; + self.apply_builtin_side_effects(&result); result = self.apply_redirections(result, redirects).await?; Ok(result) } else { @@ -6596,6 +6547,43 @@ impl Interpreter { Ok(stdin) } + /// Process structured side effects from builtin execution. + fn apply_builtin_side_effects(&mut self, result: &ExecResult) { + for effect in &result.side_effects { + match effect { + builtins::BuiltinSideEffect::SetArray { name, elements } => { + let mut arr = HashMap::new(); + for (i, word) in elements.iter().enumerate() { + if !word.is_empty() { + arr.insert(i, word.clone()); + } + } + self.insert_array_checked(name.clone(), arr); + } + builtins::BuiltinSideEffect::ShiftPositional(n) => { + if let Some(frame) = self.call_stack.last_mut() { + if *n <= frame.positional.len() { + frame.positional.drain(..*n); + } else { + frame.positional.clear(); + } + } + } + builtins::BuiltinSideEffect::SetPositional(new_positional) => { + if let Some(frame) = self.call_stack.last_mut() { + frame.positional = new_positional.clone(); + } else { + self.call_stack.push(CallFrame { + name: String::new(), + locals: HashMap::new(), + positional: new_positional.clone(), + }); + } + } + } + } + } + /// Apply output redirections to command output async fn apply_redirections( &mut self, diff --git a/crates/bashkit/src/interpreter/state.rs b/crates/bashkit/src/interpreter/state.rs index b15cab09..eef851bb 100644 --- a/crates/bashkit/src/interpreter/state.rs +++ b/crates/bashkit/src/interpreter/state.rs @@ -13,6 +13,22 @@ pub enum ControlFlow { Return(i32), } +/// Structured side-effect channel for builtins that need to communicate +/// state changes back to the interpreter. +/// +/// Replaces magic-prefixed variables (`_SHIFT_COUNT`, `_SET_POSITIONAL`, +/// `_ARRAY_READ_*`) with a typed enum. The interpreter reads these from +/// `ExecResult::side_effects` after builtin execution. +#[derive(Debug, Clone)] +pub enum BuiltinSideEffect { + /// Shift N positional parameters (replaces `_SHIFT_COUNT`). + ShiftPositional(usize), + /// Replace all positional parameters (replaces `_SET_POSITIONAL`). + SetPositional(Vec), + /// Populate an indexed array variable (replaces `_ARRAY_READ_*`). + SetArray { name: String, elements: Vec }, +} + /// Result of executing a bash script. #[derive(Debug, Clone, Default)] pub struct ExecResult { @@ -32,6 +48,9 @@ pub struct ExecResult { pub final_env: Option>, /// Structured trace events (empty when `TraceMode::Off`). pub events: Vec, + /// Structured side effects from builtin execution. + /// The interpreter processes these after the builtin returns. + pub side_effects: Vec, } impl ExecResult {