1233249 - Refactor for-loop head parsing so that declaration-parsing code parses the entire head of for-in/of loops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

I was going to just do this for bug 449811 and bug 1069480, because by the time I'd done all this it was intertwingled with fixes for those things and very hard to extricate.  But shu tells me he independently wants this fixing, now, for annex B reasons.  So extract it out.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: WIP

Yeah, it's rather ponderously large.  I'd like to think it's, overall, an improvement on the existing code even so.

There are still various issues to sort through in this, like syntax-parsing being inconsistent with full-parsing.  But it does seem to fix bug 1232674 and a few currently-broken things, so it's most-ish of the way.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: WIP 2, possibly even ready to go, pending tests

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Ready to go

Yeah, it's a bit long.  Yeah, I probably could have refactored it a little less, in some places.  But honestly, there are enough special cases for the different forms of declaration, wrt for-loops, that I think the splitting-out of stuff is completely justified.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07c8bb770815&selectedJob=14737770

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Rebased

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8700483 [details] [diff] [review]
Rebased

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

A fine patch. Clearer declaration code *and* unifying forStatement parsing between the full and syntax parsers.

Would like to see renamings we discussed on IRC, onlyDeclaration -> initialDeclaration and a clearer name for declarationNameWithInitializer.

::: js/src/frontend/Parser.cpp
@@ +3562,5 @@
>  
>      ExclusiveContext* cx = parser->context;
> +
> +    // Most lexical declaration patterns can't bind the name 'let'.
> +    if (data->mustNotBindLet() && name == cx->names().let) {

Great, thanks for fixing this.

@@ +4387,5 @@
> +template <typename ParseHandler>
> +typename ParseHandler::Node
> +Parser<ParseHandler>::declarationPattern(Node decl, TokenKind tt, BindData<ParseHandler>* data,
> +                                         bool onlyDeclaration, YieldHandling yieldHandling,
> +                                         ParseNodeKind* forHeadKind,

Nit: forHeadKindOut, maybe?

@@ +4425,5 @@
> +                return null();
> +            }
> +
> +            if (!checkDestructuringPattern(data, pattern))
> +                return null();

This seems like it should get the same bindBeforeInitializer logic as below and elsewhere.

@@ +4455,5 @@
> +        // For for(;;) declarations, consistency with |for (;| parsing requires
> +        // that the ';' first be examined as Operand, even though absence of a
> +        // binary operator (examined with modifier None) terminated |init|.
> +        // For all other declarations, through ASI's infinite majesty, a next
> +        // token on a new line would begin an expression.

uh

@@ +4615,5 @@
> +
> +        bool constRequiringInitializer = handler.declarationIsConst(decl);
> +        if (onlyDeclaration && forHeadKind) {
> +            bool isForIn, isForOf;
> +            if (!matchInOrOf(&isForIn, &isForOf))

While you're here, I think it'd be easier for this to just take a ParseNodeKind* and return the for head kind directly.

@@ +4619,5 @@
> +            if (!matchInOrOf(&isForIn, &isForOf))
> +                return null();
> +
> +            if (isForIn || isForOf) {
> +                MOZ_ASSERT(isForIn != isForOf);

This seems like something matchInOrOf should assert, not its callers.

@@ +4636,5 @@
> +            report(ParseError, false, binding, JSMSG_BAD_CONST_DECL);
> +            return null();
> +        }
> +
> +        bool bindBeforeInitializer = handler.declarationIsVar(decl);

Note for followup: it'd be nice to refactor this bindBeforeInitializer stuff to a higher-order function that takes a lambda.

@@ +4665,5 @@
>   * context, and the let-initializer of a for-statement.
>   */
>  template <typename ParseHandler>
>  typename ParseHandler::Node
> +Parser<ParseHandler>::declaration(YieldHandling yieldHandling,

Should this really be named declarationList?

@@ +4913,5 @@
>       *
>       * See 8.1.1.1.6 and the note in 13.2.1.
>       */
>      ParseNodeKind kind = isConst ? PNK_CONST : PNK_LET;
> +    ParseNode* decl = declaration(yieldHandling, kind, CurrentLexicalStaticBlock(pc));

Existing nit: |isConst ? PNK_CONST : PNK_LET| could probably be inlined into the argument position and be clearer.

@@ +5658,5 @@
> +    // to Parser::declaration.
> +    if (tt == TOK_VAR) {
> +        tokenStream.consumeKnownToken(tt, TokenStream::Operand);
> +
> +        *forInitialPart = declaration(yieldHandling, PNK_VAR, nullptr, forHeadKind,

Comment for blockObj being nullptr here.

@@ +5729,5 @@
> +    MOZ_ASSERT(isForIn != isForOf);
> +
> +    // In a for-of loop, 'let' that starts the loop head is a |let| keyword,
> +    // per the [lookahead ≠ let] restriction on the LeftHandSideExpression
> +    // variant of such loops.  See ES6 13.7.

A small example would help here, like e.g., |for (let.x of [1])| that you told me on IRC.

@@ +5908,5 @@
>              return null();
>          }
>  
> +        // Look for an operand here, because |for (;| means we might have
> +        // already examined this semicolon with that modifier.

ughh

@@ +5929,5 @@
> +        }
> +
> +        MUST_MATCH_TOKEN_MOD(TOK_SEMI, mod, JSMSG_SEMI_AFTER_FOR_COND);
> +
> +

Nit: extra newline

@@ -5912,5 @@
> -    if (!statement(yieldHandling))
> -        return null();
> -
> -    return SyntaxParseHandler::NodeGeneric;
> -}

Huzzah unifying the two forStatement templates.

::: js/src/frontend/Parser.h
@@ +715,5 @@
> +                         VarContext varContext);
> +    bool declarationNameWithInitializer(Node decl, Node binding, Handle<PropertyName*> name,
> +                                        BindData<ParseHandler>* data, bool onlyDeclaration,
> +                                        YieldHandling yieldHandling, ParseNodeKind* forHeadKind,
> +                                        Node* forInOrOfExpression);

For this group of methods, could you comment on some general invariants on |initialDeclaration| and |forHeadKind|? i.e., that |initialDeclaration| is true iff it's the initial decl in a list, and forHeadKind is non-null if parsing a for-head.

::: js/src/frontend/SyntaxParseHandler.h
@@ +451,5 @@
> +        // parsing would *already* have aborted when it saw a destructuring
> +        // pattern.  So we can just say any old thing here, because the only
> +        // time we'll be wrong is a case that syntax parsing has already
> +        // rejected.  Use NodeUnparenthesizedName so the SyntaxParseHandler
> +        // Parser::cloneLeftHandSide can assert it sees only this.

uh... sure

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b834fb9e739

Comment 7

[Jeff Walden [:Waldo]]

One final minor change, not noted previously in this bug: I removed the VarHoisting extra parameter to the declaration stuff.  Everyone was passing HoistVars, so clearly it didn't make sense to have it at all.  A straightforward change, so didn't seem necessary to reraise it here for another review, try-run would do:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00abf7499825

(In reply to Shu-yu Guo [:shu] from comment #5)
> Nit: forHeadKindOut, maybe?

I was never much a fan of name-sigils, be they "p" or "out" or whatever.  Type system will enforce proper use, and in context it's a negligible speed bump to screw it up.

> @@ +4425,5 @@
> > +                return null();
> > +            }
> > +
> > +            if (!checkDestructuringPattern(data, pattern))
> > +                return null();
> 
> This seems like it should get the same bindBeforeInitializer logic as below
> and elsewhere.

Hmm.  Maybe.  Will investigate and alter, if needed, in subsequent patching.  Gonna pocket this as-is for now.

> > +            bool isForIn, isForOf;
> > +            if (!matchInOrOf(&isForIn, &isForOf))
> 
> While you're here, I think it'd be easier for this to just take a
> ParseNodeKind* and return the for head kind directly.

I distinctly remember having discussed this before in a separate bug.  There was some reason we came up with that this wasn't great.

I think it might have been that the method is lower-level than parse nodes -- it operates on tokens.  There's no TOK_OF, so it's hard to indicate the "of" case holding.  If you intermixed abstractions and returned a kind, you'd now have the problem of it being non-obvious to the caller how to test for the not-in/of case -- you'd have to know the PNK_FOR{IN,OF} kinds and test for both.  And really it all just gets messy at that point.

> This seems like something matchInOrOf should assert, not its callers.

Added.

> Note for followup: it'd be nice to refactor this bindBeforeInitializer stuff
> to a higher-order function that takes a lambda.

Noted.

> Should this really be named declarationList?

Seems plausible, now that you mention it.  Changed.

> > +        *forInitialPart = declaration(yieldHandling, PNK_VAR, nullptr, forHeadKind,
> 
> Comment for blockObj being nullptr here.

Hmm.  I mean, I suppose, but I always thought it was part of near-baseline knowledge to know that only lexical declarations used the static block.  |var|s being not lexical wouldn't use it, so of course passing nullptr would only make sense.  Added comments anyway.

> Huzzah unifying the two forStatement templates.

So much huzzah.  I basically had this refactoring all done -- except syntax parsing, which was totally unsynced.  Fortunately the two were close enough to unify with only a few handler-methods.  I would not have relished making the syntax-parsing specialization consistent with full-parsing.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b834fb9e739