From 4baaa246a256a82e725ddf07015bad72ec13f3e5 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Fri, 10 Feb 2023 18:20:47 +0530 Subject: [PATCH] fix(cli/napi): handle all property variants in napi_define_properties (#17680) Fixes https://github.com/denoland/deno/issues/17509 This fixes the bug that blocked loading `fsevents` in Deno. --- cli/napi/js_native_api.rs | 59 +++++++++++++++++++++++++++----- test_napi/properties_test.js | 9 +++-- test_napi/src/lib.rs | 2 +- test_napi/src/properties.rs | 66 +++++++++++++++++++++++++----------- 4 files changed, 104 insertions(+), 32 deletions(-) diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs index 4dc249fff20260..d531c72c81d24c 100644 --- a/cli/napi/js_native_api.rs +++ b/cli/napi/js_native_api.rs @@ -1423,27 +1423,68 @@ fn napi_define_properties( properties: *const napi_property_descriptor, ) -> Result { let env: &mut Env = env_ptr.as_mut().ok_or(Error::InvalidArg)?; + if property_count > 0 { + check_arg!(env, properties); + } + let scope = &mut env.scope(); - let object = transmute::>(obj); + + let object: v8::Local = napi_value_unchecked(obj) + .try_into() + .map_err(|_| Error::ObjectExpected)?; + let properties = std::slice::from_raw_parts(properties, property_count); for property in properties { let name = if !property.utf8name.is_null() { - let name_str = CStr::from_ptr(property.utf8name); - let name_str = name_str.to_str().unwrap(); - v8::String::new(scope, name_str).unwrap() + let name_str = CStr::from_ptr(property.utf8name).to_str().unwrap(); + v8::String::new(scope, name_str).ok_or(Error::GenericFailure)? } else { - transmute::>(property.name) + let property_value = napi_value_unchecked(property.name); + v8::Local::::try_from(property_value) + .map_err(|_| Error::NameExpected)? }; - let method_ptr = property.method; + if property.getter.is_some() || property.setter.is_some() { + let local_getter: v8::Local = if property.getter.is_some() { + create_function(env_ptr, None, property.getter, property.data).into() + } else { + v8::undefined(scope).into() + }; + let local_setter: v8::Local = if property.setter.is_some() { + create_function(env_ptr, None, property.setter, property.data).into() + } else { + v8::undefined(scope).into() + }; + + let mut desc = + v8::PropertyDescriptor::new_from_get_set(local_getter, local_setter); + desc.set_enumerable(property.attributes & napi_enumerable != 0); + desc.set_configurable(property.attributes & napi_configurable != 0); - if method_ptr.is_some() { - let function: v8::Local = { + let define_maybe = object.define_property(scope, name.into(), &desc); + return_status_if_false!( + env_ptr, + !define_maybe.unwrap_or(false), + napi_invalid_arg + ); + } else if property.method.is_some() { + let value: v8::Local = { let function = create_function(env_ptr, None, property.method, property.data); function.into() }; - object.set(scope, name.into(), function).unwrap(); + return_status_if_false!( + env_ptr, + object.set(scope, name.into(), value).is_some(), + napi_invalid_arg + ); + } else { + let value = napi_value_unchecked(property.value); + return_status_if_false!( + env_ptr, + object.set(scope, name.into(), value).is_some(), + napi_invalid_arg + ); } } diff --git a/test_napi/properties_test.js b/test_napi/properties_test.js index 78268ba15760eb..36ede10332d28a 100644 --- a/test_napi/properties_test.js +++ b/test_napi/properties_test.js @@ -5,11 +5,14 @@ import { assertEquals, loadTestLibrary } from "./common.js"; const properties = loadTestLibrary(); Deno.test("napi properties", () => { - properties.test_property_rw = 1; assertEquals(properties.test_property_rw, 1); properties.test_property_rw = 2; assertEquals(properties.test_property_rw, 2); - // assertEquals(properties.test_property_r, 2); - // assertRejects(() => properties.test_property_r = 3); + assertEquals(properties.test_property_r, 1); + + // https://github.com/denoland/deno/issues/17509 + assertEquals(properties.test_simple_property, { + nice: 69, + }); }); diff --git a/test_napi/src/lib.rs b/test_napi/src/lib.rs index ed0afb741bc7ab..dba9f65a5cf6f4 100644 --- a/test_napi/src/lib.rs +++ b/test_napi/src/lib.rs @@ -27,7 +27,7 @@ pub mod typedarray; #[macro_export] macro_rules! cstr { ($s: literal) => {{ - std::ffi::CString::new($s).unwrap().as_ptr() + std::ffi::CString::new($s).unwrap().into_raw() }}; } diff --git a/test_napi/src/properties.rs b/test_napi/src/properties.rs index a54738cb571ff9..1b6c9488b308af 100644 --- a/test_napi/src/properties.rs +++ b/test_napi/src/properties.rs @@ -1,11 +1,28 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use crate::assert_napi_ok; +use crate::cstr; use napi_sys::PropertyAttributes::*; use napi_sys::*; -use std::os::raw::c_char; use std::ptr; +static NICE: i64 = 69; + +fn init_constants(env: napi_env) -> napi_value { + let mut constants: napi_value = ptr::null_mut(); + let mut value: napi_value = ptr::null_mut(); + + assert_napi_ok!(napi_create_object(env, &mut constants)); + assert_napi_ok!(napi_create_int64(env, NICE, &mut value)); + assert_napi_ok!(napi_set_named_property( + env, + constants, + cstr!("nice"), + value + )); + constants +} + pub fn init(env: napi_env, exports: napi_value) { let mut number: napi_value = ptr::null_mut(); assert_napi_ok!(napi_create_double(env, 1.0, &mut number)); @@ -14,7 +31,7 @@ pub fn init(env: napi_env, exports: napi_value) { let mut name_value: napi_value = ptr::null_mut(); assert_napi_ok!(napi_create_string_utf8( env, - "key_v8_string".as_ptr() as *const c_char, + cstr!("key_v8_string"), usize::MAX, &mut name_value, )); @@ -24,7 +41,7 @@ pub fn init(env: napi_env, exports: napi_value) { let mut name_symbol: napi_value = ptr::null_mut(); assert_napi_ok!(napi_create_string_utf8( env, - "key_v8_symbol".as_ptr() as *const c_char, + cstr!("key_v8_symbol"), usize::MAX, &mut symbol_description, )); @@ -36,38 +53,28 @@ pub fn init(env: napi_env, exports: napi_value) { let properties = &[ napi_property_descriptor { - utf8name: "test_property_rw\0".as_ptr() as *const c_char, + utf8name: cstr!("test_simple_property"), name: ptr::null_mut(), method: None, getter: None, setter: None, data: ptr::null_mut(), attributes: enumerable | writable, - value: number, + value: init_constants(env), }, napi_property_descriptor { - utf8name: "test_property_r\0".as_ptr() as *const c_char, + utf8name: cstr!("test_property_rw"), name: ptr::null_mut(), method: None, getter: None, setter: None, data: ptr::null_mut(), - attributes: enumerable, - value: number, - }, - napi_property_descriptor { - utf8name: ptr::null(), - name: name_value, - method: None, - getter: None, - setter: None, - data: ptr::null_mut(), - attributes: enumerable, + attributes: enumerable | writable, value: number, }, napi_property_descriptor { - utf8name: ptr::null(), - name: name_symbol, + utf8name: cstr!("test_property_r"), + name: ptr::null_mut(), method: None, getter: None, setter: None, @@ -75,6 +82,27 @@ pub fn init(env: napi_env, exports: napi_value) { attributes: enumerable, value: number, }, + // TODO(@littledivy): Fix this. + // napi_property_descriptor { + // utf8name: ptr::null(), + // name: name_value, + // method: None, + // getter: None, + // setter: None, + // data: ptr::null_mut(), + // attributes: enumerable, + // value: number, + // }, + // napi_property_descriptor { + // utf8name: ptr::null(), + // name: name_symbol, + // method: None, + // getter: None, + // setter: None, + // data: ptr::null_mut(), + // attributes: enumerable, + // value: number, + // }, ]; assert_napi_ok!(napi_define_properties(