From e8cf252343f27d84d72180088ec73e140e370ae9 Mon Sep 17 00:00:00 2001 From: "Sergio Gonzalez Martin (from Dev Box)" Date: Mon, 16 Mar 2026 02:36:58 +0100 Subject: [PATCH] fix: Error subclass conformance improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AggregateError: store raw JsValues in errors field (§20.5.7.1) instead of wrapping non-Error values in JsError objects - Error.captureStackTrace: handle PlainObject targets by adding .stack property; return undefined per V8 API contract - Update JsError.errors type from Vec> to Vec - Update all call sites, tests, and doc comments Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/stator_core/src/builtins/error.rs | 30 +++++++++------ .../src/builtins/install_globals.rs | 38 +++++++++---------- crates/stator_core/src/interpreter/mod.rs | 18 ++++----- 3 files changed, 45 insertions(+), 41 deletions(-) diff --git a/crates/stator_core/src/builtins/error.rs b/crates/stator_core/src/builtins/error.rs index 73848a86..01133aa2 100644 --- a/crates/stator_core/src/builtins/error.rs +++ b/crates/stator_core/src/builtins/error.rs @@ -31,7 +31,6 @@ //! `Error.captureStackTrace` / `Error.stackTraceLimit` API. use std::cell::RefCell; -use std::rc::Rc; use crate::error::{StatorError, StatorResult}; use crate::objects::property_map::PropertyMap; @@ -276,7 +275,10 @@ pub struct JsError { /// The formatted stack trace string (captured at construction time). pub stack: String, /// Inner errors for `AggregateError` (empty for all other kinds). - pub errors: Vec>, + /// + /// Per §20.5.7.1 the values are stored as-is from the iterable argument, + /// so arbitrary [`JsValue`]s are kept rather than wrapping in `JsError`. + pub errors: Vec, /// The ES2022 `cause` property — the underlying reason for this error. /// /// Set when the constructor receives an options object with a `cause` @@ -320,19 +322,23 @@ impl JsError { /// Create a new `AggregateError` wrapping `errors` with the given message. /// + /// The `errors` values are stored as-is per §20.5.7.1 — they do not need + /// to be `JsError` instances. + /// /// # Examples /// /// ``` /// use std::rc::Rc; /// use stator_core::builtins::error::{JsError, ErrorKind}; + /// use stator_core::objects::value::JsValue; /// - /// let e1 = Rc::new(JsError::new(ErrorKind::TypeError, "bad type".to_string())); - /// let e2 = Rc::new(JsError::new(ErrorKind::RangeError, "out of range".to_string())); + /// let e1 = JsValue::Error(Rc::new(JsError::new(ErrorKind::TypeError, "bad type".to_string()))); + /// let e2 = JsValue::Smi(42); /// let agg = JsError::new_aggregate(vec![e1, e2], "All promises rejected".to_string()); /// assert_eq!(agg.name(), "AggregateError"); /// assert_eq!(agg.errors.len(), 2); /// ``` - pub fn new_aggregate(errors: Vec>, message: String) -> Self { + pub fn new_aggregate(errors: Vec, message: String) -> Self { let stack = capture_stack_trace("AggregateError", &message); Self { kind: ErrorKind::AggregateError, @@ -507,12 +513,13 @@ pub fn eval_error_new(message: String) -> JsError { /// ``` /// use std::rc::Rc; /// use stator_core::builtins::error::{aggregate_error_new, type_error_new, ErrorKind}; -/// let inner = Rc::new(type_error_new("bad type".to_string())); +/// use stator_core::objects::value::JsValue; +/// let inner = JsValue::Error(Rc::new(type_error_new("bad type".to_string()))); /// let e = aggregate_error_new(vec![inner], "All promises rejected".to_string()); /// assert_eq!(e.kind, ErrorKind::AggregateError); /// assert_eq!(e.errors.len(), 1); /// ``` -pub fn aggregate_error_new(errors: Vec>, message: String) -> JsError { +pub fn aggregate_error_new(errors: Vec, message: String) -> JsError { JsError::new_aggregate(errors, message) } @@ -523,6 +530,7 @@ pub fn aggregate_error_new(errors: Vec>, message: String) -> JsError #[cfg(test)] mod tests { use super::*; + use std::rc::Rc; // ── ErrorKind ──────────────────────────────────────────────────────────── @@ -594,8 +602,8 @@ mod tests { #[test] fn test_aggregate_error_new() { - let e1 = Rc::new(type_error_new("bad type".to_string())); - let e2 = Rc::new(range_error_new("out of range".to_string())); + let e1 = JsValue::Error(Rc::new(type_error_new("bad type".to_string()))); + let e2 = JsValue::Error(Rc::new(range_error_new("out of range".to_string()))); let agg = aggregate_error_new( vec![e1.clone(), e2.clone()], "multiple failures".to_string(), @@ -604,8 +612,8 @@ mod tests { assert_eq!(agg.message(), "multiple failures"); assert_eq!(agg.name(), "AggregateError"); assert_eq!(agg.errors.len(), 2); - assert_eq!(agg.errors[0].kind, ErrorKind::TypeError); - assert_eq!(agg.errors[1].kind, ErrorKind::RangeError); + assert!(matches!(&agg.errors[0], JsValue::Error(e) if e.kind == ErrorKind::TypeError)); + assert!(matches!(&agg.errors[1], JsValue::Error(e) if e.kind == ErrorKind::RangeError)); } // ── to_error_string ────────────────────────────────────────────────────── diff --git a/crates/stator_core/src/builtins/install_globals.rs b/crates/stator_core/src/builtins/install_globals.rs index b6706e0b..6ac7b074 100644 --- a/crates/stator_core/src/builtins/install_globals.rs +++ b/crates/stator_core/src/builtins/install_globals.rs @@ -713,18 +713,8 @@ fn make_aggregate_error_constructor() -> JsValue { let call = native(|args| { // First arg: errors (iterable — we accept Array). let errors_val = args.first().unwrap_or(&JsValue::Undefined); - let inner_errors: Vec> = match errors_val { - JsValue::Array(arr) => arr - .borrow() - .iter() - .map(|v| match v { - JsValue::Error(e) => Rc::clone(e), - other => Rc::new(JsError::new( - ErrorKind::Error, - other.to_js_string().unwrap_or_default(), - )), - }) - .collect(), + let inner_errors: Vec = match errors_val { + JsValue::Array(arr) => arr.borrow().clone(), _ => Vec::new(), }; // Second arg: message. @@ -827,19 +817,27 @@ fn install_error_constructors(globals: &mut HashMap) { }), ); // V8 extension: Error.captureStackTrace(targetObject [, constructorOpt]) + // Adds a `.stack` property to the target and returns undefined. error_props.insert( "captureStackTrace".into(), native(|args| { let target = args.first().cloned().unwrap_or(JsValue::Undefined); - if let JsValue::Error(e) = target { - // Rc::try_unwrap is unlikely to succeed for shared Rcs, so - // clone-and-mutate. - let mut cloned = (*e).clone(); - error_capture_stack_trace(&mut cloned, None); - Ok(JsValue::Error(Rc::new(cloned))) - } else { - Ok(JsValue::Undefined) + match target { + JsValue::Error(e) => { + let mut cloned = (*e).clone(); + error_capture_stack_trace(&mut cloned, None); + // V8 mutates in-place; we cannot mutate Rc so this is + // best-effort. Return undefined per the V8 API contract. + } + JsValue::PlainObject(ref map) => { + // V8: set `.stack` on the plain object. + let stack_str = crate::builtins::error::capture_stack_trace("Error", ""); + map.borrow_mut() + .insert("stack".to_string(), JsValue::String(stack_str.into())); + } + _ => {} } + Ok(JsValue::Undefined) }), ); // V8 extension: Error.stackTraceLimit (getter/setter via property) diff --git a/crates/stator_core/src/interpreter/mod.rs b/crates/stator_core/src/interpreter/mod.rs index 5eddf8d6..6210a0b0 100644 --- a/crates/stator_core/src/interpreter/mod.rs +++ b/crates/stator_core/src/interpreter/mod.rs @@ -4577,12 +4577,7 @@ pub(super) fn proto_lookup(obj: &JsValue, key: &str) -> JsValue { "cause" => e.cause().cloned().unwrap_or(JsValue::Undefined), "errors" => { if e.kind == crate::builtins::error::ErrorKind::AggregateError { - JsValue::new_array( - e.errors - .iter() - .map(|ie| JsValue::Error(Rc::clone(ie))) - .collect(), - ) + JsValue::new_array(e.errors.clone()) } else { JsValue::Undefined } @@ -11259,17 +11254,20 @@ mod tests { fn test_proto_lookup_aggregate_error_errors_property() { use crate::builtins::error::{ErrorKind, JsError}; - let inner1 = Rc::new(JsError::new(ErrorKind::TypeError, "bad type".to_string())); - let inner2 = Rc::new(JsError::new( + let inner1 = JsValue::Error(Rc::new(JsError::new( + ErrorKind::TypeError, + "bad type".to_string(), + ))); + let inner2 = JsValue::Error(Rc::new(JsError::new( ErrorKind::RangeError, "out of range".to_string(), - )); + ))); let agg = JsValue::Error(Rc::new(JsError::new_aggregate( vec![inner1.clone(), inner2.clone()], "multiple failures".to_string(), ))); - // errors property should be an Array of Error values. + // errors property should be an Array of the original values. if let JsValue::Array(arr) = proto_lookup(&agg, "errors") { assert_eq!(arr.borrow().len(), 2); assert!(