1206980 - Separate MatchOrInsertSemicolon into 2 functions for after expression and non-expression.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Currently MatchOrInsertSemicolon receives modifier argument, but it would be better to have 2 functions for Operand and None case, with explicit name for each situation.

ASI happens in following situations (where MatchOrInsertSemicolon is called):

  modifier is None, ends with expression
   * function declaration with expression closure
   * var declaration
   * let declaration
   * export declaration with var
   * export declaration with default, without function or class
   * export declaration with default, function expression (handled in functionStmt)
   * expression statement
   * return statement with expression
   * throw statement

  modifier is Operand, ends with non-expression
   * import declaration, which ends with module name string
   * export declaration with from (including {...} and *), which ends with module name string (bug 1206750)
   * export declaration without from, peeked token for |from| is Operand
   * continue statement, which ends with optional label identifier, peeked token for label is Operand
   * break statement, which ends with optional label identifier, peeked token for label is Operand
   * return statement without expression, peeked token for expression is Operand
   * debugger statement

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Separate MatchOrInsertSemicolon into 2 functions for after expression and non-expression.

Added MatchOrInsertSemicolonAfterExpression and MatchOrInsertSemicolonAfterNonExpression, each for TokenStream::None and TokenStream::Operand.  Both of them call MatchOrInsertSemicolon internally.
Might be better to rename MatchOrInsertSemicolon to something else now? (or any other way to hide it?)

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8665981 [details] [diff] [review]
Separate MatchOrInsertSemicolon into 2 functions for after expression and non-expression.

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

::: js/src/frontend/Parser.cpp
@@ +1469,2 @@
>  static bool
> +MatchOrInsertSemicolon(TokenStream& ts, TokenStream::Modifier modifier)

Tack a "Helper" onto the name so people are less likely to call it?  That'll also break wrongly-rebased in-progress patches.

@@ +4995,5 @@
>              if (!binding)
>                  return null();
>              kid = assignExpr(InAllowed, YieldIsKeyword);
>              if (kid) {
> +                if (!MatchOrInsertSemicolonAfterExpression(tokenStream))

This looks (preexistingly) wrong.  Shouldn't it be

    kid = assignExpr(...);
    if (!kid || !Match...)
        return null();

Probably fix in a followup micro-rev (r=me), for sanity.

Comment 3

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> @@ +4995,5 @@
> >              if (!binding)
> >                  return null();
> >              kid = assignExpr(InAllowed, YieldIsKeyword);
> >              if (kid) {
> > +                if (!MatchOrInsertSemicolonAfterExpression(tokenStream))
> 
> This looks (preexistingly) wrong.  Shouldn't it be
> 
>     kid = assignExpr(...);
>     if (!kid || !Match...)
>         return null();
> 
> Probably fix in a followup micro-rev (r=me), for sanity.

I feel it's still a little confusing, as |if (!kid) return null();| also exists after the |switch|.
Maybe we should move it into each |case| in |switch| like following?

        switch (tt) {
          case TOK_FUNCTION:
            kid = functionStmt(YieldIsKeyword, AllowDefaultName);
            if (!kid)
                return null();
            break;
          case TOK_CLASS:
            kid = classDefinition(YieldIsKeyword, ClassStatement, AllowDefaultName);
            if (!kid)
                return null();
            break;
          default:
            tokenStream.ungetToken();
            RootedPropertyName name(context, context->names().starDefaultStar);
            binding = makeInitializedLexicalBinding(name, true, pos());
            if (!binding)
                return null();
            kid = assignExpr(InAllowed, YieldIsKeyword);
            if (!kid)
                return null();
            if (!MatchOrInsertSemicolonAfterExpression(tokenStream))
                return null();
            break;
        }

Comment 4

[Tooru Fujisawa [:arai]]

will file a separated bug for it, for clarity.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10194aec7255

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10194aec7255