Report unexpected lexical decl also without destructuring

Report unexpected lexical decl also without destructuring

https://codereview.chromium.org/2694003002/ introduced
"SyntaxError: Lexical declaration cannot appear in a single-statement context"
for the case when let + desctructuring from a list happen.

As was pointed out in https://codereview.chromium.org/2694003002/#msg18, the
case without destructuring would also benefit from a better message: if a
single statement is expected and "let identifier = ..." is seen, the error is
indeed again that the lexical declaration is not a statement. However, the current
error is "Unexpected identifier", because the parser tries to accept "let" as
an identifier in an expression statement, and then gives up seeing the other
identifier after "let".

This CL ensures that the parser recognises the error properly and reports
accordingly. It also renames the existing test, which contains destructuring,
and adds the one with a non-destructuring lexical declaration.

BUG=v8:5686
Review-Url: https://codereview.chromium.org/2697193007
Cr-Commit-Position: refs/heads/master@{#43275}
Committed: https://chromium.googlesource.com/v8/v8/+/454816f08f8c4f52b15176f204e123488474e4a4

[vabr (Chromium), 2017-02-16 20:57:28]

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

[commit-bot: I haz the power, 2017-02-16 20:57:35]

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 20:57:40]

Description was changed from

==========
Report unexpected lexical decl also without destructuring

https://codereview.chromium.org/2694003002/ introduced
SyntaxError: Lexical declaration cannot appear in a single-statement context
for the case when let + desctructuring from a list happen.

As was pointed out in https://codereview.chromium.org/2694003002/#msg18, the
case without destructuring would also benefit from a better message: if a
single statement is expected and "let identifier = ..." is seen, the error is
indeed that the lexical declaration is not a statement. However, the current
error is "Unexpected identifier", because the parser tries to accept "let" as
an identifier in an expression statement, and then gives up seeing the other
identifier after "let".

This CL ensures that the parser recognises the error properly and reports
accordingly. It also renames the existing test, which contains destructuring,
and adds the one with a non-destructuring lexical declaration.

BUG=v85686
==========

to

==========
Report unexpected lexical decl also without destructuring

https://codereview.chromium.org/2694003002/ introduced
SyntaxError: Lexical declaration cannot appear in a single-statement context
for the case when let + desctructuring from a list happen.

As was pointed out in https://codereview.chromium.org/2694003002/#msg18, the
case without destructuring would also benefit from a better message: if a
single statement is expected and "let identifier = ..." is seen, the error is
indeed that the lexical declaration is not a statement. However, the current
error is "Unexpected identifier", because the parser tries to accept "let" as
an identifier in an expression statement, and then gives up seeing the other
identifier after "let".

This CL ensures that the parser recognises the error properly and reports
accordingly. It also renames the existing test, which contains destructuring,
and adds the one with a non-destructuring lexical declaration.

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

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

Description was changed from

==========
Report unexpected lexical decl also without destructuring

https://codereview.chromium.org/2694003002/ introduced
SyntaxError: Lexical declaration cannot appear in a single-statement context
for the case when let + desctructuring from a list happen.

As was pointed out in https://codereview.chromium.org/2694003002/#msg18, the
case without destructuring would also benefit from a better message: if a
single statement is expected and "let identifier = ..." is seen, the error is
indeed that the lexical declaration is not a statement. However, the current
error is "Unexpected identifier", because the parser tries to accept "let" as
an identifier in an expression statement, and then gives up seeing the other
identifier after "let".

This CL ensures that the parser recognises the error properly and reports
accordingly. It also renames the existing test, which contains destructuring,
and adds the one with a non-destructuring lexical declaration.

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

to

==========
Report unexpected lexical decl also without destructuring

https://codereview.chromium.org/2694003002/ introduced
"SyntaxError: Lexical declaration cannot appear in a single-statement context"
for the case when let + desctructuring from a list happen.

As was pointed out in https://codereview.chromium.org/2694003002/#msg18, the
case without destructuring would also benefit from a better message: if a
single statement is expected and "let identifier = ..." is seen, the error is
indeed again that the lexical declaration is not a statement. However, the
current
error is "Unexpected identifier", because the parser tries to accept "let" as
an identifier in an expression statement, and then gives up seeing the other
identifier after "let".

This CL ensures that the parser recognises the error properly and reports
accordingly. It also renames the existing test, which contains destructuring,
and adds the one with a non-destructuring lexical declaration.

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

[commit-bot: I haz the power, 2017-02-16 21:25:50]

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

[commit-bot: I haz the power, 2017-02-16 21:25:51]

Dry run: This issue passed the CQ dry run.

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

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

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

Hi Dan,

Is this a good follow-up to your comment about the ugly error for "with ({}) let
x = 1;"?

Cheers,
Vaclav

[Dan Ehrenberg, 2017-02-17 08:37:14]

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

https://codereview.chromium.org/2697193007/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4939: if (next_next != Token::LBRACK && next_next !=
Token::IDENTIFIER) break;
Another case you can include here is "let {", which will not be syntactically
valid here and may come up as a destructuring case (by contrast, `let:` and `let
+ let` and `let()` are valid).

[vabr (Chromium), 2017-02-17 09:36:30]

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

[commit-bot: I haz the power, 2017-02-17 09:36:41]

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-17 09:36:50]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2017-02-17 09:51:52]

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

[commit-bot: I haz the power, 2017-02-17 09:52:01]

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-17 09:52:08]

Thanks, Dan!

Please have a look at how I addressed your comment.

Cheers,
Vaclav

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

https://codereview.chromium.org/2697193007/diff/1/src/parsing/parser-base.h#n...
src/parsing/parser-base.h:4939: if (next_next != Token::LBRACK && next_next !=
Token::IDENTIFIER) break;
On 2017/02/17 08:37:14, Dan Ehrenberg wrote:
> Another case you can include here is "let {", which will not be syntactically
> valid here and may come up as a destructuring case (by contrast, `let:` and
`let
> + let` and `let()` are valid).

Good idea! Done.

https://codereview.chromium.org/2697193007/diff/20001/test/message/let-lexica...
File test/message/let-lexical-declaration-destructuring-in-single-statement.js
(right):

https://codereview.chromium.org/2697193007/diff/20001/test/message/let-lexica...
test/message/let-lexical-declaration-destructuring-in-single-statement.js:5:
with ({}) let [a] = [42];
Note: I added an element in the list on the right-hand side, to highlight that
the important error here is the lexical decl replacing a single statement, not
the failure to destructure an empty list.

[Dan Ehrenberg, 2017-02-17 09:54:35]

lgtm

Great job here, I admire your attention to details.

https://codereview.chromium.org/2697193007/diff/20001/test/message/let-lexica...
File test/message/let-lexical-declaration-destructuring-in-single-statement.js
(right):

https://codereview.chromium.org/2697193007/diff/20001/test/message/let-lexica...
test/message/let-lexical-declaration-destructuring-in-single-statement.js:5:
with ({}) let [a] = [42];
On 2017/02/17 09:52:07, vabr (Chromium) wrote:
> Note: I added an element in the list on the right-hand side, to highlight that
> the important error here is the lexical decl replacing a single statement, not
> the failure to destructure an empty list.

Note that that wouldn't fail anyway.

[commit-bot: I haz the power, 2017-02-17 10:16:01]

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

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

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-02-17 10:55:46]

Thanks for your quick and helpful reviews!

Vaclav

https://codereview.chromium.org/2697193007/diff/20001/test/message/let-lexica...
File test/message/let-lexical-declaration-destructuring-in-single-statement.js
(right):

https://codereview.chromium.org/2697193007/diff/20001/test/message/let-lexica...
test/message/let-lexical-declaration-destructuring-in-single-statement.js:5:
with ({}) let [a] = [42];
On 2017/02/17 09:54:35, Dan Ehrenberg wrote:
> On 2017/02/17 09:52:07, vabr (Chromium) wrote:
> > Note: I added an element in the list on the right-hand side, to highlight
that
> > the important error here is the lexical decl replacing a single statement,
not
> > the failure to destructure an empty list.
> 
> Note that that wouldn't fail anyway.

Ah, interesting. I see now that 'a' will become undefined.
This is due to
"3. If iteratorRecord.[[Done]] is true, let value be undefined."
in [1]'s AssignmentElement block, right?

[1]
https://tc39.github.io/ecma262/#sec-runtime-semantics-iteratordestructuringas...

[vabr (Chromium), 2017-02-17 10:55:52]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-02-17 10:55:57]

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-17 10:57:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487328952719630,
"parent_rev": "4b07a97978b9392eae52d397447d51725dd9d912", "commit_rev":
"454816f08f8c4f52b15176f204e123488474e4a4"}

[commit-bot: I haz the power, 2017-02-17 10:57:37]

Description was changed from

==========
Report unexpected lexical decl also without destructuring

https://codereview.chromium.org/2694003002/ introduced
"SyntaxError: Lexical declaration cannot appear in a single-statement context"
for the case when let + desctructuring from a list happen.

As was pointed out in https://codereview.chromium.org/2694003002/#msg18, the
case without destructuring would also benefit from a better message: if a
single statement is expected and "let identifier = ..." is seen, the error is
indeed again that the lexical declaration is not a statement. However, the
current
error is "Unexpected identifier", because the parser tries to accept "let" as
an identifier in an expression statement, and then gives up seeing the other
identifier after "let".

This CL ensures that the parser recognises the error properly and reports
accordingly. It also renames the existing test, which contains destructuring,
and adds the one with a non-destructuring lexical declaration.

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

to

==========
Report unexpected lexical decl also without destructuring

https://codereview.chromium.org/2694003002/ introduced
"SyntaxError: Lexical declaration cannot appear in a single-statement context"
for the case when let + desctructuring from a list happen.

As was pointed out in https://codereview.chromium.org/2694003002/#msg18, the
case without destructuring would also benefit from a better message: if a
single statement is expected and "let identifier = ..." is seen, the error is
indeed again that the lexical declaration is not a statement. However, the
current
error is "Unexpected identifier", because the parser tries to accept "let" as
an identifier in an expression statement, and then gives up seeing the other
identifier after "let".

This CL ensures that the parser recognises the error properly and reports
accordingly. It also renames the existing test, which contains destructuring,
and adds the one with a non-destructuring lexical declaration.

BUG=v8:5686

Review-Url: https://codereview.chromium.org/2697193007
Cr-Commit-Position: refs/heads/master@{#43275}
Committed:
https://chromium.googlesource.com/v8/v8/+/454816f08f8c4f52b15176f204e12348847...
==========

[commit-bot: I haz the power, 2017-02-17 10:57:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/454816f08f8c4f52b15176f204e12348847...