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 466 matching lines @@
   ExpressionT result = ParseExpression(accept_IN, &classifier, CHECK_OK);
   Traits::RewriteNonPattern(&classifier, CHECK_OK);
   return result;
 }
 
 template <class Traits>
 typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseExpression(
     bool accept_IN, ExpressionClassifier* classifier, bool* ok) {
   // Expression ::
   //   AssignmentExpression
   //   Expression ',' AssignmentExpression

[jwolfe, 2016/06/22 18:04:41]

If I'm supposed to update the grammar comments, I don't know what to do about
this one.

[caitp (gmail), 2016/06/22 18:56:20]

The grammar that needs changing here is
CoverParenthesizedExpressionAndArrowParameterList, not this one. Don't worry
about this one.

 
   ExpressionT result = this->EmptyExpression();
   {
     ExpressionClassifier binding_classifier(this);
     result = this->ParseAssignmentExpression(accept_IN, &binding_classifier,
                                              CHECK_OK);
     classifier->Accumulate(&binding_classifier,
                            ExpressionClassifier::AllProductions);
   }
   bool is_simple_parameter_list = this->IsIdentifier(result);
   bool seen_rest = false;
   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

[caitp (gmail), 2016/06/22 18:56:20]

Is this approach using PeekAhead() faster than just using the peek() and
ExpressionClassifier? We should try both and measure

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

I have the same question :)

I'll be sad if this slows down the parser at all, actually.

+      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 426 matching lines @@
                                      literal_index,
                                      number_of_boilerplate_properties,
                                      pos);
 }
 
 template <class Traits>
 typename Traits::Type::ExpressionList ParserBase<Traits>::ParseArguments(
     Scanner::Location* first_spread_arg_loc, bool maybe_arrow,
     ExpressionClassifier* classifier, bool* ok) {
   // Arguments ::
   //   '(' (AssignmentExpression)*[','] ')'

[jwolfe, 2016/06/22 18:04:41]

Does this comment say that a trailing comma is already allowed?

[caitp (gmail), 2016/06/22 18:56:20]

I think this is just wrong, this doesn't even make sense as a YACC input. But
this line is very very old, so its origins are (to me) unknown, and long predate
trailing commas

 
   Scanner::Location spread_arg = Scanner::Location::invalid();
   typename Traits::Type::ExpressionList result =
       this->NewExpressionList(4, zone_);
   Expect(Token::LPAREN, CHECK_OK_CUSTOM(NullExpressionList));
   bool done = (peek() == Token::RPAREN);
   bool was_unspread = false;
   int unspread_sequences_count = 0;
   while (!done) {
     int start_pos = peek_position();
@@ skipping 23 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 996 matching lines @@
   //   FormalParameterList[?Yield, ?GeneratorParameter]
   //
   // 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]

[jwolfe, 2016/06/22 18:04:41]

Should I update this comment to the new grammar? Really, this function
implements two different grammars depending on the
allow_harmony_trailing_commas_in_parameters flag

[caitp (gmail), 2016/06/22 18:56:20]

Honestly I'd prefer the jeffmo proposal just refactored itself to be similar to
the existing grammar.

Jeffmo says he thinks it was necessary to "cleanly" explain the trailing comma
--- I think we can express it in the comment using the well-known terms of the
current grammar though.

 
   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));
 
-    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() &&

[caitp (gmail), 2016/06/22 18:56:20]

For a lot of these, I feel like a WebKit-ism of defining helper macros which
more clearly explain what this is doing would be good, like
`breakIfTrailingComma()` or something.

I don't think it really fits into the chromium code style though, so I am fine
with it as-is.

+          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