Use Symbol.hasInstance() for napi_instanceof on V8, when available

[gabrielschulhof]

This can be accomplished by retaining Symbol.hasInstance() in a static Persistent at the top of node_api.cc and then calling it. If the persistent is empty, we do what we do now. The persistent is initialized from napi_module_register_cb().

[gabrielschulhof]

... though perhaps we shouldn't store it but rather retrieve it all the time, because it could be for a different context :/ This reduces the performance of napi_instanceof() on V8, but it may still be an improvement compared to the walk up the prototype chain that it does now.

[jasongin]

Do we actually want that behavior? Does a native addon want its instanceof checks to be overridable? Does v8::FunctionTemplate::HasInstance() use Symbol.hasInstance()? Or should we rename napi_instanceof() to something that doesn't imply exactly the same behavior as the JS instanceof operator?

[gabrielschulhof]

What do you mean by "its instanceof checks"? An addon may check whether anything is an instance of anything else. It need not necessarily restrict itself to checks of instances against constructors it provides itself. This is to be a generic API, right?

As such, we should absolutely replicate precisely what the instanceof operator does, because the operator is fundamentally inaccessible from the native side, yet there is use for it. After all, ChakraCore provides access to it, and it likely takes into account Symbol.hasInstance ...

So, if the operator is now customizable via Symbol.hasInstance then we must capture that. Providing something that resembles instanceof but isn't complete, under any name whatsoever would be of dubious use.

Additionally, in V8 using Symbol.hasInstance is more efficient than the walk up the prototype chain because it makes fewer calls into V8 - of course depending on the depth of the prototype chain.

TBH, I was unaware of Symbol.hasInstance as a way of exposing instanceof functionality, otherwise I would've used it in the original implementation of napi_instanceof.

Of course, I say all this without having tested whether the function that is retrieved from the constructor's Symbol.hasInstance key actually provides the functionality we want. I'm working on that now. My motivation stems merely from the attitude that, if the engine provides a facility, we should not re-implement that facility.

[jasongin]

Does v8::FunctionTemplate::HasInstance() use Symbol.hasInstance()? Or is it unaffected by that customization?

[gabrielschulhof]

I'm checking that too. Even with plain JS though, only classses can be customized by way of Symbol.hasInstance, not plain functions used as constructors.

[gabrielschulhof]

Oh, actually, v8::FunctionTemplate::HasInstance() is not affected by Symbol.hasInstance AFAICT. Probably because it counts as a plain-old-function, not a class.

[jasongin]

only classses can be customized by way of Symbol.hasInstance, not plain functions used as constructors

I don't understand -- a class in JS is just syntactic sugar for a constructor function with prototype properties.

[gabrielschulhof]

Here's the JS-only code I'm using for testing:

class MyClass {

static [ Symbol.hasInstance ]( candidate ) {
	return ( candidate.mark === "is an instance" );
}

}

var x = new MyClass();
var y = new MyClass();

console.log( x instanceof MyClass );
console.log( y instanceof MyClass );

y.mark = "is an instance";

console.log( x instanceof MyClass );
console.log( y instanceof MyClass );

console.log( typeof MyClass );

function MyFunction() {}
MyFunction[ Symbol.hasInstance ] = function( candidate ) {
	return ( candidate.mark === "is an instance" );
}

var a = new MyFunction();
var b = new MyFunction();

console.log( a instanceof MyFunction );
console.log( b instanceof MyFunction );

[gabrielschulhof]

For the plain-old-function I tried setting all of these as well:

MyFunction.prototype[ Symbol.hasInstance ] = ...
MyFunction.constructor[ Symbol.hasInstance ] = ...
MyFunction.__proto__[Symbol.hasInstance ] = ...

and a and b are still considered instances, despite the supposed check. So, that's why I concluded that it only works for classes. Looks like they're more than just sugar.

[jasongin]

It's static, so the equivalent would be:

MyFunction[Symbol.hasInstance] = ...

[gabrielschulhof]

OK, so that was my original code. So, does the fact that it does not affect instanceof against plain-old-functions mean that there's a bug in V8 in that it does not conform to spec?

[gabrielschulhof]

Note that for our implementation of napi_instanceof() I think we still need to go with hasInstance, because we cannot distinguish whether the constructor was created with class {} and should therefore be influenced by custom hasInstance()-ers or whether it was created with function() {} - as well we shouldn't, since it is supposed to be just sugar ...

[jasongin]

Take a look at this: http://node.green/#ES2015-built-ins-well-known-symbols-Symbol-hasInstance

Here's the code they use for the test:

function(){ 
var passed = false;
var obj = { foo: true };
var C = function(){};
Object.defineProperty(C, Symbol.hasInstance, {
  value: function(inst) { passed = inst.foo; return false; }
});
obj instanceof C;
return passed;
} 

V8 passes the test on Node >= 6.10.

[gabrielschulhof]

Oh, defineProperty, eh?

[gabrielschulhof]

Wow! OK, so if I use defineProperty() then it affects instanceof.

[gabrielschulhof]

v8::FunctionTemplate::HasInstance does not take into account hasInstance:

var hello = require( "bindings" )( "hello" );

var MyClass = hello.MyClass;
var hasInstance = hello.hasInstance;

var x = new MyClass();
var y = new MyClass();

console.log( "x: plain: native: " + ( x instanceof MyClass ) );
console.log( "x: plain: hasInstance: " + hasInstance( x ) );
console.log( "y: plain: native: " + ( y instanceof MyClass ) );
console.log( "y: plain: hasInstance: " + hasInstance( y ) );

console.log( "Setting constraint" );

Object.defineProperty( MyClass, Symbol.hasInstance, {
	value: function( candidate ) {
		return ( "mark" in candidate );
	}
} );

console.log( "x: plain: native: " + ( x instanceof MyClass ) );
console.log( "x: plain: hasInstance: " + hasInstance( x ) );
console.log( "y: plain: native: " + ( y instanceof MyClass ) );
console.log( "y: plain: hasInstance: " + hasInstance( y ) );

console.log( "Satisfying constraint for x" );

x.mark = "abc";

console.log( "x: plain: native: " + ( x instanceof MyClass ) );
console.log( "x: plain: hasInstance: " + hasInstance( x ) );
console.log( "y: plain: native: " + ( y instanceof MyClass ) );
console.log( "y: plain: hasInstance: " + hasInstance( y ) );

and hello.cc:

#include <nan.h>

using namespace v8;

static Nan::Persistent<FunctionTemplate> theTemplate;

NAN_METHOD(hasInstance) {
	info.GetReturnValue().Set(Nan::New((theTemplate))->HasInstance(info[0]));
}

NAN_MODULE_INIT(Init) {
	Local<FunctionTemplate> localTemplate = Nan::New<FunctionTemplate>();
	localTemplate->SetClassName(Nan::New("MyClass").ToLocalChecked());
	theTemplate.Reset(localTemplate);
	Nan::Set(target, Nan::New("MyClass").ToLocalChecked(),
	  Nan::GetFunction(localTemplate).ToLocalChecked());
	Nan::Set(target, Nan::New("hasInstance").ToLocalChecked(),
		Nan::GetFunction(Nan::New<FunctionTemplate>(hasInstance))
			.ToLocalChecked());
}

NODE_MODULE(hello, Init)

results in

x: plain: native: true
x: plain: hasInstance: true
y: plain: native: true
y: plain: hasInstance: true
Setting constraint
x: plain: native: false
x: plain: hasInstance: true
y: plain: native: false
y: plain: hasInstance: true
Satisfying constraint for x
x: plain: native: true
x: plain: hasInstance: true
y: plain: native: false
y: plain: hasInstance: true

[gabrielschulhof]

So, since FunctionTemplate is a V8-specific construct, seeming based on the above to express only relationships between objects as stored in the VM, I still think we should encapsulate the logic of the native instanceof operator rather than of the V8-specific HasInstance() and call it napi_instanceof().

[jasongin]

Agreed. We could still provide a separate napi_ordinary_has_instance() that matches the current implementation as suggested in this comment. But I'm not sure any native module would really depend on the distinction.

[gabrielschulhof]

Yeah, let's take the lazy (i.e. let someone file a bug requesting the feature) approach with napi_ordinary_has_instance() :)