diff --git a/Cargo.lock b/Cargo.lock index f59d100c71f..c8014f2de8c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -84,7 +84,6 @@ dependencies = [ "criterion", "dyn-clone", "fast-float", - "flexstr", "float-cmp", "gc", "icu_datetime", @@ -527,15 +526,6 @@ dependencies = [ "writeable", ] -[[package]] -name = "flexstr" -version = "0.9.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4d50aef14619d336a54fca5a592d952eb39037b1a1e7e6afd9f91c892ac7ef65" -dependencies = [ - "static_assertions", -] - [[package]] name = "float-cmp" version = "0.9.0" diff --git a/boa_engine/Cargo.toml b/boa_engine/Cargo.toml index cb0ff03f36d..364c265211a 100644 --- a/boa_engine/Cargo.toml +++ b/boa_engine/Cargo.toml @@ -50,8 +50,7 @@ unicode-normalization = "0.1.22" dyn-clone = "1.0.9" once_cell = "1.15.0" tap = "1.0.1" -thiserror = "1.0.35" -flexstr = "0.9.2" +thiserror = "1.0.37" icu_locale_canonicalizer = { version = "0.6.0", features = ["serde"], optional = true } icu_locid = { version = "0.6.0", features = ["serde"], optional = true } icu_datetime = { version = "0.6.0", features = ["serde"], optional = true } diff --git a/boa_engine/src/builtins/error/mod.rs b/boa_engine/src/builtins/error/mod.rs index 9e1ad67563a..bec2d2e4364 100644 --- a/boa_engine/src/builtins/error/mod.rs +++ b/boa_engine/src/builtins/error/mod.rs @@ -44,6 +44,18 @@ pub(crate) use self::uri::UriError; use super::JsArgs; +/// The kind of a `NativeError` object, per the [ECMAScript spec][spec]. +/// +/// This is used internally to convert between [`JsObject`] and +/// [`JsNativeError`] correctly, but it can also be used to manually create `Error` +/// objects. However, the recommended way to create them is to construct a +/// `JsNativeError` first, then call [`JsNativeError::to_opaque`], +/// which will assign its prototype, properties and kind automatically. +/// +/// For a description of every error kind and its usage, see +/// [`JsNativeErrorKind`][crate::error::JsNativeErrorKind]. +/// +/// [spec]: https://tc39.es/ecma262/#sec-error-objects #[derive(Debug, Copy, Clone, Eq, PartialEq)] pub enum ErrorKind { Aggregate, diff --git a/boa_engine/src/environments/runtime.rs b/boa_engine/src/environments/runtime.rs index 510da1d9a45..1028d299d2a 100644 --- a/boa_engine/src/environments/runtime.rs +++ b/boa_engine/src/environments/runtime.rs @@ -1,6 +1,5 @@ use crate::{ - environments::CompileTimeEnvironment, error::JsNativeError, object::JsObject, Context, - JsResult, JsValue, + environments::CompileTimeEnvironment, error::JsNativeError, object::JsObject, Context, JsValue, }; use boa_gc::{Cell, Finalize, Gc, Trace}; use boa_interner::Sym; @@ -156,17 +155,17 @@ impl FunctionSlots { /// - [ECMAScript specification][spec] /// /// [spec]: https://tc39.es/ecma262/#sec-function-environment-records-getthisbinding - pub(crate) fn get_this_binding(&self) -> Option<&JsValue> { + pub(crate) fn get_this_binding(&self) -> Result<&JsValue, JsNativeError> { // 1. Assert: envRec.[[ThisBindingStatus]] is not lexical. debug_assert!(self.this_binding_status != ThisBindingStatus::Lexical); // 2. If envRec.[[ThisBindingStatus]] is uninitialized, throw a ReferenceError exception. if self.this_binding_status == ThisBindingStatus::Uninitialized { - return None; + Err(JsNativeError::reference().with_message("Must call super constructor in derived class before accessing 'this' or returning from derived constructor")) + } else { + // 3. Return envRec.[[ThisValue]]. + Ok(&self.this) } - - // 3. Return envRec.[[ThisValue]]. - Some(&self.this) } } @@ -796,14 +795,15 @@ impl BindingLocator { /// Helper method to throws an error if the binding access is illegal. #[inline] - pub(crate) fn throw_mutate_immutable(&self, context: &mut Context) -> JsResult<()> { + pub(crate) fn throw_mutate_immutable( + &self, + context: &mut Context, + ) -> Result<(), JsNativeError> { if self.mutate_immutable { - Err(JsNativeError::typ() - .with_message(format!( - "cannot mutate an immutable binding '{}'", - context.interner().resolve_expect(self.name) - )) - .into()) + Err(JsNativeError::typ().with_message(format!( + "cannot mutate an immutable binding '{}'", + context.interner().resolve_expect(self.name) + ))) } else { Ok(()) } diff --git a/boa_engine/src/error.rs b/boa_engine/src/error.rs index 744ee6dfe44..9ac2f844a5a 100644 --- a/boa_engine/src/error.rs +++ b/boa_engine/src/error.rs @@ -9,8 +9,6 @@ use crate::{ Context, JsValue, }; use boa_gc::{Finalize, Trace}; -use flexstr::LocalStr; -use std::{borrow::Cow, fmt::Display}; use thiserror::Error; /// The error type returned by all operations related @@ -50,10 +48,16 @@ pub struct JsError { inner: Repr, } -// hiding the enum makes it a bit more difficult -// to treat `JsError` as a proper enum, which should -// make it a bit harder to try to match against a native error -// without calling `try_native` first. +/// Internal representation of a [`JsError`]. +/// +/// `JsError` is represented by an opaque enum because it restricts +/// matching against `JsError` without calling `try_native` first. +/// This allows us to provide a safe API for `Error` objects that extracts +/// their info as a native `Rust` type ([`JsNativeError`]). +/// +/// This should never be used outside of this module. If that's not the case, +/// you should add methods to either `JsError` or `JsNativeError` to +/// represent that special use case. #[derive(Debug, Clone, Trace, Finalize)] enum Repr { Native(JsNativeError), @@ -189,32 +193,30 @@ impl JsError { .as_error() .ok_or_else(|| TryNativeError::NotAnErrorObject(val.clone()))?; - let message_err = |e| TryNativeError::InaccessibleProperty { - property: "message", - source: e, + let try_get_property = |key, context: &mut Context| { + obj.has_property(key, context) + .map_err(|e| TryNativeError::InaccessibleProperty { + property: key, + source: e, + })? + .then(|| obj.get(key, context)) + .transpose() + .map_err(|e| TryNativeError::InaccessibleProperty { + property: key, + source: e, + }) }; - let message = if obj.has_property("message", context).map_err(message_err)? { - obj.get("message", context) - .map_err(message_err)? - .as_string() - .map(LocalStr::from_ref) + let message = if let Some(msg) = try_get_property("message", context)? { + msg.as_string() .ok_or(TryNativeError::InvalidPropertyType("message"))? + .as_str() + .into() } else { - JsNativeError::DEFAULT_MSG + Box::default() }; - let cause_err = |e| TryNativeError::InaccessibleProperty { - property: "cause", - source: e, - }; - - let cause = obj - .has_property("cause", context) - .map_err(cause_err)? - .then(|| obj.get("cause", context)) - .transpose() - .map_err(cause_err)?; + let cause = try_get_property("cause", context)?; let kind = match error { ErrorKind::Error => JsNativeErrorKind::Error, @@ -328,7 +330,7 @@ impl From for JsError { } } -impl Display for JsError { +impl std::fmt::Display for JsError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.inner { Repr::Native(e) => e.fmt(f), @@ -364,16 +366,15 @@ pub struct JsNativeError { /// The kind of native error (e.g. `TypeError`, `SyntaxError`, etc.) pub kind: JsNativeErrorKind, #[unsafe_ignore_trace] - message: LocalStr, + message: Box, #[source] cause: Option>, } impl JsNativeError { - const DEFAULT_MSG: LocalStr = LocalStr::from_static(""); /// Creates a new `JsNativeError` from its `kind`, `message` and (optionally) /// its `cause`. - fn new(kind: JsNativeErrorKind, message: LocalStr, cause: Option>) -> Self { + fn new(kind: JsNativeErrorKind, message: Box, cause: Option>) -> Self { Self { kind, message, @@ -400,11 +401,7 @@ impl JsNativeError { /// )); /// ``` pub fn aggregate(errors: Vec) -> Self { - Self::new( - JsNativeErrorKind::Aggregate(errors), - Self::DEFAULT_MSG, - None, - ) + Self::new(JsNativeErrorKind::Aggregate(errors), Box::default(), None) } /// Creates a new `JsNativeError` of kind `Error`, with empty `message` @@ -419,7 +416,7 @@ impl JsNativeError { /// assert!(matches!(error.kind, JsNativeErrorKind::Error)); /// ``` pub fn error() -> Self { - Self::new(JsNativeErrorKind::Error, Self::DEFAULT_MSG, None) + Self::new(JsNativeErrorKind::Error, Box::default(), None) } /// Creates a new `JsNativeError` of kind `EvalError`, with empty `message` @@ -434,7 +431,7 @@ impl JsNativeError { /// assert!(matches!(error.kind, JsNativeErrorKind::Eval)); /// ``` pub fn eval() -> Self { - Self::new(JsNativeErrorKind::Eval, Self::DEFAULT_MSG, None) + Self::new(JsNativeErrorKind::Eval, Box::default(), None) } /// Creates a new `JsNativeError` of kind `RangeError`, with empty `message` @@ -449,7 +446,7 @@ impl JsNativeError { /// assert!(matches!(error.kind, JsNativeErrorKind::Range)); /// ``` pub fn range() -> Self { - Self::new(JsNativeErrorKind::Range, Self::DEFAULT_MSG, None) + Self::new(JsNativeErrorKind::Range, Box::default(), None) } /// Creates a new `JsNativeError` of kind `ReferenceError`, with empty `message` @@ -464,7 +461,7 @@ impl JsNativeError { /// assert!(matches!(error.kind, JsNativeErrorKind::Reference)); /// ``` pub fn reference() -> Self { - Self::new(JsNativeErrorKind::Reference, Self::DEFAULT_MSG, None) + Self::new(JsNativeErrorKind::Reference, Box::default(), None) } /// Creates a new `JsNativeError` of kind `SyntaxError`, with empty `message` @@ -479,7 +476,7 @@ impl JsNativeError { /// assert!(matches!(error.kind, JsNativeErrorKind::Syntax)); /// ``` pub fn syntax() -> Self { - Self::new(JsNativeErrorKind::Syntax, Self::DEFAULT_MSG, None) + Self::new(JsNativeErrorKind::Syntax, Box::default(), None) } /// Creates a new `JsNativeError` of kind `TypeError`, with empty `message` @@ -494,7 +491,7 @@ impl JsNativeError { /// assert!(matches!(error.kind, JsNativeErrorKind::Type)); /// ``` pub fn typ() -> Self { - Self::new(JsNativeErrorKind::Type, Self::DEFAULT_MSG, None) + Self::new(JsNativeErrorKind::Type, Box::default(), None) } /// Creates a new `JsNativeError` of kind `UriError`, with empty `message` @@ -509,7 +506,7 @@ impl JsNativeError { /// assert!(matches!(error.kind, JsNativeErrorKind::Uri)); /// ``` pub fn uri() -> Self { - Self::new(JsNativeErrorKind::Uri, Self::DEFAULT_MSG, None) + Self::new(JsNativeErrorKind::Uri, Box::default(), None) } /// Appends a message to this error. @@ -532,13 +529,9 @@ impl JsNativeError { #[must_use] pub fn with_message(mut self, message: S) -> Self where - S: Into>, + S: Into>, { - let message = match message.into() { - Cow::Borrowed(str) => LocalStr::from_static(str), - Cow::Owned(string) => LocalStr::from_ref(string), - }; - self.message = message; + self.message = message.into(); self } @@ -770,7 +763,7 @@ pub enum JsNativeErrorKind { Uri, } -impl Display for JsNativeErrorKind { +impl std::fmt::Display for JsNativeErrorKind { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { JsNativeErrorKind::Aggregate(_) => "AggregateError", diff --git a/boa_engine/src/object/jsset.rs b/boa_engine/src/object/jsset.rs index 7273594bb56..57eb612ab24 100644 --- a/boa_engine/src/object/jsset.rs +++ b/boa_engine/src/object/jsset.rs @@ -75,9 +75,9 @@ impl JsSet { where T: Into, { + // TODO: Make `delete` return a native `bool` match Set::delete(&self.inner.clone().into(), &[value.into()], context)? { JsValue::Boolean(bool) => Ok(bool), - // TODO: find a better solution to this, without impacting perf _ => unreachable!("`delete` must always return a bool"), } } @@ -91,9 +91,9 @@ impl JsSet { where T: Into, { + // TODO: Make `has` return a native `bool` match Set::has(&self.inner.clone().into(), &[value.into()], context)? { JsValue::Boolean(bool) => Ok(bool), - // TODO: find a better solution to this, without impacting perf _ => unreachable!("`has` must always return a bool"), } } diff --git a/boa_engine/src/object/mod.rs b/boa_engine/src/object/mod.rs index 5cfd10bb31c..d6d5f84a7ba 100644 --- a/boa_engine/src/object/mod.rs +++ b/boa_engine/src/object/mod.rs @@ -463,7 +463,7 @@ impl ObjectData { } /// Create the `Error` object data - pub fn error(error: ErrorKind) -> Self { + pub(crate) fn error(error: ErrorKind) -> Self { Self { kind: ObjectKind::Error(error), internal_methods: &ORDINARY_INTERNAL_METHODS, @@ -1083,6 +1083,7 @@ impl Object { ) } + #[inline] pub fn as_error(&self) -> Option { match self.data { ObjectData { diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index 6bf4182a19d..c9cf58fcb7a 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/boa_engine/src/vm/code_block.rs @@ -1443,15 +1443,12 @@ impl JsObject { .expect("must be function environment") .as_function_slots() .expect("must be function environment"); - if let Some(this_binding) = function_env.borrow().get_this_binding() { - Ok(this_binding - .as_object() - .expect("this binding must be object") - .clone()) - } else { - //Err(JsNativeError::typ().with_message("Function constructor must not return non-object").into()) - Err(JsNativeError::reference().with_message("Must call super constructor in derived class before accessing 'this' or returning from derived constructor").into()) - } + Ok(function_env + .borrow() + .get_this_binding()? + .as_object() + .expect("this binding must be object") + .clone()) } } Function::Generator { .. } diff --git a/boa_engine/src/vm/mod.rs b/boa_engine/src/vm/mod.rs index 2afda98fe02..a9fa8d683a8 100644 --- a/boa_engine/src/vm/mod.rs +++ b/boa_engine/src/vm/mod.rs @@ -1530,13 +1530,7 @@ impl Context { let env = self.realm.environments.get_this_environment(); match env { EnvironmentSlots::Function(env) => { - let env_b = env.borrow(); - if let Some(this) = env_b.get_this_binding() { - self.vm.push(this); - } else { - drop(env_b); - return Err(JsNativeError::reference().with_message("Must call super constructor in derived class before accessing 'this' or returning from derived constructor").into()); - } + self.vm.push(env.borrow().get_this_binding()?); } EnvironmentSlots::Global => { let this = self.realm.global_object(); @@ -1545,32 +1539,21 @@ impl Context { } } Opcode::Super => { - let env = self - .realm - .environments - .get_this_environment() - .as_function_slots() - .expect("super access must be in a function environment"); - - let home = if env.borrow().get_this_binding().is_some() { + let home = { + let env = self + .realm + .environments + .get_this_environment() + .as_function_slots() + .expect("super access must be in a function environment"); let env = env.borrow(); + let this = env.get_this_binding()?; let function_object = env.function_object().borrow(); let function = function_object .as_function() .expect("must be function object"); - let mut home_object = function.get_home_object().cloned(); - - if home_object.is_none() { - home_object = env - .get_this_binding() - .expect("can not get `this` object") - .as_object() - .cloned(); - } - home_object - } else { - return Err(JsNativeError::range().with_message("Must call super constructor in derived class before accessing 'this' or returning from derived constructor").into()); + function.get_home_object().or(this.as_object()).cloned() }; if let Some(home) = home {