Optimize a fast path for instanceof binary expressions

[trueadm]

Release notes: none

Looking through our internal bundles and there are frequent cases where instanceof is used where the left-hand side is a primitive and the right-hand side is an abstract value. In the case where the left-hand side is a primitive and the right-hand side is a simple object, the instanceof binary expression should always return false.

[sebmarkbage]

So this is approaching a problem space we don't really talk about much but in ES2015 this assumption is actually not true:

let Stringish = {[Symbol.hasInstance](v) { return typeof v === 'string'; }};
'foo' instanceof Stringish; // true

[trueadm]

@sebmarkbage This sucks a bit.

function fn(x) {
  var y = undefined;
  if (y instanceof x) {
    return y.somethingThatDoesNotExist;
  }
}

Prepack will spit out this:

  var _2 = function (x) {
    var _$0 = void 0 instanceof x;

    var _4 = (__constructor.prototype = _$3, new __constructor());

    $$0.value = "null or undefined", _$5(_4, "message", $$0);
    $$0.value = "TypeError: null or undefined\n    at fn (/Users/ADM/Projects/prepack/fb-www/input.js:6:12)", _$5(_4, "stack", $$0);
    if (_$0) throw _4;else {}
    return void 0;
  };

Which is somewhat problematic in the cases of code bloat. Interestingly, Closure Compiler also optimizes the same thing above away even though it supports ES2015.

[NTillmann]

Ignoring the correctness issue (if we make unsound assumptions or optimizations, we need to document that somewhere!), how does this change relate to the simplifier? Shouldn't this be a simplification rule?

[trueadm]

@NTillmann Nope, as this creates a temporal with createOperationDescriptor("BINARY_EXPRESSION", { binaryOperator: op })

[sebmarkbage]

At the very least we can do this optimization for simple objects as the right hand.

For objects that are not topVal as the right hand, we should just continue executing since it'll resolve the real value.

If it's not an object on the right hand we can continue too since that'll throw.

If it's a topVal object on the right hand, we can issue a recoverable error. Since we can assume continue under the assumption it's false.

[trueadm]

@sebmarkbage I've refined the code to add the simple check.

[sebmarkbage]

Let's document this somewhere.

[sebmarkbage]

The rval instanceof ObjectValue is unnecessary since one of them needs to be an abstract value.

You could expand the lval to include abstract values with Value.isTypeCompatibleWith(lval.getType(), PrimitiveValue). That way you handle the abstract primitive case too.

https://github.com/facebook/prepack/blob/master/src/methods/to.js#L682

[hermanventer]

Seems safe to me.

[facebook-github-bot]

trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.