From d01304437cd4c661f0eda4deb84eb34d7e533f32 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 20 Mar 2023 14:22:49 -0700 Subject: [PATCH] src: interject class TypeTaggable Derive class `TypeTaggable` from `Value`, and let it serve as the base class for `Object` and `External`. That way, the type tagging is implemented for both classes. Additionally, exclude deleted .js files from linting. Signed-off-by: Gabriel Schulhof Refs: https://github.com/nodejs/node-addon-api/issues/1293 PR-URL: https://github.com/nodejs/node-addon-api/pull/1298 Reviewed-By: Michael Dawson --- doc/external.md | 4 +-- doc/object.md | 31 ++-------------- doc/type_taggable.md | 38 ++++++++++++++++++++ napi-inl.h | 48 ++++++++++++++++--------- napi.h | 18 +++++++--- test/binding.cc | 4 +-- test/binding.gyp | 2 +- test/index.js | 2 +- test/object/object_type_tag.cc | 39 -------------------- test/object/object_type_tag.js | 55 ---------------------------- test/type_taggable.cc | 66 ++++++++++++++++++++++++++++++++++ test/type_taggable.js | 60 +++++++++++++++++++++++++++++++ tools/eslint-format.js | 4 +-- 13 files changed, 218 insertions(+), 153 deletions(-) create mode 100644 doc/type_taggable.md delete mode 100644 test/object/object_type_tag.cc delete mode 100644 test/object/object_type_tag.js create mode 100644 test/type_taggable.cc create mode 100644 test/type_taggable.js diff --git a/doc/external.md b/doc/external.md index 229cc7fcc..ce42e112a 100644 --- a/doc/external.md +++ b/doc/external.md @@ -1,6 +1,6 @@ # External (template) -Class `Napi::External` inherits from class [`Napi::Value`][]. +Class `Napi::External` inherits from class [`Napi::TypeTaggable`][]. The `Napi::External` template class implements the ability to create a `Napi::Value` object with arbitrary C++ data. It is the user's responsibility to manage the memory for the arbitrary C++ data. @@ -67,4 +67,4 @@ T* Napi::External::Data() const; Returns a pointer to the arbitrary C++ data held by the `Napi::External` object. -[`Napi::Value`]: ./value.md +[`Napi::TypeTaggable`]: ./type_taggable.md diff --git a/doc/object.md b/doc/object.md index d660fc39e..a4d3280b8 100644 --- a/doc/object.md +++ b/doc/object.md @@ -1,6 +1,6 @@ # Object -Class `Napi::Object` inherits from class [`Napi::Value`][]. +Class `Napi::Object` inherits from class [`Napi::TypeTaggable`][]. The `Napi::Object` class corresponds to a JavaScript object. It is extended by the following node-addon-api classes that you may use when working with more specific types: @@ -241,33 +241,6 @@ from being added to it and marking all existing properties as non-configurable. Values of present properties can still be changed as long as they are writable. -### TypeTag() - -```cpp -void Napi::Object::TypeTag(const napi_type_tag* type_tag) const; -``` - -- `[in] type_tag`: The tag with which this object is to be marked. - -The `Napi::Object::TypeTag()` method associates the value of the `type_tag` -pointer with this JavaScript object. `Napi::Object::CheckTypeTag()` can then be -used to compare the tag that was attached to this object with one owned by the -addon to ensure that this object has the right type. - -### CheckTypeTag() - -```cpp -bool Napi::Object::CheckTypeTag(const napi_type_tag* type_tag) const; -``` - -- `[in] type_tag`: The tag with which to compare any tag found on this object. - -The `Napi::Object::CheckTypeTag()` method compares the pointer given as -`type_tag` with any that can be found on this JavaScript object. If no tag is -found on this object or, if a tag is found but it does not match `type_tag`, -then the return value is `false`. If a tag is found and it matches `type_tag`, -then the return value is `true`. - ### operator\[\]() ```cpp @@ -434,5 +407,5 @@ void Increment(const CallbackInfo& info) { } ``` -[`Napi::Value`]: ./value.md +[`Napi::TypeTaggable`]: ./type_taggable.md [`Napi::Value::From`]: ./value.md#from diff --git a/doc/type_taggable.md b/doc/type_taggable.md new file mode 100644 index 000000000..fb505477d --- /dev/null +++ b/doc/type_taggable.md @@ -0,0 +1,38 @@ +# TypeTaggable + +Class `Napi::TypeTaggable` inherits from class [`Napi::Value`][]. + +The `Napi::TypeTaggable` class is the base class for [`Napi::Object`][] and +[`Napi::External`][]. It adds type-tagging capabilities to both. It is an +abstract-only base class. + +### TypeTag() + +```cpp +void Napi::TypeTaggable::TypeTag(const napi_type_tag* type_tag) const; +``` + +- `[in] type_tag`: The tag with which this object or external is to be marked. + +The `Napi::TypeTaggable::TypeTag()` method associates the value of the +`type_tag` pointer with this JavaScript object or external. +`Napi::TypeTaggable::CheckTypeTag()` can then be used to compare the tag that +was attached with one owned by the add-on to ensure that this object or external +has the right type. + +### CheckTypeTag() + +```cpp +bool Napi::TypeTaggable::CheckTypeTag(const napi_type_tag* type_tag) const; +``` + +- `[in] type_tag`: The tag with which to compare any tag found on this object or + external. + +The `Napi::TypeTaggable::CheckTypeTag()` method compares the pointer given as +`type_tag` with any that can be found on this JavaScript object or external. If +no tag is found or if a tag is found but it does not match `type_tag`, then the +return value is `false`. If a tag is found and it matches `type_tag`, then the +return value is `true`. + +[`Napi::Value`]: ./value.md diff --git a/napi-inl.h b/napi-inl.h index db0530231..ddd366c2a 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -1248,6 +1248,32 @@ String String::From(napi_env env, const T& value) { return Helper::From(env, value); } +//////////////////////////////////////////////////////////////////////////////// +// TypeTaggable class +//////////////////////////////////////////////////////////////////////////////// + +inline TypeTaggable::TypeTaggable() : Value() {} + +inline TypeTaggable::TypeTaggable(napi_env _env, napi_value _value) + : Value(_env, _value) {} + +#if NAPI_VERSION >= 8 + +inline void TypeTaggable::TypeTag(const napi_type_tag* type_tag) const { + napi_status status = napi_type_tag_object(_env, _value, type_tag); + NAPI_THROW_IF_FAILED_VOID(_env, status); +} + +inline bool TypeTaggable::CheckTypeTag(const napi_type_tag* type_tag) const { + bool result; + napi_status status = + napi_check_object_type_tag(_env, _value, type_tag, &result); + NAPI_THROW_IF_FAILED(_env, status, false); + return result; +} + +#endif // NAPI_VERSION >= 8 + //////////////////////////////////////////////////////////////////////////////// // Object class //////////////////////////////////////////////////////////////////////////////// @@ -1287,9 +1313,10 @@ inline Object Object::New(napi_env env) { return Object(env, value); } -inline Object::Object() : Value() {} +inline Object::Object() : TypeTaggable() {} -inline Object::Object(napi_env env, napi_value value) : Value(env, value) {} +inline Object::Object(napi_env env, napi_value value) + : TypeTaggable(env, value) {} inline Object::PropertyLValue Object::operator[]( const char* utf8name) { @@ -1632,19 +1659,6 @@ inline MaybeOrValue Object::Seal() const { napi_status status = napi_object_seal(_env, _value); NAPI_RETURN_OR_THROW_IF_FAILED(_env, status, status == napi_ok, bool); } - -inline void Object::TypeTag(const napi_type_tag* type_tag) const { - napi_status status = napi_type_tag_object(_env, _value, type_tag); - NAPI_THROW_IF_FAILED_VOID(_env, status); -} - -inline bool Object::CheckTypeTag(const napi_type_tag* type_tag) const { - bool result; - napi_status status = - napi_check_object_type_tag(_env, _value, type_tag, &result); - NAPI_THROW_IF_FAILED(_env, status, false); - return result; -} #endif // NAPI_VERSION >= 8 //////////////////////////////////////////////////////////////////////////////// @@ -1706,11 +1720,11 @@ inline External External::New(napi_env env, } template -inline External::External() : Value() {} +inline External::External() : TypeTaggable() {} template inline External::External(napi_env env, napi_value value) - : Value(env, value) {} + : TypeTaggable(env, value) {} template inline T* External::Data() const { diff --git a/napi.h b/napi.h index a7d54027d..b651db55b 100644 --- a/napi.h +++ b/napi.h @@ -714,8 +714,19 @@ class Symbol : public Name { napi_value value); ///< Wraps a Node-API value primitive. }; +class TypeTaggable : public Value { + public: +#if NAPI_VERSION >= 8 + void TypeTag(const napi_type_tag* type_tag) const; + bool CheckTypeTag(const napi_type_tag* type_tag) const; +#endif // NAPI_VERSION >= 8 + protected: + TypeTaggable(); + TypeTaggable(napi_env env, napi_value value); +}; + /// A JavaScript object value. -class Object : public Value { +class Object : public TypeTaggable { public: /// Enables property and element assignments using indexing syntax. /// @@ -991,14 +1002,11 @@ class Object : public Value { /// See /// https://tc39.es/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-getprototypeof MaybeOrValue Seal() const; - - void TypeTag(const napi_type_tag* type_tag) const; - bool CheckTypeTag(const napi_type_tag* type_tag) const; #endif // NAPI_VERSION >= 8 }; template -class External : public Value { +class External : public TypeTaggable { public: static External New(napi_env env, T* data); diff --git a/test/binding.cc b/test/binding.cc index f0aaa3c2a..40fdbcc23 100644 --- a/test/binding.cc +++ b/test/binding.cc @@ -76,7 +76,7 @@ Object InitVersionManagement(Env env); Object InitThunkingManual(Env env); #if (NAPI_VERSION > 7) Object InitObjectFreezeSeal(Env env); -Object InitObjectTypeTag(Env env); +Object InitTypeTaggable(Env env); #endif #if defined(NODE_ADDON_API_ENABLE_MAYBE) @@ -169,7 +169,7 @@ Object Init(Env env, Object exports) { exports.Set("thunking_manual", InitThunkingManual(env)); #if (NAPI_VERSION > 7) exports.Set("object_freeze_seal", InitObjectFreezeSeal(env)); - exports.Set("object_type_tag", InitObjectTypeTag(env)); + exports.Set("type_taggable", InitTypeTaggable(env)); #endif #if defined(NODE_ADDON_API_ENABLE_MAYBE) diff --git a/test/binding.gyp b/test/binding.gyp index bc426847e..3817c2c9d 100644 --- a/test/binding.gyp +++ b/test/binding.gyp @@ -48,7 +48,6 @@ 'object/has_property.cc', 'object/object.cc', 'object/object_freeze_seal.cc', - 'object/object_type_tag.cc', 'object/set_property.cc', 'object/subscript_operator.cc', 'promise.cc', @@ -60,6 +59,7 @@ 'threadsafe_function/threadsafe_function_sum.cc', 'threadsafe_function/threadsafe_function_unref.cc', 'threadsafe_function/threadsafe_function.cc', + 'type_taggable.cc', 'typed_threadsafe_function/typed_threadsafe_function_ctx.cc', 'typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc', 'typed_threadsafe_function/typed_threadsafe_function_ptr.cc', diff --git a/test/index.js b/test/index.js index 53d5dbddb..f04349a8f 100644 --- a/test/index.js +++ b/test/index.js @@ -134,7 +134,7 @@ if (majorNodeVersion < 12 && !filterConditionsProvided) { if (napiVersion < 8 && !filterConditionsProvided) { testModules.splice(testModules.indexOf('object/object_freeze_seal'), 1); - testModules.splice(testModules.indexOf('object/object_type_tag'), 1); + testModules.splice(testModules.indexOf('type_taggable'), 1); } (async function () { diff --git a/test/object/object_type_tag.cc b/test/object/object_type_tag.cc deleted file mode 100644 index c55005a3d..000000000 --- a/test/object/object_type_tag.cc +++ /dev/null @@ -1,39 +0,0 @@ -#include "napi.h" - -#if (NAPI_VERSION > 7) - -using namespace Napi; - -static const napi_type_tag type_tags[5] = { - {0xdaf987b3cc62481a, 0xb745b0497f299531}, - {0xbb7936c374084d9b, 0xa9548d0762eeedb9}, - {0xa5ed9ce2e4c00c38, 0}, - {0, 0}, - {0xa5ed9ce2e4c00c38, 0xdaf987b3cc62481a}, -}; - -Value TypeTaggedInstance(const CallbackInfo& info) { - Object instance = Object::New(info.Env()); - uint32_t type_index = info[0].As().Int32Value(); - - instance.TypeTag(&type_tags[type_index]); - - return instance; -} - -Value CheckTypeTag(const CallbackInfo& info) { - uint32_t type_index = info[0].As().Int32Value(); - Object instance = info[1].As(); - - return Boolean::New(info.Env(), - instance.CheckTypeTag(&type_tags[type_index])); -} - -Object InitObjectTypeTag(Env env) { - Object exports = Object::New(env); - exports["checkTypeTag"] = Function::New(env, CheckTypeTag); - exports["typedTaggedInstance"] = Function::New(env, TypeTaggedInstance); - return exports; -} - -#endif diff --git a/test/object/object_type_tag.js b/test/object/object_type_tag.js deleted file mode 100644 index ef3ee1e2a..000000000 --- a/test/object/object_type_tag.js +++ /dev/null @@ -1,55 +0,0 @@ -'use strict'; - -const assert = require('assert'); - -module.exports = require('../common').runTest(test); - -// eslint-disable-next-line camelcase -function test ({ object_type_tag }) { - const obj1 = object_type_tag.typedTaggedInstance(0); - const obj2 = object_type_tag.typedTaggedInstance(1); - - // Verify that type tags are correctly accepted. - assert.strictEqual(object_type_tag.checkTypeTag(0, obj1), true); - assert.strictEqual(object_type_tag.checkTypeTag(1, obj2), true); - - // Verify that wrongly tagged objects are rejected. - assert.strictEqual(object_type_tag.checkTypeTag(0, obj2), false); - assert.strictEqual(object_type_tag.checkTypeTag(1, obj1), false); - - // Verify that untagged objects are rejected. - assert.strictEqual(object_type_tag.checkTypeTag(0, {}), false); - assert.strictEqual(object_type_tag.checkTypeTag(1, {}), false); - - // Node v14 and v16 have an issue checking type tags if the `upper` in - // `napi_type_tag` is 0, so these tests can only be performed on Node version - // >=18. See: - // - https://github.com/nodejs/node/issues/43786 - // - https://github.com/nodejs/node/pull/43788 - const nodeVersion = parseInt(process.versions.node.split('.')[0]); - if (nodeVersion < 18) { - return; - } - - const obj3 = object_type_tag.typedTaggedInstance(2); - const obj4 = object_type_tag.typedTaggedInstance(3); - - // Verify that untagged objects are rejected. - assert.strictEqual(object_type_tag.checkTypeTag(0, {}), false); - assert.strictEqual(object_type_tag.checkTypeTag(1, {}), false); - - // Verify that type tags are correctly accepted. - assert.strictEqual(object_type_tag.checkTypeTag(0, obj1), true); - assert.strictEqual(object_type_tag.checkTypeTag(1, obj2), true); - assert.strictEqual(object_type_tag.checkTypeTag(2, obj3), true); - assert.strictEqual(object_type_tag.checkTypeTag(3, obj4), true); - - // Verify that wrongly tagged objects are rejected. - assert.strictEqual(object_type_tag.checkTypeTag(0, obj2), false); - assert.strictEqual(object_type_tag.checkTypeTag(1, obj1), false); - assert.strictEqual(object_type_tag.checkTypeTag(0, obj3), false); - assert.strictEqual(object_type_tag.checkTypeTag(1, obj4), false); - assert.strictEqual(object_type_tag.checkTypeTag(2, obj4), false); - assert.strictEqual(object_type_tag.checkTypeTag(3, obj3), false); - assert.strictEqual(object_type_tag.checkTypeTag(4, obj3), false); -} diff --git a/test/type_taggable.cc b/test/type_taggable.cc new file mode 100644 index 000000000..2320dd81c --- /dev/null +++ b/test/type_taggable.cc @@ -0,0 +1,66 @@ +#include "napi.h" + +#if (NAPI_VERSION > 7) + +using namespace Napi; + +static const napi_type_tag type_tags[5] = { + {0xdaf987b3cc62481a, 0xb745b0497f299531}, + {0xbb7936c374084d9b, 0xa9548d0762eeedb9}, + {0xa5ed9ce2e4c00c38, 0}, + {0, 0}, + {0xa5ed9ce2e4c00c38, 0xdaf987b3cc62481a}, +}; + +template +class TestTypeTaggable { + public: + static Value TypeTaggedInstance(const CallbackInfo& info) { + TypeTaggable instance = Factory(info.Env()); + uint32_t type_index = info[0].As().Int32Value(); + + instance.TypeTag(&type_tags[type_index]); + + return instance; + } + + static Value CheckTypeTag(const CallbackInfo& info) { + uint32_t type_index = info[0].As().Int32Value(); + TypeTaggable instance = info[1].As(); + + return Boolean::New(info.Env(), + instance.CheckTypeTag(&type_tags[type_index])); + } +}; + +TypeTaggable ObjectFactory(Env env) { + return Object::New(env); +} + +TypeTaggable ExternalFactory(Env env) { + // External does not accept a nullptr for its data. + return External::New(env, reinterpret_cast(0x1)); +} + +using TestObject = TestTypeTaggable; +using TestExternal = TestTypeTaggable>; + +Object InitTypeTaggable(Env env) { + Object exports = Object::New(env); + + Object external = Object::New(env); + exports["external"] = external; + external["checkTypeTag"] = Function::New(env, &TestExternal::CheckTypeTag); + external["typeTaggedInstance"] = + Function::New(env, &TestExternal::TypeTaggedInstance); + + Object object = Object::New(env); + exports["object"] = object; + object["checkTypeTag"] = Function::New(env, &TestObject::CheckTypeTag); + object["typeTaggedInstance"] = + Function::New(env, &TestObject::TypeTaggedInstance); + + return exports; +} + +#endif diff --git a/test/type_taggable.js b/test/type_taggable.js new file mode 100644 index 000000000..263befa9d --- /dev/null +++ b/test/type_taggable.js @@ -0,0 +1,60 @@ +'use strict'; + +const assert = require('assert'); + +module.exports = require('./common').runTest(test); + +function testTypeTaggable ({ typeTaggedInstance, checkTypeTag }) { + const obj1 = typeTaggedInstance(0); + const obj2 = typeTaggedInstance(1); + + // Verify that type tags are correctly accepted. + assert.strictEqual(checkTypeTag(0, obj1), true); + assert.strictEqual(checkTypeTag(1, obj2), true); + + // Verify that wrongly tagged objects are rejected. + assert.strictEqual(checkTypeTag(0, obj2), false); + assert.strictEqual(checkTypeTag(1, obj1), false); + + // Verify that untagged objects are rejected. + assert.strictEqual(checkTypeTag(0, {}), false); + assert.strictEqual(checkTypeTag(1, {}), false); + + // Node v14 and v16 have an issue checking type tags if the `upper` in + // `napi_type_tag` is 0, so these tests can only be performed on Node version + // >=18. See: + // - https://github.com/nodejs/node/issues/43786 + // - https://github.com/nodejs/node/pull/43788 + const nodeVersion = parseInt(process.versions.node.split('.')[0]); + if (nodeVersion < 18) { + return; + } + + const obj3 = typeTaggedInstance(2); + const obj4 = typeTaggedInstance(3); + + // Verify that untagged objects are rejected. + assert.strictEqual(checkTypeTag(0, {}), false); + assert.strictEqual(checkTypeTag(1, {}), false); + + // Verify that type tags are correctly accepted. + assert.strictEqual(checkTypeTag(0, obj1), true); + assert.strictEqual(checkTypeTag(1, obj2), true); + assert.strictEqual(checkTypeTag(2, obj3), true); + assert.strictEqual(checkTypeTag(3, obj4), true); + + // Verify that wrongly tagged objects are rejected. + assert.strictEqual(checkTypeTag(0, obj2), false); + assert.strictEqual(checkTypeTag(1, obj1), false); + assert.strictEqual(checkTypeTag(0, obj3), false); + assert.strictEqual(checkTypeTag(1, obj4), false); + assert.strictEqual(checkTypeTag(2, obj4), false); + assert.strictEqual(checkTypeTag(3, obj3), false); + assert.strictEqual(checkTypeTag(4, obj3), false); +} + +// eslint-disable-next-line camelcase +function test ({ type_taggable }) { + testTypeTaggable(type_taggable.external); + testTypeTaggable(type_taggable.object); +} diff --git a/tools/eslint-format.js b/tools/eslint-format.js index 1dda44495..6923ab7b6 100644 --- a/tools/eslint-format.js +++ b/tools/eslint-format.js @@ -23,7 +23,7 @@ function main (args) { // Check js files that change on unstaged file const fileUnStaged = spawn( 'git', - ['diff', '--name-only', FORMAT_START, filesToCheck], + ['diff', '--name-only', '--diff-filter=d', FORMAT_START, filesToCheck], { encoding: 'utf-8' } @@ -32,7 +32,7 @@ function main (args) { // Check js files that change on staged file const fileStaged = spawn( 'git', - ['diff', '--name-only', '--cached', FORMAT_START, filesToCheck], + ['diff', '--name-only', '--cached', '--diff-filter=d', FORMAT_START, filesToCheck], { encoding: 'utf-8' }