Remove duplicated code from comma-separated Expression parsing

This removes two bits of duplication:
  - Parsing of each AssignmentExpression, which previously was called
    first outside the loop and then inside the loop.
  - Parsing of arrow rest parameters, which previously was handled
    separately for the one-arg and N-arg cases.

The only change in behavior is in a few error messages.

Review-Url: https://codereview.chromium.org/2279363002
Cr-Commit-Position: refs/heads/master@{#39030}

src/messages.h

@@ -550,8 +550,6 @@ class ErrorUtils : public AllStatic {
   T(NoCatchOrFinally, "Missing catch or finally after try")                    \
   T(NotIsvar, "builtin %%IS_VAR: not a variable")                              \
   T(ParamAfterRest, "Rest parameter must be last formal parameter")            \
-  T(InvalidRestParameter,                                                      \
-    "Rest parameter must be an identifier or destructuring pattern")           \
   T(PushPastSafeLength,                                                        \
     "Pushing % elements on an array-like of length % "                         \
     "is disallowed, as the total surpasses 2**53-1")                           \

src/parsing/parser-base.h

@@ -1616,39 +1616,6 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParsePrimaryExpression(
                                           MessageTemplate::kUnexpectedToken,
                                           Token::String(Token::RPAREN));
         return factory()->NewEmptyParentheses(beg_pos);
-      } else if (Check(Token::ELLIPSIS)) {
-        // (...x)=>x.  The continuation that looks for the => is in
-        // ParseAssignmentExpression.
-        int ellipsis_pos = position();
-        int expr_pos = peek_position();
-        classifier->RecordExpressionError(scanner()->location(),
-                                          MessageTemplate::kUnexpectedToken,
-                                          Token::String(Token::ELLIPSIS));
-        classifier->RecordNonSimpleParameter();
-        ExpressionClassifier binding_classifier(this);
-        // TODO(adamk): The grammar for this is
-        // BindingIdentifier | BindingPattern, so
-        // ParseAssignmentExpression is overkill.
-        ExpressionT expr =
-            ParseAssignmentExpression(true, &binding_classifier, CHECK_OK);
-        // This already couldn't be an expression or a pattern, so the
-        // only remaining possibility is an arrow formal parameter list.
-        classifier->Accumulate(
-            &binding_classifier,
-            ExpressionClassifier::ArrowFormalParametersProduction);
-        if (!impl()->IsIdentifier(expr) && !IsValidPattern(expr)) {
-          classifier->RecordArrowFormalParametersError(
-              Scanner::Location(ellipsis_pos, scanner()->location().end_pos),
-              MessageTemplate::kInvalidRestParameter);
-        }
-        if (peek() == Token::COMMA) {
-          impl()->ReportMessageAt(scanner()->peek_location(),
-                                  MessageTemplate::kParamAfterRest);
-          *ok = false;
-          return impl()->EmptyExpression();
-        }
-        Expect(Token::RPAREN, CHECK_OK);
-        return factory()->NewSpread(expr, ellipsis_pos, expr_pos);
       }
       // Heuristically try to detect immediately called functions before
       // seeing the call parentheses.
@@ -1721,66 +1688,57 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseExpression(
   //   AssignmentExpression
   //   Expression ',' AssignmentExpression
 
-  ExpressionT result;
-
-  // No need to accumulate binding pattern-related errors, since
-  // an Expression can't be a binding pattern anyway.
-  static const unsigned kExpressionProductions =
-      ExpressionClassifier::AllProductions &
-      ~(ExpressionClassifier::BindingPatternProduction |
-        ExpressionClassifier::LetPatternProduction);
-  {
-    ExpressionClassifier binding_classifier(this);
-    result =
-        ParseAssignmentExpression(accept_IN, &binding_classifier, CHECK_OK);
-    classifier->Accumulate(&binding_classifier, kExpressionProductions);
-  }
-  bool is_simple_parameter_list = impl()->IsIdentifier(result);
-  bool seen_rest = false;
-  while (peek() == Token::COMMA) {
+  ExpressionT result = impl()->EmptyExpression();
+  while (true) {
     CheckNoTailCallExpressions(classifier, CHECK_OK);
-    if (seen_rest) {
-      // At this point the production can't possibly be valid, but we don't know
-      // which error to signal.
+    int comma_pos = position();
+    ExpressionClassifier binding_classifier(this);
+    ExpressionT right;
+    if (Check(Token::ELLIPSIS)) {
+      // 'x, y, ...z' in CoverParenthesizedExpressionAndArrowParameterList only
+      // as the formal parameters of'(x, y, ...z) => foo', and is not itself a
+      // valid expression.
+      binding_classifier.RecordExpressionError(
+          scanner()->location(), MessageTemplate::kUnexpectedToken,
+          Token::String(Token::ELLIPSIS));
+      int ellipsis_pos = position();
+      int pattern_pos = peek_position();
+      ExpressionT pattern =
+          ParsePrimaryExpression(&binding_classifier, CHECK_OK);
+      ValidateBindingPattern(&binding_classifier, CHECK_OK);
+      right = factory()->NewSpread(pattern, ellipsis_pos, pattern_pos);
+    } else {
+      right =
+          ParseAssignmentExpression(accept_IN, &binding_classifier, CHECK_OK);
+    }
+    // No need to accumulate binding pattern-related errors, since
+    // an Expression can't be a binding pattern anyway.
+    classifier->Accumulate(
+        &binding_classifier,
+        ExpressionClassifier::AllProductions &
+            ~(ExpressionClassifier::BindingPatternProduction |
+              ExpressionClassifier::LetPatternProduction));
+    if (!impl()->IsIdentifier(right)) classifier->RecordNonSimpleParameter();
+    if (impl()->IsEmptyExpression(result)) {
+      // First time through the loop.
+      result = right;
+    } else {
+      result =
+          factory()->NewBinaryOperation(Token::COMMA, result, right, comma_pos);
+    }
+
+    if (!Check(Token::COMMA)) break;
+
+    if (right->IsSpread()) {
       classifier->RecordArrowFormalParametersError(
-          scanner()->peek_location(), MessageTemplate::kParamAfterRest);
+          scanner()->location(), MessageTemplate::kParamAfterRest);
     }
-    Consume(Token::COMMA);
-    bool is_rest = false;
+
     if (allow_harmony_trailing_commas() && peek() == Token::RPAREN &&
         PeekAhead() == Token::ARROW) {
       // a trailing comma is allowed at the end of an arrow parameter list
       break;
-    } else if (peek() == Token::ELLIPSIS) {
-      // 'x, y, ...z' in CoverParenthesizedExpressionAndArrowParameterList only
-      // as the formal parameters of'(x, y, ...z) => foo', and is not itself a
-      // valid expression or binding pattern.
-      ExpressionUnexpectedToken(classifier);
-      BindingPatternUnexpectedToken(classifier);
-      Consume(Token::ELLIPSIS);
-      // TODO(adamk): If is_rest is true we don't need to parse an assignment
-      // expression, the grammar is BindingIdentifier | BindingPattern.
-      seen_rest = is_rest = true;
     }
-    int pos = position(), expr_pos = peek_position();
-    ExpressionClassifier binding_classifier(this);
-    ExpressionT right =
-        ParseAssignmentExpression(accept_IN, &binding_classifier, CHECK_OK);
-    classifier->Accumulate(&binding_classifier, kExpressionProductions);
-    if (is_rest) {
-      if (!impl()->IsIdentifier(right) && !IsValidPattern(right)) {
-        classifier->RecordArrowFormalParametersError(
-            Scanner::Location(pos, scanner()->location().end_pos),
-            MessageTemplate::kInvalidRestParameter);
-      }
-      right = factory()->NewSpread(right, pos, expr_pos);
-    }
-    is_simple_parameter_list =
-        is_simple_parameter_list && impl()->IsIdentifier(right);
-    result = factory()->NewBinaryOperation(Token::COMMA, result, right, pos);
-  }
-  if (!is_simple_parameter_list || seen_rest) {
-    classifier->RecordNonSimpleParameter();
   }
 
   return result;

src/parsing/parser.h

@@ -881,6 +881,10 @@ class Parser : public ParserBase<Parser> {
   }
   V8_INLINE static FunctionLiteral* EmptyFunctionLiteral() { return nullptr; }
 
+  V8_INLINE static bool IsEmptyExpression(Expression* expr) {
+    return expr == nullptr;
+  }
+
   // Used in error return values.
   V8_INLINE static ZoneList<Expression*>* NullExpressionList() {
     return nullptr;

src/parsing/preparser.h

@@ -123,10 +123,12 @@ class PreParserIdentifier {
 
 class PreParserExpression {
  public:
-  PreParserExpression() : code_(TypeField::encode(kExpression)) {}
+  PreParserExpression() : code_(TypeField::encode(kEmpty)) {}
+
+  static PreParserExpression Empty() { return PreParserExpression(); }
 
   static PreParserExpression Default() {
-    return PreParserExpression();
+    return PreParserExpression(TypeField::encode(kExpression));
   }
 
   static PreParserExpression Spread(PreParserExpression expression) {
@@ -206,6 +208,8 @@ class PreParserExpression {
         ExpressionTypeField::encode(kNoTemplateTagExpression));
   }
 
+  bool IsEmpty() const { return TypeField::decode(code_) == kEmpty; }
+
   bool IsIdentifier() const {
     return TypeField::decode(code_) == kIdentifierExpression;
   }
@@ -282,7 +286,7 @@ class PreParserExpression {
            ExpressionTypeField::decode(code_) == kNoTemplateTagExpression;
   }
 
-  bool IsSpreadExpression() const {
+  bool IsSpread() const {
     return TypeField::decode(code_) == kSpreadExpression;
   }
 
@@ -305,6 +309,7 @@ class PreParserExpression {
 
  private:
   enum Type {
+    kEmpty,
     kExpression,
     kIdentifierExpression,
     kStringLiteralExpression,
@@ -999,7 +1004,7 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserIdentifier::Default();
   }
   V8_INLINE static PreParserExpression EmptyExpression() {
-    return PreParserExpression::Default();
+    return PreParserExpression::Empty();
   }
   V8_INLINE static PreParserExpression EmptyLiteral() {
     return PreParserExpression::Default();
@@ -1011,6 +1016,10 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserExpression::Default();
   }
 
+  V8_INLINE static bool IsEmptyExpression(PreParserExpression expr) {
+    return expr.IsEmpty();
+  }
+
   V8_INLINE static PreParserExpressionList NullExpressionList() {
     return PreParserExpressionList();
   }

test/message/arrow-invalid-rest-2.out

-*%(basename)s:7: SyntaxError: Rest parameter must be an identifier or destructuring pattern
+*%(basename)s:7: SyntaxError: Unexpected token =
 var f = (a, ...x = 10) => x;
-            ^^^^^^^^^
-SyntaxError: Rest parameter must be an identifier or destructuring pattern
+                 ^
+SyntaxError: Unexpected token =

test/message/arrow-invalid-rest.out

-*%(basename)s:7: SyntaxError: Rest parameter must be an identifier or destructuring pattern
+*%(basename)s:7: SyntaxError: Unexpected token =
 var f = (...x = 10) => x;
-         ^^^^^^^^^
-SyntaxError: Rest parameter must be an identifier or destructuring pattern
+              ^
+SyntaxError: Unexpected token =

test/mjsunit/es8/syntactic-tail-call-parsing-sloppy.js

@@ -142,12 +142,6 @@ var SyntaxErrorTests = [
       { src: `()=>{ (1, 2, 3, continue f() ) => {} }`,
         err: `                ^^^^^^^^^^^^`,
       },
-      { src: `()=>{ (... continue f()) => {}  }`,
-        err: `           ^^^^^^^^^^^^`,
-      },
-      { src: `()=>{ (a, b, c, ... continue f() ) => {}  }`,
-        err: `                    ^^^^^^^^^^^^`,
-      },
       { src: `()=>{ return a <= continue f(); }`,
         err: `                  ^^^^^^^^^^^^`,
       },
@@ -333,6 +327,16 @@ var SyntaxErrorTests = [
       },
     ],
   },
+  { msg: "Unexpected token continue",
+    tests: [
+      { src: `()=>{ (... continue f()) => {}  }`,
+        err: `           ^^^^^^^^`,
+      },
+      { src: `()=>{ (a, b, c, ... continue f() ) => {}  }`,
+        err: `                    ^^^^^^^^`,
+      },
+    ],
+  },
   { msg: "Undefined label 'foo'",
     tests: [
       { src: `()=>{ continue  foo () ; }`,

test/mjsunit/es8/syntactic-tail-call-parsing.js

@@ -136,12 +136,6 @@ var SyntaxErrorTests = [
       { src: `()=>{ (1, 2, 3, continue f() ) => {} }`,
         err: `                ^^^^^^^^^^^^`,
       },
-      { src: `()=>{ (... continue f()) => {}  }`,
-        err: `           ^^^^^^^^^^^^`,
-      },
-      { src: `()=>{ (a, b, c, ... continue f() ) => {}  }`,
-        err: `                    ^^^^^^^^^^^^`,
-      },
       { src: `()=>{ return a <= continue f(); }`,
         err: `                  ^^^^^^^^^^^^`,
       },
@@ -295,6 +289,16 @@ var SyntaxErrorTests = [
       },
     ],
   },
+  { msg: "Unexpected token continue",
+    tests: [
+      { src: `()=>{ (... continue f()) => {}  }`,
+        err: `           ^^^^^^^^`,
+      },
+      { src: `()=>{ (a, b, c, ... continue f() ) => {}  }`,
+        err: `                    ^^^^^^^^`,
+      },
+    ],
+  },
   { msg: "Undefined label 'foo'",
     tests: [
       { src: `()=>{ continue  foo () ; }`,