Ship ES2015 sloppy-mode function hoisting, let, class

Ship ES2015 sloppy-mode function hoisting, let, class

This patch doesn't ship all features of ES2015 variable/scoping
changes, notably omitting the removal of legacy const. I think
function hoisting, let and class in sloppy mode can stand to
themselves as a package, and the legacy const change is much
riskier and more likely to be reverted, so my intention is to
pursue those as a separate, follow-on patch.

R=adamk@chromium.org
BUG=v8:4285, v8:3305
LOG=Y
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

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

[Dan Ehrenberg, 2015-12-25 22:32:29]

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

[commit-bot: I haz the power, 2015-12-25 22:32:41]

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

[commit-bot: I haz the power, 2015-12-25 22:45:43]

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

[commit-bot: I haz the power, 2015-12-25 22:45:44]

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

[Dan Ehrenberg, 2015-12-28 00:54:00]

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

[commit-bot: I haz the power, 2015-12-28 00:54:07]

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

[commit-bot: I haz the power, 2015-12-28 01:11:53]

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

[commit-bot: I haz the power, 2015-12-28 01:11:54]

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

[Dan Ehrenberg, 2015-12-28 02:38:44]

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

[commit-bot: I haz the power, 2015-12-28 02:38:53]

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

[commit-bot: I haz the power, 2015-12-28 03:19:28]

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

[commit-bot: I haz the power, 2015-12-28 03:19:29]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2015-12-30 19:59:00]

Description was changed from

==========
Ship ES2015 sloppy-mode function hoisting, let, class

This patch doesn't ship all features of ES2015 variable/scoping
changes, notably omitting the removal of legacy const. I think
function hoisting, let and class in sloppy mode can stand to
themselves as a package, and the legacy const change is much
riskier and more likely to be reverted, so my intention is to
pursue those as a separate, follow-on patch.

R=adamk@chromium.org
BUG=v8:4285,v8:3305
LOG=Y
==========

to

==========
Ship ES2015 sloppy-mode function hoisting, let, class

This patch doesn't ship all features of ES2015 variable/scoping
changes, notably omitting the removal of legacy const. I think
function hoisting, let and class in sloppy mode can stand to
themselves as a package, and the legacy const change is much
riskier and more likely to be reverted, so my intention is to
pursue those as a separate, follow-on patch.

R=adamk@chromium.org
BUG=v8:4285,v8:3305
LOG=Y
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

[Dan Ehrenberg, 2015-12-30 19:59:25]

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

[commit-bot: I haz the power, 2015-12-30 19:59:33]

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

[commit-bot: I haz the power, 2015-12-30 21:40:11]

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

[commit-bot: I haz the power, 2015-12-30 21:40:12]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

[Dan Ehrenberg, 2015-12-30 22:43:16]

Adam, did you mention something about how we are supposed to omit defining
'arguments' when it appears in the formal parameter list, but we fail to do this
when it's part of a destructuring? That seems to be what's behind the test
failure in LayoutTests here. A minimal test case:

dehrenberg@dehrenberg:~/v8/v8$ rlwrap out/x64.release/d8
--harmony-sloppy-function
V8 version 4.9.0 (candidate)
d8> function f([arguments]) {}
(d8):1: SyntaxError: Duplicate parameter name not allowed in this context
function f([arguments]) {}
                         ^
SyntaxError: Duplicate parameter name not allowed in this context

Before this patch:

dehrenberg@dehrenberg:~/v8/v8$ rlwrap out/x64.release/d8
V8 version 4.9.0 (candidate)
d8> function f([arguments]) {}
undefined

The error is coming from Parser::Declare in its check against redeclaration of
lexically declared arguments. Is there a bug open for this issue?

[adamk, 2016-01-04 18:59:36]

On 2015/12/30 22:43:16, Dan Ehrenberg wrote:
> Adam, did you mention something about how we are supposed to omit defining
> 'arguments' when it appears in the formal parameter list, but we fail to do
this
> when it's part of a destructuring? That seems to be what's behind the test
> failure in LayoutTests here. A minimal test case:
> 
> dehrenberg@dehrenberg:~/v8/v8$ rlwrap out/x64.release/d8
> --harmony-sloppy-function
> V8 version 4.9.0 (candidate)
> d8> function f([arguments]) {}
> (d8):1: SyntaxError: Duplicate parameter name not allowed in this context
> function f([arguments]) {}
>                          ^
> SyntaxError: Duplicate parameter name not allowed in this context
> 
> Before this patch:
> 
> dehrenberg@dehrenberg:~/v8/v8$ rlwrap out/x64.release/d8
> V8 version 4.9.0 (candidate)
> d8> function f([arguments]) {}
> undefined
> 
> The error is coming from Parser::Declare in its check against redeclaration of
> lexically declared arguments. Is there a bug open for this issue?

Yeah, this is the bug: https://bugs.chromium.org/p/v8/issues/detail?id=4577

[Dan Ehrenberg, 2016-01-05 01:19:13]

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

[commit-bot: I haz the power, 2016-01-05 01:19:22]

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

[commit-bot: I haz the power, 2016-01-05 01:20:45]

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

[commit-bot: I haz the power, 2016-01-05 01:20:46]

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

[Dan Ehrenberg, 2016-01-05 05:27:25]

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

[commit-bot: I haz the power, 2016-01-05 05:27:36]

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

[commit-bot: I haz the power, 2016-01-05 05:30:19]

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

[commit-bot: I haz the power, 2016-01-05 05:30:19]

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

[Dan Ehrenberg, 2016-01-05 05:47:55]

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

[commit-bot: I haz the power, 2016-01-05 05:48:04]

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

[commit-bot: I haz the power, 2016-01-05 06:47:40]

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

[commit-bot: I haz the power, 2016-01-05 06:47:41]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-01-05 18:18:00]

Now that test expectations are updated, this patch looks green. PTAL.

[adamk, 2016-01-05 19:19:06]

https://codereview.chromium.org/1551443002/diff/80001/test/message/no-legacy-...
File test/message/no-legacy-const.js (left):

https://codereview.chromium.org/1551443002/diff/80001/test/message/no-legacy-...
test/message/no-legacy-const.js:5: // Flags: --no-legacy-const
Can't you change the flags here instead of deleting the code? As long as the
flags live it seems reasonable to have some amount of coverage, imho. This could
be "--no-legacy-const --no-harmony-sloppy", right?

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/es6/block-...
File test/mjsunit/es6/block-early-errors.js (left):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/es6/block-...
test/mjsunit/es6/block-early-errors.js:50: Check("let x;");
Seems like this test could stay with added flags too. The test could then be
deleted when the flag is.

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/es6/block-...
File test/mjsunit/es6/block-non-strict-errors.js (left):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/es6/block-...
test/mjsunit/es6/block-non-strict-errors.js:23: CheckError("let x = 1;");
Same here.

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/mjsunit.st...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:1096: 'regress/regress-crbug-504729': [SKIP],
Guessing these got skipped because ignition doesn't support hoistable sloppy
block functions?

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-91120.js (right):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-91120.js:28: // With ES2015 function hoisting
semantics, functions are only "hoisted" out
One more test that could be retained with proper flagging. I could imagine
duplicating this test, but it seems weird to just change it, since it's very
precisely testing behavior.

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-crbug-323936.js (right):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-crbug-323936.js:43: assertEquals("argument", e); 
// debugger did not overwrite
Do you understand if this test still testing the thing it was built for? It's
hard for me to tell..

[Dan Ehrenberg, 2016-01-06 00:27:57]

https://codereview.chromium.org/1551443002/diff/80001/test/message/no-legacy-...
File test/message/no-legacy-const.js (left):

https://codereview.chromium.org/1551443002/diff/80001/test/message/no-legacy-...
test/message/no-legacy-const.js:5: // Flags: --no-legacy-const
On 2016/01/05 at 19:19:05, adamk wrote:
> Can't you change the flags here instead of deleting the code? As long as the
flags live it seems reasonable to have some amount of coverage, imho. This could
be "--no-legacy-const --no-harmony-sloppy", right?

Done

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/es6/block-...
File test/mjsunit/es6/block-non-strict-errors.js (left):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/es6/block-...
test/mjsunit/es6/block-non-strict-errors.js:23: CheckError("let x = 1;");
On 2016/01/05 at 19:19:05, adamk wrote:
> Same here.

Done

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/mjsunit.st...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:1096: 'regress/regress-crbug-504729': [SKIP],
On 2016/01/05 at 19:19:05, adamk wrote:
> Guessing these got skipped because ignition doesn't support hoistable sloppy
block functions?

Yeah, these seem to have sloppy mode function hoisting. I've just been
auto-disabling tests with Ignition failures.

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-91120.js (right):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-91120.js:28: // With ES2015 function hoisting
semantics, functions are only "hoisted" out
On 2016/01/05 at 19:19:05, adamk wrote:
> One more test that could be retained with proper flagging. I could imagine
duplicating this test, but it seems weird to just change it, since it's very
precisely testing behavior.

The bug was all about a crash. This regression test still verifies that we don't
crash. I'd rather verify that we don't crash in the released build rather than
with some funny flag combination.

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-crbug-323936.js (right):

https://codereview.chromium.org/1551443002/diff/80001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-crbug-323936.js:43: assertEquals("argument", e); 
// debugger did not overwrite
On 2016/01/05 at 19:19:05, adamk wrote:
> Do you understand if this test still testing the thing it was built for? It's
hard for me to tell..

The original bug was about the debugger being able to observe the correct values
within the nested, overlapping scope and interact with that code with correct,
consistent semantics. I think the test changes maintain that, with the current
semantics, and that this is more useful than turning on an obsolete flag.

[adamk, 2016-01-06 00:33:21]

Thanks for the regression test explanations.

lgtm, except that some of the tests (the message ones, looks like) still show up
as deleted in Rietveld. Hopefully that's just a display bug?

[adamk, 2016-01-06 00:36:46]

FWIW the raw patch looks fine.

[Dan Ehrenberg, 2016-01-06 01:01:45]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2016-01-06 01:01:53]

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

[commit-bot: I haz the power, 2016-01-06 02:02:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-06 02:03:14]

Description was changed from

==========
Ship ES2015 sloppy-mode function hoisting, let, class

This patch doesn't ship all features of ES2015 variable/scoping
changes, notably omitting the removal of legacy const. I think
function hoisting, let and class in sloppy mode can stand to
themselves as a package, and the legacy const change is much
riskier and more likely to be reverted, so my intention is to
pursue those as a separate, follow-on patch.

R=adamk@chromium.org
BUG=v8:4285,v8:3305
LOG=Y
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

to

==========
Ship ES2015 sloppy-mode function hoisting, let, class

This patch doesn't ship all features of ES2015 variable/scoping
changes, notably omitting the removal of legacy const. I think
function hoisting, let and class in sloppy mode can stand to
themselves as a package, and the legacy const change is much
riskier and more likely to be reverted, so my intention is to
pursue those as a separate, follow-on patch.

R=adamk@chromium.org
BUG=v8:4285,v8:3305
LOG=Y
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-01-06 02:03:15]

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

[Dan Ehrenberg, 2016-01-07 19:32:54]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1565263002/ by littledan@chromium.org.

The reason for reverting is: Causes frequent crashes in Canary: chromium:537816.