Allow trailing commas in function parameter lists

src/parsing/parser-base.h

Index: src/parsing/parser-base.h
diff --git a/src/parsing/parser-base.h b/src/parsing/parser-base.h
index dc08f5c5803923dd9ee29bf92228af10d5f0efe3..90c68d509d148753a9ca615a0aef55ac7a234b9f 100644
--- a/src/parsing/parser-base.h
+++ b/src/parsing/parser-base.h
@@ -199,7 +199,8 @@ class ParserBase : public Traits {
         allow_harmony_function_name_(false),
         allow_harmony_function_sent_(false),
         allow_harmony_async_await_(false),
-        allow_harmony_restrictive_generators_(false) {}
+        allow_harmony_restrictive_generators_(false),
+        allow_harmony_trailing_commas_in_parameters_(false) {}
 
 #define ALLOW_ACCESSORS(name)                           \
   bool allow_##name() const { return allow_##name##_; } \
@@ -221,6 +222,7 @@ class ParserBase : public Traits {
   ALLOW_ACCESSORS(harmony_function_sent);
   ALLOW_ACCESSORS(harmony_async_await);
   ALLOW_ACCESSORS(harmony_restrictive_generators);
+  ALLOW_ACCESSORS(harmony_trailing_commas_in_parameters);
   SCANNER_ACCESSORS(harmony_exponentiation_operator);
 
 #undef SCANNER_ACCESSORS
@@ -1192,6 +1194,7 @@ class ParserBase : public Traits {
   bool allow_harmony_function_sent_;
   bool allow_harmony_async_await_;
   bool allow_harmony_restrictive_generators_;
+  bool allow_harmony_trailing_commas_in_parameters_;
 };
 
 template <class Traits>
@@ -1700,7 +1703,11 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseExpression(
     }
     Consume(Token::COMMA);
     bool is_rest = false;
-    if (peek() == Token::ELLIPSIS) {
+    if (allow_harmony_trailing_commas_in_parameters() &&
+        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.
@@ -2191,6 +2198,11 @@ typename Traits::Type::ExpressionList ParserBase<Traits>::ParseArguments(
     done = (peek() != Token::COMMA);
     if (!done) {
       Next();
+      if (allow_harmony_trailing_commas_in_parameters() &&
+          peek() == Token::RPAREN) {
+        // allow trailing comma
+        done = true;
+      }
     }
   }
   Scanner::Location location = scanner_->location();
@@ -3195,24 +3207,21 @@ void ParserBase<Traits>::ParseFormalParameter(
 template <class Traits>
 void ParserBase<Traits>::ParseFormalParameterList(
     FormalParametersT* parameters, ExpressionClassifier* classifier, bool* ok) {
-  // FormalParameters[Yield,GeneratorParameter] :
+  // FormalParameters[Yield] :
   //   [empty]
-  //   FormalParameterList[?Yield, ?GeneratorParameter]
-  //
-  // FormalParameterList[Yield,GeneratorParameter] :
   //   FunctionRestParameter[?Yield]
-  //   FormalsList[?Yield, ?GeneratorParameter]
-  //   FormalsList[?Yield, ?GeneratorParameter] , FunctionRestParameter[?Yield]
+  //   FormalParameterList[?Yield]
+  //   FormalParameterList[?Yield] ,
+  //   FormalParameterList[?Yield] , FunctionRestParameter[?Yield]
   //
-  // FormalsList[Yield,GeneratorParameter] :
-  //   FormalParameter[?Yield, ?GeneratorParameter]
-  //   FormalsList[?Yield, ?GeneratorParameter] ,
-  //     FormalParameter[?Yield,?GeneratorParameter]
+  // FormalParameterList[Yield] :
+  //   FormalParameter[?Yield]
+  //   FormalParameterList[?Yield] , FormalParameter[?Yield]
 
   DCHECK_EQ(0, parameters->Arity());
 
   if (peek() != Token::RPAREN) {
-    do {
+    while (true) {
       if (parameters->Arity() > Code::kMaxArguments) {
         ReportMessage(MessageTemplate::kTooManyParameters);
         *ok = false;
@@ -3221,16 +3230,23 @@ void ParserBase<Traits>::ParseFormalParameterList(
       parameters->has_rest = Check(Token::ELLIPSIS);
       ParseFormalParameter(parameters, classifier, ok);
       if (!*ok) return;
-    } while (!parameters->has_rest && Check(Token::COMMA));

[adamk, 2016/06/28 00:39:47]

Can't you keep this same do/while form and just add:

... && (!allow_harmony_trailing_commas_in_parameters() || peek() !=
Token::RPAREN)

Or did you rewrite it this way for clarity? I personally find the multiple
breaks a bit confusing.

[jwolfe, 2016/06/28 21:04:16]

I rewrote it for clarity. I find multiple breaks much easier to understand than
a complicated while expression. So I guess this is just a matter of opinion.

It makes sense to me to make minimal changes to the source, so I think your way
has an advantage over mine. I will change it back if you think that's best.

[adamk, 2016/06/28 21:10:51]

I'm OK with the multiple breaks too, mostly wanted to make sure I wasn't missing
something about the logic.

 
-    if (parameters->has_rest) {
-      parameters->is_simple = false;
-      classifier->RecordNonSimpleParameter();
-      if (peek() == Token::COMMA) {
-        ReportMessageAt(scanner()->peek_location(),
-                      MessageTemplate::kParamAfterRest);
-        *ok = false;
-        return;
+      if (parameters->has_rest) {
+        parameters->is_simple = false;
+        classifier->RecordNonSimpleParameter();
+        if (peek() == Token::COMMA) {
+          ReportMessageAt(scanner()->peek_location(),
+                          MessageTemplate::kParamAfterRest);
+          *ok = false;
+          return;
+        }
+        break;
+      }
+      if (!Check(Token::COMMA)) break;
+      if (allow_harmony_trailing_commas_in_parameters() &&
+          peek() == Token::RPAREN) {
+        // allow the trailing comma
+        break;
       }
     }
   }