Apply code review

boa_engine/Cargo.toml

@@ -53,7 +53,7 @@ once_cell = "1.15.0"
 tap = "1.0.1"
 sptr = "0.3.2"
 static_assertions = "1.1.0"
-thiserror = "1.0.35"
+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

@@ -46,6 +46,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,6 @@
 use crate::{
     environments::CompileTimeEnvironment, error::JsNativeError, object::JsObject,
-    syntax::ast::expression::Identifier, Context, JsResult, JsValue,
+    syntax::ast::expression::Identifier, Context, JsValue,
 };
 use boa_gc::{Cell, Finalize, Gc, Trace};
 
@@ -156,17 +156,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 +796,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.sym())
-                ))
-                .into())
+            Err(JsNativeError::typ().with_message(format!(
+                "cannot mutate an immutable binding '{}'",
+                context.interner().resolve_expect(self.name.sym())
+            )))
         } else {
             Ok(())
         }

boa_engine/src/error/mod.rs

@@ -48,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,15 +195,22 @@ 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()
+                let message = if let Some(msg) = try_get_property("message", context)? {
+                    msg.as_string()
                         .map(JsString::to_std_string)
                         .transpose()
                         .map_err(|_| TryNativeError::InvalidMessageEncoding)?
@@ -207,17 +220,7 @@ impl JsError {
                     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,

boa_engine/src/object/builtins/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

@@ -455,7 +455,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,
@@ -1075,6 +1075,7 @@ impl Object {
         )
     }
 
+    #[inline]
     pub fn as_error(&self) -> Option<ErrorKind> {
         match self.data {
             ObjectData {

boa_engine/src/vm/code_block.rs

@@ -1454,15 +1454,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 { .. }

boa_engine/src/vm/mod.rs

@@ -1577,13 +1577,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();
@@ -1592,32 +1586,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 {