1288459 - Don't throw SyntaxError for "let \n {" in expression statement context

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
function f() {
  with ({}) let // Implicit semicolon here.
  {}
  ;
}
---

Expected: No SyntaxError
Actual: Throws a SyntaxError

Note: The specification only disallows "let [" in ExpressionStatement, "let {" is valid (if "let" is followed by a line break).

Spec:
https://tc39.github.io/ecma262/#sec-expression-statement

Comment 1

[Shu-yu Guo [:shu]]

Waldo, all yours. :)

Comment 2

[Jeff Walden [:Waldo]]

When parsing we don't differentiate StatementListItem from Statement.  A consequence of this is that we allow declarations (including LexicalDeclaration) to be parsed in places where they're actually prohibited.  We (eventually) do the right thing, later in parsing -- but only *after* we've decided we're parsing a lexical declaration, too late to cleanly undo our work far enough to deal with this.

What this needs/requires, is for Parser::statement to be forked into that plus Parser::statementListItem, with only the latter permitting various declaration forms.  Then Parser::statement can properly perform lookahead to treat: 'let [' as a LexicalDeclaration; 'let {' as an ExpressionStatement followed by a '{' if ASI intrudes after 'let', otherwise as a LexicalDeclaration.  And Parser::statementList can treat: 'let [' as a LexicalDeclaration; 'let {' as an ExpressionStatement regardless whether ASI intrudes in the middle.  (And something something for each for 'let <name>', that I'm disinclined to think deeply enough about now to write something authoritative.)

I expect this change to be large enough, and to interact with scoping enough, that we shouldn't do it on the current parser but rather should wait for bug 1263355.  Either that or it should be written and applied both places.  I tend to think this isn't so pressingly important that it should jump that gun.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> When parsing we don't differentiate StatementListItem from Statement.  A
> consequence of this is that we allow declarations (including
> LexicalDeclaration) to be parsed in places where they're actually
> prohibited.  We (eventually) do the right thing, later in parsing -- but
> only *after* we've decided we're parsing a lexical declaration, too late to
> cleanly undo our work far enough to deal with this.
> 
> What this needs/requires, is for Parser::statement to be forked into that
> plus Parser::statementListItem, with only the latter permitting various
> declaration forms.  Then Parser::statement can properly perform lookahead to
> treat: 'let [' as a LexicalDeclaration; 'let {' as an ExpressionStatement
> followed by a '{' if ASI intrudes after 'let', otherwise as a
> LexicalDeclaration.  And Parser::statementList can treat: 'let [' as a
> LexicalDeclaration; 'let {' as an ExpressionStatement regardless whether ASI
> intrudes in the middle.  (And something something for each for 'let <name>',
> that I'm disinclined to think deeply enough about now to write something
> authoritative.)
> 
> I expect this change to be large enough, and to interact with scoping
> enough, that we shouldn't do it on the current parser but rather should wait
> for bug 1263355.  Either that or it should be written and applied both
> places.  I tend to think this isn't so pressingly important that it should
> jump that gun.

Yikes, I'm certainly fine with waiting. Agree that this does not seem at all urgent, assigned it to you because I'm pretty sure you're the only one who understands this in our code.

Comment 4

[Jeff Walden [:Waldo]]

I have a series of patches (sans tests for now) that fix this.  Waiting on bug 1263355 to land to post them.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Add tests for parsing Statement and StatementList using distinct grammar productions

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Rename Parser::statements to Parser::statementList, the actual grammar term

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Split variable-statement parsing into its own parser function

Seeing as in a moment it's going to be used in two places.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Copy Parser::statement into an identical (but not-yet-called) Parser::statementListItem

Exact copying, no changes.  The theory is that this will make patches to the two functions clearer in diffs -- unfortunately this appears not to be the case with some diff algorithms depending on greediness.  (And hg's diff algorithm appears to be greedy.)  Sest la vee.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Make Parser::labeledStatement use a new Parser::labeledItem function to parse its nested statement, anticipating when Parser::statement no longer parses FunctionDeclarations

This doesn't yet change any behavior, because Parser::statement{,ListItem} are identical.  But once they change, this will start doing the right thing.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Convert a few existing calls to Parser::statement over to Parser::statementListItem (which still has identical functionality at present), before the two functions' behavior is made to differ

With labels handled in the previous patch, this cleans up the rest of them.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Make Parser::statement and Parser::statementListItem behave exactly as the spec productions do -- in particular, Parser::statement no longer accepts function and lexical declarations

You might ask, can we have the majority of these two nearly-identical switches shared somehow?  Unfortunately, not really.  The problem is Parser::expressionStatement *can't* be called by both of them, because 'let' in Statement and StatementList both *can* be an Identifier as part of an ExpressionStatement.  If we see 'let' starting either production, we would have to look at the next token -- bearing in mind same-line-ness -- to determine what to do.

And if we did that, we would have a super-massive switch across all TokenKinds to write.  Here's a comment from my patchwork attempting that, which I got a ways in on, then checked es6draft and found it didn't have any such thing, and the reason for that was that the two algorithms functionally had separate mega-switches.

// Prepare to be horrified by casuistry explicitly enumerating EVERY
// SINGLE TokenKind.  There is no other way.  What distinguishes these
// valid possibilities:
//
//     if (1) let     // EOF here
//     if (1) let ;
//     if (1) let + 3;
//   { if (1) let }
//     if (1) let `foo`;
//     if (1) let ();
//     if (1) let -= 7;
//     if (1) let ? 2 : 3;
//     if (1) let , 0;
//     if (1) let => x; // invalid in strict mode code
//     if (1) let : 42; // invalid in strict mode code
//
// from these invalid possibilities:
//
//     if (1) let break
//     if (1) let x
//     if (1) let "foo"
//     if (1) let ...
//     if (1) let {   // followed by anything or nothing
//     if (1) let 5
//     if (1) let [
//     if (1) let /x/
//
// The answer is nothing, except nitpickeries latent in the
// ExpressionStatement production.  We must enumerate them all.

Such a massive switch on TokenKinds, with so many distinctions, is far worse than a switch over all TokenKinds that can start a Statement or StatementListItem.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Implement static semantics restrictions on labeled functions when 'function' is first encountered after a label, not some weird time later

Parser::checkFunctionDefinition is still a bit fugly -- seems to me that block-scoping semantics should be implemented in a different spot, perhaps with Parser::statementList split into a version that tracks block-scoped functions and a version that handles body-level stuff -- but this at least improves *some* of it.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Forbid GeneratorDeclaration as a child of LabelledStatement

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8785107 [details] [diff] [review]
Make Parser::labeledStatement use a new Parser::labeledItem function to parse its nested statement, anticipating when Parser::statement no longer parses FunctionDeclarations

Review of attachment 8785107 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/Syntax/lexical-declaration-forbidden-in-label.js
@@ +1,1 @@
> +// |reftest| skip-if(!xulRuntime.shell)

this could be removed.

Comment 15

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> Created attachment 8785111 [details] [diff] [review]
> Forbid GeneratorDeclaration as a child of LabelledStatement

This should fix bug 1204026, correct?

Comment 16

[Jeff Walden [:Waldo]]

(In reply to André Bargull from comment #15)
> This should fix bug 1204026, correct?

Yes.

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8785109 [details] [diff] [review]
Make Parser::statement and Parser::statementListItem behave exactly as the spec productions do -- in particular, Parser::statement no longer accepts function and lexical declarations

Review of attachment 8785109 [details] [diff] [review]:
-----------------------------------------------------------------

wow... sad diff...
(I checked diff for each separated function locally)

Comment 18

[Tooru Fujisawa [:arai]]

Comment on attachment 8785109 [details] [diff] [review]
Make Parser::statement and Parser::statementListItem behave exactly as the spec productions do -- in particular, Parser::statement no longer accepts function and lexical declarations

clearing r+ for now, per the IRC conversation,
and to check this patch again with another patch for B.3.4.
https://tc39.github.io/ecma262/#sec-functiondeclarations-in-ifstatement-statement-clauses

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Make the statement-like children of an |if(-else)| properly understand FunctionStatement

This goes atop the "Convert a few existing calls" patch and seems to do the if-else trick.

Comment 20

[Tooru Fujisawa [:arai]]

Just noticed that the change to Parser<ParseHandler>::statement needs the corresponding change in js/src/asmjs/AsmJS.cpp.
https://dxr.mozilla.org/mozilla-central/rev/7963ebdd52b93f96b812eff2eab8d94097147b9c/js/src/asmjs/AsmJS.cpp#789
> static bool
> ParseVarOrConstStatement(AsmJSParser& parser, ParseNode** var)
> {
>     TokenKind tk;
>     if (!PeekToken(parser, &tk))
>         return false;
>     if (tk != TOK_VAR && tk != TOK_CONST) {
>         *var = nullptr;
>         return true;
>     }
> 
>     *var = parser.statement(YieldIsName);
>     if (!*var)
>         return false;
> 
>     MOZ_ASSERT((*var)->isKind(PNK_VAR) || (*var)->isKind(PNK_CONST));
>     return true;
> }

Comment 21

[Tooru Fujisawa [:arai]]

Comment on attachment 8785195 [details] [diff] [review]
Make the statement-like children of an |if(-else)| properly understand FunctionStatement

Review of attachment 8785195 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming error handling code will be removed later.

::: js/src/frontend/Parser.cpp
@@ +4860,5 @@
>  }
>  
> +template <class ParseHandler>
> +typename ParseHandler::Node
> +Parser<ParseHandler>::consequentOrAlternative(YieldHandling yieldHandling)

Can we have "if" or something in the method name to clarify what this is a part of?
or perhaps "consequent" and "alternative" are common term specific for if-statement?

@@ +4876,5 @@
> +            return functionStmt(yieldHandling, NameRequired);
> +
> +        // Otherwise immediately report an error.  Don't fall through and allow
> +        // Parser::statement to handle this: it'll interpret this as an
> +        // ExpressionStatement containing a FunctionExpression.

This comment will become obsolete after the change to Parser::statement, right?

@@ +4877,5 @@
> +
> +        // Otherwise immediately report an error.  Don't fall through and allow
> +        // Parser::statement to handle this: it'll interpret this as an
> +        // ExpressionStatement containing a FunctionExpression.
> +        report(ParseError, false, null(), JSMSG_FUNCTION_LABEL);

report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN, "statement", TokenKindToDesc(next));

or perhaps "statement except function" would be more helpful here?

anyway, I think this handling could be merged to Parser::statement later.

Comment 22

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #21)
> or perhaps "consequent" and "alternative" are common term specific for
> if-statement?

I think they are, yes.  It's sort of too bad the spec doesn't have a grammar production specifically for them, that we could reuse.

> > +        // Otherwise immediately report an error.  Don't fall through and allow
> > +        // Parser::statement to handle this: it'll interpret this as an
> > +        // ExpressionStatement containing a FunctionExpression.
> 
> This comment will become obsolete after the change to Parser::statement,
> right?

Okay, sure.  Doesn't *need* to, but it can and will.

> anyway, I think this handling could be merged to Parser::statement later.

Yeah, will do.  (Also it's updated in this hunk locally, but then I overwrite it later, so it'll be as if it didn't exist in the end.)

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Add a test for 'let' as a label in strict and non-strict code

We don't correctly forbid 'let' as a label in strict mode right now.  Here's a test that detects this and exercises various relevant edge cases.

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Add a test for |yield| as identifier in various contexts

Failure of Parser::shouldParseLetDeclaration to acknowledge TOK_YIELD and YieldHandling means we're buggy about |let yield| in various places.  Here's a test of that.

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Make Parser::statement and Parser::statementListItem behave exactly as the spec productions do. In particular: Parser::statement no longer accepts function and lexical declarations, "let" is no longer a valid label in strict mode code, and "yield" is som

Sigh, this metastasized.  I did the original thing -- make Parser::statement{,ListItem} be pretty distinct.  Then I realized the old code (and what I then had) allowed 'let' as label in non-strict code (and in the "use strict"+ASI+let case), so I fixed that.  Then I realized the existing should-parse-let-decl test mishandled |yield|, so I fixed that.  Then there was dealing with followups from your earlier comments.

So, overall a good point, but this snowballed.  Hopefully it won't snowball further during review!

Comment 26

[Jeff Walden [:Waldo]]

Attachment: diff --minimal version of the previous patch

Maybe hg won't do this, but I can always copy the previous patch's Parser.cpp to the side, hg qpu, then do a |diff --minimal| against this patch's Parser.cpp.  Should be helpful for reviewing, I hope.  Putting a f? in place so you're sure to see it (I sometimes lose track of comments that aren't requests, when I'm reviewing).

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Implement static semantics restrictions on labeled functions when 'function' is first encountered after a label, not some weird time later

So actually the semantics in the prior version of this were just really broken.  :-(  I failed to understand some things about the code, and everything died.  This is a super-simpler version that's almost self-evidently correct, but it's different enough from the steaming pile you reviewed that really you should look again.  :-\

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 8785561 [details] [diff] [review]
diff --minimal version of the previous patch

Review of attachment 8785561 [details] [diff] [review]:
-----------------------------------------------------------------

::: /tmp/old.cpp
@@ +6529,4 @@
>  
> +        // Statement context forbids LexicalDeclaration.
> +        if ((next == TOK_LB || next == TOK_LC || next == TOK_NAME) &&
> +            tokenStream.currentName() == context->names().let)

Just in case the comments don't clarify it, this whole |if| *could* just be

if (next == TOK_LB && tokenStream.currentName() == context->names().let) {
    report(ParseError, false, null(), JSMSG_FORBIDDEN_AS_STATEMENT,
           "lexical declarations");
    return null();
}

and it'd be equally standards-compliant.  But it seemed so much nicer to see

js> if (1) let {} = {};
typein:1:7 SyntaxError: lexical declarations can't appear in single-statement context:
typein:1:7 if (1) let {} = {};
typein:1:7 .......^

than to see

js> if (1) let {} = {};
typein:1:11 SyntaxError: missing ; before statement:
typein:1:11 if (1) let {} = {};
typein:1:11 ...........^

and a same-line check to improve things seemed simple enough that I couldn't pass it up.

Comment 29

[Tooru Fujisawa [:arai]]

Comment on attachment 8785558 [details] [diff] [review]
Add a test for 'let' as a label in strict and non-strict code

empty :3

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Add a test for 'let' as a label in strict and non-strict code

The two XXX comments and explicit throw will be removed later -- they're just added because, where this patch sits in my queue right now, they're necessary for the test to not fail.

Comment 31

[Tooru Fujisawa [:arai]]

Comment on attachment 8785560 [details] [diff] [review]
Make Parser::statement and Parser::statementListItem behave exactly as the spec productions do.  In particular: Parser::statement no longer accepts function and lexical declarations, "let" is no longer a valid label in strict mode code, and "yield" is som

Review of attachment 8785560 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +6560,5 @@
> +            }
> +        }
> +
> +        // NOTE: It's unfortunately allowed to have a label named 'let' in
> +        //       non-strict code.

Comment 32

[Tooru Fujisawa [:arai]]

Comment on attachment 8785561 [details] [diff] [review]
diff --minimal version of the previous patch

Review of attachment 8785561 [details] [diff] [review]:
-----------------------------------------------------------------

thanks :)

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd3ac4f2793
Rename Parser::statements to Parser::statementList, the actual grammar term.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1875e8356bc
Split variable-statement parsing into its own parser function.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fc25ab8420b
Make Parser::labeledStatement use a new Parser::labeledItem function to parse its nested statement, anticipating when Parser::statement no longer parses FunctionDeclarations.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/660db63ca25e
Make the statement-like children of an |if(-else)| properly understand FunctionStatement.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/937f3feed15f
Add a test for 'let' as a label in strict and non-strict code.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed54ff909ef
Copy Parser::statement into an identical (but not-yet-called) Parser::statementListItem.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d266c0db461d
Convert a few existing calls to Parser::statement over to Parser::statementListItem (which still has identical functionality at present), before the two functions' behavior is made to differ.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd3461f216a
Make Parser::statement and Parser::statementListItem behave exactly as the spec productions do.  Particularly: function/lexical declarations are disallowed as Statements, "let" isn't a valid label in strict mode code, and "yield" is sometimes a permissible identifier in |let| declarations.  Also add various tests.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1a4a994a9d
Implement static semantics restrictions on labeled functions when 'function' is first encountered after a label, not some weird time later.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd9274acb82
Forbid GeneratorDeclaration as a child of LabelledStatement.  r=arai

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cbd3ac4f2793
https://hg.mozilla.org/mozilla-central/rev/c1875e8356bc
https://hg.mozilla.org/mozilla-central/rev/3fc25ab8420b
https://hg.mozilla.org/mozilla-central/rev/660db63ca25e
https://hg.mozilla.org/mozilla-central/rev/937f3feed15f
https://hg.mozilla.org/mozilla-central/rev/7ed54ff909ef
https://hg.mozilla.org/mozilla-central/rev/d266c0db461d
https://hg.mozilla.org/mozilla-central/rev/5bd3461f216a
https://hg.mozilla.org/mozilla-central/rev/da1a4a994a9d
https://hg.mozilla.org/mozilla-central/rev/bfd9274acb82