Raise SyntaxError on let [ starting an ExpressionStatement

Raise SyntaxError on let [ starting an ExpressionStatement

ES2017 forbids the sequence of tokens "let [" in in expression statements [1].

This CL makes ParserBase report those instances as SyntaxError. It also adds a
customised error message for that, because the standard "Unexpected token" is
not applicable: "let" itself is not forbidden in those context, only the
sequence of "let [".

[1] https://tc39.github.io/ecma262/#sec-expression-statement

BUG=v8:5686
Review-Url: https://codereview.chromium.org/2694003002
Cr-Commit-Position: refs/heads/master@{#43258}
Committed: https://chromium.googlesource.com/v8/v8/+/94bf354af55261a9a3c6f994b338627bb56231d2

[vabr (Chromium), 2017-02-13 18:53:01]

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

[commit-bot: I haz the power, 2017-02-13 18:53:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-13 19:13:46]

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

[commit-bot: I haz the power, 2017-02-13 19:13:46]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Dan Ehrenberg, 2017-02-13 19:26:57]

littledan@chromium.org changed reviewers:
+ littledan@chromium.org

[Dan Ehrenberg, 2017-02-13 19:26:57]

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

https://codereview.chromium.org/2694003002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:5461: }
I'm not sure I agree with your reading of the specification. Yes, the case Andre
mentioned (which you handled above, and which seems good to me AFAICT) is
prohibited, but I think the negative lookahead in IterationStatement is more for
disambiguation, so that "let [" is forced to be parsed as part of a
LexicalDeclaration rather than an Expression (otherwise, there's ambiguity and
lookahead would have to continue further). I believe this disagreement is what's
causing your test failures.

[vabr (Chromium), 2017-02-13 20:14:46]

Thanks for the comment!
However, please note that this CL has not been posted for review yet. I will
post it shortly, once all the bots are green.

Thanks!
Vaclav

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

https://codereview.chromium.org/2694003002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:5461: }
On 2017/02/13 19:26:57, Dan Ehrenberg wrote:
> I'm not sure I agree with your reading of the specification. Yes, the case
Andre
> mentioned (which you handled above, and which seems good to me AFAICT) is
> prohibited, but I think the negative lookahead in IterationStatement is more
for
> disambiguation, so that "let [" is forced to be parsed as part of a
> LexicalDeclaration rather than an Expression (otherwise, there's ambiguity and
> lookahead would have to continue further). I believe this disagreement is
what's
> causing your test failures.

The test failures are due to not restricting this check to for(;;) (as opposed
to for-each and for-in).

If the parser gets here, the attempt to parse the "let [" as LexicalDeclaration
has already failed (see ParseStatementListItem).

[vabr (Chromium), 2017-02-13 20:15:43]

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

[commit-bot: I haz the power, 2017-02-13 20:15:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2017-02-13 20:29:20]

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

https://codereview.chromium.org/2694003002/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:5461: }
On 2017/02/13 20:14:46, vabr (OOO until late Feb) wrote:
> On 2017/02/13 19:26:57, Dan Ehrenberg wrote:
> > I'm not sure I agree with your reading of the specification. Yes, the case
> Andre
> > mentioned (which you handled above, and which seems good to me AFAICT) is
> > prohibited, but I think the negative lookahead in IterationStatement is more
> for
> > disambiguation, so that "let [" is forced to be parsed as part of a
> > LexicalDeclaration rather than an Expression (otherwise, there's ambiguity
and
> > lookahead would have to continue further). I believe this disagreement is
> what's
> > causing your test failures.
> 
> The test failures are due to not restricting this check to for(;;) (as opposed
> to for-each and for-in).
> 
> If the parser gets here, the attempt to parse the "let [" as
LexicalDeclaration
> has already failed (see ParseStatementListItem).

Hm, now I see how I misunderstood your comment and mixed my observation about
the ExpressionStatement case.

Let me think about this a little more, I will update the CL with the next
proposal (which might be to restrict this change just to address the
ExpressionStatement case).

[vabr (Chromium), 2017-02-13 20:32:42]

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

[commit-bot: I haz the power, 2017-02-13 20:32:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2017-02-13 20:33:47]

Description was changed from

==========
Raise SyntaxError on let [ in wrong context

There are exactly two contexts in ES2017 which forbid the sequence of tokens
"let [":
 * in expression statement [1], and
 * in the initializer of a for loop [2]

This CL makes ParserBase report those instances as SyntaxError. It also adds a
customised error message for that, because the standard "Unexpected token" is
not applicable: "let" itself is not forbidden in those context, only the
sequence of "let [".

[1] https://tc39.github.io/ecma262/#sec-expression-statement
[2] https://tc39.github.io/ecma262/#sec-iteration-statements

BUG=v8:5686
==========

to

==========
Raise SyntaxError on let [ starting an ExpressionStatement

ES2017 forbids the sequence of tokens "let [" in in expression statements [1].

This CL makes ParserBase report those instances as SyntaxError. It also adds a
customised error message for that, because the standard "Unexpected token" is
not applicable: "let" itself is not forbidden in those context, only the
sequence of "let [".

[1] https://tc39.github.io/ecma262/#sec-expression-statement

BUG=v8:5686
==========

[commit-bot: I haz the power, 2017-02-13 21:05:32]

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

[commit-bot: I haz the power, 2017-02-13 21:05:33]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-02-13 21:16:21]

Hi Dan,

Could you please review the current patch set (3)?

Cheers,
Vaclav

[vabr (Chromium), 2017-02-16 08:33:07]

Hi Dan,

A friendly reminder in case this CL got lost. I restricted the change to the
prohibited case only, could you please review?

Thanks for all your recent reviews and advice!
Vaclav

[Dan Ehrenberg, 2017-02-16 09:16:52]

https://codereview.chromium.org/2694003002/diff/40001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2694003002/diff/40001/src/messages.h#newcode641
src/messages.h:641: T(UnexpectedTokenLetLBrack, "Unexpected token sequence let
[")               \
From a user's perspective, the error they made was putting a lexical declaration
as the body of something expecting a single statement. I like SpiderMonkey's
error here: " lexical declarations can't appear in single-statement context:"  .
Note that we already have a pretty ugly error for "with ({}) let x = 1;"
("unexpected identifier"), so arguably cleaning up the messages here could be a
task for another patch.

https://codereview.chromium.org/2694003002/diff/40001/test/mjsunit/invalid-so...
File test/mjsunit/invalid-source-element.js (right):

https://codereview.chromium.org/2694003002/diff/40001/test/mjsunit/invalid-so...
test/mjsunit/invalid-source-element.js:34: assertThrows("function() { with ({})
let [a] = [];}", SyntaxError);
Could you add a "messages" test, similar to
test/message/let-lexical-name-in-array-prohibited.js? This will have a few
benefits:
- Tests the preparser as well as the parser
- Tests that the particular error message that you added is the one that is used

[vabr (Chromium), 2017-02-16 17:08:40]

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

[commit-bot: I haz the power, 2017-02-16 17:08:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2017-02-16 17:09:33]

Thanks for the review and useful suggesitons!

Please take a look again at how I addressed your comments.

Cheers,
Vaclav

https://codereview.chromium.org/2694003002/diff/40001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2694003002/diff/40001/src/messages.h#newcode641
src/messages.h:641: T(UnexpectedTokenLetLBrack, "Unexpected token sequence let
[")               \
On 2017/02/16 09:16:51, Dan Ehrenberg wrote:
> From a user's perspective, the error they made was putting a lexical
declaration
> as the body of something expecting a single statement. I like SpiderMonkey's
> error here: " lexical declarations can't appear in single-statement context:" 
.
> Note that we already have a pretty ugly error for "with ({}) let x = 1;"
> ("unexpected identifier"), so arguably cleaning up the messages here could be
a
> task for another patch.

Thanks, that's a great point.
I adopted the message from SpiderMonkey you suggested, it makes sense to do this
right when adding this as new.

I agree with you about the other error as well, and will do a separate patch for
that, to allow selective reverting if needed.

https://codereview.chromium.org/2694003002/diff/40001/test/mjsunit/invalid-so...
File test/mjsunit/invalid-source-element.js (right):

https://codereview.chromium.org/2694003002/diff/40001/test/mjsunit/invalid-so...
test/mjsunit/invalid-source-element.js:34: assertThrows("function() { with ({})
let [a] = [];}", SyntaxError);
On 2017/02/16 09:16:52, Dan Ehrenberg wrote:
> Could you add a "messages" test, similar to
> test/message/let-lexical-name-in-array-prohibited.js? This will have a few
> benefits:
> - Tests the preparser as well as the parser
> - Tests that the particular error message that you added is the one that is
used

Thanks for teaching me about the "messages" tests.
I did as you suggested, and I am removing this mjsunit change, because it is now
redundant.

[Dan Ehrenberg, 2017-02-16 17:17:36]

lgtm

[vabr (Chromium), 2017-02-16 17:25:15]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2017-02-16 17:25:24]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-02-16 17:25:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 17:37:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487265924927460,
"parent_rev": "b593f1a56edaf3fb754d85def26f3e8a85169440", "commit_rev":
"94bf354af55261a9a3c6f994b338627bb56231d2"}

[commit-bot: I haz the power, 2017-02-16 17:37:27]

Description was changed from

==========
Raise SyntaxError on let [ starting an ExpressionStatement

ES2017 forbids the sequence of tokens "let [" in in expression statements [1].

This CL makes ParserBase report those instances as SyntaxError. It also adds a
customised error message for that, because the standard "Unexpected token" is
not applicable: "let" itself is not forbidden in those context, only the
sequence of "let [".

[1] https://tc39.github.io/ecma262/#sec-expression-statement

BUG=v8:5686
==========

to

==========
Raise SyntaxError on let [ starting an ExpressionStatement

ES2017 forbids the sequence of tokens "let [" in in expression statements [1].

This CL makes ParserBase report those instances as SyntaxError. It also adds a
customised error message for that, because the standard "Unexpected token" is
not applicable: "let" itself is not forbidden in those context, only the
sequence of "let [".

[1] https://tc39.github.io/ecma262/#sec-expression-statement

BUG=v8:5686

Review-Url: https://codereview.chromium.org/2694003002
Cr-Commit-Position: refs/heads/master@{#43258}
Committed:
https://chromium.googlesource.com/v8/v8/+/94bf354af55261a9a3c6f994b338627bb56...
==========

[commit-bot: I haz the power, 2017-02-16 17:37:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/94bf354af55261a9a3c6f994b338627bb56...