n-api: Update property attrs enum to match JS spec

src/node_api.cc

@@ -35,18 +35,26 @@ class napi_env__ {
 namespace v8impl {
 
 // convert from n-api property attributes to v8::PropertyAttribute
-static inline v8::PropertyAttribute V8PropertyAttributesFromAttributes(
-    napi_property_attributes attributes) {
-  unsigned int attribute_flags = v8::None;
-  if (attributes & napi_read_only) {
-    attribute_flags |= v8::ReadOnly;
+static inline v8::PropertyAttribute V8PropertyAttributesFromDescriptor(
+    const napi_property_descriptor* descriptor) {
+  unsigned int attribute_flags = v8::PropertyAttribute::None;
+
+  if (descriptor->getter != nullptr || descriptor->setter != nullptr) {
+    // The napi_writable attribute is ignored for accessor descriptors, but
+    // V8 requires the ReadOnly attribute to match nonexistence of a setter.
+    attribute_flags |= (descriptor->setter == nullptr ?
+      v8::PropertyAttribute::ReadOnly : v8::PropertyAttribute::None);
+  } else if ((descriptor->attributes & napi_writable) == 0) {
+    attribute_flags |= v8::PropertyAttribute::ReadOnly;
   }
-  if (attributes & napi_dont_enum) {
-    attribute_flags |= v8::DontEnum;
+
+  if ((descriptor->attributes & napi_enumerable) == 0) {
+    attribute_flags |= v8::PropertyAttribute::DontEnum;
   }
-  if (attributes & napi_dont_delete) {
-    attribute_flags |= v8::DontDelete;
+  if ((descriptor->attributes & napi_configurable) == 0) {
+    attribute_flags |= v8::PropertyAttribute::DontDelete;
   }
+
   return static_cast<v8::PropertyAttribute>(attribute_flags);
 }
 
@@ -775,7 +783,7 @@ napi_status napi_define_class(napi_env env,
   for (size_t i = 0; i < property_count; i++) {
     const napi_property_descriptor* p = properties + i;
 
-    if ((p->attributes & napi_static_property) != 0) {
+    if ((p->attributes & napi_static) != 0) {
       // Static properties are handled separately below.
       static_property_count++;
       continue;
@@ -784,25 +792,11 @@ napi_status napi_define_class(napi_env env,
     v8::Local<v8::String> property_name;
     CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
     v8::PropertyAttribute attributes =
-        v8impl::V8PropertyAttributesFromAttributes(p->attributes);
+        v8impl::V8PropertyAttributesFromDescriptor(p);
 
-    // This code is similar to that in napi_define_property(); the
+    // This code is similar to that in napi_define_properties(); the
     // difference is it applies to a template instead of an object.
-    if (p->method) {
-      v8::Local<v8::Object> cbdata =
-          v8impl::CreateFunctionCallbackData(env, p->method, p->data);
-
-      RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
-
-      v8::Local<v8::FunctionTemplate> t =
-          v8::FunctionTemplate::New(isolate,
-                                    v8impl::FunctionCallbackWrapper::Invoke,
-                                    cbdata,
-                                    v8::Signature::New(isolate, tpl));
-      t->SetClassName(property_name);
-
-      tpl->PrototypeTemplate()->Set(property_name, t, attributes);
-    } else if (p->getter || p->setter) {
+    if (p->getter != nullptr || p->setter != nullptr) {
       v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
         env, p->getter, p->setter, p->data);
 
@@ -813,6 +807,20 @@ napi_status napi_define_class(napi_env env,
         cbdata,
         v8::AccessControl::DEFAULT,
         attributes);
+    } else if (p->method != nullptr) {
+      v8::Local<v8::Object> cbdata =
+          v8impl::CreateFunctionCallbackData(env, p->method, p->data);
+
+      RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
+
+      v8::Local<v8::FunctionTemplate> t =
+        v8::FunctionTemplate::New(isolate,
+          v8impl::FunctionCallbackWrapper::Invoke,
+          cbdata,
+          v8::Signature::New(isolate, tpl));
+      t->SetClassName(property_name);
+
+      tpl->PrototypeTemplate()->Set(property_name, t, attributes);
     } else {
       v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
       tpl->PrototypeTemplate()->Set(property_name, value, attributes);
@@ -827,7 +835,7 @@ napi_status napi_define_class(napi_env env,
 
     for (size_t i = 0; i < property_count; i++) {
       const napi_property_descriptor* p = properties + i;
-      if ((p->attributes & napi_static_property) != 0) {
+      if ((p->attributes & napi_static) != 0) {
         static_descriptors.push_back(*p);
       }
     }
@@ -1094,10 +1102,28 @@ napi_status napi_define_properties(napi_env env,
     CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
 
     v8::PropertyAttribute attributes =
-        v8impl::V8PropertyAttributesFromAttributes(
-            (napi_property_attributes)(p->attributes & ~napi_static_property));
+          v8impl::V8PropertyAttributesFromDescriptor(p);
+
+    if (p->getter != nullptr || p->setter != nullptr) {
+      v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
+        env,
+        p->getter,
+        p->setter,
+        p->data);
 
-    if (p->method) {
+      auto set_maybe = obj->SetAccessor(
+        context,
+        name,
+        p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
+        p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
+        cbdata,
+        v8::AccessControl::DEFAULT,
+        attributes);
+
+      if (!set_maybe.FromMaybe(false)) {
+        return napi_set_last_error(env, napi_invalid_arg);
+      }
+    } else if (p->method != nullptr) {
       v8::Local<v8::Object> cbdata =
           v8impl::CreateFunctionCallbackData(env, p->method, p->data);
 
@@ -1112,25 +1138,6 @@ napi_status napi_define_properties(napi_env env,
       if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_generic_failure);
       }
-    } else if (p->getter || p->setter) {
-      v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
-        env,
-        p->getter,
-        p->setter,
-        p->data);
-
-      auto set_maybe = obj->SetAccessor(
-          context,
-          name,
-          p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
-          p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
-          cbdata,
-          v8::AccessControl::DEFAULT,
-          attributes);
-
-      if (!set_maybe.FromMaybe(false)) {
-        return napi_set_last_error(env, napi_invalid_arg);
-      }
     } else {
       v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
 

src/node_api_types.h

@@ -25,13 +25,13 @@ typedef void (*napi_finalize)(napi_env env,
 
 typedef enum {
   napi_default = 0,
-  napi_read_only = 1 << 0,
-  napi_dont_enum = 1 << 1,
-  napi_dont_delete = 1 << 2,
+  napi_writable = 1 << 0,
+  napi_enumerable = 1 << 1,
+  napi_configurable = 1 << 2,
 
   // Used with napi_define_class to distinguish static properties
   // from instance properties. Ignored by napi_define_properties.
-  napi_static_property = 1 << 10,
+  napi_static = 1 << 10,
 } napi_property_attributes;
 
 typedef struct {

test/addons-napi/test_constructor/test.js

@@ -17,7 +17,7 @@ assert.throws(() => { test_object.readonlyValue = 3; });
 
 assert.ok(test_object.hiddenValue);
 
-// All properties except 'hiddenValue' should be enumerable.
+// Properties with napi_enumerable attribute should be enumerable.
 const propertyNames = [];
 for (const name in test_object) {
   propertyNames.push(name);
@@ -26,3 +26,17 @@ assert.ok(propertyNames.indexOf('echo') >= 0);
 assert.ok(propertyNames.indexOf('readwriteValue') >= 0);
 assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
 assert.ok(propertyNames.indexOf('hiddenValue') < 0);
+assert.ok(propertyNames.indexOf('readwriteAccessor1') < 0);
+assert.ok(propertyNames.indexOf('readwriteAccessor2') < 0);
+assert.ok(propertyNames.indexOf('readonlyAccessor1') < 0);
+assert.ok(propertyNames.indexOf('readonlyAccessor2') < 0);
+
+// The napi_writable attribute should be ignored for accessors.
+test_object.readwriteAccessor1 = 1;
+assert.strictEqual(test_object.readwriteAccessor1, 1);
+assert.strictEqual(test_object.readonlyAccessor1, 1);
+assert.throws(() => { test_object.readonlyAccessor1 = 3; });
+test_object.readwriteAccessor2 = 2;
+assert.strictEqual(test_object.readwriteAccessor2, 2);
+assert.strictEqual(test_object.readonlyAccessor2, 2);
+assert.throws(() => { test_object.readonlyAccessor2 = 3; });

test/addons-napi/test_constructor/test_constructor.c

@@ -82,11 +82,14 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
   if (status != napi_ok) return;
 
   napi_property_descriptor properties[] = {
-      { "echo", Echo, 0, 0, 0, napi_default, 0 },
-      { "accessorValue", 0, GetValue, SetValue, 0, napi_default, 0},
-      { "readwriteValue", 0, 0, 0, number, napi_default, 0 },
-      { "readonlyValue", 0, 0, 0, number, napi_read_only, 0},
-      { "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0},
+      { "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
+      { "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
+      { "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
+      { "hiddenValue", 0, 0, 0, number, napi_default, 0},
+      { "readwriteAccessor1", 0, GetValue, SetValue, 0, napi_default, 0},
+      { "readwriteAccessor2", 0, GetValue, SetValue, 0, napi_writable, 0},
+      { "readonlyAccessor1", 0, GetValue, NULL, 0, napi_default, 0},
+      { "readonlyAccessor2", 0, GetValue, NULL, 0, napi_writable, 0},
   };
 
   napi_value cons;

test/addons-napi/test_properties/test.js

@@ -16,7 +16,7 @@ assert.throws(() => { test_object.readonlyValue = 3; });
 
 assert.ok(test_object.hiddenValue);
 
-// All properties except 'hiddenValue' should be enumerable.
+// Properties with napi_enumerable attribute should be enumerable.
 const propertyNames = [];
 for (const name in test_object) {
   propertyNames.push(name);
@@ -25,3 +25,17 @@ assert.ok(propertyNames.indexOf('echo') >= 0);
 assert.ok(propertyNames.indexOf('readwriteValue') >= 0);
 assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
 assert.ok(propertyNames.indexOf('hiddenValue') < 0);
+assert.ok(propertyNames.indexOf('readwriteAccessor1') < 0);
+assert.ok(propertyNames.indexOf('readwriteAccessor2') < 0);
+assert.ok(propertyNames.indexOf('readonlyAccessor1') < 0);
+assert.ok(propertyNames.indexOf('readonlyAccessor2') < 0);
+
+// The napi_writable attribute should be ignored for accessors.
+test_object.readwriteAccessor1 = 1;
+assert.strictEqual(test_object.readwriteAccessor1, 1);
+assert.strictEqual(test_object.readonlyAccessor1, 1);
+assert.throws(() => { test_object.readonlyAccessor1 = 3; });
+test_object.readwriteAccessor2 = 2;
+assert.strictEqual(test_object.readwriteAccessor2, 2);
+assert.strictEqual(test_object.readonlyAccessor2, 2);
+assert.throws(() => { test_object.readonlyAccessor2 = 3; });

test/addons-napi/test_properties/test_properties.c

@@ -70,11 +70,14 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
   if (status != napi_ok) return;
 
   napi_property_descriptor properties[] = {
-      { "echo", Echo, 0, 0, 0, napi_default, 0 },
-      { "accessorValue", 0, GetValue, SetValue, 0, napi_default, 0 },
-      { "readwriteValue", 0, 0, 0, number, napi_default, 0 },
-      { "readonlyValue", 0, 0, 0, number, napi_read_only, 0 },
-      { "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0 },
+      { "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
+      { "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
+      { "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
+      { "hiddenValue", 0, 0, 0, number, napi_default, 0},
+      { "readwriteAccessor1", 0, GetValue, SetValue, 0, napi_default, 0},
+      { "readwriteAccessor2", 0, GetValue, SetValue, 0, napi_writable, 0},
+      { "readonlyAccessor1", 0, GetValue, NULL, 0, napi_default, 0},
+      { "readonlyAccessor2", 0, GetValue, NULL, 0, napi_writable, 0},
   };
 
   status = napi_define_properties(