Closed Bug 1288459 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: anba, Assigned: Waldo)

References

Details

Attachments

(13 files, 3 obsolete files)

5.44 KB, patch
arai
: review+
Details | Diff | Splinter Review
8.62 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.01 KB, patch
arai
: review+
Details | Diff | Splinter Review
8.18 KB, patch
arai
: review+
Details | Diff | Splinter Review
4.80 KB, patch
arai
: review+
Details | Diff | Splinter Review
2.24 KB, patch
arai
: review+
Details | Diff | Splinter Review
4.27 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.03 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.92 KB, patch
arai
: review+
Details | Diff | Splinter Review
33.43 KB, patch
arai
: review+
Details | Diff | Splinter Review
17.61 KB, patch
arai
: feedback+
Details | Diff | Splinter Review
4.32 KB, patch
arai
: review+
Details | Diff | Splinter Review
1.51 KB, patch
arai
: review+
Details | Diff | Splinter Review
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
Waldo, all yours. :)
Assignee: nobody → jwalden+bmo
Flags: needinfo?(jwalden+bmo)
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.
Depends on: 1263355
Flags: needinfo?(jwalden+bmo)
(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.
I have a series of patches (sans tests for now) that fix this.  Waiting on bug 1263355 to land to post them.
Seeing as in a moment it's going to be used in two places.
Attachment #8785105 - Flags: review?(arai.unmht)
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.
Attachment #8785106 - Flags: review?(arai.unmht)
This doesn't yet change any behavior, because Parser::statement{,ListItem} are identical.  But once they change, this will start doing the right thing.
Attachment #8785107 - Flags: review?(arai.unmht)
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.
Attachment #8785109 - Flags: review?(arai.unmht)
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.
Attachment #8785110 - Flags: review?(arai.unmht)
Attachment #8785103 - Flags: review?(arai.unmht) → review+
Attachment #8785104 - Flags: review?(arai.unmht) → review+
Attachment #8785105 - Flags: review?(arai.unmht) → review+
Attachment #8785106 - Flags: review?(arai.unmht) → review+
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.
Attachment #8785107 - Flags: review?(arai.unmht) → review+
Attachment #8785108 - Flags: review?(arai.unmht) → review+
(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?
(In reply to André Bargull from comment #15)
> This should fix bug 1204026, correct?

Yes.
Blocks: 1204026
Status: NEW → ASSIGNED
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)
Attachment #8785109 - Flags: review?(arai.unmht) → review+
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
Attachment #8785109 - Flags: review+
This goes atop the "Convert a few existing calls" patch and seems to do the if-else trick.
Attachment #8785195 - Flags: review?(arai.unmht)
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 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.
Attachment #8785195 - Flags: review?(arai.unmht) → review+
Attachment #8785110 - Flags: review?(arai.unmht) → review+
Attachment #8785111 - Flags: review?(arai.unmht) → review+
(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.)
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.
Attachment #8785558 - Flags: review?(arai.unmht)
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.
Attachment #8785559 - Flags: review?(arai.unmht)
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!
Attachment #8785560 - Flags: review?(arai.unmht)
Attachment #8785109 - Attachment is obsolete: true
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).
Attachment #8785561 - Flags: feedback?(arai.unmht)
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.  :-\
Attachment #8785562 - Flags: review?(arai.unmht)
Attachment #8785110 - Attachment is obsolete: true
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 on attachment 8785558 [details] [diff] [review]
Add a test for 'let' as a label in strict and non-strict code

empty :3
Attachment #8785558 - Flags: review?(arai.unmht)
Attachment #8785559 - Flags: review?(arai.unmht) → review+
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.
Attachment #8785594 - Flags: review?(arai.unmht)
Attachment #8785558 - Attachment is obsolete: true
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.
Attachment #8785560 - Flags: review?(arai.unmht) → review+
Comment on attachment 8785561 [details] [diff] [review]
diff --minimal version of the previous patch

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

thanks :)
Attachment #8785561 - Flags: feedback?(arai.unmht) → feedback+
Attachment #8785594 - Flags: review?(arai.unmht) → review+
Attachment #8785562 - Flags: review?(arai.unmht) → review+
Pushed by jwalden@mit.edu:
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
You need to log in before you can comment on or make changes to this bug.