Allow trailing commas in function parameter lists

src/parsing/parser-base.h

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef V8_PARSING_PARSER_BASE_H
 #define V8_PARSING_PARSER_BASE_H
 
 #include "src/ast/scopes.h"
 #include "src/bailout-reason.h"
 #include "src/base/hashmap.h"
@@ skipping 181 matching lines @@
         stack_overflow_(false),
         allow_lazy_(false),
         allow_natives_(false),
         allow_tailcalls_(false),
         allow_harmony_restrictive_declarations_(false),
         allow_harmony_do_expressions_(false),
         allow_harmony_for_in_(false),
         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##_; } \
   void set_allow_##name(bool allow) { allow_##name##_ = allow; }
 
 #define SCANNER_ACCESSORS(name)                                  \
   bool allow_##name() const { return scanner_->allow_##name(); } \
   void set_allow_##name(bool allow) {                            \
     return scanner_->set_allow_##name(allow);                    \
   }
 
   ALLOW_ACCESSORS(lazy);
   ALLOW_ACCESSORS(natives);
   ALLOW_ACCESSORS(tailcalls);
   ALLOW_ACCESSORS(harmony_restrictive_declarations);
   ALLOW_ACCESSORS(harmony_do_expressions);
   ALLOW_ACCESSORS(harmony_for_in);
   ALLOW_ACCESSORS(harmony_function_name);
   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
 #undef ALLOW_ACCESSORS
 
   uintptr_t stack_limit() const { return stack_limit_; }
 
  protected:
   enum AllowRestrictedIdentifiers {
     kAllowRestrictedIdentifiers,
@@ skipping 951 matching lines @@
   bool allow_lazy_;
   bool allow_natives_;
   bool allow_tailcalls_;
   bool allow_harmony_restrictive_declarations_;
   bool allow_harmony_do_expressions_;
   bool allow_harmony_for_in_;
   bool allow_harmony_function_name_;
   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>
 ParserBase<Traits>::FunctionState::FunctionState(
     FunctionState** function_state_stack, Scope** scope_stack, Scope* scope,
     FunctionKind kind, typename Traits::Type::Factory* factory)
     : next_materialized_literal_index_(0),
       expected_property_count_(0),
       this_location_(Scanner::Location::invalid()),
       return_location_(Scanner::Location::invalid()),
@@ skipping 488 matching lines @@
   while (peek() == Token::COMMA) {
     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.
       classifier->RecordArrowFormalParametersError(
           scanner()->peek_location(), MessageTemplate::kParamAfterRest);
     }
     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.
       ExpressionUnexpectedToken(classifier);
       BindingPatternUnexpectedToken(classifier);
       Consume(Token::ELLIPSIS);
       seen_rest = is_rest = true;
     }
     int pos = position(), expr_pos = peek_position();
     ExpressionClassifier binding_classifier(this);
@@ skipping 470 matching lines @@
     }
 
     if (result->length() > Code::kMaxArguments) {
       ReportMessage(MessageTemplate::kTooManyArguments);
       *ok = false;
       return this->NullExpressionList();
     }
     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();
   if (Token::RPAREN != Next()) {
     ReportMessageAt(location, MessageTemplate::kUnterminatedArgList);
     *ok = false;
     return this->NullExpressionList();
   }
   *first_spread_arg_loc = spread_arg;
 
@@ skipping 984 matching lines @@
   }
 
   Traits::AddFormalParameter(parameters, pattern, initializer,
                              scanner()->location().end_pos, is_rest);
 }
 
 
 template <class Traits>
 void ParserBase<Traits>::ParseFormalParameterList(
     FormalParametersT* parameters, ExpressionClassifier* classifier, bool* ok) {
-  // FormalParameters[Yield,GeneratorParameter] :
+  // FormalParameters[Yield] :
   //   [empty]
-  //   FormalParameterList[?Yield, ?GeneratorParameter]
+  //   FunctionRestParameter[?Yield]
+  //   FormalParameterList[?Yield]
+  //   FormalParameterList[?Yield] ,
+  //   FormalParameterList[?Yield] , FunctionRestParameter[?Yield]
   //
-  // FormalParameterList[Yield,GeneratorParameter] :
-  //   FunctionRestParameter[?Yield]
-  //   FormalsList[?Yield, ?GeneratorParameter]
-  //   FormalsList[?Yield, ?GeneratorParameter] , 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;
         return;
       }
       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;
       }
     }
   }
 
   for (int i = 0; i < parameters->Arity(); ++i) {
     auto parameter = parameters->at(i);
     Traits::DeclareFormalParameter(parameters->scope, parameter, classifier);
   }
 }
 
@@ skipping 383 matching lines @@
     has_seen_constructor_ = true;
     return;
   }
 }
 
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_PARSING_PARSER_BASE_H