From 9159f2a63dd9f0565d2f250f39999ba184e88dd6 Mon Sep 17 00:00:00 2001 From: James Tippett Date: Thu, 9 Jul 2026 15:53:01 +0700 Subject: [PATCH] fix: make array mutator methods mutate the receiver in place push, pop, shift, unshift, splice, reverse, and fill computed their return value against a clone and never wrote back, so arr.push(x) did not persist. They now mutate in place and write the result back to the receiver variable. --- crates/zapcode-core/src/vm/builtins.rs | 80 +++++++++++++++++++++ crates/zapcode-core/src/vm/mod.rs | 34 +++++++++ crates/zapcode-core/tests/objects_arrays.rs | 46 ++++++++++++ 3 files changed, 160 insertions(+) diff --git a/crates/zapcode-core/src/vm/builtins.rs b/crates/zapcode-core/src/vm/builtins.rs index 2b20d44..7a1548d 100644 --- a/crates/zapcode-core/src/vm/builtins.rs +++ b/crates/zapcode-core/src/vm/builtins.rs @@ -707,6 +707,86 @@ fn call_array_method(arr: &[Value], method: &str, args: &[Value]) -> Result bool { + matches!( + method, + "push" | "pop" | "shift" | "unshift" | "splice" | "reverse" | "fill" + ) +} + +/// Apply an in-place array mutation, returning the method's JS result value. +/// The caller is responsible for writing `arr` back to the receiver variable. +pub(crate) fn call_array_mutator( + arr: &mut Vec, + method: &str, + args: &[Value], +) -> Option { + let result = match method { + "push" => { + arr.extend(args.iter().cloned()); + Value::Int(arr.len() as i64) + } + "pop" => arr.pop().unwrap_or(Value::Undefined), + "shift" => { + if arr.is_empty() { + Value::Undefined + } else { + arr.remove(0) + } + } + "unshift" => { + for (i, a) in args.iter().enumerate() { + arr.insert(i, a.clone()); + } + Value::Int(arr.len() as i64) + } + "reverse" => { + arr.reverse(); + Value::Array(arr.clone()) + } + "fill" => { + let len = arr.len(); + let fill_val = args.first().cloned().unwrap_or(Value::Undefined); + let start = if args.len() > 1 { + normalize_index(arg_int(args, 1), len as i64) + } else { + 0 + }; + let end = if args.len() > 2 { + normalize_index(arg_int(args, 2), len as i64) + } else { + len + }; + for item in arr.iter_mut().take(end.min(len)).skip(start) { + *item = fill_val.clone(); + } + Value::Array(arr.clone()) + } + "splice" => { + let len = arr.len() as i64; + let raw_start = if args.is_empty() { 0 } else { arg_int(args, 0) }; + let start = if raw_start < 0 { + (len + raw_start).max(0) as usize + } else { + (raw_start as usize).min(arr.len()) + }; + let delete_count = if args.len() > 1 { + (arg_int(args, 1).max(0) as usize).min(arr.len() - start) + } else { + arr.len() - start + }; + let inserts = args.iter().skip(2).cloned(); + let deleted: Vec = arr.splice(start..start + delete_count, inserts).collect(); + Value::Array(deleted) + } + _ => return None, + }; + Some(result) +} + // ── Object static methods ──────────────────────────────────────────── fn call_object_method(method: &str, args: &[Value]) -> Result> { diff --git a/crates/zapcode-core/src/vm/mod.rs b/crates/zapcode-core/src/vm/mod.rs index f0f39d9..49f3b2e 100644 --- a/crates/zapcode-core/src/vm/mod.rs +++ b/crates/zapcode-core/src/vm/mod.rs @@ -203,6 +203,25 @@ impl Vm { .ok_or_else(|| ZapcodeError::RuntimeError("stack underflow".to_string())) } + /// Write a value back to the variable a method receiver was loaded from. + /// Used to give value-typed arrays/objects reference-like mutation semantics + /// for in-place methods (e.g. `arr.push(x)`) and `this` mutation. + fn write_back_receiver(&mut self, source: &ReceiverSource, value: Value) { + match source { + ReceiverSource::Global(name) => { + self.globals.insert(name.clone(), value); + } + ReceiverSource::Local { frame_index, slot } => { + if let Some(target_frame) = self.frames.get_mut(*frame_index) { + while target_frame.locals.len() <= *slot { + target_frame.locals.push(Value::Undefined); + } + target_frame.locals[*slot] = value; + } + } + } + } + fn peek(&self) -> Result<&Value> { self.stack .last() @@ -1622,6 +1641,21 @@ impl Vm { } => { let receiver = self.last_receiver.take(); let result = match object_name.as_ref() { + "__array__" if builtins::is_mutating_array_method(&method_name) => { + // Owned in-place mutation, then write the result + // back to the receiver variable (arrays are + // value-typed on the stack). + if let Some(Value::Array(mut arr)) = receiver { + let ret = + builtins::call_array_mutator(&mut arr, &method_name, &args); + if let Some(source) = self.last_receiver_source.take() { + self.write_back_receiver(&source, Value::Array(arr)); + } + ret + } else { + None + } + } "__array__" => { if let Some(Value::Array(arr)) = &receiver { // Check if this is a callback method first diff --git a/crates/zapcode-core/tests/objects_arrays.rs b/crates/zapcode-core/tests/objects_arrays.rs index 71d79b3..fd3ecf6 100644 --- a/crates/zapcode-core/tests/objects_arrays.rs +++ b/crates/zapcode-core/tests/objects_arrays.rs @@ -226,3 +226,49 @@ fn test_if_block_with_user_function_and_object_arg() { let result = eval_ts("function f(x){ return x; }\nif (true) { f({ a: 1 }); }").unwrap(); assert_eq!(result, Value::Undefined); } +// ── in-place array mutation (regression: push/pop/etc. never persisted) ────── + +#[test] +fn test_push_persists() { + let r = eval_ts("const a = []; a.push(1); a.push(2, 3); a.join(',')").unwrap(); + assert_eq!(r, Value::String("1,2,3".into())); +} + +#[test] +fn test_push_in_loop_builds_array() { + let r = + eval_ts("const a = []; for (const n of [1, 2, 3]) { a.push(n * 2); } a.join(',')").unwrap(); + assert_eq!(r, Value::String("2,4,6".into())); +} + +#[test] +fn test_pop_shift_unshift() { + let r = + eval_ts("const a = [1, 2]; a.unshift(0); const p = a.pop(); a.shift(); [p, a.join(',')]") + .unwrap(); + match r { + Value::Array(v) => { + assert_eq!(v[0], Value::Int(2)); + assert_eq!(v[1], Value::String("1".into())); + } + other => panic!("expected array, got {:?}", other), + } +} + +#[test] +fn test_reverse_in_place() { + let r = eval_ts("const a = [1, 2, 3]; a.reverse(); a.join(',')").unwrap(); + assert_eq!(r, Value::String("3,2,1".into())); +} + +#[test] +fn test_splice_mutates_and_returns_removed() { + let r = eval_ts("const a = [1, 2, 3, 4]; const removed = a.splice(1, 2, 'x'); removed.join(',') + '|' + a.join(',')").unwrap(); + assert_eq!(r, Value::String("2,3|1,x,4".into())); +} + +#[test] +fn test_fill_in_place() { + let r = eval_ts("const a = [1, 2, 3]; a.fill(0, 1); a.join(',')").unwrap(); + assert_eq!(r, Value::String("1,0,0".into())); +}