`nativeFunc` should be called with `new` when appropriate

[bioball]

When calling into native functions, they should be called with the new keyword if parsed code is also called with new.

Here's the source of this issue: https://github.com/NeilFraser/JS-Interpreter/blob/master/interpreter.js#L3213

I think it should read something like:

if (this.calledWithNew()) {
  state.value = new (Function.prototype.bind.apply(func.nativeFunc, state.arguments_));
} else {
  state.value = func.nativeFunc.apply(state.funcThis_, state.arguments_);
}

[NeilFraser]

Can you elaborate on the effect this would have? If I add this change, I need to be able to test it, and I'm currently unable to come up with code that doesn't function under the current system.

Here's what I've got so far:

Init code

      var wrapper = function() {
        alert('Foo!');
      };
      interpreter.setProperty(scope, 'Foo',
          interpreter.createNativeFunction(wrapper));

Interpreted code

Foo.prototype = {
  bar: 2
};
var f = new Foo();
alert(f.bar);

As expected, the 'Foo!' constructor is called, and the resulting object has a .bar property of 2.

[cpcallen]

I think the point is to wrap a native function that behaves differently when called with and without new (e.g. the native Date constructor).

Of course it's not obvious what should happen in this case:

• You can't (usefully) pass the newly-created pseudoObject to any of the builtin constructors (like Date) when calling them with new, since any existing this binding is ignored by [[Construct]].

• Doing the new (….bind.apply(…)) call suggested by @bioball, requires trusting that the native function being wrapped will not return a native object.

• In the case that the underlying native function is a JS-Interpreter-specific wrapper, then there is no need for the @bioball's suggested change, because that wrapper can itself call .calledWithNew.

• In the case where the underlying native function is some arbitrary function, not-JS-Interpreter specific wrapper, then the suggested code is only helpful if:

  • The native function behaves differently depending on whether it is invoked with new (n.b.: from ES6 onwards this can now be reliably detected via new.target!), AND
  • The native function never returns an object value.

Another option would be to insert an automatic nativeToPseudo on the return value:

if (this.calledWithNew()) {
  state.value = this.pseudoToNative(new (Function.prototype.bind.apply(func.nativeFunc, state.arguments_)));
} else {
  state.value = func.nativeFunc.apply(state.funcThis_, state.arguments_);
}

… but this is unlikely to make people happy, since generally the point of new is to create a new instance of a class, and psuedoToNative doesn't really handle setting the prototype of the pseudoObject correctly except in a few limited cases (i.e., the builtins).

[bioball]

The reason I'm proposing this is because I'd like to write a function wrapper that wraps generically, for both constructors and non-constructors. In order to get it to work for constructors, I need to know if the user is calling with new or not.

My wrapper looks something like this:

const wrapFunction = (value, interpreter) => {
  const wrapped = function (...args) {
    const unwrappedThis = unwrapInterpreterValue(this, interpreter);
    const unwrappedArgs = args.map((i) => unwrapInterpreterValue(i, interpreter));
    // If this method was called using `new`, call the underlying function with `new` as well.
    let ret;
    if (target.new) {
      ret = new value(...unwrappedArgs);
    } else {
      ret = value.apply(unwrappedThis, unwrappedArgs);
    }
    return wrapNativeValue(ret, interpreter);
  };
  const wrappedFunction = interpreter.createNativeFunction(wrapped);
  // decorate the function with any properties attached to it
  return wrapObject(value, interpreter, wrappedFunction);
};

the new (….bind.apply(…)) call suggested by @bioball, requires trusting that the native function being wrapped will not return a native object

I think it's fair to trust this. Calling nativeFunc already expects the return value to not be a native object

In the case that the underlying native function is a JS-Interpreter-specific wrapper, then there is no need for the @bioball's suggested change, because that wrapper can itself call .calledWithNew.

Huh... that's a good point. For some reason, this didn't occur to me. I've changed my wrapper from using target.new to interpreter.calledWithNew(), and it works! This solves my use-case quite nicely.

I'll let you guys decide whether this issue is still worth implementing. If not, I'm happy to close it.

[cpcallen]

Calling nativeFunc already expects the return value to not be a native object.

Absolutely true.

I've changed my wrapper from using target.new to interpreter.calledWithNew(), and it works!

Excellent.

[bioball]

Closing this one out, as it solves my use-case