n-api: add napi_has_own_property()

[cjihrig]

Refs: #13925

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

Affected core subsystem(s)

n-api

[addaleax]

I would suggest being explicit about Symbol support here

[fhinkel]

Can we add a note about side effects? E.g., what if the object is a revoked proxy? If the key is not to name?

[fhinkel]

Can we add a test case where the key is not a name?

[fhinkel]

Rather than returning false, shouldn't we call the ToPrimitive method on the key? I'm thinking of an example like this:

var x ={hello: 17} var o ={[Symbol.toPrimitive] : function(){return 'hello'}} x.hasOwnProperty(o) // true 

[cjihrig]

@fhinkel what ToPrimitive() method? I'm not seeing anything exposed by the V8 API except for retrieving the symbol itself, but maybe I'm just missing it.

Also cc @nodejs/n-api as this case isn't handled anywhere else in n-api.

[fhinkel]

I don't think we expose it by itself. According to the spec, the key is converted to a name, which calls toPrimitive, toPropertyKey.

So, I think we should delete the return false if not IsName(), and let V8 handle it in a spec compliant way. V8 only needs a value, not a string/symbol here.

[cjihrig]

I'm all for letting V8 handle this, but I get a compiler error passing in a Value :-/

../src/node_api.cc:1042:60: note: in instantiation of function template specialization 'v8::Local<v8::Name>::Local<v8::Value>' requested here v8::Maybe<bool> has_maybe = obj->HasOwnProperty(context, k); 

It looks like V8 expects a Name.

[fhinkel]

You're right, I was looking at a comment for has(). Sorry for the confusion. I think we should make sure though that it's obvious that the n-api function is not identical to JS behavior, in particular, that we'll never use the ToPrimitive symbol if the key is an object. (We should probably point that out in the V8 API, but there it might be less of an issue because we only allow names.)

[cjihrig]

@fhinkel I added a note to the docs that no type conversions will be performed. That should cover ToPrimitive and any other implicit conversions.

[cjihrig]

Nits addressed, minus the ToPrimitive() piece.

[fhinkel]

You're right, I was looking at a comment for has(). Sorry for the confusion. I think we should make sure though that it's obvious that the n-api function is not identical to JS behavior, in particular, that we'll never use the ToPrimitive symbol if the key is an object. (We should probably point that out in the V8 API, but there it might be less of an issue because we only allow names.)

[cjihrig]

CI: https://ci.nodejs.org/job/node-test-pull-request/9017/