Apply code review

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 }

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,

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(())
         }

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<JsNativeError> 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<str>,
     #[source]
     cause: Option<Box<JsError>>,
 }
 
 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<Box<JsError>>) -> Self {
+    fn new(kind: JsNativeErrorKind, message: Box<str>, cause: Option<Box<JsError>>) -> Self {
         Self {
             kind,
             message,
@@ -400,11 +401,7 @@ impl JsNativeError {
     /// ));
     /// ```
     pub fn aggregate(errors: Vec<JsError>) -> 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<S>(mut self, message: S) -> Self
     where
-        S: Into<Cow<'static, str>>,
+        S: Into<Box<str>>,
     {
-        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",

boa_engine/src/object/jsset.rs

@@ -75,9 +75,9 @@ impl JsSet {
     where
         T: Into<JsValue>,
     {
+        // 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<JsValue>,
     {
+        // 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"),
         }
     }

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<ErrorKind> {
         match self.data {
             ObjectData {

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 { .. }