From d7522c53fb5b5e013515723d53425d5f06dfd5cd Mon Sep 17 00:00:00 2001 From: Alexandre MAI Date: Sat, 29 Aug 2026 18:11:37 +0200 Subject: [PATCH] fix: clean up stale try handlers Centralize frame and handler cleanup, preserve generator handlers across yields, cancel invalid continuations, and unwind try scopes for break and continue. --- crates/zapcode-core/src/compiler/mod.rs | 39 ++- crates/zapcode-core/src/vm/mod.rs | 155 +++++++----- crates/zapcode-core/tests/error_handling.rs | 251 +++++++++++++++++++- 3 files changed, 375 insertions(+), 70 deletions(-) diff --git a/crates/zapcode-core/src/compiler/mod.rs b/crates/zapcode-core/src/compiler/mod.rs index 9d2284e..5095c5a 100644 --- a/crates/zapcode-core/src/compiler/mod.rs +++ b/crates/zapcode-core/src/compiler/mod.rs @@ -31,12 +31,14 @@ struct Compiler { local_indices: HashMap, functions: Vec, loop_stack: Vec, + try_depth: usize, external_functions: HashSet, } struct LoopInfo { break_patches: Vec, continue_patches: Vec, + try_depth: usize, } impl Compiler { @@ -47,6 +49,7 @@ impl Compiler { local_indices: HashMap::new(), functions: Vec::new(), loop_stack: Vec::new(), + try_depth: 0, external_functions, } } @@ -222,6 +225,7 @@ impl Compiler { self.loop_stack.push(LoopInfo { break_patches: Vec::new(), continue_patches: Vec::new(), + try_depth: self.try_depth, }); self.compile_expr(test)?; @@ -248,6 +252,7 @@ impl Compiler { self.loop_stack.push(LoopInfo { break_patches: Vec::new(), continue_patches: Vec::new(), + try_depth: self.try_depth, }); for s in body { @@ -282,6 +287,7 @@ impl Compiler { self.loop_stack.push(LoopInfo { break_patches: Vec::new(), continue_patches: Vec::new(), + try_depth: self.try_depth, }); let exit_jump = if let Some(test) = test { @@ -329,6 +335,7 @@ impl Compiler { self.loop_stack.push(LoopInfo { break_patches: Vec::new(), continue_patches: Vec::new(), + try_depth: self.try_depth, }); self.emit(Instruction::Dup); @@ -382,9 +389,11 @@ impl Compiler { } => { let setup = self.emit(Instruction::SetupTry(0, None)); + self.try_depth += 1; for s in try_body { self.compile_statement(s)?; } + self.try_depth -= 1; self.emit(Instruction::EndTry); let jump_past_catch = self.emit(Instruction::Jump(0)); @@ -413,16 +422,34 @@ impl Compiler { } } Statement::Break { .. } => { - let idx = self.emit(Instruction::Jump(0)); - if let Some(loop_info) = self.loop_stack.last_mut() { - loop_info.break_patches.push(idx); + let Some(target_try_depth) = self.loop_stack.last().map(|info| info.try_depth) + else { + return Err(ZapcodeError::CompileError( + "illegal break statement (not inside a loop)".to_string(), + )); + }; + for _ in target_try_depth..self.try_depth { + self.emit(Instruction::EndTry); } + let idx = self.emit(Instruction::Jump(0)); + self.loop_stack.last_mut().unwrap().break_patches.push(idx); } Statement::Continue { .. } => { - let idx = self.emit(Instruction::Jump(0)); - if let Some(loop_info) = self.loop_stack.last_mut() { - loop_info.continue_patches.push(idx); + let Some(target_try_depth) = self.loop_stack.last().map(|info| info.try_depth) + else { + return Err(ZapcodeError::CompileError( + "illegal continue statement (not inside a loop)".to_string(), + )); + }; + for _ in target_try_depth..self.try_depth { + self.emit(Instruction::EndTry); } + let idx = self.emit(Instruction::Jump(0)); + self.loop_stack + .last_mut() + .unwrap() + .continue_patches + .push(idx); } Statement::FunctionDecl { func_index, .. } => { self.emit(Instruction::CreateClosure(*func_index)); diff --git a/crates/zapcode-core/src/vm/mod.rs b/crates/zapcode-core/src/vm/mod.rs index 047ca57..8f45e05 100644 --- a/crates/zapcode-core/src/vm/mod.rs +++ b/crates/zapcode-core/src/vm/mod.rs @@ -99,6 +99,7 @@ pub struct Vm { last_load_source: Option, /// Counter for assigning unique generator IDs. next_generator_id: u64, + generator_try_handlers: HashMap>, } #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] @@ -108,6 +109,11 @@ pub(crate) struct TryInfo { pub(crate) stack_depth: usize, } +struct SuspendedTryHandler { + catch_ip: usize, + stack_offset: usize, +} + impl Vm { fn new( program: CompiledProgram, @@ -135,6 +141,7 @@ impl Vm { last_global_name: None, last_load_source: None, next_generator_id: 0, + generator_try_handlers: HashMap::new(), } } @@ -181,6 +188,7 @@ impl Vm { last_global_name: None, last_load_source: None, next_generator_id: 0, + generator_try_handlers: HashMap::new(), } } @@ -203,6 +211,60 @@ impl Vm { .ok_or_else(|| ZapcodeError::RuntimeError("stack underflow".to_string())) } + fn pop_call_frame(&mut self) -> Option { + let frame = self.frames.pop()?; + self.tracker.pop_frame(); + let remaining_depth = self.frames.len(); + self.try_stack + .retain(|try_info| try_info.frame_depth <= remaining_depth); + Some(frame) + } + + fn take_try_handler(&mut self, min_frame_depth: usize) -> Option { + while let Some(try_info) = self.try_stack.last() { + if try_info.frame_depth > self.frames.len() { + self.try_stack.pop(); + continue; + } + if try_info.frame_depth <= min_frame_depth { + return None; + } + return self.try_stack.pop(); + } + None + } + + fn try_handle_error(&mut self, err: &ZapcodeError, min_frame_depth: usize) -> Result { + let Some(try_info) = self.take_try_handler(min_frame_depth) else { + return Ok(false); + }; + + self.continuations.retain(|continuation| { + let callback_frame_index = match continuation { + Continuation::ArrayMap { + callback_frame_index, + .. + } + | Continuation::ArrayForEach { + callback_frame_index, + .. + } => *callback_frame_index, + }; + callback_frame_index < try_info.frame_depth + }); + while self.frames.len() > try_info.frame_depth { + self.pop_call_frame(); + } + self.stack.truncate(try_info.stack_depth); + self.push(Value::String(Arc::from(err.to_string())))?; + let frame = self + .frames + .last_mut() + .ok_or_else(|| ZapcodeError::RuntimeError("no active call frame".to_string()))?; + frame.ip = try_info.catch_ip; + Ok(true) + } + fn peek(&self) -> Result<&Value> { self.stack .last() @@ -364,8 +426,7 @@ impl Vm { return Ok(VmState::Complete(result)); } else { // Return from function - let frame = self.frames.pop().unwrap(); - self.tracker.pop_frame(); + let frame = self.pop_call_frame().unwrap(); // If this was a constructor, return `this` if let Some(this_val) = frame.this_value { self.stack.truncate(frame.stack_base); @@ -392,22 +453,7 @@ impl Vm { } } Err(err) => { - // Try to catch the error - if let Some(try_info) = self.try_stack.pop() { - // Unwind to catch block - while self.frames.len() > try_info.frame_depth { - self.frames.pop(); - self.tracker.pop_frame(); - } - self.stack.truncate(try_info.stack_depth); - - // Push error value - let error_val = Value::String(Arc::from(err.to_string())); - self.push(error_val)?; - - // Jump to catch - self.current_frame_mut().ip = try_info.catch_ip; - } else { + if !self.try_handle_error(&err, 0)? { return Err(err); } } @@ -590,14 +636,12 @@ impl Vm { // End of function without explicit return if self.frames.len() > target_frame_depth + 1 { // Inner function ended, pop and continue - self.frames.pop(); - self.tracker.pop_frame(); + self.pop_call_frame(); self.push(Value::Undefined)?; continue; } else { // Our target function ended - self.frames.pop(); - self.tracker.pop_frame(); + self.pop_call_frame(); return Ok(Value::Undefined); } } @@ -624,17 +668,7 @@ impl Vm { } } Err(err) => { - // Try to catch the error within the callback - if let Some(try_info) = self.try_stack.pop() { - while self.frames.len() > try_info.frame_depth { - self.frames.pop(); - self.tracker.pop_frame(); - } - self.stack.truncate(try_info.stack_depth); - let error_val = Value::String(Arc::from(err.to_string())); - self.push(error_val)?; - self.current_frame_mut().ip = try_info.catch_ip; - } else { + if !self.try_handle_error(&err, target_frame_depth)? { return Err(err); } } @@ -1033,6 +1067,15 @@ impl Vm { this_value: None, receiver_source: None, }); + let frame_depth = self.frames.len(); + if let Some(handlers) = self.generator_try_handlers.remove(&gen_obj.id) { + self.try_stack + .extend(handlers.into_iter().map(|handler| TryInfo { + catch_ip: handler.catch_ip, + frame_depth, + stack_depth: stack_base + handler.stack_offset, + })); + } self.run_generator_until_yield_or_return(gen_obj) } } @@ -1044,6 +1087,7 @@ impl Vm { let gen_key = format!("__gen_{}", gen_obj.id); if gen_obj.done { self.globals.remove(&gen_key); + self.generator_try_handlers.remove(&gen_obj.id); } else { self.globals.insert(gen_key, Value::Generator(gen_obj)); } @@ -1074,8 +1118,7 @@ impl Vm { }; if frame.ip >= instructions.len() { if self.frames.len() > target_frame_depth + 1 { - let frame = self.frames.pop().unwrap(); - self.tracker.pop_frame(); + let frame = self.pop_call_frame().unwrap(); if let Some(this_val) = frame.this_value { self.stack.truncate(frame.stack_base); self.push(this_val)?; @@ -1084,8 +1127,7 @@ impl Vm { } continue; } - let frame = self.frames.pop().unwrap(); - self.tracker.pop_frame(); + let frame = self.pop_call_frame().unwrap(); self.stack.truncate(frame.stack_base); let result = self.finish_generator(gen_obj, Value::Undefined); return Ok(result); @@ -1094,9 +1136,24 @@ impl Vm { if matches!(instr, Instruction::Yield) { self.current_frame_mut().ip += 1; let yielded_value = self.pop()?; - let frame = self.frames.pop().unwrap(); - self.tracker.pop_frame(); + let frame_depth = self.frames.len(); + let frame_stack_base = self.current_frame().stack_base; + let try_handlers: Vec = self + .try_stack + .iter() + .filter(|try_info| try_info.frame_depth == frame_depth) + .map(|try_info| SuspendedTryHandler { + catch_ip: try_info.catch_ip, + stack_offset: try_info.stack_depth.saturating_sub(frame_stack_base), + }) + .collect(); + let frame = self.pop_call_frame().unwrap(); let frame_stack: Vec = self.stack.drain(frame.stack_base..).collect(); + if try_handlers.is_empty() { + self.generator_try_handlers.remove(&gen_obj.id); + } else { + self.generator_try_handlers.insert(gen_obj.id, try_handlers); + } gen_obj.suspended = Some(SuspendedFrame { ip: frame.ip, locals: frame.locals, @@ -1110,14 +1167,12 @@ impl Vm { self.current_frame_mut().ip += 1; let return_val = self.pop().unwrap_or(Value::Undefined); if self.frames.len() > target_frame_depth + 1 { - let frame = self.frames.pop().unwrap(); - self.tracker.pop_frame(); + let frame = self.pop_call_frame().unwrap(); self.stack.truncate(frame.stack_base); self.push(return_val)?; continue; } - let frame = self.frames.pop().unwrap(); - self.tracker.pop_frame(); + let frame = self.pop_call_frame().unwrap(); self.stack.truncate(frame.stack_base); let result = self.finish_generator(gen_obj, return_val); return Ok(result); @@ -1138,16 +1193,7 @@ impl Vm { } } Err(err) => { - if let Some(try_info) = self.try_stack.pop() { - while self.frames.len() > try_info.frame_depth { - self.frames.pop(); - self.tracker.pop_frame(); - } - self.stack.truncate(try_info.stack_depth); - let error_val = Value::String(Arc::from(err.to_string())); - self.push(error_val)?; - self.current_frame_mut().ip = try_info.catch_ip; - } else { + if !self.try_handle_error(&err, target_frame_depth)? { return Err(err); } } @@ -1783,8 +1829,7 @@ impl Vm { return Ok(Some(VmState::Complete(return_val))); } - let frame = self.frames.pop().unwrap(); - self.tracker.pop_frame(); + let frame = self.pop_call_frame().unwrap(); // If this was a constructor frame (has this_value), return the // updated `this` instead of the explicit return value (unless diff --git a/crates/zapcode-core/tests/error_handling.rs b/crates/zapcode-core/tests/error_handling.rs index 7001619..e0e4547 100644 --- a/crates/zapcode-core/tests/error_handling.rs +++ b/crates/zapcode-core/tests/error_handling.rs @@ -55,11 +55,6 @@ fn test_try_no_error() { assert_eq!(result, Value::Int(42)); } -// Regression: a throw escaping a nested array callback (or a callback inside a -// class method) emptied the VM frame stack; execute() then hit -// frames.last().unwrap() and aborted the host process. These must surface an -// error to the caller, never panic. (The guest-level catch not observing the -// throw is a separate, pre-existing unwinding issue.) #[test] fn test_throw_from_nested_callback_does_not_panic() { let result = eval_ts( @@ -70,8 +65,9 @@ fn test_throw_from_nested_callback_does_not_panic() { } catch (e) { out = 3; } out "#, - ); - assert!(result.is_err()); + ) + .unwrap(); + assert_eq!(result, Value::Int(3)); } #[test] @@ -83,6 +79,243 @@ fn test_throw_from_class_method_callback_does_not_panic() { try { new A().run(); } catch (e) { out = 6; } out "#, - ); - assert!(result.is_err()); + ) + .unwrap(); + assert_eq!(result, Value::Int(6)); +} + +#[test] +fn test_return_inside_try_does_not_leave_stale_handler() { + let result = eval_ts( + r#" + function seed() { + try { return 1; } catch (e) {} + } + seed(); + + let out = 0; + try { + [1].map(a => [2].map(b => { throw "n"; })); + } catch (e) { out = 9; } + out + "#, + ) + .unwrap(); + assert_eq!(result, Value::Int(9)); +} + +#[test] +fn test_generator_return_inside_try_does_not_leave_stale_handler() { + let result = eval_ts( + r#" + function* seed() { + try { return 1; } catch (e) {} + } + seed().next(); + + let out = 0; + try { + [1].map(a => [2].map(b => { throw "n"; })); + } catch (e) { out = 11; } + out + "#, + ) + .unwrap(); + assert_eq!(result, Value::Int(11)); +} + +#[test] +fn test_generator_throw_reaches_outer_try_handler() { + let result = eval_ts( + r#" + function* fail() { throw "generator error"; } + let out = 0; + try { fail().next(); } catch (e) { out = 13; } + out + "#, + ) + .unwrap(); + assert_eq!(result, Value::Int(13)); +} + +#[test] +fn test_generator_try_handler_survives_yield() { + let result = eval_ts( + r#" + function* values() { + try { + yield 1; + throw "failure"; + } catch (e) { + yield 2; + } + } + const iterator = values(); + iterator.next(); + iterator.next().value + "#, + ) + .unwrap(); + assert_eq!(result, Value::Int(2)); +} + +#[test] +fn test_async_callback_throw_cancels_continuation() { + let result = eval_ts( + r#" + let caught = false; + try { + [1].map(async () => { throw "failure"; }); + } catch (e) { + caught = true; + } + caught + "#, + ) + .unwrap(); + assert_eq!(result, Value::Bool(true)); +} + +#[test] +fn test_nested_generator_try_handlers_survive_multiple_yields() { + let result = eval_ts( + r#" + function* values() { + try { + yield 1; + try { + yield 2; + throw "inner"; + } catch (e) { + yield 3; + } + throw "outer"; + } catch (e) { + yield 4; + } + } + const iterator = values(); + const first = iterator.next().value; + const second = iterator.next().value; + const third = iterator.next().value; + const fourth = iterator.next().value; + `${first},${second},${third},${fourth}` + "#, + ) + .unwrap(); + assert_eq!(result, Value::String("1,2,3,4".into())); +} + +#[test] +fn test_nested_async_callback_throw_preserves_outer_continuation() { + let result = eval_ts( + r#" + const output = [1].map(async () => { + let marker = 0; + try { + [1].map(async () => { throw "inner"; }); + } catch (e) { + marker = 7; + } + return marker; + }); + output[0] + "#, + ) + .unwrap(); + assert_eq!(result, Value::Int(7)); +} + +#[test] +fn test_break_out_of_try_removes_handler() { + let result = eval_ts( + r#" + let marker = 0; + try { + for (let i = 0; i < 1; i++) { + try { break; } catch (e) { marker += 1; } + } + null.missing; + } catch (e) { + marker += 10; + } + marker + "#, + ) + .unwrap(); + assert_eq!(result, Value::Int(10)); +} + +#[test] +fn test_continue_out_of_try_removes_handler() { + let result = eval_ts( + r#" + let marker = 0; + try { + for (let i = 0; i < 1; i++) { + try { continue; } catch (e) { marker += 1; } + } + null.missing; + } catch (e) { + marker += 10; + } + marker + "#, + ) + .unwrap(); + assert_eq!(result, Value::Int(10)); +} + +#[test] +fn test_generator_break_out_of_try_does_not_suspend_stale_handler() { + let result = eval_ts( + r#" + function* values() { + let marker = 0; + try { + for (let i = 0; i < 1; i++) { + try { break; } catch (e) { marker += 1; } + } + yield marker; + throw "outer"; + } catch (e) { + marker += 10; + } + yield marker; + } + const iterator = values(); + const first = iterator.next().value; + const second = iterator.next().value; + `${first},${second}` + "#, + ) + .unwrap(); + assert_eq!(result, Value::String("0,10".into())); +} + +#[test] +fn test_generator_continue_out_of_try_does_not_suspend_stale_handler() { + let result = eval_ts( + r#" + function* values() { + let marker = 0; + try { + for (let i = 0; i < 1; i++) { + try { continue; } catch (e) { marker += 1; } + } + yield marker; + throw "outer"; + } catch (e) { + marker += 10; + } + yield marker; + } + const iterator = values(); + const first = iterator.next().value; + const second = iterator.next().value; + `${first},${second}` + "#, + ) + .unwrap(); + assert_eq!(result, Value::String("0,10".into())); }