From 84602845c6f2a015b7b214751543d0793e45a5a9 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Wed, 5 Apr 2017 10:18:56 -0700 Subject: [PATCH] n-api: Update property attrs enum to match JS spec The napi_property_attributes enum used names and values from v8::PropertyAttribute, but those negative flag names were outdated along with the default behavior of a property being writable, enumerable, and configurable unless otherwise specified. To match the ES5 standard property descriptor those attributes should be positive flags and should default to false unless otherwise specified. PR-URL: https://github.com/nodejs/node/pull/12240 Fixes: https://github.com/nodejs/abi-stable-node/issues/221 Reviewed-By: Michael Dawson Reviewed-By: Timothy Gu --- src/node_api.cc | 107 ++++++++++-------- src/node_api_types.h | 8 +- test/addons-napi/test_constructor/test.js | 16 ++- .../test_constructor/test_constructor.c | 13 ++- test/addons-napi/test_properties/test.js | 16 ++- .../test_properties/test_properties.c | 13 ++- 6 files changed, 107 insertions(+), 66 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 70696aad6bbb24..be30a8461ac3cc 100644 --- a/src/node_api.cc +++ b/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(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 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 cbdata = - v8impl::CreateFunctionCallbackData(env, p->method, p->data); - - RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); - - v8::Local 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 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 cbdata = + v8impl::CreateFunctionCallbackData(env, p->method, p->data); + + RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure); + + v8::Local 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 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 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 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 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 value = v8impl::V8LocalValueFromJsValue(p->value); diff --git a/src/node_api_types.h b/src/node_api_types.h index 7cb242368a367e..f3f3faaa641cbb 100644 --- a/src/node_api_types.h +++ b/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 { diff --git a/test/addons-napi/test_constructor/test.js b/test/addons-napi/test_constructor/test.js index 9feae5a1360b2c..b1762c2253b99a 100644 --- a/test/addons-napi/test_constructor/test.js +++ b/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; }); diff --git a/test/addons-napi/test_constructor/test_constructor.c b/test/addons-napi/test_constructor/test_constructor.c index 5a45da30739d6d..d0382f2c65ba5e 100644 --- a/test/addons-napi/test_constructor/test_constructor.c +++ b/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; diff --git a/test/addons-napi/test_properties/test.js b/test/addons-napi/test_properties/test.js index 8e19903dcfe6e6..fb0c19ceedb086 100644 --- a/test/addons-napi/test_properties/test.js +++ b/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; }); diff --git a/test/addons-napi/test_properties/test_properties.c b/test/addons-napi/test_properties/test_properties.c index 9474e97266649b..de6b39ba027577 100644 --- a/test/addons-napi/test_properties/test_properties.c +++ b/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(