ES6: Desugaring of instanceof to support @@hasInstance

ES6: Desugaring of instanceof to support @@hasInstance

This is a rework of the instanceof operator to support ES6 semantics
(as per section 12.10.4 of the spec:
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N

Committed: https://crrev.com/deb7d5b090a9964cd37d20760a21afd6835438b6
Cr-Commit-Position: refs/heads/master@{#34170}

[mvstanton, 2016-02-12 13:29:38]

Description was changed from

==========
Desugaring of instanceof.

BUG=
==========

to

==========
ES6: Desugaring of instanceof.

It's behind flag --harmony-instanceof for now.

BUG=v8:4447
==========

[mvstanton, 2016-02-12 13:29:39]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org, rossberg@chromium.org

[mvstanton, 2016-02-12 13:30:42]

Hi guys,
Take a look.
Andreas, I'm not sure about the way to enable test262 tests with the flag on. I
could also move the flag to the staging category...I believe the implementation
is complete. Do we run test262 with --es6-staging on some bots?

Thanks,
--Mike

[rossberg, 2016-02-12 13:55:25]

> Andreas, I'm not sure about the way to enable test262 tests with the flag on.
I
> could also move the flag to the staging category...I believe the
implementation
> is complete. Do we run test262 with --es6-staging on some bots?

Yes, test262 runs with the staging flag. So it should pick it up as soon as you
stage this.

In any case, it wouldn't harm to also have a few V8-side tests for this. ;)

[rossberg, 2016-02-12 14:11:32]

https://codereview.chromium.org/1692713005/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1692713005/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:2495: void Genesis::InitializeGlobal_harmony_instanceof() {
Use the EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE macro, see lines 2324+

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2249: if (FLAG_harmony_instanceof && op ==
Token::INSTANCEOF) {
Nit: make this and 'else if'. Also, s/op/cmp/

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6243: // ====================
Nit: underline all of it?

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6257: //       if (!%IsCallable(handler)) {
Is this actually necessary? %_Call would throw the type error anyway. Or is this
just for better error message?

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6262: //   }
result;   as the last statement?

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6293: Statement* skip = factory->NewEmptyStatement(nopos);
It looks like you are using this AST node twice. That isn't really supported,
and might potentially cause problems.

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6318: Variable* var_result =
scope->NewTemporary(avfactory->empty_string());
Nit: you can probably reuse the same temporary for handler and result.

[mvstanton, 2016-02-18 02:12:17]

Hi guys,
Thanks for the comments.
Let me send you this, and in the morning I'll add a test file under
mjsunit/harmony that hits the ES6 highlights I'm currently relying on test262
for.
--Mike

https://codereview.chromium.org/1692713005/diff/40001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/1692713005/diff/40001/src/bootstrapper.cc#new...
src/bootstrapper.cc:2495: void Genesis::InitializeGlobal_harmony_instanceof() {
On 2016/02/12 14:11:31, rossberg wrote:
> Use the EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE macro, see lines 2324+

Done.

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser-base...
src/parsing/parser-base.h:2249: if (FLAG_harmony_instanceof && op ==
Token::INSTANCEOF) {
On 2016/02/12 14:11:31, rossberg wrote:
> Nit: make this and 'else if'. Also, s/op/cmp/

Done.

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6243: // ====================
On 2016/02/12 14:11:31, rossberg wrote:
> Nit: underline all of it?

Done.

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6255: //       result = %ordinary_has_instance(C, O);
I had to add an IS_CALLABLE check here, because %ordinary_has_instance tolerates
a non-callable by returning false...but when called in this context it needs to
be guarded with a TypeError. Go figure :p.

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6257: //       if (!%IsCallable(handler)) {
On 2016/02/12 14:11:32, rossberg wrote:
> Is this actually necessary? %_Call would throw the type error anyway. Or is
this
> just for better error message?

Indeed, I was able to remove it, thanks!

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6262: //   }
On 2016/02/12 14:11:31, rossberg wrote:
> result;   as the last statement?

Done.

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6293: Statement* skip = factory->NewEmptyStatement(nopos);
On 2016/02/12 14:11:31, rossberg wrote:
> It looks like you are using this AST node twice. That isn't really supported,
> and might potentially cause problems.

Done.

https://codereview.chromium.org/1692713005/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:6318: Variable* var_result =
scope->NewTemporary(avfactory->empty_string());
On 2016/02/12 14:11:32, rossberg wrote:
> Nit: you can probably reuse the same temporary for handler and result.

Done.

[mvstanton, 2016-02-18 02:13:38]

Description was changed from

==========
ES6: Desugaring of instanceof.

It's behind flag --harmony-instanceof for now.

BUG=v8:4447
==========

to

==========
ES6: Desugaring of instanceof.

It's behind flag --harmony-instanceof for now.

BUG=v8:4447
LOG=N
==========

[mvstanton, 2016-02-18 02:14:30]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 02:14:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1692713005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1692713005/60001

[commit-bot: I haz the power, 2016-02-18 02:33:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 02:33:43]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/13930)

[mvstanton, 2016-02-18 02:48:29]

Looks like there is an ignition failure, I'll see if I can fix it, otherwise let
Ross and folks have a look...

[Benedikt Meurer, 2016-02-18 05:09:45]

LGTM with nit.

https://codereview.chromium.org/1692713005/diff/60001/src/runtime/runtime-fun...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/1692713005/diff/60001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:243: // PrintF("object %p is callable %d\n",
(void*) object, object->IsCallable());
Nit: Remove this PrintF

[mvstanton, 2016-02-18 19:49:05]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 19:49:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1692713005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1692713005/100001

[commit-bot: I haz the power, 2016-02-18 19:50:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 19:50:38]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/1597)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[mvstanton, 2016-02-18 20:19:40]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 20:19:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1692713005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1692713005/120001

[commit-bot: I haz the power, 2016-02-18 20:41:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 20:41:35]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/1595)
  v8_linux_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[mvstanton, 2016-02-18 20:51:54]

The CQ bit was checked by mvstanton@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 20:52:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1692713005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1692713005/140001

[mvstanton, 2016-02-18 21:19:15]

Hi Andreas,
After much ignition and rebase hilarity, here is the CL. Thanks for the look!
--Mike

https://codereview.chromium.org/1692713005/diff/60001/src/runtime/runtime-fun...
File src/runtime/runtime-function.cc (right):

https://codereview.chromium.org/1692713005/diff/60001/src/runtime/runtime-fun...
src/runtime/runtime-function.cc:243: // PrintF("object %p is callable %d\n",
(void*) object, object->IsCallable());
On 2016/02/18 05:09:44, Benedikt Meurer wrote:
> Nit: Remove this PrintF

Doh...actually removed the whole function as Dan pointed out a nice way to check
Callable with typeof.

[commit-bot: I haz the power, 2016-02-18 21:28:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 21:28:17]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2016-02-19 07:02:19]

Description was changed from

==========
ES6: Desugaring of instanceof.

It's behind flag --harmony-instanceof for now.

BUG=v8:4447
LOG=N
==========

to

==========
ES6: Desugaring of instanceof.

This is a rework of the instanceof operator to support ES6 semantics (as per
section 12.10.4 of the spec:
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N
==========

[mvstanton, 2016-02-19 07:03:40]

Description was changed from

==========
ES6: Desugaring of instanceof.

This is a rework of the instanceof operator to support ES6 semantics (as per
section 12.10.4 of the spec:
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N
==========

to

==========
ES6: Desugaring of instanceof to support @@hasInstance

This is a rework of the instanceof operator to support ES6 semantics
(as per section 12.10.4 of the spec: 
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N
==========

[rossberg, 2016-02-19 12:19:00]

LGTM

https://codereview.chromium.org/1692713005/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1692713005/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:6450: Statement* ParserTraits::CheckCallable(Variable*
var, Expression* error) {
Given that my CL has landed, can you now change FinalizeForOfStatement to invoke
this function?

[mvstanton, 2016-02-19 18:15:59]

Thanks guys, submitting...

https://codereview.chromium.org/1692713005/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1692713005/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:6450: Statement* ParserTraits::CheckCallable(Variable*
var, Expression* error) {
On 2016/02/19 12:19:00, rossberg wrote:
> Given that my CL has landed, can you now change FinalizeForOfStatement to
invoke
> this function?

Done.

[mvstanton, 2016-02-19 18:16:09]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-02-19 18:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1692713005/#ps160001
(title: "REBASE plus comment response.")

[commit-bot: I haz the power, 2016-02-19 18:16:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1692713005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1692713005/160001

[commit-bot: I haz the power, 2016-02-19 19:19:59]

Description was changed from

==========
ES6: Desugaring of instanceof to support @@hasInstance

This is a rework of the instanceof operator to support ES6 semantics
(as per section 12.10.4 of the spec: 
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N
==========

to

==========
ES6: Desugaring of instanceof to support @@hasInstance

This is a rework of the instanceof operator to support ES6 semantics
(as per section 12.10.4 of the spec: 
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N
==========

[commit-bot: I haz the power, 2016-02-19 19:20:00]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-19 19:20:48]

Description was changed from

==========
ES6: Desugaring of instanceof to support @@hasInstance

This is a rework of the instanceof operator to support ES6 semantics
(as per section 12.10.4 of the spec: 
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N
==========

to

==========
ES6: Desugaring of instanceof to support @@hasInstance

This is a rework of the instanceof operator to support ES6 semantics
(as per section 12.10.4 of the spec:
https://tc39.github.io/ecma262/#sec-instanceofoperator).

It's behind flag --harmony-instanceof for now, which is turned on for staging.

BUG=v8:4447
LOG=N

Committed: https://crrev.com/deb7d5b090a9964cd37d20760a21afd6835438b6
Cr-Commit-Position: refs/heads/master@{#34170}
==========

[commit-bot: I haz the power, 2016-02-19 19:20:49]

Patchset 9 (id:??) landed as
https://crrev.com/deb7d5b090a9964cd37d20760a21afd6835438b6
Cr-Commit-Position: refs/heads/master@{#34170}