From 0550c56fcf8eeec61d2e5abfa00c92a990c34e25 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Mon, 25 Mar 2024 15:11:11 +0800 Subject: [PATCH] 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 | 2 +- .../src/bindgen_runtime/js_values/external.rs | 69 ++++++++++++------- crates/napi/src/env.rs | 41 ++++++----- examples/napi-compat-mode/src/class.rs | 4 +- .../src/napi4/tsfn_dua_instance.rs | 2 +- examples/napi/__tests__/values.spec.ts | 5 +- examples/napi/src/external.rs | 8 +-- examples/napi/src/fn_strict.rs | 4 +- 8 files changed, 78 insertions(+), 57 deletions(-) diff --git a/bench/src/get_set_property.rs b/bench/src/get_set_property.rs index 5a0219b5..89cbb3c5 100644 --- a/bench/src/get_set_property.rs +++ b/bench/src/get_set_property.rs @@ -69,7 +69,7 @@ fn test_class_constructor(ctx: CallContext) -> Result { line_join: LineJoin::Miter, }; let mut this = ctx.this_unchecked::(); - ctx.env.wrap(&mut this, native)?; + ctx.env.wrap(&mut this, native, None)?; ctx.env.get_undefined() } diff --git a/crates/napi/src/bindgen_runtime/js_values/external.rs b/crates/napi/src/bindgen_runtime/js_values/external.rs index 420878c1..13f31824 100644 --- a/crates/napi/src/bindgen_runtime/js_values/external.rs +++ b/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 crate::{check_status, sys, Error, Status, TaggedObject}; +use super::{FromNapiMutRef, FromNapiRef, ToNapiValue, TypeName, ValidateNapiValue}; +use crate::{check_status, sys, Error, Status}; +#[repr(C)] pub struct External { - obj: *mut TaggedObject, + type_id: TypeId, + obj: T, size_hint: usize, pub adjusted_size: i64, } @@ -15,7 +17,7 @@ pub struct External { unsafe impl Send for External {} unsafe impl Sync for External {} -impl TypeName for External { +impl TypeName for &External { fn type_name() -> &'static str { "External" } @@ -31,12 +33,13 @@ impl From for External { } } -impl ValidateNapiValue for External {} +impl ValidateNapiValue for &External {} impl External { pub fn new(value: T) -> Self { Self { - obj: Box::into_raw(Box::new(TaggedObject::new(value))), + type_id: TypeId::of::(), + obj: value, size_hint: 0, adjusted_size: 0, } @@ -49,15 +52,19 @@ impl External { /// 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::(), + obj: value, size_hint, adjusted_size: 0, } } } -impl FromNapiValue for External { - unsafe fn from_napi_value(env: sys::napi_env, napi_val: sys::napi_value) -> crate::Result { +impl FromNapiMutRef for External { + 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 FromNapiValue for External { let type_id = unknown_tagged_object as *const TypeId; if unsafe { *type_id } == TypeId::of::() { - let tagged_object = unknown_tagged_object as *mut TaggedObject; - Ok(Self { - obj: tagged_object, - size_hint: 0, - adjusted_size: 0, - }) + let tagged_object = unknown_tagged_object as *mut External; + Ok(Box::leak(unsafe { Box::from_raw(tagged_object) })) } 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::() + ), )) } } } +impl FromNapiRef for External { + 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 AsRef for External { fn as_ref(&self) -> &T { - unsafe { Box::leak(Box::from_raw(self.obj)).object.as_ref().unwrap() } + &self.obj } } impl AsMut for External { 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 DerefMut for External { } impl ToNapiValue for External { - unsafe fn to_napi_value(env: sys::napi_env, mut val: Self) -> crate::Result { + unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> crate::Result { 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 _, - Some(crate::raw_finalize::), - Box::into_raw(Box::new(Some(val.size_hint as i64))) as *mut _, + obj_ptr.cast(), + Some(crate::raw_finalize::>), + size_hint_ptr.cast(), &mut napi_value, ) }, @@ -125,12 +143,12 @@ impl ToNapiValue for External { 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 ToNapiValue for External { )?; }; - 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) } diff --git a/crates/napi/src/env.rs b/crates/napi/src/env.rs index 83eb58cc..f7fd1585 100644 --- a/crates/napi/src/env.rs +++ b/crates/napi/src/env.rs @@ -772,14 +772,19 @@ impl Env { } #[allow(clippy::needless_pass_by_ref_mut)] - pub fn wrap(&self, js_object: &mut JsObject, native_object: T) -> Result<()> { + pub fn wrap( + &self, + js_object: &mut JsObject, + native_object: T, + size_hint: Option, + ) -> 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::), - ptr::null_mut(), + Some(raw_finalize::>), + 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::), - Box::into_raw(Box::new(size_hint)).cast(), + Some(raw_finalize::>), + 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(&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( finalize_data: *mut c_void, finalize_hint: *mut c_void, ) { - let tagged_object = finalize_data as *mut TaggedObject; + 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) }; - if let Some(changed) = size_hint { - if changed != 0 { - let mut adjusted = 0i64; - let status = unsafe { sys::napi_adjust_external_memory(env, -changed, &mut adjusted) }; - debug_assert!( - status == sys::Status::napi_ok, - "Calling napi_adjust_external_memory failed" - ); - } - }; - } + let size_hint = unsafe { *Box::from_raw(finalize_hint as *mut i64) }; + if size_hint != 0 { + let mut adjusted = 0i64; + let status = unsafe { sys::napi_adjust_external_memory(env, -size_hint, &mut adjusted) }; + debug_assert!( + status == sys::Status::napi_ok, + "Calling napi_adjust_external_memory failed" + ); + } + }; } #[cfg(feature = "napi6")] diff --git a/examples/napi-compat-mode/src/class.rs b/examples/napi-compat-mode/src/class.rs index 484f31f0..c5854f95 100644 --- a/examples/napi-compat-mode/src/class.rs +++ b/examples/napi-compat-mode/src/class.rs @@ -27,7 +27,7 @@ fn test_class_constructor(ctx: CallContext) -> Result { 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 { fn renew_wrapped(ctx: CallContext) -> Result { let mut this: JsObject = ctx.this_unchecked(); ctx.env.drop_wrapped::(&this)?; - ctx.env.wrap(&mut this, NativeClass { value: 42 })?; + ctx.env.wrap(&mut this, NativeClass { value: 42 }, None)?; ctx.env.get_undefined() } diff --git a/examples/napi-compat-mode/src/napi4/tsfn_dua_instance.rs b/examples/napi-compat-mode/src/napi4/tsfn_dua_instance.rs index 7a44122c..75872132 100644 --- a/examples/napi-compat-mode/src/napi4/tsfn_dua_instance.rs +++ b/examples/napi-compat-mode/src/napi4/tsfn_dua_instance.rs @@ -26,7 +26,7 @@ pub fn constructor(ctx: CallContext) -> napi::Result { 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() } diff --git a/examples/napi/__tests__/values.spec.ts b/examples/napi/__tests__/values.spec.ts index d10a9244..f7861e0f 100644 --- a/examples/napi/__tests__/values.spec.ts +++ b/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( - e?.message, - 'T on `get_value_external` is not the type of wrapped object', - ) + t.is(e?.message, ' on `External` is not the type of wrapped object') }) test('should be able to run script', async (t) => { diff --git a/examples/napi/src/external.rs b/examples/napi/src/external.rs index 581b2710..c6aba2e9 100644 --- a/examples/napi/src/external.rs +++ b/examples/napi/src/external.rs @@ -11,11 +11,11 @@ pub fn create_external_string(content: String) -> External { } #[napi] -pub fn get_external(external: External) -> u32 { - *external +pub fn get_external(external: &External) -> u32 { + **external } #[napi] -pub fn mutate_external(mut external: External, new_val: u32) { - *external = new_val; +pub fn mutate_external(external: &mut External, new_val: u32) { + **external = new_val; } diff --git a/examples/napi/src/fn_strict.rs b/examples/napi/src/fn_strict.rs index 7a8ffae2..f007174e 100644 --- a/examples/napi/src/fn_strict.rs +++ b/examples/napi/src/fn_strict.rs @@ -54,8 +54,8 @@ fn validate_date_time(_d: DateTime) -> i64 { } #[napi(strict)] -fn validate_external(e: External) -> u32 { - *e +fn validate_external(e: &External) -> u32 { + **e } #[napi(strict, ts_args_type = "cb: () => number")]