From 44cef983fe8587a24d1b40e9f6f63bd8f62ff936 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. Fixes: https://github.com/nodejs/abi-stable-node/issues/221 --- src/node_api.cc | 33 ++++++++++++------- src/node_api_types.h | 8 ++--- .../test_constructor/test_constructor.c | 10 +++--- .../test_properties/test_properties.c | 10 +++--- 4 files changed, 36 insertions(+), 25 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index 70696aad6bbb24..b623580f2f7dc1 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -38,13 +38,13 @@ namespace v8impl { static inline v8::PropertyAttribute V8PropertyAttributesFromAttributes( napi_property_attributes attributes) { unsigned int attribute_flags = v8::None; - if (attributes & napi_read_only) { + if ((attributes & napi_writable) == 0) { attribute_flags |= v8::ReadOnly; } - if (attributes & napi_dont_enum) { + if ((attributes & napi_enumerable) == 0) { attribute_flags |= v8::DontEnum; } - if (attributes & napi_dont_delete) { + if ((attributes & napi_configurable) == 0) { attribute_flags |= v8::DontDelete; } return static_cast(attribute_flags); @@ -775,7 +775,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; @@ -788,7 +788,7 @@ napi_status napi_define_class(napi_env env, // This code is similar to that in napi_define_property(); the // difference is it applies to a template instead of an object. - if (p->method) { + if (p->method != nullptr) { v8::Local cbdata = v8impl::CreateFunctionCallbackData(env, p->method, p->data); @@ -802,10 +802,16 @@ napi_status napi_define_class(napi_env env, t->SetClassName(property_name); tpl->PrototypeTemplate()->Set(property_name, t, attributes); - } else if (p->getter || p->setter) { + } else if (p->getter != nullptr || p->setter != nullptr) { v8::Local cbdata = v8impl::CreateAccessorCallbackData( env, p->getter, p->setter, p->data); + // The napi_writable attribute is ignored for accessor descriptors, + // but V8 requires the ReadOnly attribute to match existence of a setter. + attributes = static_cast(p->setter != nullptr + ? attributes & ~v8::PropertyAttribute::ReadOnly + : attributes | v8::PropertyAttribute::ReadOnly); + tpl->PrototypeTemplate()->SetAccessor( property_name, p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr, @@ -827,7 +833,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 +1100,9 @@ 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::V8PropertyAttributesFromAttributes(p->attributes); - if (p->method) { + if (p->method != nullptr) { v8::Local cbdata = v8impl::CreateFunctionCallbackData(env, p->method, p->data); @@ -1112,13 +1117,19 @@ 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) { + } else if (p->getter != nullptr || p->setter != nullptr) { v8::Local cbdata = v8impl::CreateAccessorCallbackData( env, p->getter, p->setter, p->data); + // The napi_writable attribute is ignored for accessor descriptors, + // but V8 requires the ReadOnly attribute to match existence of a setter. + attributes = static_cast(p->setter != nullptr + ? attributes & ~v8::PropertyAttribute::ReadOnly + : attributes | v8::PropertyAttribute::ReadOnly); + auto set_maybe = obj->SetAccessor( context, name, 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_constructor.c b/test/addons-napi/test_constructor/test_constructor.c index 5a45da30739d6d..689342f12dd43b 100644 --- a/test/addons-napi/test_constructor/test_constructor.c +++ b/test/addons-napi/test_constructor/test_constructor.c @@ -82,11 +82,11 @@ 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 }, + { "accessorValue", 0, GetValue, SetValue, 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}, }; napi_value cons; diff --git a/test/addons-napi/test_properties/test_properties.c b/test/addons-napi/test_properties/test_properties.c index 9474e97266649b..137a45f57039bd 100644 --- a/test/addons-napi/test_properties/test_properties.c +++ b/test/addons-napi/test_properties/test_properties.c @@ -70,11 +70,11 @@ 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 }, + { "accessorValue", 0, GetValue, SetValue, 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}, }; status = napi_define_properties(