From 5a1ea586b4d99a8e3028d51899b921acca484648 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Thu, 6 Oct 2022 20:51:08 -0700 Subject: [PATCH] refactor(napi): simplify `napi_value` interface (#16170) --- cli/napi/js_native_api.rs | 122 ++++++++++++++++--------------- cli/napi/threadsafe_functions.rs | 18 +---- cli/napi_sym/lib.rs | 3 +- ext/napi/lib.rs | 31 +++++--- ext/napi/value.rs | 58 +++++++++++++++ 5 files changed, 147 insertions(+), 85 deletions(-) create mode 100644 ext/napi/value.rs diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs index 6270dbe5cf..31e021f708 100644 --- a/cli/napi/js_native_api.rs +++ b/cli/napi/js_native_api.rs @@ -23,6 +23,14 @@ macro_rules! check_arg { }; } +macro_rules! check_arg_option { + ($ptr: expr) => { + if $ptr.is_none() { + return Err(Error::InvalidArg); + } + }; +} + /// Returns napi_value that represents a new JavaScript Array. #[napi_sym::napi_sym] fn napi_create_array(env: *mut Env, result: *mut napi_value) -> Result { @@ -30,7 +38,7 @@ fn napi_create_array(env: *mut Env, result: *mut napi_value) -> Result { check_arg!(result); let value = v8::Array::new(&mut env.scope(), 0); - *result = transmute::, napi_value>(value.into()); + *result = value.into(); Ok(()) } @@ -44,7 +52,7 @@ fn napi_create_array_with_length( check_arg!(result); let value = v8::Array::new(&mut env.scope(), len); - *result = transmute::, napi_value>(value.into()); + *result = value.into(); Ok(()) } @@ -63,7 +71,7 @@ fn napi_create_arraybuffer( *data = get_array_buffer_ptr(value); } - *result = transmute::, napi_value>(value.into()); + *result = value.into(); Ok(()) } @@ -77,7 +85,7 @@ fn napi_create_bigint_int64( check_arg!(result); let value = v8::BigInt::new_from_i64(&mut env.scope(), value); - *result = transmute::, napi_value>(value.into()); + *result = value.into(); Ok(()) } @@ -90,7 +98,7 @@ fn napi_create_bigint_uint64( let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::BigInt::new_from_u64(&mut env.scope(), value).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -110,7 +118,7 @@ fn napi_create_bigint_words( ) .unwrap() .into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -128,7 +136,7 @@ fn napi_create_buffer( } let value = v8::Uint8Array::new(&mut env.scope(), value, 0, len).unwrap(); let value: v8::Local = value.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -149,7 +157,7 @@ fn napi_create_buffer_copy( } let value = v8::Uint8Array::new(&mut env.scope(), value, 0, len).unwrap(); let value: v8::Local = value.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -163,7 +171,7 @@ fn napi_coerce_to_bool( let value = transmute::>(value); let coerced = value.to_boolean(&mut env.scope()); let value: v8::Local = coerced.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -179,7 +187,7 @@ fn napi_coerce_to_number( .to_number(&mut env.scope()) .ok_or(Error::NumberExpected)?; let value: v8::Local = coerced.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -193,7 +201,7 @@ fn napi_coerce_to_object( let value = transmute::>(value); let coerced = value.to_object(&mut env.scope()).unwrap(); let value: v8::Local = coerced.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -207,7 +215,7 @@ fn napi_coerce_to_string( let value = transmute::>(value); let coerced = value.to_string(&mut env.scope()).unwrap(); let value: v8::Local = coerced.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -240,7 +248,7 @@ fn napi_create_dataview( ) .unwrap(); let value: v8::Local = value.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -253,7 +261,7 @@ fn napi_create_date( let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::Date::new(&mut env.scope(), time).unwrap().into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -266,7 +274,7 @@ fn napi_create_double( let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::Number::new(&mut env.scope(), value).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -285,7 +293,7 @@ fn napi_create_error( let msg = msg.to_string(&mut env.scope()).unwrap(); let error = v8::Exception::error(&mut env.scope(), msg); - *result = transmute::, napi_value>(error); + *result = error.into(); Ok(()) } @@ -302,7 +310,7 @@ fn napi_create_external( let value: v8::Local = v8::External::new(&mut env.scope(), value).into(); // TODO: finalization - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -353,7 +361,7 @@ fn napi_create_external_arraybuffer( let ab = v8::ArrayBuffer::with_backing_store(&mut env.scope(), &store.make_shared()); let value: v8::Local = ab.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -382,7 +390,7 @@ fn napi_create_external_buffer( let value = v8::Uint8Array::new(&mut env.scope(), ab, 0, slice.len()).unwrap(); let value: v8::Local = value.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -416,7 +424,7 @@ fn napi_create_function( let function = create_function(env_ptr, name, cb, cb_info); let value: v8::Local = function.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -429,7 +437,7 @@ fn napi_create_int32( let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::Number::new(&mut env.scope(), value as f64).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -442,7 +450,7 @@ fn napi_create_int64( let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::Number::new(&mut env.scope(), value as f64).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -450,7 +458,7 @@ fn napi_create_int64( fn napi_create_object(env: *mut Env, result: *mut napi_value) -> Result { let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let object = v8::Object::new(&mut env.scope()); - *result = transmute::, napi_value>(object.into()); + *result = object.into(); Ok(()) } @@ -466,7 +474,7 @@ fn napi_create_promise( let mut global_ptr = global.into_raw(); let promise = resolver.get_promise(&mut env.scope()); *deferred = global_ptr.as_mut() as *mut _ as napi_deferred; - *promise_out = transmute::, napi_value>(promise.into()); + *promise_out = promise.into(); Ok(()) } @@ -486,7 +494,7 @@ fn napi_create_range_error( let msg = msg.to_string(&mut env.scope()).unwrap(); let error = v8::Exception::range_error(&mut env.scope(), msg); - *result = transmute::, napi_value>(error); + *result = error.into(); Ok(()) } @@ -531,7 +539,7 @@ fn napi_create_string_latin1( ) { Some(v8str) => { let value: v8::Local = v8str.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); } None => return Err(Error::GenericFailure), } @@ -555,7 +563,7 @@ fn napi_create_string_utf16( ) .unwrap(); let value: v8::Local = v8str.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -578,7 +586,7 @@ fn napi_create_string_utf8( }; let v8str = v8::String::new(&mut env.scope(), string).unwrap(); let value: v8::Local = v8str.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -590,7 +598,7 @@ fn napi_create_symbol( result: *mut napi_value, ) -> Result { let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; - let description = match description.is_null() { + let description = match description.is_none() { true => None, false => Some( transmute::>(description) @@ -599,7 +607,7 @@ fn napi_create_symbol( ), }; let sym = v8::Symbol::new(&mut env.scope(), description); - *result = transmute::, napi_value>(sym.into()); + *result = sym.into(); Ok(()) } @@ -618,7 +626,7 @@ fn napi_create_type_error( let msg = msg.to_string(&mut env.scope()).unwrap(); let error = v8::Exception::type_error(&mut env.scope(), msg); - *result = transmute::, napi_value>(error); + *result = error.into(); Ok(()) } @@ -695,7 +703,7 @@ fn napi_create_typedarray( return Err(Error::InvalidArg); } }; - *result = transmute::, napi_value>(typedarray); + *result = typedarray.into(); Ok(()) } @@ -708,7 +716,7 @@ fn napi_create_uint32( let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::Number::new(&mut env.scope(), value as f64).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -723,7 +731,7 @@ fn napi_make_callback( result: *mut napi_value, ) -> Result { let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; - check_arg!(recv); + check_arg_option!(recv); if argc > 0 { check_arg!(argv); } @@ -1172,7 +1180,7 @@ fn napi_define_class( } let value: v8::Local = tpl.get_function(scope).unwrap().into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1260,11 +1268,11 @@ fn napi_detach_arraybuffer(_env: *mut Env, value: napi_value) -> Result { } #[napi_sym::napi_sym] -fn napi_escape_handle( +fn napi_escape_handle<'s>( _env: *mut Env, _handle_scope: napi_escapable_handle_scope, - escapee: napi_value, - result: *mut napi_value, + escapee: napi_value<'s>, + result: *mut napi_value<'s>, ) -> Result { // TODO *result = escapee; @@ -1286,7 +1294,7 @@ fn napi_get_and_clear_last_exception( // TODO: just return undefined for now we don't cache // exceptions in env. let value: v8::Local = v8::undefined(&mut env.scope()).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1326,7 +1334,7 @@ fn napi_get_boolean( let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::Boolean::new(&mut env.scope(), value).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1370,7 +1378,7 @@ fn napi_get_cb_info( if !this_arg.is_null() { let mut this = args.this(); - *this_arg = transmute::, napi_value>(this.into()); + *this_arg = this.into(); } let len = args.length(); @@ -1383,7 +1391,7 @@ fn napi_get_cb_info( let mut v_argv = std::slice::from_raw_parts_mut(argv, v_argc as usize); for i in 0..v_argc { let mut arg = args.get(i); - v_argv[i as usize] = transmute::, napi_value>(arg); + v_argv[i as usize] = arg.into(); } } @@ -1437,7 +1445,7 @@ fn napi_get_element( let array = v8::Local::::try_from(object).unwrap(); let value: v8::Local = array.get_index(&mut env.scope(), index).unwrap(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1448,7 +1456,7 @@ fn napi_get_global(env: *mut Env, result: *mut napi_value) -> Result { let context = &mut env.scope().get_current_context(); let global = context.global(&mut env.scope()); let value: v8::Local = global.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1493,7 +1501,7 @@ fn napi_get_named_property( .unwrap() .get(&mut env.scope(), name.into()) .unwrap(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1513,7 +1521,7 @@ fn napi_get_null(env: *mut Env, result: *mut napi_value) -> Result { let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::null(&mut env.scope()).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1528,7 +1536,7 @@ fn napi_get_property( let object = transmute::>(object); let key = transmute::>(key); let value: v8::Local = object.get(&mut env.scope(), key).unwrap(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1546,7 +1554,7 @@ fn napi_get_property_names( .get_property_names(&mut env.scope(), Default::default()) .unwrap(); let value: v8::Local = array.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1560,7 +1568,7 @@ fn napi_get_prototype( let value = transmute::>(value); let obj = value.to_object(&mut env.scope()).unwrap(); let proto = obj.get_prototype(&mut env.scope()).unwrap(); - *result = transmute::, napi_value>(proto); + *result = proto.into(); Ok(()) } @@ -1574,7 +1582,7 @@ fn napi_get_reference_value( let _env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value = transmute::>(reference); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1604,7 +1612,7 @@ fn napi_get_typedarray_info( fn napi_get_undefined(env: *mut Env, result: *mut napi_value) -> Result { let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; let value: v8::Local = v8::undefined(&mut env.scope()).into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -1704,8 +1712,8 @@ fn napi_instanceof( result: *mut bool, ) -> Result { let env: &mut Env = env.as_mut().ok_or(Error::InvalidArg)?; - check_arg!(constructor); - check_arg!(value); + check_arg_option!(constructor); + check_arg_option!(value); let value = transmute::>(value); let constructor = transmute::>(constructor); @@ -1853,7 +1861,7 @@ fn napi_new_instance( transmute(std::slice::from_raw_parts(argv, argc)); let inst = constructor.new_instance(&mut env.scope(), args).unwrap(); let value: v8::Local = inst.into(); - *result = transmute::, napi_value>(value); + *result = value.into(); Ok(()) } @@ -2000,7 +2008,7 @@ fn napi_run_script( let rv = script.run(&mut env.scope()); if let Some(rv) = rv { - *result = transmute::, napi_value>(rv); + *result = rv.into(); } else { return Err(Error::GenericFailure); } @@ -2187,7 +2195,7 @@ fn napi_typeof( value: napi_value, result: *mut napi_valuetype, ) -> Result { - if value.is_null() { + if value.is_none() { *result = napi_undefined; return Ok(()); } @@ -2269,7 +2277,7 @@ fn node_api_create_syntax_error( let msg = msg.to_string(&mut env.scope()).unwrap(); let error = v8::Exception::syntax_error(&mut env.scope(), msg); - *result = transmute::, napi_value>(error); + *result = error.into(); Ok(()) } diff --git a/cli/napi/threadsafe_functions.rs b/cli/napi/threadsafe_functions.rs index 5374b61598..1a9704dbec 100644 --- a/cli/napi/threadsafe_functions.rs +++ b/cli/napi/threadsafe_functions.rs @@ -49,22 +49,12 @@ impl TsFn { let func: v8::Local = func.open(scope).to_object(scope).unwrap().into(); unsafe { - call_js_cb( - env as *mut c_void, - transmute::, napi_value>(func), - context, - data, - ) + call_js_cb(env as *mut c_void, func.into(), context, data) }; } None => { unsafe { - call_js_cb( - env as *mut c_void, - std::ptr::null_mut(), - context, - data, - ) + call_js_cb(env as *mut c_void, std::mem::zeroed(), context, data) }; } } @@ -110,9 +100,7 @@ fn napi_create_threadsafe_function( return Err(Error::InvalidArg); } let maybe_func = func - .as_mut() - .map(|func| { - let value = transmute::>(func); + .map(|value| { let func = v8::Local::::try_from(value) .map_err(|_| Error::FunctionExpected)?; Ok(v8::Global::new(&mut env_ref.scope(), func)) diff --git a/cli/napi_sym/lib.rs b/cli/napi_sym/lib.rs index caef3da657..769dddba96 100644 --- a/cli/napi_sym/lib.rs +++ b/cli/napi_sym/lib.rs @@ -27,6 +27,7 @@ pub fn napi_sym(_attr: TokenStream, item: TokenStream) -> TokenStream { let block = &func.block; let inputs = &func.sig.inputs; let output = &func.sig.output; + let generics = &func.sig.generics; let ret_ty = match output { syn::ReturnType::Default => panic!("expected a return type"), syn::ReturnType::Type(_, ty) => quote! { #ty }, @@ -34,7 +35,7 @@ pub fn napi_sym(_attr: TokenStream, item: TokenStream) -> TokenStream { TokenStream::from(quote! { // SAFETY: it's an NAPI function. #[no_mangle] - pub unsafe extern "C" fn #name(#inputs) -> napi_status { + pub unsafe extern "C" fn #name #generics (#inputs) -> napi_status { let mut inner = || -> #ret_ty { #block }; diff --git a/ext/napi/lib.rs b/ext/napi/lib.rs index 5bcb296435..5fd3085ca9 100644 --- a/ext/napi/lib.rs +++ b/ext/napi/lib.rs @@ -12,18 +12,12 @@ use deno_core::futures::channel::mpsc; use deno_core::futures::StreamExt; use deno_core::op; use deno_core::serde_v8; -pub use deno_core::v8; use deno_core::Extension; use deno_core::OpState; use std::cell::RefCell; -pub use std::ffi::CStr; use std::ffi::CString; -pub use std::mem::transmute; -pub use std::os::raw::c_char; -pub use std::os::raw::c_void; use std::path::Path; use std::path::PathBuf; -pub use std::ptr; use std::task::Poll; use std::thread_local; @@ -33,11 +27,21 @@ use libloading::os::unix::*; #[cfg(windows)] use libloading::os::windows::*; +// Expose common stuff for ease of use. +// `use deno_napi::*` +pub use deno_core::v8; +pub use std::ffi::CStr; +pub use std::mem::transmute; +pub use std::os::raw::c_char; +pub use std::os::raw::c_void; +pub use std::ptr; +pub use value::napi_value; + pub mod function; +mod value; pub type napi_status = i32; pub type napi_env = *mut c_void; -pub type napi_value = *mut c_void; pub type napi_callback_info = *mut c_void; pub type napi_deferred = *mut c_void; pub type napi_ref = *mut c_void; @@ -81,7 +85,7 @@ type napi_addon_register_func = extern "C" fn(env: napi_env, exports: napi_value) -> napi_value; #[repr(C)] -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct NapiModule { pub nm_version: i32, pub nm_flags: u32, @@ -211,7 +215,10 @@ pub struct napi_type_tag { } pub type napi_callback = Option< - unsafe extern "C" fn(env: napi_env, info: napi_callback_info) -> napi_value, + unsafe extern "C" fn( + env: napi_env, + info: napi_callback_info, + ) -> napi_value<'static>, >; pub type napi_finalize = unsafe extern "C" fn( @@ -250,13 +257,13 @@ pub const napi_default_jsproperty: napi_property_attributes = #[repr(C)] #[derive(Copy, Clone, Debug)] -pub struct napi_property_descriptor { +pub struct napi_property_descriptor<'a> { pub utf8name: *const c_char, - pub name: napi_value, + pub name: napi_value<'a>, pub method: napi_callback, pub getter: napi_callback, pub setter: napi_callback, - pub value: napi_value, + pub value: napi_value<'a>, pub attributes: napi_property_attributes, pub data: *mut c_void, } diff --git a/ext/napi/value.rs b/ext/napi/value.rs new file mode 100644 index 0000000000..bb3966593f --- /dev/null +++ b/ext/napi/value.rs @@ -0,0 +1,58 @@ +// Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. + +use deno_core::v8; +use std::mem::transmute; +use std::ops::Deref; +use std::os::raw::c_void; +use std::ptr::NonNull; + +/// An FFI-opaque, nullable wrapper around v8::Local. +/// rusty_v8 Local handle cannot be empty but napi_value can be. +#[repr(transparent)] +#[derive(Clone, Copy, Debug)] +pub struct NapiValue<'s>( + Option>, + std::marker::PhantomData<&'s ()>, +); + +pub type napi_value<'s> = NapiValue<'s>; + +impl<'s> Deref for napi_value<'s> { + type Target = Option>; + fn deref(&self) -> &Self::Target { + // SAFETY: It is safe to transmute `Option>` to `Option<*const T>`. + // v8::Local guarantees that *const T is not null but napi_value *can* be null. + unsafe { transmute::<&Self, &Self::Target>(self) } + } +} + +impl<'s, T> From> for napi_value<'s> +where + v8::Local<'s, T>: Into>, +{ + fn from(v: v8::Local<'s, T>) -> Self { + // SAFETY: It is safe to cast v8::Local that implements Into>. + // `fn into(self)` transmutes anyways. + Self(unsafe { transmute(v) }, std::marker::PhantomData) + } +} + +const _: () = { + assert!( + std::mem::size_of::() == std::mem::size_of::<*mut c_void>() + ); + // Assert "nullable pointer optimization" on napi_value + unsafe { + type Src<'a> = napi_value<'a>; + type Dst = usize; + assert!(std::mem::size_of::() == std::mem::size_of::()); + union Transmute<'a> { + src: Src<'a>, + dst: Dst, + } + Transmute { + src: NapiValue(None, std::marker::PhantomData), + } + .dst + }; +};