Restrict `ClosureFunction` to only `Copy` closures (#1518)

Cargo.lock

@@ -10,6 +10,7 @@ dependencies = [
  "boa_unicode",
  "chrono",
  "criterion",
+ "dyn-clone",
  "fast-float",
  "float-cmp",
  "gc",
@@ -358,6 +359,12 @@ version = "0.4.8"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "56899898ce76aaf4a0f24d914c97ea6ed976d42fec6ad33fcbb0a1103e07b2b0"
 
+[[package]]
+name = "dyn-clone"
+version = "1.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "ee2626afccd7561a06cf1367e2950c4718ea04565e20fb5029b6c7d8ad09abcf"
+
 [[package]]
 name = "either"
 version = "1.6.1"

boa/Cargo.toml

@@ -37,6 +37,7 @@ ryu-js = "0.2.1"
 chrono = "0.4.19"
 fast-float = "0.2.0"
 unicode-normalization = "0.1.19"
+dyn-clone = "1.0.4"
 
 # Optional Dependencies
 measureme = { version = "9.1.2", optional = true }

boa/examples/closures.rs

@@ -1,14 +1,15 @@
-use boa::{Context, JsString, JsValue};
+use boa::{Context, JsValue};
 
 fn main() -> Result<(), JsValue> {
     let mut context = Context::new();
 
-    let variable = JsString::new("I am a captured variable");
+    let variable = "I am a captured variable";
 
     // We register a global closure function that has the name 'closure' with length 0.
     context.register_global_closure("closure", 0, move |_, _, _| {
         // This value is captured from main function.
-        Ok(variable.clone().into())
+        println!("variable = {}", variable);
+        Ok(JsValue::new(variable))
     })?;
 
     assert_eq!(

boa/src/builtins/function/mod.rs

@@ -15,25 +15,54 @@ use crate::object::PROTOTYPE;
 use crate::{
     builtins::{Array, BuiltIn},
     environment::lexical_environment::Environment,
-    gc::{custom_trace, empty_trace, Finalize, Trace},
+    gc::{empty_trace, Finalize, Trace},
     object::{ConstructorBuilder, FunctionBuilder, JsObject, Object, ObjectData},
     property::{Attribute, PropertyDescriptor},
     syntax::ast::node::{FormalParameter, RcStatementList},
     BoaProfiler, Context, JsResult, JsValue,
 };
 use bitflags::bitflags;
-
+use dyn_clone::DynClone;
+use sealed::Sealed;
 use std::fmt::{self, Debug};
-use std::rc::Rc;
 
 #[cfg(test)]
 mod tests;
 
-/// _fn(this, arguments, context) -> ResultValue_ - The signature of a native built-in function
+// Allows restricting closures to only `Copy` ones.
+// Used the sealed pattern to disallow external implementations
+// of `DynCopy`.
+mod sealed {
+    pub trait Sealed {}
+    impl<T: Copy> Sealed for T {}
+}
+pub trait DynCopy: Sealed {}
+impl<T: Copy> DynCopy for T {}
+
+/// Type representing a native built-in function a.k.a. function pointer.
+///
+/// Native functions need to have this signature in order to
+/// be callable from Javascript.
 pub type NativeFunction = fn(&JsValue, &[JsValue], &mut Context) -> JsResult<JsValue>;
 
-/// _fn(this, arguments, context) -> ResultValue_ - The signature of a closure built-in function
+/// Trait representing a native built-in closure.
-pub type ClosureFunction = dyn Fn(&JsValue, &[JsValue], &mut Context) -> JsResult<JsValue>;
+///
+/// Closures need to have this signature in order to
+/// be callable from Javascript, but most of the time the compiler
+/// is smart enough to correctly infer the types.
+pub trait ClosureFunction:
+    Fn(&JsValue, &[JsValue], &mut Context) -> JsResult<JsValue> + DynCopy + DynClone + 'static
+{
+}
+
+// The `Copy` bound automatically infers `DynCopy` and `DynClone`
+impl<T> ClosureFunction for T where
+    T: Fn(&JsValue, &[JsValue], &mut Context) -> JsResult<JsValue> + Copy + 'static
+{
+}
+
+// Allows cloning Box<dyn ClosureFunction>
+dyn_clone::clone_trait_object!(ClosureFunction);
 
 #[derive(Clone, Copy, Finalize)]
 pub struct BuiltInFunction(pub(crate) NativeFunction);
@@ -83,14 +112,15 @@ unsafe impl Trace for FunctionFlags {
 /// FunctionBody is specific to this interpreter, it will either be Rust code or JavaScript code (AST Node)
 ///
 /// <https://tc39.es/ecma262/#sec-ecmascript-function-objects>
-#[derive(Finalize)]
+#[derive(Trace, Finalize)]
 pub enum Function {
     Native {
         function: BuiltInFunction,
         constructable: bool,
     },
     Closure {
-        function: Rc<ClosureFunction>,
+        #[unsafe_ignore_trace]
+        function: Box<dyn ClosureFunction>,
         constructable: bool,
     },
     Ordinary {
@@ -107,18 +137,6 @@ impl Debug for Function {
     }
 }
 
-unsafe impl Trace for Function {
-    custom_trace!(this, {
-        match this {
-            Function::Native { .. } => {}
-            Function::Closure { .. } => {}
-            Function::Ordinary { environment, .. } => {
-                mark(environment);
-            }
-        }
-    });
-}
-
 impl Function {
     // Adds the final rest parameters to the Environment as an array
     pub(crate) fn add_rest_param(

boa/src/context.rs

@@ -635,7 +635,7 @@ impl Context {
     #[inline]
     pub fn register_global_closure<F>(&mut self, name: &str, length: usize, body: F) -> JsResult<()>
     where
-        F: Fn(&JsValue, &[JsValue], &mut Context) -> JsResult<JsValue> + 'static,
+        F: Fn(&JsValue, &[JsValue], &mut Context) -> JsResult<JsValue> + Copy + 'static,
     {
         let function = FunctionBuilder::closure(self, body)
             .name(name)

boa/src/object/gcobject.rs

@@ -25,7 +25,6 @@ use std::{
     collections::HashMap,
     error::Error,
     fmt::{self, Debug, Display},
-    rc::Rc,
     result::Result as StdResult,
 };
 
@@ -46,7 +45,7 @@ pub struct JsObject(Gc<GcCell<Object>>);
 enum FunctionBody {
     BuiltInFunction(NativeFunction),
     BuiltInConstructor(NativeFunction),
-    Closure(Rc<ClosureFunction>),
+    Closure(Box<dyn ClosureFunction>),
     Ordinary(RcStatementList),
 }
 

boa/src/object/mod.rs

@@ -15,13 +15,12 @@ use crate::{
     context::StandardConstructor,
     gc::{Finalize, Trace},
     property::{Attribute, PropertyDescriptor, PropertyKey},
-    BoaProfiler, Context, JsBigInt, JsString, JsSymbol, JsValue,
+    BoaProfiler, Context, JsBigInt, JsResult, JsString, JsSymbol, JsValue,
 };
 use std::{
     any::Any,
     fmt::{self, Debug, Display},
     ops::{Deref, DerefMut},
-    rc::Rc,
 };
 
 #[cfg(test)]
@@ -1109,12 +1108,12 @@ impl<'context> FunctionBuilder<'context> {
     #[inline]
     pub fn closure<F>(context: &'context mut Context, function: F) -> Self
     where
-        F: Fn(&JsValue, &[JsValue], &mut Context) -> Result<JsValue, JsValue> + 'static,
+        F: Fn(&JsValue, &[JsValue], &mut Context) -> JsResult<JsValue> + Copy + 'static,
     {
         Self {
             context,
             function: Some(Function::Closure {
-                function: Rc::new(function),
+                function: Box::new(function),
                 constructable: false,
             }),
             name: JsString::default(),