Restrict FunctionDeclarations in Statement position

Restrict FunctionDeclarations in Statement position

ES2015 generally bans FunctionDeclarations in positions which expect a Statement,
as opposed to a StatementListItem, such as a FunctionDeclaration which constitutes
the body of a for loop. However, Annex B 3.2 and 3.4 make exceptions for labeled
function declarations and function declarations as the body of an if statement in
sloppy mode, in the latter case specifying that the semantics are as if the
function declaration occurred in a block. Chrome has historically permitted
further extensions, for the body of any flow control construct.

This patch addresses both the syntactic and semantic mismatches between V8 and
the spec. For the semantic mismatch, function declarations as the body of if
statements change from unconditionally hoisting in certain cases to acquiring
the sloppy mode function in block semantics (based on Annex B 3.3). For the
extra syntax permitted, this patch adds a flag,
--harmony-restrictive-declarations, which excludes disallowed function declaration
cases. A new UseCounter, LegacyFunctionDeclaration, is added to count how often
function declarations occur as the body of other constructs in sloppy mode. With
this patch, the code generally follows the form of the specification with respect
to parsing FunctionDeclarations, rather than allowing them in arbitrary Statement
positions, and makes it more clear where our extensions occur.

BUG=v8:4647
R=adamk
LOG=Y

Committed: https://crrev.com/0e7f095c6dd4363d66f8b14a428f9c1f07f7dba3
Cr-Commit-Position: refs/heads/master@{#34470}

[Dan Ehrenberg, 2016-03-01 21:22:06]

Description was changed from

==========
Beginning of implementing Annex B 3.4

BUG=v8:4647
R=adamk
LOG=Y
==========

to

==========
Restrict function declarations as Statement to if statements

Annex B 3.4 of the ES2016 draft specification permits function declarations as
the body of an if statement in sloppy mode, with semantics that are identical
to enclosing them in an extra set of curly brackets. It does not permit function
declarations as the body of other control flow constructs, such as for loops,
but
Chrome as allowed these until now. This patch implements the new semantics of
implicitly introducing a block (not behind a flag) and adds a new flag,
--harmony-restrictive-declarations, which implements the stricter behavior
prohibiting it in other cases. A new UseCounter, LegacyFunctionDeclaration, is
added to count how often function declarations occur as the body of other
constructs in sloppy mode.

BUG=v8:4647
R=adamk
LOG=Y
==========

[Dan Ehrenberg, 2016-03-01 23:18:06]

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

[commit-bot: I haz the power, 2016-03-01 23:18:10]

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

[commit-bot: I haz the power, 2016-03-01 23:33:53]

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

[commit-bot: I haz the power, 2016-03-01 23:33:54]

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/14583)

[Dan Ehrenberg, 2016-03-01 23:34:36]

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

[commit-bot: I haz the power, 2016-03-01 23:34:43]

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

[commit-bot: I haz the power, 2016-03-01 23:51:15]

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

[commit-bot: I haz the power, 2016-03-01 23:51:16]

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

[Dan Ehrenberg, 2016-03-01 23:56:03]

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

[commit-bot: I haz the power, 2016-03-01 23:56:05]

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

[Dan Ehrenberg, 2016-03-02 00:03:07]

Description was changed from

==========
Restrict function declarations as Statement to if statements

Annex B 3.4 of the ES2016 draft specification permits function declarations as
the body of an if statement in sloppy mode, with semantics that are identical
to enclosing them in an extra set of curly brackets. It does not permit function
declarations as the body of other control flow constructs, such as for loops,
but
Chrome as allowed these until now. This patch implements the new semantics of
implicitly introducing a block (not behind a flag) and adds a new flag,
--harmony-restrictive-declarations, which implements the stricter behavior
prohibiting it in other cases. A new UseCounter, LegacyFunctionDeclaration, is
added to count how often function declarations occur as the body of other
constructs in sloppy mode.

BUG=v8:4647
R=adamk
LOG=Y
==========

to

==========
Restrict FunctionDeclarations in Statement position

ES2015 generally bans FunctionDeclarations in positions which expect a
Statement,
as opposed to a StatementListItem, such as a FunctionDeclaration which
constitutes
the body of a for loop. However, Annex B 3.2 and 3.4 make exceptions for labeled
function declarations and function declarations as the body of an if statement
in
sloppy mode, in the latter case specifying that the semantics are as if the
function declaration occurred in a block. Chrome has historically permitted
further extensions, for the body of any flow control construct.

This patch addresses both the syntactic and semantic mismatches between V8 and
the spec. For the semantic mismatch, function declarations as the body of if
statements change from unconditionally hoisting in certain cases to acquiring
the sloppy mode function in block semantics (based on Annex B 3.3). For the
extra syntax permitted, this patch adds a flag,
--harmony-restrictive-declarations, which excludes disallowed function
declaration
cases. A new UseCounter, LegacyFunctionDeclaration, is added to count how often
function declarations occur as the body of other constructs in sloppy mode. With
this patch, the code generally follows the form of the specification with
respect
to parsing FunctionDeclarations, rather than allowing them in arbitrary
Statement
positions, and makes it more clear where our extensions occur.

BUG=v8:4647
R=adamk
LOG=Y
==========

[Dan Ehrenberg, 2016-03-02 00:03:37]

Description was changed from

==========
Restrict FunctionDeclarations in Statement position

ES2015 generally bans FunctionDeclarations in positions which expect a
Statement,
as opposed to a StatementListItem, such as a FunctionDeclaration which
constitutes
the body of a for loop. However, Annex B 3.2 and 3.4 make exceptions for labeled
function declarations and function declarations as the body of an if statement
in
sloppy mode, in the latter case specifying that the semantics are as if the
function declaration occurred in a block. Chrome has historically permitted
further extensions, for the body of any flow control construct.

This patch addresses both the syntactic and semantic mismatches between V8 and
the spec. For the semantic mismatch, function declarations as the body of if
statements change from unconditionally hoisting in certain cases to acquiring
the sloppy mode function in block semantics (based on Annex B 3.3). For the
extra syntax permitted, this patch adds a flag,
--harmony-restrictive-declarations, which excludes disallowed function
declaration
cases. A new UseCounter, LegacyFunctionDeclaration, is added to count how often
function declarations occur as the body of other constructs in sloppy mode. With
this patch, the code generally follows the form of the specification with
respect
to parsing FunctionDeclarations, rather than allowing them in arbitrary
Statement
positions, and makes it more clear where our extensions occur.

BUG=v8:4647
R=adamk
LOG=Y
==========

to

==========
Restrict FunctionDeclarations in Statement position

ES2015 generally bans FunctionDeclarations in positions which expect a
Statement,
as opposed to a StatementListItem, such as a FunctionDeclaration which
constitutes
the body of a for loop. However, Annex B 3.2 and 3.4 make exceptions for labeled
function declarations and function declarations as the body of an if statement
in
sloppy mode, in the latter case specifying that the semantics are as if the
function declaration occurred in a block. Chrome has historically permitted
further extensions, for the body of any flow control construct.

This patch addresses both the syntactic and semantic mismatches between V8 and
the spec. For the semantic mismatch, function declarations as the body of if
statements change from unconditionally hoisting in certain cases to acquiring
the sloppy mode function in block semantics (based on Annex B 3.3). For the
extra syntax permitted, this patch adds a flag,
--harmony-restrictive-declarations, which excludes disallowed function
declaration
cases. A new UseCounter, LegacyFunctionDeclaration, is added to count how often
function declarations occur as the body of other constructs in sloppy mode. With
this patch, the code generally follows the form of the specification with
respect
to parsing FunctionDeclarations, rather than allowing them in arbitrary
Statement
positions, and makes it more clear where our extensions occur.

BUG=v8:4647
R=adamk
LOG=Y
==========

[commit-bot: I haz the power, 2016-03-02 00:09:56]

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

[commit-bot: I haz the power, 2016-03-02 00:09:57]

Dry run: Exceeded global retry quota

[Dan Ehrenberg, 2016-03-02 00:18:17]

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

[commit-bot: I haz the power, 2016-03-02 00:18:28]

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

[adamk, 2016-03-02 00:33:45]

High-level question, as I start the review: why is the
addition-of-a-block-in-if-statements part not behind a flag? Is it because
they're still subject to Annex B.3.3 hoisting, which is assumed to take care of
most cases on the web?

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
File test/mjsunit/harmony/sloppy-implicit-block-function.js (right):

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
test/mjsunit/harmony/sloppy-implicit-block-function.js:11: (function() {
Maybe add assertEquals(undefined, f) before the if statement to demonstrate that
hoisting is still going on.

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
test/mjsunit/harmony/sloppy-implicit-block-function.js:12: if (false) function
f() { };
What about if (true)? I assume that results in the function being visible later?

[commit-bot: I haz the power, 2016-03-02 00:35:02]

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

[commit-bot: I haz the power, 2016-03-02 00:35:03]

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

[Dan Ehrenberg, 2016-03-02 00:49:54]

High-level answer: Yes, the idea is that we already did the main
function-in-block semantics change, and this is cleaning up the edge cases.
Plus, we are at the beginning of Canary, so we'll have some time to test, and
also Firefox has shipped this for a long time. Prohibiting the function
declarations, rather than the semantics change, seems riskier. Another factor is
that I couldn't think of a clean way to keep the previous semantics behind a
flag.

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
File test/mjsunit/harmony/sloppy-implicit-block-function.js (right):

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
test/mjsunit/harmony/sloppy-implicit-block-function.js:11: (function() {
On 2016/03/02 at 00:33:45, adamk wrote:
> Maybe add assertEquals(undefined, f) before the if statement to demonstrate
that hoisting is still going on.

Done

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
test/mjsunit/harmony/sloppy-implicit-block-function.js:12: if (false) function
f() { };
On 2016/03/02 at 00:33:45, adamk wrote:
> What about if (true)? I assume that results in the function being visible
later?

Good point, added tests for this, and for the 'else' case.

[Dan Ehrenberg, 2016-03-02 00:50:04]

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

[adamk, 2016-03-02 00:51:10]

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

https://codereview.chromium.org/1757543003/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:2579: // ES#sec-labelled-function-declarations Labelled
Function Declarations
Can you add tests feature labels? It seems we currently parse them...

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
File test/mjsunit/harmony/sloppy-implicit-block-function.js (right):

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
test/mjsunit/harmony/sloppy-implicit-block-function.js:21: `, SyntaxError);
All these parsing tests will better-exercise the parser in cctest/test-parsing.
At the moment I think you have no PreParser support at all. Please move the
tests to test-parsing, which should point the way towards the changes you need
in the PreParser.

[Dan Ehrenberg, 2016-03-03 02:32:58]

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

https://codereview.chromium.org/1757543003/diff/100001/src/parsing/parser.cc#...
src/parsing/parser.cc:2579: // ES#sec-labelled-function-declarations Labelled
Function Declarations
On 2016/03/02 at 00:51:10, adamk wrote:
> Can you add tests feature labels? It seems we currently parse them...

Done

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
File test/mjsunit/harmony/sloppy-implicit-block-function.js (right):

https://codereview.chromium.org/1757543003/diff/100001/test/mjsunit/harmony/s...
test/mjsunit/harmony/sloppy-implicit-block-function.js:21: `, SyntaxError);
On 2016/03/02 at 00:51:10, adamk wrote:
> All these parsing tests will better-exercise the parser in
cctest/test-parsing. At the moment I think you have no PreParser support at all.
Please move the tests to test-parsing, which should point the way towards the
changes you need in the PreParser.

Oops, not sure how I completely forgot about the preparser! Moved a bunch of
these tests to cctest and implemented preparser support.

[Dan Ehrenberg, 2016-03-03 02:33:06]

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

[commit-bot: I haz the power, 2016-03-03 02:33:10]

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

[commit-bot: I haz the power, 2016-03-03 02:37:48]

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

[commit-bot: I haz the power, 2016-03-03 02:37:49]

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/11815)

[adamk, 2016-03-03 19:51:37]

lgtm once you've added the discussed-offline tests (for labelled function in an
if statement). looks like you need a git cl format too.

[Dan Ehrenberg, 2016-03-03 21:10:30]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2016-03-03 21:10:30]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1757543003/#ps210001
(title: "git cl format")

[commit-bot: I haz the power, 2016-03-03 21:10:41]

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

[commit-bot: I haz the power, 2016-03-03 21:34:01]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-03-03 21:34:35]

Description was changed from

==========
Restrict FunctionDeclarations in Statement position

ES2015 generally bans FunctionDeclarations in positions which expect a
Statement,
as opposed to a StatementListItem, such as a FunctionDeclaration which
constitutes
the body of a for loop. However, Annex B 3.2 and 3.4 make exceptions for labeled
function declarations and function declarations as the body of an if statement
in
sloppy mode, in the latter case specifying that the semantics are as if the
function declaration occurred in a block. Chrome has historically permitted
further extensions, for the body of any flow control construct.

This patch addresses both the syntactic and semantic mismatches between V8 and
the spec. For the semantic mismatch, function declarations as the body of if
statements change from unconditionally hoisting in certain cases to acquiring
the sloppy mode function in block semantics (based on Annex B 3.3). For the
extra syntax permitted, this patch adds a flag,
--harmony-restrictive-declarations, which excludes disallowed function
declaration
cases. A new UseCounter, LegacyFunctionDeclaration, is added to count how often
function declarations occur as the body of other constructs in sloppy mode. With
this patch, the code generally follows the form of the specification with
respect
to parsing FunctionDeclarations, rather than allowing them in arbitrary
Statement
positions, and makes it more clear where our extensions occur.

BUG=v8:4647
R=adamk
LOG=Y
==========

to

==========
Restrict FunctionDeclarations in Statement position

ES2015 generally bans FunctionDeclarations in positions which expect a
Statement,
as opposed to a StatementListItem, such as a FunctionDeclaration which
constitutes
the body of a for loop. However, Annex B 3.2 and 3.4 make exceptions for labeled
function declarations and function declarations as the body of an if statement
in
sloppy mode, in the latter case specifying that the semantics are as if the
function declaration occurred in a block. Chrome has historically permitted
further extensions, for the body of any flow control construct.

This patch addresses both the syntactic and semantic mismatches between V8 and
the spec. For the semantic mismatch, function declarations as the body of if
statements change from unconditionally hoisting in certain cases to acquiring
the sloppy mode function in block semantics (based on Annex B 3.3). For the
extra syntax permitted, this patch adds a flag,
--harmony-restrictive-declarations, which excludes disallowed function
declaration
cases. A new UseCounter, LegacyFunctionDeclaration, is added to count how often
function declarations occur as the body of other constructs in sloppy mode. With
this patch, the code generally follows the form of the specification with
respect
to parsing FunctionDeclarations, rather than allowing them in arbitrary
Statement
positions, and makes it more clear where our extensions occur.

BUG=v8:4647
R=adamk
LOG=Y

Committed: https://crrev.com/0e7f095c6dd4363d66f8b14a428f9c1f07f7dba3
Cr-Commit-Position: refs/heads/master@{#34470}
==========

[commit-bot: I haz the power, 2016-03-03 21:34:36]

Patchset 12 (id:??) landed as
https://crrev.com/0e7f095c6dd4363d66f8b14a428f9c1f07f7dba3
Cr-Commit-Position: refs/heads/master@{#34470}