Redesign native functions and closures API (#2499)

This PR is a complete redesign of our current native functions and closures API. I was a bit dissatisfied with our previous design (even though I created it 😆), because it had a lot of superfluous traits, a forced usage of `Gc<GcCell<T>>` and an overly restrictive `NativeObject` bound. This redesign, on the other hand, simplifies a lot our public API, with a simple `NativeCallable` struct that has several constructors for each type of required native function. This new design doesn't require wrapping every capture type with `Gc<GcCell<T>>`, relaxes the trait requirement to `Trace + 'static` for captures, can be reused in both `JsObject` functions and (soonish) host defined functions, and is (in my opinion) a bit cleaner than the previous iteration. It also offers an `unsafe` API as an escape hatch for users that want to pass non-Copy closures which don't capture traceable types. Would ask for bikeshedding about the names though, because I don't know if `NativeCallable` is the most precise name for this. Same about the constructor names; I added the `from` prefix to all of them because it's the "standard" practice, but seeing the API doesn't have any other method aside from `call`, it may be better to just remove the prefix altogether. Let me know what you think :)
boa-dev · Jan 8, 2023 · edd404b · edd404b
1 parent 082d362
commit edd404b
Show file tree

Hide file tree

Showing 46 changed files with 1,897 additions and 1,486 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/boa_engine/Cargo.toml b/boa_engine/Cargo.toml
@@ -57,7 +57,6 @@ ryu-js = "0.2.2"
 chrono = "0.4.23"
 fast-float = "0.2.0"
 unicode-normalization = "0.1.22"
-dyn-clone = "1.0.10"
 once_cell = "1.17.0"
 tap = "1.0.1"
 sptr = "0.3.2"

diff --git a/boa_engine/src/builtins/array/mod.rs b/boa_engine/src/builtins/array/mod.rs
@@ -26,9 +26,10 @@ use crate::{
     context::intrinsics::StandardConstructors,
     error::JsNativeError,
     js_string,
+    native_function::NativeFunction,
     object::{
-        internal_methods::get_prototype_from_constructor, ConstructorBuilder, FunctionBuilder,
-        JsFunction, JsObject, ObjectData,
+        internal_methods::get_prototype_from_constructor, ConstructorBuilder,
+        FunctionObjectBuilder, JsFunction, JsObject, ObjectData,
     },
     property::{Attribute, PropertyDescriptor, PropertyNameKind},
     symbol::WellKnownSymbols,
@@ -50,10 +51,11 @@ impl BuiltIn for Array {
         let symbol_iterator = WellKnownSymbols::iterator();
         let symbol_unscopables = WellKnownSymbols::unscopables();
 
-        let get_species = FunctionBuilder::native(context, Self::get_species)
-            .name("get [Symbol.species]")
-            .constructor(false)
-            .build();
+        let get_species =
+            FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::get_species))
+                .name("get [Symbol.species]")
+                .constructor(false)
+                .build();
 
         let values_function = context.intrinsics().objects().array_prototype_values();
         let unscopables_object = Self::unscopables_intrinsic(context);
@@ -2847,7 +2849,7 @@ impl Array {
 
     /// Creates an `Array.prototype.values( )` function object.
     pub(crate) fn create_array_prototype_values(context: &mut Context<'_>) -> JsFunction {
-        FunctionBuilder::native(context, Self::values)
+        FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::values))
             .name("values")
             .length(0)
             .constructor(false)

diff --git a/boa_engine/src/builtins/array_buffer/mod.rs b/boa_engine/src/builtins/array_buffer/mod.rs
@@ -14,9 +14,10 @@ use crate::{
     builtins::{typed_array::TypedArrayKind, BuiltIn, JsArgs},
     context::intrinsics::StandardConstructors,
     error::JsNativeError,
+    native_function::NativeFunction,
     object::{
-        internal_methods::get_prototype_from_constructor, ConstructorBuilder, FunctionBuilder,
-        JsObject, ObjectData,
+        internal_methods::get_prototype_from_constructor, ConstructorBuilder,
+        FunctionObjectBuilder, JsObject, ObjectData,
     },
     property::Attribute,
     symbol::WellKnownSymbols,
@@ -55,14 +56,16 @@ impl BuiltIn for ArrayBuffer {
 
         let flag_attributes = Attribute::CONFIGURABLE | Attribute::NON_ENUMERABLE;
 
-        let get_species = FunctionBuilder::native(context, Self::get_species)
-            .name("get [Symbol.species]")
-            .constructor(false)
-            .build();
+        let get_species =
+            FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::get_species))
+                .name("get [Symbol.species]")
+                .constructor(false)
+                .build();
 
-        let get_byte_length = FunctionBuilder::native(context, Self::get_byte_length)
-            .name("get byteLength")
-            .build();
+        let get_byte_length =
+            FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::get_byte_length))
+                .name("get byteLength")
+                .build();
 
         ConstructorBuilder::with_standard_constructor(
             context,

diff --git a/boa_engine/src/builtins/async_function/mod.rs b/boa_engine/src/builtins/async_function/mod.rs
@@ -12,6 +12,7 @@ use crate::{
         function::{ConstructorKind, Function},
         BuiltIn,
     },
+    native_function::NativeFunction,
     object::ObjectData,
     property::PropertyDescriptor,
     symbol::WellKnownSymbols,
@@ -62,7 +63,7 @@ impl BuiltIn for AsyncFunction {
             .configurable(false);
         constructor.borrow_mut().insert("prototype", property);
         constructor.borrow_mut().data = ObjectData::function(Function::Native {
-            function: Self::constructor,
+            function: NativeFunction::from_fn_ptr(Self::constructor),
             constructor: Some(ConstructorKind::Base),
         });
 

diff --git a/boa_engine/src/builtins/async_generator/mod.rs b/boa_engine/src/builtins/async_generator/mod.rs
@@ -11,7 +11,8 @@ use crate::{
         promise::if_abrupt_reject_promise, promise::PromiseCapability, BuiltIn, JsArgs, Promise,
     },
     error::JsNativeError,
-    object::{ConstructorBuilder, FunctionBuilder, JsObject, ObjectData},
+    native_function::NativeFunction,
+    object::{ConstructorBuilder, FunctionObjectBuilder, JsObject, ObjectData},
     property::{Attribute, PropertyDescriptor},
     symbol::WellKnownSymbols,
     value::JsValue,
@@ -627,65 +628,69 @@ impl AsyncGenerator {
 
         // 7. Let fulfilledClosure be a new Abstract Closure with parameters (value) that captures generator and performs the following steps when called:
         // 8. Let onFulfilled be CreateBuiltinFunction(fulfilledClosure, 1, "", « »).
-        let on_fulfilled = FunctionBuilder::closure_with_captures(
+        let on_fulfilled = FunctionObjectBuilder::new(
             context,
-            |_this, args, generator, context| {
-                let mut generator_borrow_mut = generator.borrow_mut();
-                let gen = generator_borrow_mut
-                    .as_async_generator_mut()
-                    .expect("already checked before");
+            NativeFunction::from_copy_closure_with_captures(
+                |_this, args, generator, context| {
+                    let mut generator_borrow_mut = generator.borrow_mut();
+                    let gen = generator_borrow_mut
+                        .as_async_generator_mut()
+                        .expect("already checked before");
 
-                // a. Set generator.[[AsyncGeneratorState]] to completed.
-                gen.state = AsyncGeneratorState::Completed;
+                    // a. Set generator.[[AsyncGeneratorState]] to completed.
+                    gen.state = AsyncGeneratorState::Completed;
 
-                // b. Let result be NormalCompletion(value).
-                let result = Ok(args.get_or_undefined(0).clone());
+                    // b. Let result be NormalCompletion(value).
+                    let result = Ok(args.get_or_undefined(0).clone());
 
-                // c. Perform AsyncGeneratorCompleteStep(generator, result, true).
-                let next = gen.queue.pop_front().expect("must have one entry");
-                drop(generator_borrow_mut);
-                Self::complete_step(&next, result, true, context);
+                    // c. Perform AsyncGeneratorCompleteStep(generator, result, true).
+                    let next = gen.queue.pop_front().expect("must have one entry");
+                    drop(generator_borrow_mut);
+                    Self::complete_step(&next, result, true, context);
 
-                // d. Perform AsyncGeneratorDrainQueue(generator).
-                Self::drain_queue(generator, context);
+                    // d. Perform AsyncGeneratorDrainQueue(generator).
+                    Self::drain_queue(generator, context);
 
-                // e. Return undefined.
-                Ok(JsValue::undefined())
-            },
-            generator.clone(),
+                    // e. Return undefined.
+                    Ok(JsValue::undefined())
+                },
+                generator.clone(),
+            ),
         )
         .name("")
         .length(1)
         .build();
 
         // 9. Let rejectedClosure be a new Abstract Closure with parameters (reason) that captures generator and performs the following steps when called:
         // 10. Let onRejected be CreateBuiltinFunction(rejectedClosure, 1, "", « »).
-        let on_rejected = FunctionBuilder::closure_with_captures(
+        let on_rejected = FunctionObjectBuilder::new(
             context,
-            |_this, args, generator, context| {
-                let mut generator_borrow_mut = generator.borrow_mut();
-                let gen = generator_borrow_mut
-                    .as_async_generator_mut()
-                    .expect("already checked before");
+            NativeFunction::from_copy_closure_with_captures(
+                |_this, args, generator, context| {
+                    let mut generator_borrow_mut = generator.borrow_mut();
+                    let gen = generator_borrow_mut
+                        .as_async_generator_mut()
+                        .expect("already checked before");
 
-                // a. Set generator.[[AsyncGeneratorState]] to completed.
-                gen.state = AsyncGeneratorState::Completed;
+                    // a. Set generator.[[AsyncGeneratorState]] to completed.
+                    gen.state = AsyncGeneratorState::Completed;
 
-                // b. Let result be ThrowCompletion(reason).
-                let result = Err(JsError::from_opaque(args.get_or_undefined(0).clone()));
+                    // b. Let result be ThrowCompletion(reason).
+                    let result = Err(JsError::from_opaque(args.get_or_undefined(0).clone()));
 
-                // c. Perform AsyncGeneratorCompleteStep(generator, result, true).
-                let next = gen.queue.pop_front().expect("must have one entry");
-                drop(generator_borrow_mut);
-                Self::complete_step(&next, result, true, context);
+                    // c. Perform AsyncGeneratorCompleteStep(generator, result, true).
+                    let next = gen.queue.pop_front().expect("must have one entry");
+                    drop(generator_borrow_mut);
+                    Self::complete_step(&next, result, true, context);
 
-                // d. Perform AsyncGeneratorDrainQueue(generator).
-                Self::drain_queue(generator, context);
+                    // d. Perform AsyncGeneratorDrainQueue(generator).
+                    Self::drain_queue(generator, context);
 
-                // e. Return undefined.
-                Ok(JsValue::undefined())
-            },
-            generator,
+                    // e. Return undefined.
+                    Ok(JsValue::undefined())
+                },
+                generator,
+            ),
         )
         .name("")
         .length(1)

diff --git a/boa_engine/src/builtins/async_generator_function/mod.rs b/boa_engine/src/builtins/async_generator_function/mod.rs
@@ -10,6 +10,7 @@ use crate::{
         function::{BuiltInFunctionObject, ConstructorKind, Function},
         BuiltIn,
     },
+    native_function::NativeFunction,
     object::ObjectData,
     property::PropertyDescriptor,
     symbol::WellKnownSymbols,
@@ -67,7 +68,7 @@ impl BuiltIn for AsyncGeneratorFunction {
             .configurable(false);
         constructor.borrow_mut().insert("prototype", property);
         constructor.borrow_mut().data = ObjectData::function(Function::Native {
-            function: Self::constructor,
+            function: NativeFunction::from_fn_ptr(Self::constructor),
             constructor: Some(ConstructorKind::Base),
         });
 

diff --git a/boa_engine/src/builtins/dataview/mod.rs b/boa_engine/src/builtins/dataview/mod.rs
@@ -11,9 +11,10 @@ use crate::{
     builtins::{array_buffer::SharedMemoryOrder, typed_array::TypedArrayKind, BuiltIn, JsArgs},
     context::intrinsics::StandardConstructors,
     error::JsNativeError,
+    native_function::NativeFunction,
     object::{
-        internal_methods::get_prototype_from_constructor, ConstructorBuilder, FunctionBuilder,
-        JsObject, ObjectData,
+        internal_methods::get_prototype_from_constructor, ConstructorBuilder,
+        FunctionObjectBuilder, JsObject, ObjectData,
     },
     property::Attribute,
     symbol::WellKnownSymbols,
@@ -37,17 +38,20 @@ impl BuiltIn for DataView {
     fn init(context: &mut Context<'_>) -> Option<JsValue> {
         let flag_attributes = Attribute::CONFIGURABLE | Attribute::NON_ENUMERABLE;
 
-        let get_buffer = FunctionBuilder::native(context, Self::get_buffer)
-            .name("get buffer")
-            .build();
+        let get_buffer =
+            FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::get_buffer))
+                .name("get buffer")
+                .build();
 
-        let get_byte_length = FunctionBuilder::native(context, Self::get_byte_length)
-            .name("get byteLength")
-            .build();
+        let get_byte_length =
+            FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::get_byte_length))
+                .name("get byteLength")
+                .build();
 
-        let get_byte_offset = FunctionBuilder::native(context, Self::get_byte_offset)
-            .name("get byteOffset")
-            .build();
+        let get_byte_offset =
+            FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::get_byte_offset))
+                .name("get byteOffset")
+                .build();
 
         ConstructorBuilder::with_standard_constructor(
             context,

diff --git a/boa_engine/src/builtins/error/type.rs b/boa_engine/src/builtins/error/type.rs
@@ -19,6 +19,7 @@ use crate::{
     builtins::{function::Function, BuiltIn, JsArgs},
     context::intrinsics::StandardConstructors,
     error::JsNativeError,
+    native_function::NativeFunction,
     object::{
         internal_methods::get_prototype_from_constructor, ConstructorBuilder, JsObject, ObjectData,
     },
@@ -103,7 +104,7 @@ pub(crate) fn create_throw_type_error(context: &mut Context<'_>) -> JsObject {
     let function = JsObject::from_proto_and_data(
         context.intrinsics().constructors().function().prototype(),
         ObjectData::function(Function::Native {
-            function: throw_type_error,
+            function: NativeFunction::from_fn_ptr(throw_type_error),
             constructor: None,
         }),
     );

diff --git a/boa_engine/src/builtins/eval/mod.rs b/boa_engine/src/builtins/eval/mod.rs
@@ -13,7 +13,8 @@ use crate::{
     builtins::{BuiltIn, JsArgs},
     environments::DeclarativeEnvironment,
     error::JsNativeError,
-    object::FunctionBuilder,
+    native_function::NativeFunction,
+    object::FunctionObjectBuilder,
     property::Attribute,
     Context, JsResult, JsString, JsValue,
 };
@@ -37,7 +38,7 @@ impl BuiltIn for Eval {
     fn init(context: &mut Context<'_>) -> Option<JsValue> {
         let _timer = Profiler::global().start_event(Self::NAME, "init");
 
-        let object = FunctionBuilder::native(context, Self::eval)
+        let object = FunctionObjectBuilder::new(context, NativeFunction::from_fn_ptr(Self::eval))
             .name("eval")
             .length(1)
             .constructor(false)