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.

4 files changed, 104 insertions, 32 deletions

diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs
index 4dc249fff..d531c72c8 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::<napi_value, v8::Local<v8::Object>>(obj);
+
+  let object: v8::Local<v8::Object> = 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::<napi_value, v8::Local<v8::String>>(property.name)
+      let property_value = napi_value_unchecked(property.name);
+      v8::Local::<v8::String>::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<v8::Value> = 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<v8::Value> = 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<v8::Value> = {
+      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<v8::Value> = {
         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 78268ba15..36ede1033 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 ed0afb741..dba9f65a5 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 a54738cb5..1b6c9488b 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(