fix(napi): External should impl FromNapiRef rather than FromNapiValue (#2013)

- Close https://github.com/napi-rs/napi-rs/issues/1994

bench/src/get_set_property.rs

@@ -69,7 +69,7 @@ fn test_class_constructor(ctx: CallContext) -> Result<JsUndefined> {
     line_join: LineJoin::Miter,
   };
   let mut this = ctx.this_unchecked::<JsObject>();
-  ctx.env.wrap(&mut this, native)?;
+  ctx.env.wrap(&mut this, native, None)?;
   ctx.env.get_undefined()
 }
 

crates/napi/src/bindgen_runtime/js_values/external.rs

@@ -3,11 +3,13 @@ use std::{
   ops::{Deref, DerefMut},
 };
 
-use super::{FromNapiValue, ToNapiValue, TypeName, ValidateNapiValue};
+use super::{FromNapiMutRef, FromNapiRef, ToNapiValue, TypeName, ValidateNapiValue};
-use crate::{check_status, sys, Error, Status, TaggedObject};
+use crate::{check_status, sys, Error, Status};
 
+#[repr(C)]
 pub struct External<T: 'static> {
-  obj: *mut TaggedObject<T>,
+  type_id: TypeId,
+  obj: T,
   size_hint: usize,
   pub adjusted_size: i64,
 }
@@ -15,7 +17,7 @@ pub struct External<T: 'static> {
 unsafe impl<T: 'static + Send> Send for External<T> {}
 unsafe impl<T: 'static + Sync> Sync for External<T> {}
 
-impl<T: 'static> TypeName for External<T> {
+impl<T: 'static> TypeName for &External<T> {
   fn type_name() -> &'static str {
     "External"
   }
@@ -31,12 +33,13 @@ impl<T: 'static> From<T> for External<T> {
   }
 }
 
-impl<T: 'static> ValidateNapiValue for External<T> {}
+impl<T: 'static> ValidateNapiValue for &External<T> {}
 
 impl<T: 'static> External<T> {
   pub fn new(value: T) -> Self {
     Self {
-      obj: Box::into_raw(Box::new(TaggedObject::new(value))),
+      type_id: TypeId::of::<T>(),
+      obj: value,
       size_hint: 0,
       adjusted_size: 0,
     }
@@ -49,15 +52,19 @@ impl<T: 'static> External<T> {
   /// If your `External` object is not effect to GC, you can use `External::new` instead.
   pub fn new_with_size_hint(value: T, size_hint: usize) -> Self {
     Self {
-      obj: Box::into_raw(Box::new(TaggedObject::new(value))),
+      type_id: TypeId::of::<T>(),
+      obj: value,
       size_hint,
       adjusted_size: 0,
     }
   }
 }
 
-impl<T: 'static> FromNapiValue for External<T> {
+impl<T: 'static> FromNapiMutRef for External<T> {
-  unsafe fn from_napi_value(env: sys::napi_env, napi_val: sys::napi_value) -> crate::Result<Self> {
+  unsafe fn from_napi_mut_ref(
+    env: sys::napi_env,
+    napi_val: sys::napi_value,
+  ) -> crate::Result<&'static mut Self> {
     let mut unknown_tagged_object = std::ptr::null_mut();
     check_status!(
       unsafe { sys::napi_get_value_external(env, napi_val, &mut unknown_tagged_object) },
@@ -66,30 +73,38 @@ impl<T: 'static> FromNapiValue for External<T> {
 
     let type_id = unknown_tagged_object as *const TypeId;
     if unsafe { *type_id } == TypeId::of::<T>() {
-      let tagged_object = unknown_tagged_object as *mut TaggedObject<T>;
+      let tagged_object = unknown_tagged_object as *mut External<T>;
-      Ok(Self {
+      Ok(Box::leak(unsafe { Box::from_raw(tagged_object) }))
-        obj: tagged_object,
-        size_hint: 0,
-        adjusted_size: 0,
-      })
     } else {
       Err(Error::new(
         Status::InvalidArg,
-        "T on `get_value_external` is not the type of wrapped object".to_owned(),
+        format!(
+          "<{}> on `External` is not the type of wrapped object",
+          std::any::type_name::<T>()
+        ),
       ))
     }
   }
 }
 
+impl<T: 'static> FromNapiRef for External<T> {
+  unsafe fn from_napi_ref(
+    env: sys::napi_env,
+    napi_val: sys::napi_value,
+  ) -> crate::Result<&'static Self> {
+    unsafe { Self::from_napi_mut_ref(env, napi_val) }.map(|v| v as &Self)
+  }
+}
+
 impl<T: 'static> AsRef<T> for External<T> {
   fn as_ref(&self) -> &T {
-    unsafe { Box::leak(Box::from_raw(self.obj)).object.as_ref().unwrap() }
+    &self.obj
   }
 }
 
 impl<T: 'static> AsMut<T> for External<T> {
   fn as_mut(&mut self) -> &mut T {
-    unsafe { Box::leak(Box::from_raw(self.obj)).object.as_mut().unwrap() }
+    &mut self.obj
   }
 }
 
@@ -108,15 +123,18 @@ impl<T: 'static> DerefMut for External<T> {
 }
 
 impl<T: 'static> ToNapiValue for External<T> {
-  unsafe fn to_napi_value(env: sys::napi_env, mut val: Self) -> crate::Result<sys::napi_value> {
+  unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> crate::Result<sys::napi_value> {
     let mut napi_value = std::ptr::null_mut();
+    let size_hint = val.size_hint as i64;
+    let size_hint_ptr = Box::into_raw(Box::new(size_hint));
+    let obj_ptr = Box::into_raw(Box::new(val));
     check_status!(
       unsafe {
         sys::napi_create_external(
           env,
-          val.obj as *mut _,
+          obj_ptr.cast(),
-          Some(crate::raw_finalize::<T>),
+          Some(crate::raw_finalize::<External<T>>),
-          Box::into_raw(Box::new(Some(val.size_hint as i64))) as *mut _,
+          size_hint_ptr.cast(),
           &mut napi_value,
         )
       },
@@ -125,12 +143,12 @@ impl<T: 'static> ToNapiValue for External<T> {
 
     let mut adjusted_external_memory_size = std::mem::MaybeUninit::new(0);
 
-    if val.size_hint != 0 {
+    if size_hint != 0 {
       check_status!(
         unsafe {
           sys::napi_adjust_external_memory(
             env,
-            val.size_hint as i64,
+            size_hint,
             adjusted_external_memory_size.as_mut_ptr(),
           )
         },
@@ -138,7 +156,8 @@ impl<T: 'static> ToNapiValue for External<T> {
       )?;
     };
 
-    val.adjusted_size = unsafe { adjusted_external_memory_size.assume_init() };
+    (Box::leak(unsafe { Box::from_raw(obj_ptr) })).adjusted_size =
+      unsafe { adjusted_external_memory_size.assume_init() };
 
     Ok(napi_value)
   }

crates/napi/src/env.rs

@@ -772,14 +772,19 @@ impl Env {
   }
 
   #[allow(clippy::needless_pass_by_ref_mut)]
-  pub fn wrap<T: 'static>(&self, js_object: &mut JsObject, native_object: T) -> Result<()> {
+  pub fn wrap<T: 'static>(
+    &self,
+    js_object: &mut JsObject,
+    native_object: T,
+    size_hint: Option<usize>,
+  ) -> Result<()> {
     check_status!(unsafe {
       sys::napi_wrap(
         self.0,
         js_object.0.value,
         Box::into_raw(Box::new(TaggedObject::new(native_object))).cast(),
-        Some(raw_finalize::<T>),
+        Some(raw_finalize::<TaggedObject<T>>),
-        ptr::null_mut(),
+        Box::into_raw(Box::new(size_hint.unwrap_or(0) as i64)).cast(),
         ptr::null_mut(),
       )
     })
@@ -904,6 +909,7 @@ impl Env {
     Ok(unsafe { T::from_raw_unchecked(self.0, js_value) })
   }
 
+  #[deprecated(since = "3.0.0", note = "Please use `External::new` instead")]
   /// If `size_hint` provided, `Env::adjust_external_memory` will be called under the hood.
   ///
   /// If no `size_hint` provided, global garbage collections will be triggered less times than expected.
@@ -919,8 +925,8 @@ impl Env {
       sys::napi_create_external(
         self.0,
         Box::into_raw(Box::new(TaggedObject::new(native_object))).cast(),
-        Some(raw_finalize::<T>),
+        Some(raw_finalize::<TaggedObject<T>>),
-        Box::into_raw(Box::new(size_hint)).cast(),
+        Box::into_raw(Box::new(size_hint.unwrap_or(0))).cast(),
         &mut object_value,
       )
     })?;
@@ -935,6 +941,7 @@ impl Env {
     Ok(unsafe { JsExternal::from_raw_unchecked(self.0, object_value) })
   }
 
+  #[deprecated(since = "3.0.0", note = "Please use `&External` instead")]
   pub fn get_value_external<T: 'static>(&self, js_external: &JsExternal) -> Result<&mut T> {
     unsafe {
       let mut unknown_tagged_object = ptr::null_mut();
@@ -1382,21 +1389,19 @@ pub(crate) unsafe extern "C" fn raw_finalize<T>(
   finalize_data: *mut c_void,
   finalize_hint: *mut c_void,
 ) {
-  let tagged_object = finalize_data as *mut TaggedObject<T>;
+  let tagged_object = finalize_data as *mut T;
   drop(unsafe { Box::from_raw(tagged_object) });
   if !finalize_hint.is_null() {
-    let size_hint = unsafe { *Box::from_raw(finalize_hint as *mut Option<i64>) };
+    let size_hint = unsafe { *Box::from_raw(finalize_hint as *mut i64) };
-    if let Some(changed) = size_hint {
+    if size_hint != 0 {
-      if changed != 0 {
+      let mut adjusted = 0i64;
-        let mut adjusted = 0i64;
+      let status = unsafe { sys::napi_adjust_external_memory(env, -size_hint, &mut adjusted) };
-        let status = unsafe { sys::napi_adjust_external_memory(env, -changed, &mut adjusted) };
+      debug_assert!(
-        debug_assert!(
+        status == sys::Status::napi_ok,
-          status == sys::Status::napi_ok,
+        "Calling napi_adjust_external_memory failed"
-          "Calling napi_adjust_external_memory failed"
+      );
-        );
+    }
-      }
+  };
-    };
-  }
 }
 
 #[cfg(feature = "napi6")]

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

@@ -27,7 +27,7 @@ fn test_class_constructor(ctx: CallContext) -> Result<JsUndefined> {
   let mut this: JsObject = ctx.this_unchecked();
   ctx
     .env
-    .wrap(&mut this, NativeClass { value: count + 100 })?;
+    .wrap(&mut this, NativeClass { value: count + 100 }, None)?;
   this.set_named_property("count", ctx.env.create_int32(count)?)?;
   ctx.env.get_undefined()
 }
@@ -54,7 +54,7 @@ fn add_native_count(ctx: CallContext) -> Result<JsNumber> {
 fn renew_wrapped(ctx: CallContext) -> Result<JsUndefined> {
   let mut this: JsObject = ctx.this_unchecked();
   ctx.env.drop_wrapped::<NativeClass>(&this)?;
-  ctx.env.wrap(&mut this, NativeClass { value: 42 })?;
+  ctx.env.wrap(&mut this, NativeClass { value: 42 }, None)?;
   ctx.env.get_undefined()
 }
 

examples/napi-compat-mode/src/napi4/tsfn_dua_instance.rs

@@ -26,7 +26,7 @@ pub fn constructor(ctx: CallContext) -> napi::Result<JsUndefined> {
   let mut this: JsObject = ctx.this_unchecked();
   let obj = A { cb };
 
-  ctx.env.wrap(&mut this, obj)?;
+  ctx.env.wrap(&mut this, obj, None)?;
   ctx.env.get_undefined()
 }
 

examples/napi/__tests__/values.spec.ts

@@ -893,10 +893,7 @@ test('external', (t) => {
   const ext2 = createExternalString('wtf')
   // @ts-expect-error
   const e = t.throws(() => getExternal(ext2))
-  t.is(
+  t.is(e?.message, '<u32> on `External` is not the type of wrapped object')
-    e?.message,
-    'T on `get_value_external` is not the type of wrapped object',
-  )
 })
 
 test('should be able to run script', async (t) => {

examples/napi/src/external.rs

@@ -11,11 +11,11 @@ pub fn create_external_string(content: String) -> External<String> {
 }
 
 #[napi]
-pub fn get_external(external: External<u32>) -> u32 {
+pub fn get_external(external: &External<u32>) -> u32 {
-  *external
+  **external
 }
 
 #[napi]
-pub fn mutate_external(mut external: External<u32>, new_val: u32) {
+pub fn mutate_external(external: &mut External<u32>, new_val: u32) {
-  *external = new_val;
+  **external = new_val;
 }

examples/napi/src/fn_strict.rs

@@ -54,8 +54,8 @@ fn validate_date_time(_d: DateTime<Utc>) -> i64 {
 }
 
 #[napi(strict)]
-fn validate_external(e: External<u32>) -> u32 {
+fn validate_external(e: &External<u32>) -> u32 {
-  *e
+  **e
 }
 
 #[napi(strict, ts_args_type = "cb: () => number")]