feat(napi): property getter and setter with closure (#1526)

* getter with closure with segment fault

* fix getter closure pointer

* add setter

* Cleanup API

* Add test for `create_function_from_closure`

* Fix compile error

* Fix flaky test title

---------

Co-authored-by: LongYinan <lynweklm@gmail.com>

crates/napi/src/env.rs

@@ -569,9 +569,7 @@ impl Env {
     F: 'static + Fn(crate::CallContext<'_>) -> Result<R>,
     R: ToNapiValue,
   {
-    use crate::CallContext;
+    let closure_data_ptr = Box::into_raw(Box::new(callback));
-    let boxed_callback = Box::new(callback);
-    let closure_data_ptr: *mut F = Box::into_raw(boxed_callback);
 
     let mut raw_result = ptr::null_mut();
     let len = name.len();
@@ -581,85 +579,7 @@ impl Env {
         self.0,
         name.as_ptr(),
         len,
-        Some({
+        Some(trampoline::<R, F>),
-          unsafe extern "C" fn trampoline<R: ToNapiValue, F: Fn(CallContext<'_>) -> Result<R>>(
-            raw_env: sys::napi_env,
-            cb_info: sys::napi_callback_info,
-          ) -> sys::napi_value {
-            use ::std::panic::{self, AssertUnwindSafe};
-            panic::catch_unwind(AssertUnwindSafe(|| {
-              let (raw_this, ref raw_args, closure_data_ptr) = {
-                let argc = {
-                  let mut argc = 0;
-                  let status = unsafe {
-                    sys::napi_get_cb_info(
-                      raw_env,
-                      cb_info,
-                      &mut argc,
-                      ptr::null_mut(),
-                      ptr::null_mut(),
-                      ptr::null_mut(),
-                    )
-                  };
-                  debug_assert!(
-                    Status::from(status) == Status::Ok,
-                    "napi_get_cb_info failed"
-                  );
-                  argc
-                };
-                let mut raw_args = vec![ptr::null_mut(); argc];
-                let mut raw_this = ptr::null_mut();
-                let mut closure_data_ptr = ptr::null_mut();
-
-                let status = unsafe {
-                  sys::napi_get_cb_info(
-                    raw_env,
-                    cb_info,
-                    &mut { argc },
-                    raw_args.as_mut_ptr(),
-                    &mut raw_this,
-                    &mut closure_data_ptr,
-                  )
-                };
-                debug_assert!(
-                  Status::from(status) == Status::Ok,
-                  "napi_get_cb_info failed"
-                );
-                (raw_this, raw_args, closure_data_ptr)
-              };
-
-              let closure: &F = unsafe {
-                closure_data_ptr
-                  .cast::<F>()
-                  .as_ref()
-                  .expect("`napi_get_cb_info` should have yielded non-`NULL` assoc data")
-              };
-              let env = &mut unsafe { Env::from_raw(raw_env) };
-              let ctx = CallContext::new(env, cb_info, raw_this, raw_args, raw_args.len());
-              closure(ctx)
-                .and_then(|ret: R| unsafe { <R as ToNapiValue>::to_napi_value(env.0, ret) })
-            }))
-            .map_err(|e| {
-              Error::from_reason(format!(
-                "panic from Rust code: {}",
-                if let Some(s) = e.downcast_ref::<String>() {
-                  s
-                } else if let Some(s) = e.downcast_ref::<&str>() {
-                  s
-                } else {
-                  "<no error message>"
-                },
-              ))
-            })
-            .and_then(|v| v)
-            .unwrap_or_else(|e| {
-              unsafe { JsError::from(e).throw_into(raw_env) };
-              ptr::null_mut()
-            })
-          }
-
-          trampoline::<R, F>
-        }),
         closure_data_ptr.cast(), // We let it borrow the data here
         &mut raw_result,
       )
@@ -678,17 +598,7 @@ impl Env {
         self.0,
         raw_result,
         closure_data_ptr.cast(),
-        Some({
+        Some(finalize_box_trampoline::<F>),
-          unsafe extern "C" fn finalize_box_trampoline<F>(
-            _raw_env: sys::napi_env,
-            closure_data_ptr: *mut c_void,
-            _finalize_hint: *mut c_void,
-          ) {
-            drop(unsafe { Box::<F>::from_raw(closure_data_ptr.cast()) })
-          }
-
-          finalize_box_trampoline::<F>
-        }),
         ptr::null_mut(),
         ptr::null_mut(),
       )
@@ -1506,3 +1416,174 @@ unsafe extern "C" fn async_finalize<Arg, F>(
     );
   }
 }
+
+#[cfg(feature = "napi5")]
+pub(crate) unsafe extern "C" fn trampoline<
+  R: ToNapiValue,
+  F: Fn(crate::CallContext) -> Result<R>,
+>(
+  raw_env: sys::napi_env,
+  cb_info: sys::napi_callback_info,
+) -> sys::napi_value {
+  use crate::CallContext;
+
+  let (raw_this, raw_args, closure_data_ptr, argc) = {
+    // Fast path for 4 arguments or less.
+    let mut argc = 4;
+    let mut raw_args = Vec::with_capacity(4);
+    let mut raw_this = ptr::null_mut();
+    let mut closure_data_ptr = ptr::null_mut();
+
+    let status = unsafe {
+      sys::napi_get_cb_info(
+        raw_env,
+        cb_info,
+        &mut argc,
+        raw_args.as_mut_ptr(),
+        &mut raw_this,
+        &mut closure_data_ptr,
+      )
+    };
+    debug_assert!(
+      Status::from(status) == Status::Ok,
+      "napi_get_cb_info failed"
+    );
+
+    // Arguments length greater than 4, resize the vector.
+    if argc > 4 {
+      raw_args = vec![ptr::null_mut(); argc];
+      let status = unsafe {
+        sys::napi_get_cb_info(
+          raw_env,
+          cb_info,
+          &mut argc,
+          raw_args.as_mut_ptr(),
+          &mut raw_this,
+          &mut closure_data_ptr,
+        )
+      };
+      debug_assert!(
+        Status::from(status) == Status::Ok,
+        "napi_get_cb_info failed"
+      );
+    } else {
+      unsafe { raw_args.set_len(argc) };
+    }
+
+    (raw_this, raw_args, closure_data_ptr, argc)
+  };
+
+  let closure: &F = Box::leak(unsafe { Box::from_raw(closure_data_ptr.cast()) });
+  let mut env = unsafe { Env::from_raw(raw_env) };
+  let call_context = CallContext::new(&mut env, cb_info, raw_this, raw_args.as_slice(), argc);
+  closure(call_context)
+    .and_then(|ret: R| unsafe { <R as ToNapiValue>::to_napi_value(env.0, ret) })
+    .unwrap_or_else(|e| {
+      unsafe { JsError::from(e).throw_into(raw_env) };
+      ptr::null_mut()
+    })
+}
+
+#[cfg(feature = "napi5")]
+pub(crate) unsafe extern "C" fn trampoline_setter<
+  V: FromNapiValue,
+  F: Fn(Env, crate::bindgen_runtime::Object, V) -> Result<()>,
+>(
+  raw_env: sys::napi_env,
+  cb_info: sys::napi_callback_info,
+) -> sys::napi_value {
+  use crate::bindgen_runtime::Object;
+
+  let (raw_args, raw_this, closure_data_ptr) = {
+    let mut argc = 1;
+    let mut raw_args = vec![ptr::null_mut(); 1];
+    let mut raw_this = ptr::null_mut();
+    let mut closure_data_ptr = ptr::null_mut();
+
+    let status = unsafe {
+      sys::napi_get_cb_info(
+        raw_env,
+        cb_info,
+        &mut argc,
+        raw_args.as_mut_ptr(),
+        &mut raw_this,
+        &mut closure_data_ptr,
+      )
+    };
+    unsafe { raw_args.set_len(argc) };
+    debug_assert!(
+      Status::from(status) == Status::Ok,
+      "napi_get_cb_info failed"
+    );
+    (raw_args, raw_this, closure_data_ptr)
+  };
+
+  let closure: &F = Box::leak(unsafe { Box::from_raw(closure_data_ptr.cast()) });
+  let env = unsafe { Env::from_raw(raw_env) };
+  raw_args
+    .get(0)
+    .ok_or_else(|| Error::new(Status::InvalidArg, "Missing argument in property setter"))
+    .and_then(|value| unsafe { V::from_napi_value(raw_env, *value) })
+    .and_then(|value| {
+      closure(
+        env,
+        unsafe { Object::from_raw_unchecked(raw_env, raw_this) },
+        value,
+      )
+    })
+    .map(|_| std::ptr::null_mut())
+    .unwrap_or_else(|e| {
+      unsafe { JsError::from(e).throw_into(raw_env) };
+      ptr::null_mut()
+    })
+}
+
+#[cfg(feature = "napi5")]
+pub(crate) unsafe extern "C" fn trampoline_getter<
+  R: ToNapiValue,
+  F: Fn(Env, crate::bindgen_runtime::This) -> Result<R>,
+>(
+  raw_env: sys::napi_env,
+  cb_info: sys::napi_callback_info,
+) -> sys::napi_value {
+  let (raw_this, closure_data_ptr) = {
+    let mut raw_this = ptr::null_mut();
+    let mut closure_data_ptr = ptr::null_mut();
+
+    let status = unsafe {
+      sys::napi_get_cb_info(
+        raw_env,
+        cb_info,
+        &mut 0,
+        ptr::null_mut(),
+        &mut raw_this,
+        &mut closure_data_ptr,
+      )
+    };
+    debug_assert!(
+      Status::from(status) == Status::Ok,
+      "napi_get_cb_info failed"
+    );
+    (raw_this, closure_data_ptr)
+  };
+
+  let closure: &F = Box::leak(unsafe { Box::from_raw(closure_data_ptr.cast()) });
+  let env = unsafe { Env::from_raw(raw_env) };
+  closure(env, unsafe {
+    crate::bindgen_runtime::Object::from_raw_unchecked(raw_env, raw_this)
+  })
+  .and_then(|ret: R| unsafe { <R as ToNapiValue>::to_napi_value(env.0, ret) })
+  .unwrap_or_else(|e| {
+    unsafe { JsError::from(e).throw_into(raw_env) };
+    ptr::null_mut()
+  })
+}
+
+#[cfg(feature = "napi5")]
+pub(crate) unsafe extern "C" fn finalize_box_trampoline<F>(
+  _raw_env: sys::napi_env,
+  closure_data_ptr: *mut c_void,
+  _finalize_hint: *mut c_void,
+) {
+  drop(unsafe { Box::<F>::from_raw(closure_data_ptr.cast()) })
+}

crates/napi/src/js_values/mod.rs

@@ -1,9 +1,11 @@
 use std::convert::TryFrom;
+#[cfg(feature = "napi5")]
+use std::ffi::c_void;
 use std::ffi::CString;
 use std::ptr;
 
 use crate::{
-  bindgen_runtime::{FromNapiValue, TypeName, ValidateNapiValue},
+  bindgen_runtime::{FromNapiValue, ToNapiValue, TypeName, ValidateNapiValue},
   check_status, sys, type_of, Callback, Error, Result, Status, ValueType,
 };
 
@@ -308,11 +310,16 @@ macro_rules! impl_object_methods {
 
       pub fn set_named_property<T>(&mut self, name: &str, value: T) -> Result<()>
       where
-        T: NapiRaw,
+        T: ToNapiValue,
       {
         let key = CString::new(name)?;
         check_status!(unsafe {
-          sys::napi_set_named_property(self.0.env, self.0.value, key.as_ptr(), value.raw())
+          sys::napi_set_named_property(
+            self.0.env,
+            self.0.value,
+            key.as_ptr(),
+            T::to_napi_value(self.0.env, value)?,
+          )
         })
       }
 
@@ -347,7 +354,7 @@ macro_rules! impl_object_methods {
         unsafe { <T as FromNapiValue>::from_napi_value(self.0.env, raw_value) }
       }
 
-      pub fn get_named_property_unchecked<T>(&self, name: &str) -> Result<T>
+      pub fn get_named_property_unchecked<T: FromNapiValue>(&self, name: &str) -> Result<T>
       where
         T: FromNapiValue,
       {
@@ -539,14 +546,33 @@ macro_rules! impl_object_methods {
 
       /// This method allows the efficient definition of multiple properties on a given object.
       pub fn define_properties(&mut self, properties: &[Property]) -> Result<()> {
+        let properties_iter = properties.iter().map(|property| property.raw());
+        #[cfg(feature = "napi5")]
+        {
+          let mut closures = properties_iter
+            .clone()
+            .map(|p| p.data)
+            .filter(|data| !data.is_null())
+            .collect::<Vec<*mut std::ffi::c_void>>();
+          let len = Box::into_raw(Box::new(closures.len()));
+          check_status!(unsafe {
+            sys::napi_add_finalizer(
+              self.0.env,
+              self.0.value,
+              closures.as_mut_ptr().cast(),
+              Some(finalize_closures),
+              len.cast(),
+              ptr::null_mut(),
+            )
+          })?;
+          std::mem::forget(closures);
+        }
         check_status!(unsafe {
           sys::napi_define_properties(
             self.0.env,
             self.0.value,
             properties.len(),
-            properties
+            properties_iter
-              .iter()
-              .map(|property| property.raw())
               .collect::<Vec<sys::napi_property_descriptor>>()
               .as_ptr(),
           )
@@ -697,3 +723,13 @@ impl JsUnknown {
     unsafe { V::from_raw_unchecked(self.0.env, self.0.value) }
   }
 }
+
+#[cfg(feature = "napi5")]
+unsafe extern "C" fn finalize_closures(_env: sys::napi_env, data: *mut c_void, len: *mut c_void) {
+  let length: usize = *unsafe { Box::from_raw(len.cast()) };
+  let closures: Vec<*mut PropertyClosures> =
+    unsafe { Vec::from_raw_parts(data.cast(), length, length) };
+  for closure in closures.into_iter() {
+    drop(unsafe { Box::from_raw(closure) });
+  }
+}

crates/napi/src/js_values/object_property.rs

@@ -1,11 +1,35 @@
 use std::convert::From;
+#[cfg(feature = "napi5")]
+use std::ffi::c_void;
 use std::ffi::CString;
 use std::ptr;
 
 use bitflags::bitflags;
 
+#[cfg(feature = "napi5")]
+use crate::{
+  bindgen_runtime::{FromNapiValue, This, ToNapiValue},
+  Env,
+};
 use crate::{sys, Callback, NapiRaw, Result};
 
+#[cfg(feature = "napi5")]
+#[derive(Copy, Clone)]
+pub struct PropertyClosures {
+  setter_closure: *mut c_void,
+  getter_closure: *mut c_void,
+}
+
+#[cfg(feature = "napi5")]
+impl Default for PropertyClosures {
+  fn default() -> Self {
+    Self {
+      setter_closure: ptr::null_mut(),
+      getter_closure: ptr::null_mut(),
+    }
+  }
+}
+
 #[derive(Clone)]
 pub struct Property {
   pub name: CString,
@@ -15,6 +39,8 @@ pub struct Property {
   attrs: PropertyAttributes,
   value: sys::napi_value,
   pub(crate) is_ctor: bool,
+  #[cfg(feature = "napi5")]
+  pub(crate) closures: PropertyClosures,
 }
 
 impl Default for Property {
@@ -27,6 +53,8 @@ impl Default for Property {
       attrs: Default::default(),
       value: ptr::null_mut(),
       is_ctor: Default::default(),
+      #[cfg(feature = "napi5")]
+      closures: PropertyClosures::default(),
     }
   }
 }
@@ -77,11 +105,41 @@ impl Property {
     self
   }
 
+  #[cfg(feature = "napi5")]
+  pub fn with_getter_closure<R, F>(mut self, callback: F) -> Self
+  where
+    F: 'static + Fn(Env, This) -> Result<R>,
+    R: ToNapiValue,
+  {
+    let boxed_callback = Box::new(callback);
+    let closure_data_ptr: *mut F = Box::into_raw(boxed_callback);
+    self.closures.getter_closure = closure_data_ptr.cast();
+
+    let fun = crate::trampoline_getter::<R, F>;
+    self.getter = Some(fun);
+    self
+  }
+
   pub fn with_setter(mut self, callback: Callback) -> Self {
     self.setter = Some(callback);
     self
   }
 
+  #[cfg(feature = "napi5")]
+  pub fn with_setter_closure<F, V>(mut self, callback: F) -> Self
+  where
+    F: 'static + Fn(crate::Env, This, V) -> Result<()>,
+    V: FromNapiValue,
+  {
+    let boxed_callback = Box::new(callback);
+    let closure_data_ptr: *mut F = Box::into_raw(boxed_callback);
+    self.closures.setter_closure = closure_data_ptr.cast();
+
+    let fun = crate::trampoline_setter::<V, F>;
+    self.setter = Some(fun);
+    self
+  }
+
   pub fn with_property_attributes(mut self, attributes: PropertyAttributes) -> Self {
     self.attrs = attributes;
     self
@@ -93,6 +151,8 @@ impl Property {
   }
 
   pub(crate) fn raw(&self) -> sys::napi_property_descriptor {
+    #[cfg(feature = "napi5")]
+    let closures = Box::into_raw(Box::new(self.closures));
     sys::napi_property_descriptor {
       utf8name: self.name.as_ptr(),
       name: ptr::null_mut(),
@@ -101,7 +161,10 @@ impl Property {
       setter: self.setter,
       value: self.value,
       attributes: self.attrs.into(),
+      #[cfg(not(feature = "napi5"))]
       data: ptr::null_mut(),
+      #[cfg(feature = "napi5")]
+      data: closures.cast(),
     }
   }
 

examples/napi-compat-mode/__test__/function.spec.ts

@@ -31,3 +31,14 @@ test('should handle errors', (t) => {
     },
   )
 })
+
+test('should be able to create function from closure', (t) => {
+  for (let i = 0; i < 100; i++) {
+    t.is(
+      bindings.testCreateFunctionFromClosure()(
+        ...Array.from({ length: i }).map((_, i) => i),
+      ),
+      `arguments length: ${i}`,
+    )
+  }
+})

examples/napi-compat-mode/src/function.rs

@@ -43,6 +43,19 @@ pub fn call_function_error(ctx: CallContext) -> Result<JsUnknown> {
   }
 }
 
+#[js_function(0)]
+pub fn test_create_function_from_closure(ctx: CallContext) -> Result<JsFunction> {
+  ctx
+    .env
+    .create_function_from_closure("functionFromClosure", move |ctx| {
+      if ctx.length != 0 {
+        let max: u32 = ctx.get(ctx.length - 1)?;
+        assert_eq!(max, ctx.length as u32 - 1);
+      }
+      Ok(format!("arguments length: {}", ctx.length))
+    })
+}
+
 pub fn register_js(exports: &mut JsObject) -> Result<()> {
   exports.create_named_method("testCallFunction", call_function)?;
   exports.create_named_method(
@@ -51,5 +64,9 @@ pub fn register_js(exports: &mut JsObject) -> Result<()> {
   )?;
   exports.create_named_method("testCallFunctionWithThis", call_function_with_this)?;
   exports.create_named_method("testCallFunctionError", call_function_error)?;
+  exports.create_named_method(
+    "testCreateFunctionFromClosure",
+    test_create_function_from_closure,
+  )?;
   Ok(())
 }

examples/napi/__test__/typegen.spec.ts.md

@@ -325,6 +325,9 @@ Generated by [AVA](https://avajs.dev).
       value: number␊
       constructor(value: number)␊
     }␊
+    export class GetterSetterWithClosures {␊
+      constructor()␊
+    }␊
     export class ClassWithFactory {␊
       name: string␊
       static withName(name: string): ClassWithFactory␊

examples/napi/__test__/values.spec.ts

@@ -25,6 +25,7 @@ import {
   ClassWithFactory,
   CustomNumEnum,
   Context,
+  GetterSetterWithClosures,
   enumToI32,
   listObjKeys,
   createObj,
@@ -329,7 +330,7 @@ test('return function', (t) => {
   })
 })
 
-test('function return Promise', async (t) => {
+test('callback function return Promise', async (t) => {
   const cbSpy = spy()
   await callbackReturnPromise<string>(() => '1', spy)
   t.is(cbSpy.callCount, 0)
@@ -919,3 +920,14 @@ Napi5Test('Date to chrono test', (t) => {
     new Date(fixture.getTime() + 60 * 1000),
   )
 })
+
+Napi5Test('Class with getter setter closures', (t) => {
+  const instance = new GetterSetterWithClosures()
+  // @ts-expect-error
+  instance.name = 'Allie'
+  t.pass()
+  // @ts-expect-error
+  t.is(instance.name, `I'm Allie`)
+  // @ts-expect-error
+  t.is(instance.age, 0.3)
+})

examples/napi/index.d.ts

@@ -315,6 +315,9 @@ export class Width {
   value: number
   constructor(value: number)
 }
+export class GetterSetterWithClosures {
+  constructor()
+}
 export class ClassWithFactory {
   name: string
   static withName(name: string): ClassWithFactory

examples/napi/src/class.rs

@@ -1,6 +1,6 @@
 use napi::{
   bindgen_prelude::{Buffer, ClassInstance, ObjectFinalize, This, Uint8Array, Unknown},
-  Env, Result,
+  Env, Property, Result,
 };
 
 use crate::r#enum::Kind;
@@ -411,3 +411,23 @@ pub struct Width {
 pub fn plus_one(this: This<&Width>) -> i32 {
   this.value + 1
 }
+
+#[napi]
+pub struct GetterSetterWithClosures {}
+
+#[napi]
+impl GetterSetterWithClosures {
+  #[napi(constructor)]
+  pub fn new(mut this: This) -> Result<Self> {
+    this.define_properties(&[
+      Property::new("name")?
+        .with_setter_closure(move |_env, mut this, value: String| {
+          this.set_named_property("_name", format!("I'm {}", value))?;
+          Ok(())
+        })
+        .with_getter_closure(|_env, this| this.get_named_property_unchecked::<Unknown>("_name")),
+      Property::new("age")?.with_getter_closure(|_env, _this| Ok(0.3)),
+    ])?;
+    Ok(Self {})
+  }
+}