[strong] Implement static restrictions on switch statement

[strong] Implement static restrictions on switch statement

Implements the strong mode proposal's restrictions on the syntax of the
switch statement. Also fixes a minor bug with empty statements in strong
mode and improves StrongUndefinedArrow parser synch tests.

BUG=v8:3956
LOG=N

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

[conradw, 2015-04-14 16:35:59]

conradw@chromium.org changed reviewers:
+ rossberg@chromium.org

[conradw, 2015-04-14 16:36:21]

On 2015/04/14 16:35:59, conradw wrote:
> mailto:conradw@chromium.org changed reviewers:
> + mailto:rossberg@chromium.org

PTAL

The error message could probably be better.

[rossberg, 2015-04-14 20:19:50]

Did you forget to git add the JS test by any chance?

https://codereview.chromium.org/1084983002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1084983002/diff/1/src/ast.h#newcode283
src/ast.h:283: virtual bool IsStrongSwitchTerminatingStatement() const { return
false; }
I'm tempted to get rid of this and just use IsJump. AFAICS the only remaining
difference is IfStatement. I'm willing to tweak the spec to cover that. Adapting
the preparser should be straightforward as well.

https://codereview.chromium.org/1084983002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1084983002/diff/1/src/messages.js#newcode172
src/messages.js:172: strong_unterminated_switch:    ["In strong mode, each
switch statement list must be terminated by break, continue, return or throw"],
Nit: strong_switch_fallthrough

"In strong mode, switch fall-through is deprecated, terminate each case with
'break', 'continue', 'return' or 'throw'"

https://codereview.chromium.org/1084983002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1084983002/diff/1/src/parser.cc#newcode1822
src/parser.cc:1822: if (labels == NULL) {
To avoid the code duplication, how about introducing a single helper function,
say

  Statement NewLabelledStatement(ZoneList* labels, Statement)

that does the wrapping if lables isn't null, and just returns the statement
otherwise.

https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc#newcode239
src/preparser.cc:239: if (peek() == Token::SEMICOLON) {
Did you add a test covering this?

https://codereview.chromium.org/1084983002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1084983002/diff/1/src/preparser.h#newcode1107
src/preparser.h:1107: static PreParserStatement Terminating() {
...and then we can just rename this to Jump as well.

[conradw, 2015-04-15 11:40:03]

Oops! I did forget to add the JS tests.

https://codereview.chromium.org/1084983002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1084983002/diff/1/src/ast.h#newcode283
src/ast.h:283: virtual bool IsStrongSwitchTerminatingStatement() const { return
false; }
On 2015/04/14 20:19:49, rossberg wrote:
> I'm tempted to get rid of this and just use IsJump. AFAICS the only remaining
> difference is IfStatement. I'm willing to tweak the spec to cover that.
Adapting
> the preparser should be straightforward as well.

Done.

https://codereview.chromium.org/1084983002/diff/1/src/messages.js
File src/messages.js (right):

https://codereview.chromium.org/1084983002/diff/1/src/messages.js#newcode172
src/messages.js:172: strong_unterminated_switch:    ["In strong mode, each
switch statement list must be terminated by break, continue, return or throw"],
On 2015/04/14 20:19:49, rossberg wrote:
> Nit: strong_switch_fallthrough
> 
> "In strong mode, switch fall-through is deprecated, terminate each case with
> 'break', 'continue', 'return' or 'throw'"

Done.

https://codereview.chromium.org/1084983002/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1084983002/diff/1/src/parser.cc#newcode1822
src/parser.cc:1822: if (labels == NULL) {
On 2015/04/14 20:19:49, rossberg wrote:
> To avoid the code duplication, how about introducing a single helper function,
> say
> 
>   Statement NewLabelledStatement(ZoneList* labels, Statement)
> 
> that does the wrapping if lables isn't null, and just returns the statement
> otherwise.

Done, with the caveat that the ordering is kind of tricky, I wanted to use this
function for the TRY case as well, but it makes assumptions that the wrapper
block has already been created before the try-block is parsed. Ended up doing it
in two functions.

https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1084983002/diff/1/src/preparser.cc#newcode239
src/preparser.cc:239: if (peek() == Token::SEMICOLON) {
On 2015/04/14 20:19:49, rossberg wrote:
> Did you add a test covering this?

Done, was indirectly covered before but now directly covered.

https://codereview.chromium.org/1084983002/diff/1/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1084983002/diff/1/src/preparser.h#newcode1107
src/preparser.h:1107: static PreParserStatement Terminating() {
On 2015/04/14 20:19:49, rossberg wrote:
> ...and then we can just rename this to Jump as well.

Done.

[conradw, 2015-04-15 12:41:00]

Patchset #3 (id:40001) has been deleted

[conradw, 2015-04-15 17:27:47]

PTAL, unbroke it

[rossberg, 2015-04-15 19:56:35]

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1886
src/parser.cc:1886: Statement* Parser::ParseWrappedStatement(ZoneList<const
AstRawString*>* labels,
In fact, I would just inline the switch instead of creating a separate
function..

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1905
src/parser.cc:1905: *ok = false;
This case should be UNREACHABLE();

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode2874
src/parser.cc:2874: !stat->IsJump()) {
Nit: stray line break

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod...
src/preparser.cc:555: if (statement.IsJumpStatement()) {
Nit: Use return with ?:

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod...
src/preparser.cc:588: stat = (else_stat.IsJumpStatement()) ? stat :
Statement::Default();
This doesn't seem quite right, since stat may be something else then either Jump
or Default.

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod...
src/preparser.cc:715: !statement.IsJumpStatement()) {
Nit: stray line break

But more importantly, don't you need to add "&& token != Token::RBRACE"?

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
File test/mjsunit/strong/switch.js (right):

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:10: let successContexts = [
There are some cases missing, most importantly:

  switch (1) { default: null; }
  switch (1) { case 0: case 1: null; }
  switch (1) { default: {} }

Other: blocks, nested blocks, nested if's

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:16: ["switch(1) {case 1: if(1) break; else {", "}
default: break;};"]
Nit: consistent spacing (here and below)

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:19: let strongThrowContexts = [
Add some cases here as well, e.g. loops as last statement (even if they end in a
jump), illegal cases nested inside a block or if

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:63: CheckSwitch("break; ");
Nit: Make these an outer loop inside the test function

[arv (Not doing code reviews), 2015-04-15 20:55:32]

Where is the changes to Parser? I only see changes to the preparser.

[rossberg, 2015-04-16 07:37:43]

On 2015/04/15 20:55:32, arv wrote:
> Where is the changes to Parser? I only see changes to the preparser.

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode2866

What else are you missing? (Other than that it doesn't allow nested switches
yet.)

[conradw, 2015-04-16 10:59:57]

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1886
src/parser.cc:1886: Statement* Parser::ParseWrappedStatement(ZoneList<const
AstRawString*>* labels,
On 2015/04/15 19:56:34, rossberg wrote:
> In fact, I would just inline the switch instead of creating a separate
> function..

Inlining would mean duplicating the code, it has to be called in two places
(1829, 1834) due to the stupid Target scoping thing.

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode1905
src/parser.cc:1905: *ok = false;
On 2015/04/15 19:56:34, rossberg wrote:
> This case should be UNREACHABLE();

Done.

https://codereview.chromium.org/1084983002/diff/60001/src/parser.cc#newcode2874
src/parser.cc:2874: !stat->IsJump()) {
On 2015/04/15 19:56:34, rossberg wrote:
> Nit: stray line break

Done.

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod...
src/preparser.cc:555: if (statement.IsJumpStatement()) {
On 2015/04/15 19:56:34, rossberg wrote:
> Nit: Use return with ?:

Done.

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod...
src/preparser.cc:588: stat = (else_stat.IsJumpStatement()) ? stat :
Statement::Default();
On 2015/04/15 19:56:34, rossberg wrote:
> This doesn't seem quite right, since stat may be something else then either
Jump
> or Default.

Done.

https://codereview.chromium.org/1084983002/diff/60001/src/preparser.cc#newcod...
src/preparser.cc:715: !statement.IsJumpStatement()) {
On 2015/04/15 19:56:34, rossberg wrote:
> Nit: stray line break
> 
> But more importantly, don't you need to add "&& token != Token::RBRACE"?

Done, and I made the same mistake in parser

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
File test/mjsunit/strong/switch.js (right):

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:10: let successContexts = [
On 2015/04/15 19:56:34, rossberg wrote:
> There are some cases missing, most importantly:
> 
>   switch (1) { default: null; }
>   switch (1) { case 0: case 1: null; }
>   switch (1) { default: {} }
> 
> Other: blocks, nested blocks, nested if's

Done.

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:16: ["switch(1) {case 1: if(1) break; else {", "}
default: break;};"]
On 2015/04/15 19:56:34, rossberg wrote:
> Nit: consistent spacing (here and below)

Done.

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:19: let strongThrowContexts = [
On 2015/04/15 19:56:34, rossberg wrote:
> Add some cases here as well, e.g. loops as last statement (even if they end in
a
> jump), illegal cases nested inside a block or if

Done.

https://codereview.chromium.org/1084983002/diff/60001/test/mjsunit/strong/swi...
test/mjsunit/strong/switch.js:63: CheckSwitch("break; ");
On 2015/04/15 19:56:35, rossberg wrote:
> Nit: Make these an outer loop inside the test function

Done.

[rossberg, 2015-04-16 12:01:16]

Almost there, just a few tweaks to the tests.

https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6044: NULL};
Can you add cases with break/continue to label, e.g. by labelling the outer for
loop?

Also, one example of a nested if (if inside if).

https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6055: "case 1: case 2: if (1) break; default:",
Also add

  if (1) break else 0
  if (1) 0 else break

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
File test/mjsunit/strong/switch.js (right):

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:9: function CheckSwitch() {
I like this much better now.

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:12: "continue; ",
Add break and continue to label.

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:28: "if(1) break; else 1+1; ",
Also add the symmetric "if (1) 1+1 else break"

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:63: assertDoesNotThrow(strong_wrap[0] + "switch(1)
{case 1: " + code + "}}}");
Why not move this case to the successContexts?

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:93: assertDoesNotThrow("switch(1) {default: " +
code + "}");
Same here?

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:102: assertDoesNotThrow("'use strong'; switch(1)
{}");
Can't you cover all these by adding "" to the otherStatements?

[conradw, 2015-04-16 12:34:21]

https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6044: NULL};
On 2015/04/16 12:01:16, rossberg wrote:
> Can you add cases with break/continue to label, e.g. by labelling the outer
for
> loop?
> 
> Also, one example of a nested if (if inside if).

Done.

https://codereview.chromium.org/1084983002/diff/120001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:6055: "case 1: case 2: if (1) break; default:",
On 2015/04/16 12:01:16, rossberg wrote:
> Also add
> 
>   if (1) break else 0
>   if (1) 0 else break

Done.

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
File test/mjsunit/strong/switch.js (right):

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:12: "continue; ",
On 2015/04/16 12:01:16, rossberg wrote:
> Add break and continue to label.

Done.

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:28: "if(1) break; else 1+1; ",
On 2015/04/16 12:01:16, rossberg wrote:
> Also add the symmetric "if (1) 1+1 else break"

Done.

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:63: assertDoesNotThrow(strong_wrap[0] + "switch(1)
{case 1: " + code + "}}}");
On 2015/04/16 12:01:16, rossberg wrote:
> Why not move this case to the successContexts?

The successContexts are intended to not work if code is not a jump (so I can
have throwing tests inserting otherStatements). If this case was added, line 76
wouldn't come out so neat, since there's no fallthrough from the last case.

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:93: assertDoesNotThrow("switch(1) {default: " +
code + "}");
On 2015/04/16 12:01:16, rossberg wrote:
> Same here?

Similar reasoning. This for block is a specific (over)reaction to my mistake
with the right bracket condition.

https://codereview.chromium.org/1084983002/diff/120001/test/mjsunit/strong/sw...
test/mjsunit/strong/switch.js:102: assertDoesNotThrow("'use strong'; switch(1)
{}");
On 2015/04/16 12:01:16, rossberg wrote:
> Can't you cover all these by adding "" to the otherStatements?

It's currently assumed that when one of the otherStatements is used as "code"
with a successContext, it will throw in strong mode. Including "" would create
empty statements in many cases, causing "allowed fallthroughs", and no throws.
The original intention of this section was to test switches with various
"especially empty" bodies that didn't fit neatly into one of the lists, but I've
ended up with some duplicated tests. Will fix.

[rossberg, 2015-04-16 13:26:15]

lgtm

[conradw, 2015-04-16 13:26:30]

The CQ bit was checked by conradw@chromium.org

[commit-bot: I haz the power, 2015-04-16 13:26:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1084983002/140001

[commit-bot: I haz the power, 2015-04-16 13:29:34]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-04-16 13:29:46]

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