From fe9c5aff204c594631e71b6b52f470eaffda3ae3 Mon Sep 17 00:00:00 2001 From: jedel1043 Date: Sun, 2 Oct 2022 16:21:35 -0500 Subject: [PATCH] Apply code review --- boa_engine/Cargo.toml | 2 +- boa_engine/src/builtins/error/mod.rs | 12 +++++++ boa_engine/src/environments/runtime.rs | 27 +++++++------- boa_engine/src/error/mod.rs | 47 +++++++++++++------------ boa_engine/src/object/builtins/jsset.rs | 4 +-- boa_engine/src/object/mod.rs | 3 +- boa_engine/src/vm/code_block.rs | 15 ++++---- boa_engine/src/vm/mod.rs | 37 ++++++------------- 8 files changed, 72 insertions(+), 75 deletions(-) diff --git a/boa_engine/Cargo.toml b/boa_engine/Cargo.toml index a93bdd72960..3ea19abbafb 100644 --- a/boa_engine/Cargo.toml +++ b/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 } diff --git a/boa_engine/src/builtins/error/mod.rs b/boa_engine/src/builtins/error/mod.rs index b4a9d0c4e93..863417d7795 100644 --- a/boa_engine/src/builtins/error/mod.rs +++ b/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, diff --git a/boa_engine/src/environments/runtime.rs b/boa_engine/src/environments/runtime.rs index 54f6a8b9232..e7d2be0e77a 100644 --- a/boa_engine/src/environments/runtime.rs +++ b/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(()) } diff --git a/boa_engine/src/error/mod.rs b/boa_engine/src/error/mod.rs index 2544f694a4b..56903274e69 100644 --- a/boa_engine/src/error/mod.rs +++ b/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, diff --git a/boa_engine/src/object/builtins/jsset.rs b/boa_engine/src/object/builtins/jsset.rs index 9970594b4ff..b2a78ed1ff1 100644 --- a/boa_engine/src/object/builtins/jsset.rs +++ b/boa_engine/src/object/builtins/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 2adb80352a1..dbcd4225884 100644 --- a/boa_engine/src/object/mod.rs +++ b/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 { match self.data { ObjectData { diff --git a/boa_engine/src/vm/code_block.rs b/boa_engine/src/vm/code_block.rs index 973ad8a1846..3e63f15226d 100644 --- a/boa_engine/src/vm/code_block.rs +++ b/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 { .. } diff --git a/boa_engine/src/vm/mod.rs b/boa_engine/src/vm/mod.rs index f11e1af6bba..1fe3d507824 100644 --- a/boa_engine/src/vm/mod.rs +++ b/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 {