Factor formal argument parsing into ParserBase

This commit is a precursor to making lazy arrow function parsing use
similar logic to function(){} argument parsing.

R=arv@chromium.org
BUG=4020
LOG=N

Review URL: https://codereview.chromium.org/1078093002

Cr-Commit-Position: refs/heads/master@{#27773}

src/messages.js

@@ -201,7 +201,9 @@ var kMessages = {
   array_not_subclassable:        ["Subclassing Arrays is not currently supported."],
   for_in_loop_initializer:       ["for-in loop variable declaration may not have an initializer."],
   for_of_loop_initializer:       ["for-of loop variable declaration may not have an initializer."],
-  for_inof_loop_multi_bindings:  ["Invalid left-hand side in ", "%0", " loop: Must have a single binding."]
+  for_inof_loop_multi_bindings:  ["Invalid left-hand side in ", "%0", " loop: Must have a single binding."],
+  bad_getter_arity:              ["Getter must not have any formal parameters."],
+  bad_setter_arity:              ["Setter must have exactly one formal parameter."]
 };
 
 

src/parser.cc

@@ -3873,11 +3873,6 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
       function_state.set_generator_object_variable(temp);
     }
 
-    //  FormalParameterList ::
-    //    '(' (Identifier)*[','] ')'
-    Expect(Token::LPAREN, CHECK_OK);
-    scope->set_start_position(scanner()->location().beg_pos);
-
     // We don't yet know if the function will be strict, so we cannot yet
     // produce errors for parameter names or duplicates. However, we remember
     // the locations of these errors if they occur and produce the errors later.
@@ -3888,36 +3883,28 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     // Similarly for strong mode.
     Scanner::Location undefined_loc = Scanner::Location::invalid();
 
-    bool is_rest = false;
-    bool done = arity_restriction == FunctionLiteral::GETTER_ARITY ||
-        (peek() == Token::RPAREN &&
-         arity_restriction != FunctionLiteral::SETTER_ARITY);
-    while (!done) {
-      bool is_strict_reserved = false;
-      is_rest = peek() == Token::ELLIPSIS && allow_harmony_rest_params();
-      if (is_rest) {
-        Consume(Token::ELLIPSIS);
-      }
+    bool has_rest = false;
+    Expect(Token::LPAREN, CHECK_OK);
+    int start_position = scanner()->location().beg_pos;
+    ZoneList<const AstRawString*>* params =
+        ParseFormalParameterList(&eval_args_loc, &undefined_loc, &dupe_loc,
+                                 &reserved_loc, &has_rest, CHECK_OK);
+    Expect(Token::RPAREN, CHECK_OK);
+    int formals_end_position = scanner()->location().end_pos;
 
-      const AstRawString* param_name =
-          ParseIdentifierOrStrictReservedWord(&is_strict_reserved, CHECK_OK);
+    CheckArityRestrictions(params->length(), arity_restriction, start_position,
+                           formals_end_position, CHECK_OK);
 
-      // Store locations for possible future error reports.
-      if (!eval_args_loc.IsValid() && IsEvalOrArguments(param_name)) {
-        eval_args_loc = scanner()->location();
-      }
-      if (!undefined_loc.IsValid() && IsUndefined(param_name)) {
-        undefined_loc = scanner()->location();
-      }
-      if (!reserved_loc.IsValid() && is_strict_reserved) {
-        reserved_loc = scanner()->location();
-      }
-      if (!dupe_loc.IsValid() &&
-          scope_->IsDeclaredParameter(param_name)) {
-        duplicate_parameters = FunctionLiteral::kHasDuplicateParameters;
-        dupe_loc = scanner()->location();
-      }
+    scope->set_start_position(start_position);
 
+    num_parameters = params->length();
+    if (dupe_loc.IsValid()) {
+      duplicate_parameters = FunctionLiteral::kHasDuplicateParameters;
+    }
+
+    for (int i = 0; i < params->length(); i++) {
+      const AstRawString* param_name = params->at(i);
+      int is_rest = has_rest && i == params->length() - 1;
       Variable* var = scope_->DeclareParameter(param_name, VAR, is_rest);
       if (is_sloppy(scope->language_mode())) {
         // TODO(sigurds) Mark every parameter as maybe assigned. This is a
@@ -3925,25 +3912,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
         // that are assigned via the arguments array.
         var->set_maybe_assigned();
       }
-
-      num_parameters++;
-      if (num_parameters > Code::kMaxArguments) {
-        ReportMessage("too_many_parameters");
-        *ok = false;
-        return NULL;
-      }
-      if (arity_restriction == FunctionLiteral::SETTER_ARITY) break;
-      done = (peek() == Token::RPAREN);
-      if (!done) {
-        if (is_rest) {
-          ReportMessageAt(scanner()->peek_location(), "param_after_rest");
-          *ok = false;
-          return NULL;
-        }
-        Expect(Token::COMMA, CHECK_OK);
-      }
     }
-    Expect(Token::RPAREN, CHECK_OK);
 
     Expect(Token::LBRACE, CHECK_OK);
 
@@ -4025,7 +3994,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     CheckFunctionName(language_mode(), kind, function_name,
                       name_is_strict_reserved, function_name_location,
                       CHECK_OK);
-    const bool use_strict_params = is_rest || IsConciseMethod(kind);
+    const bool use_strict_params = has_rest || IsConciseMethod(kind);
     CheckFunctionParameterNames(language_mode(), use_strict_params,
                                 eval_args_loc, undefined_loc, dupe_loc,
                                 reserved_loc, CHECK_OK);

src/parser.h

@@ -557,6 +557,8 @@ class ParserTraits {
     typedef ObjectLiteral::Property* ObjectLiteralProperty;
     typedef ZoneList<v8::internal::Expression*>* ExpressionList;
     typedef ZoneList<ObjectLiteral::Property*>* PropertyList;
+    typedef const v8::internal::AstRawString* FormalParameter;
+    typedef ZoneList<const v8::internal::AstRawString*>* FormalParameterList;
     typedef ZoneList<v8::internal::Statement*>* StatementList;
 
     // For constructing objects returned by the traversing functions.
@@ -705,6 +707,10 @@ class ParserTraits {
   static ZoneList<Expression*>* NullExpressionList() {
     return NULL;
   }
+  static const AstRawString* EmptyFormalParameter() { return NULL; }
+  static ZoneList<const AstRawString*>* NullFormalParameterList() {
+    return NULL;
+  }
 
   // Non-NULL empty string.
   V8_INLINE const AstRawString* EmptyIdentifierString();
@@ -740,6 +746,10 @@ class ParserTraits {
   ZoneList<v8::internal::Statement*>* NewStatementList(int size, Zone* zone) {
     return new(zone) ZoneList<v8::internal::Statement*>(size, zone);
   }
+  ZoneList<const v8::internal::AstRawString*>* NewFormalParameterList(
+      int size, Zone* zone) {
+    return new (zone) ZoneList<const v8::internal::AstRawString*>(size, zone);
+  }
   V8_INLINE Scope* NewScope(Scope* parent_scope, ScopeType scope_type,
                             FunctionKind kind = kNormalFunction);
 

src/preparser.cc

@@ -908,11 +908,6 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   PreParserFactory factory(NULL);
   FunctionState function_state(&function_state_, &scope_, function_scope, kind,
                                &factory);
-  //  FormalParameterList ::
-  //    '(' (Identifier)*[','] ')'
-  Expect(Token::LPAREN, CHECK_OK);
-  int start_position = position();
-  DuplicateFinder duplicate_finder(scanner()->unicode_cache());
   // We don't yet know if the function will be strict, so we cannot yet produce
   // errors for parameter names or duplicates. However, we remember the
   // locations of these errors if they occur and produce the errors later.
@@ -924,46 +919,17 @@ PreParser::Expression PreParser::ParseFunctionLiteral(
   Scanner::Location undefined_loc = Scanner::Location::invalid();
 
   bool is_rest = false;
-  bool done = arity_restriction == FunctionLiteral::GETTER_ARITY ||
-      (peek() == Token::RPAREN &&
-       arity_restriction != FunctionLiteral::SETTER_ARITY);
-  while (!done) {
-    bool is_strict_reserved = false;
-    is_rest = peek() == Token::ELLIPSIS && allow_harmony_rest_params();
-    if (is_rest) {
-      Consume(Token::ELLIPSIS);
-    }
-
-    Identifier param_name =
-        ParseIdentifierOrStrictReservedWord(&is_strict_reserved, CHECK_OK);
-    if (!eval_args_loc.IsValid() && param_name.IsEvalOrArguments()) {
-      eval_args_loc = scanner()->location();
-    }
-    if (!undefined_loc.IsValid() && param_name.IsUndefined()) {
-      undefined_loc = scanner()->location();
-    }
-    if (!reserved_loc.IsValid() && is_strict_reserved) {
-      reserved_loc = scanner()->location();
-    }
-
-    int prev_value = scanner()->FindSymbol(&duplicate_finder, 1);
-
-    if (!dupe_loc.IsValid() && prev_value != 0) {
-      dupe_loc = scanner()->location();
-    }
-
-    if (arity_restriction == FunctionLiteral::SETTER_ARITY) break;
-    done = (peek() == Token::RPAREN);
-    if (!done) {
-      if (is_rest) {
-        ReportMessageAt(scanner()->peek_location(), "param_after_rest");
-        *ok = false;
-        return Expression::Default();
-      }
-      Expect(Token::COMMA, CHECK_OK);
-    }
-  }
+  Expect(Token::LPAREN, CHECK_OK);
+  int start_position = scanner()->location().beg_pos;
+  PreParserFormalParameterList params =
+      ParseFormalParameterList(&eval_args_loc, &undefined_loc, &dupe_loc,
+                               &reserved_loc, &is_rest, CHECK_OK);
   Expect(Token::RPAREN, CHECK_OK);
+  int formals_end_position = scanner()->location().end_pos;
+
+  CheckArityRestrictions(params->length(), arity_restriction, start_position,
+                         formals_end_position, ok);
+  if (!*ok) return Expression::Default();
 
   // See Parser::ParseFunctionLiteral for more information about lazy parsing
   // and lazy compilation.

src/preparser.h

@@ -51,6 +51,8 @@ namespace internal {
 //     typedef Literal;
 //     typedef ExpressionList;
 //     typedef PropertyList;
+//     typedef FormalParameter;
+//     typedef FormalParameterList;
 //     // For constructing objects returned by the traversing functions.
 //     typedef Factory;
 //   };
@@ -63,6 +65,8 @@ class ParserBase : public Traits {
   // Shorten type names defined by Traits.
   typedef typename Traits::Type::Expression ExpressionT;
   typedef typename Traits::Type::Identifier IdentifierT;
+  typedef typename Traits::Type::FormalParameter FormalParameterT;
+  typedef typename Traits::Type::FormalParameterList FormalParameterListT;
   typedef typename Traits::Type::FunctionLiteral FunctionLiteralT;
   typedef typename Traits::Type::Literal LiteralT;
   typedef typename Traits::Type::ObjectLiteralProperty ObjectLiteralPropertyT;
@@ -606,6 +610,20 @@ class ParserBase : public Traits {
   void AddTemplateExpression(ExpressionT);
   ExpressionT ParseSuperExpression(bool is_new, bool* ok);
 
+  FormalParameterT ParseFormalParameter(DuplicateFinder* duplicate_finder,
+                                        Scanner::Location* eval_args_error_loc,
+                                        Scanner::Location* undefined_error_loc,
+                                        Scanner::Location* dupe_error_loc,
+                                        Scanner::Location* reserved_error_loc,
+                                        bool* ok);
+  FormalParameterListT ParseFormalParameterList(
+      Scanner::Location* eval_args_error_loc,
+      Scanner::Location* undefined_error_loc, Scanner::Location* dupe_error_loc,
+      Scanner::Location* reserved_error_loc, bool* is_rest, bool* ok);
+  void CheckArityRestrictions(
+      int param_count, FunctionLiteral::ArityRestriction arity_restriction,
+      int formals_start_pos, int formals_end_pos, bool* ok);
+
   // Checks if the expression is a valid reference expression (e.g., on the
   // left-hand side of assignments). Although ruled out by ECMA as early errors,
   // we allow calls for web compatibility and rewrite them to a runtime throw.
@@ -1040,20 +1058,25 @@ class PreParserExpression {
 };
 
 
-// PreParserExpressionList doesn't actually store the expressions because
-// PreParser doesn't need to.
-class PreParserExpressionList {
+// The pre-parser doesn't need to build lists of expressions, identifiers, or
+// the like.
+template <typename T>
+class PreParserList {
  public:
   // These functions make list->Add(some_expression) work (and do nothing).
-  PreParserExpressionList() : length_(0) {}
-  PreParserExpressionList* operator->() { return this; }
-  void Add(PreParserExpression, void*) { ++length_; }
+  PreParserList() : length_(0) {}
+  PreParserList* operator->() { return this; }
+  void Add(T, void*) { ++length_; }
   int length() const { return length_; }
  private:
   int length_;
 };
 
 
+typedef PreParserList<PreParserExpression> PreParserExpressionList;
+typedef PreParserList<PreParserIdentifier> PreParserFormalParameterList;
+
+
 class PreParserStatement {
  public:
   static PreParserStatement Default() {
@@ -1109,16 +1132,7 @@ class PreParserStatement {
 };
 
 
-
-// PreParserStatementList doesn't actually store the statements because
-// the PreParser does not need them.
-class PreParserStatementList {
- public:
-  // These functions make list->Add(some_expression) work as no-ops.
-  PreParserStatementList() {}
-  PreParserStatementList* operator->() { return this; }
-  void Add(PreParserStatement, void*) {}
-};
+typedef PreParserList<PreParserStatement> PreParserStatementList;
 
 
 class PreParserFactory {
@@ -1283,6 +1297,8 @@ class PreParserTraits {
     typedef PreParserExpression Literal;
     typedef PreParserExpressionList ExpressionList;
     typedef PreParserExpressionList PropertyList;
+    typedef PreParserIdentifier FormalParameter;
+    typedef PreParserFormalParameterList FormalParameterList;
     typedef PreParserStatementList StatementList;
 
     // For constructing objects returned by the traversing functions.
@@ -1441,6 +1457,12 @@ class PreParserTraits {
   static PreParserExpressionList NullExpressionList() {
     return PreParserExpressionList();
   }
+  static PreParserIdentifier EmptyFormalParameter() {
+    return PreParserIdentifier::Default();
+  }
+  static PreParserFormalParameterList NullFormalParameterList() {
+    return PreParserFormalParameterList();
+  }
 
   // Odd-ball literal creators.
   static PreParserExpression GetLiteralTheHole(int position,
@@ -1505,6 +1527,11 @@ class PreParserTraits {
     return PreParserExpressionList();
   }
 
+  static PreParserFormalParameterList NewFormalParameterList(int size,
+                                                             Zone* zone) {
+    return PreParserFormalParameterList();
+  }
+
   V8_INLINE void SkipLazyFunctionBody(PreParserIdentifier function_name,
                                       int* materialized_literal_count,
                                       int* expected_property_count, bool* ok) {
@@ -3025,6 +3052,113 @@ ParserBase<Traits>::ParseMemberExpressionContinuation(ExpressionT expression,
 }
 
 
+template <class Traits>
+typename ParserBase<Traits>::FormalParameterT
+ParserBase<Traits>::ParseFormalParameter(DuplicateFinder* duplicate_finder,
+                                         Scanner::Location* eval_args_error_loc,
+                                         Scanner::Location* undefined_error_loc,
+                                         Scanner::Location* dupe_error_loc,
+                                         Scanner::Location* reserved_error_loc,
+                                         bool* ok) {
+  // FormalParameter[Yield,GeneratorParameter] :
+  //   BindingElement[?Yield, ?GeneratorParameter]
+  bool is_strict_reserved;
+  IdentifierT name =
+      ParseIdentifierOrStrictReservedWord(&is_strict_reserved, ok);
+  if (!*ok) return this->EmptyFormalParameter();
+
+  // Store locations for possible future error reports.
+  if (!eval_args_error_loc->IsValid() && this->IsEvalOrArguments(name)) {
+    *eval_args_error_loc = scanner()->location();
+  }
+  if (!undefined_error_loc->IsValid() && this->IsUndefined(name)) {
+    *undefined_error_loc = scanner()->location();
+  }
+  if (!reserved_error_loc->IsValid() && is_strict_reserved) {
+    *reserved_error_loc = scanner()->location();
+  }
+  if (!dupe_error_loc->IsValid()) {
+    int prev_value = scanner()->FindSymbol(duplicate_finder, 1);
+    if (prev_value != 0) *dupe_error_loc = scanner()->location();
+  }
+
+  return name;
+}
+
+
+template <class Traits>
+typename ParserBase<Traits>::FormalParameterListT
+ParserBase<Traits>::ParseFormalParameterList(
+    Scanner::Location* eval_args_error_loc,
+    Scanner::Location* undefined_error_loc, Scanner::Location* dupe_error_loc,
+    Scanner::Location* reserved_error_loc, bool* is_rest, bool* ok) {
+  // FormalParameters[Yield,GeneratorParameter] :
+  //   [empty]
+  //   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]
+
+  FormalParameterListT result = this->NewFormalParameterList(4, zone_);
+  DuplicateFinder duplicate_finder(scanner()->unicode_cache());
+
+  if (peek() != Token::RPAREN) {
+    do {
+      *is_rest = allow_harmony_rest_params() && Check(Token::ELLIPSIS);
+      FormalParameterT param = ParseFormalParameter(
+          &duplicate_finder, eval_args_error_loc, undefined_error_loc,
+          dupe_error_loc, reserved_error_loc, ok);
+      if (!*ok) return this->NullFormalParameterList();
+      result->Add(param, zone());
+      if (result->length() > Code::kMaxArguments) {
+        ReportMessage("too_many_parameters");
+        *ok = false;
+        return this->NullFormalParameterList();
+      }
+    } while (!*is_rest && Check(Token::COMMA));
+  }
+
+  if (is_rest && peek() == Token::COMMA) {
+    ReportMessageAt(scanner()->peek_location(), "param_after_rest");
+    *ok = false;
+    return this->NullFormalParameterList();
+  }
+
+  return result;
+}
+
+
+template <class Traits>
+void ParserBase<Traits>::CheckArityRestrictions(
+    int param_count, FunctionLiteral::ArityRestriction arity_restriction,
+    int formals_start_pos, int formals_end_pos, bool* ok) {
+  switch (arity_restriction) {
+    case FunctionLiteral::GETTER_ARITY:
+      if (param_count != 0) {
+        ReportMessageAt(Scanner::Location(formals_start_pos, formals_end_pos),
+                        "bad_getter_arity");
+        *ok = false;
+      }
+      break;
+    case FunctionLiteral::SETTER_ARITY:
+      if (param_count != 1) {
+        ReportMessageAt(Scanner::Location(formals_start_pos, formals_end_pos),
+                        "bad_setter_arity");
+        *ok = false;
+      }
+      break;
+    default:
+      break;
+  }
+}
+
 template <class Traits>
 typename ParserBase<Traits>::ExpressionT
 ParserBase<Traits>::ParseArrowFunctionLiteral(int start_pos,

test/message/formal-parameters-bad-rest.js

@@ -0,0 +1,7 @@
+// Copyright 2015 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.
+//
+// Flags: --harmony-rest-parameters
+
+function foo(...b, a) { return a }

test/message/formal-parameters-bad-rest.out

@@ -0,0 +1,4 @@
+*%(basename)s:7: SyntaxError: Rest parameter must be last formal parameter
+function foo(...b, a) { return a }
+                 ^
+SyntaxError: Rest parameter must be last formal parameter

test/message/formal-parameters-strict-body.js

@@ -0,0 +1,5 @@
+// Copyright 2015 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.
+
+function foo(b, eval) { "use strict"; return b }

test/message/formal-parameters-strict-body.out

@@ -0,0 +1,4 @@
+*%(basename)s:5: SyntaxError: Unexpected eval or arguments in strict mode
+function foo(b, eval) { "use strict"; return b }
+                ^^^^
+SyntaxError: Unexpected eval or arguments in strict mode

test/message/formal-parameters-trailing-comma.js

@@ -0,0 +1,5 @@
+// Copyright 2015 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.
+
+function foo(b, a, a,) { return a }

test/message/formal-parameters-trailing-comma.out

@@ -0,0 +1,4 @@
+*%(basename)s:5: SyntaxError: Unexpected token )
+function foo(b, a, a,) { return a }
+                     ^
+SyntaxError: Unexpected token )

test/message/strict-formal-parameters.js

@@ -0,0 +1,6 @@
+// Copyright 2015 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.
+
+"use strict";
+function foo(b, a, a, d) { return a }

test/message/strict-formal-parameters.out

@@ -0,0 +1,4 @@
+*%(basename)s:6: SyntaxError: Strict mode function may not have duplicate parameter names
+function foo(b, a, a, d) { return a }
+                   ^
+SyntaxError: Strict mode function may not have duplicate parameter names