n-api: support type-tagging objects

[gabrielschulhof]

napi_instanceof() is insufficient for reliably establishing the data
type to which a pointer stored with napi_wrap() or
napi_create_external() inside a JavaScript object points. Thus, we
need a way to "mark" an object with a value that, when later retrieved,
can unambiguously tell us whether it is safe to cast the pointer stored
inside it to a certain structure.

Re: #28164

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

Sadly, an error occurred when I tried to trigger a build. :(

[gabrielschulhof]

@argv-minus-one PTAL

[argv-minus-one]

Looks good to me. Thanks! I look forward to using this in my code.

[argv-minus-one]

One issue I noticed: Type tags are constants, not variables. Please change their type, in function declarations and documentation, to const void*.

[gabrielschulhof]

@argv-minus-one this means I need to const_cast<void*>() the type tag, because a v8::External accepts a plain void*, not a const void*. But OK, it makes sense to cast, because const void* in C land conveys the fact that whoever receives the pointer is not going to do anything with it.

[mhdawson]

@gabrielschulhof @bnoordhuis are we sure that other JS engines will be able to implement these functions (for example JerryScript)? I thought that other JS engines might not have the ability to set privates. In cases where we had used them before I think I remember that there was some other way to achieve the same thing.

But quite possible I'm not remembering correctly.

[devsnek]

We already require this functionality to associate data with objects (externals) and classes (napi_wrap). Any engine that implemented those or has WeakMap should have the infrastructure needed to associate this data.

[gabrielschulhof]

@mhdawson jerryscript supports attaching multiple type-tagged pointers to a JS object.

[mhdawson]

@devsnek, @gabrielschulhof thanks for the confirmation.

[TimothyGu]

I’m on vacation this week but would like to take a deeper look. Could you please not land this until next week?

[legendecas]

If there would be use cases that the tagged native pointer have to be removed?

const it = new Native()
it.start() // from now on the tag check should succeed
it.stop() // from now on the tag check should fail

[legendecas]

@mhdawson jerryscript supports attaching multiple type-tagged pointers to a JS object.

Does type tag in this PR support multiple tagging on single object? It seems this PR doesn't support multiple tagging.

[gabrielschulhof]

@legendecas we do not support multi-wrap, so multi-tag doesn't really make that much sense AFAICT.

Of course, we can talk about whether we wish to support multiple tagging/wrapping. I can imagine a

NAPI_EXTERN napi_status napi_multiwrap(napi_env env,
                                  napi_value js_object,
                                  void* native_object,
                                  void* type_tag,
                                  napi_finalize finalize_cb,
                                  void* finalize_hint,
                                  napi_ref* result);

[mhdawson]

@legendecas what is requirement to multi-wrap. On the N-API front we try to limit new features to the minimum that is essential. This is important to minimize the likely hood of having problems with APIs that we provide being hard to implement in future JS engines/versions of existing JS engines.

[legendecas]

@mhdawson what is requirement to multi-wrap. On the N-API front we try to limit new features to the minimum that is essential. This is important to minimize the likely hood of having problems with APIs that we provide being hard to implement in future JS engines/versions of existing JS engines.

I did not mean that multi-wrap is required in N-API. Just like to know how the API behaves and how it was expected to behave on these possible use cases on the new API.

[mhdawson]

@legendecas thanks for the clarification.

We did discuss this in the N-API team meeting today, @gabrielschulhof is going to look at a few alternatives as well so this should not land under he's had time to do that. We also want to give @TimothyGu time to review/comment as well. Marking as blocked for now.

[fhinkel]

Should this remain open?