Factor formal argument parsing into ParserBase

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

Originally landed in these three CLs:

  https://codereview.chromium.org/1078093002
  https://codereview.chromium.org/1083623002
  https://codereview.chromium.org/1083953002

These were rolled out due to a performance regression on CodeLoad.  This
patchset will fix that by avoiding creation of a DuplicateFinder in the
full parser.

R=marja@chromium.org
BUG=
LOG=N

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

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

src/messages.js

@@ -164,7 +164,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

@@ -3734,8 +3734,7 @@ Handle<FixedArray> CompileTimeValue::GetElements(Handle<FixedArray> value) {
 
 bool CheckAndDeclareArrowParameter(ParserTraits* traits, Expression* expression,
                                    Scope* scope, int* num_params,
-                                   Scanner::Location* undefined_loc,
-                                   Scanner::Location* dupe_loc) {
+                                   FormalParameterErrorLocations* locs) {
   // Case for empty parameter lists:
   //   () => ...
   if (expression == NULL) return true;
@@ -3754,21 +3753,24 @@ bool CheckAndDeclareArrowParameter(ParserTraits* traits, Expression* expression,
     if (traits->IsEvalOrArguments(raw_name) ||
         traits->IsFutureStrictReserved(raw_name))
       return false;
-    if (traits->IsUndefined(raw_name) && !undefined_loc->IsValid()) {
-      *undefined_loc = Scanner::Location(
+    if (traits->IsUndefined(raw_name) && !locs->undefined.IsValid()) {
+      locs->undefined = Scanner::Location(
           expression->position(), expression->position() + raw_name->length());
     }
-    if (scope->IsDeclared(raw_name)) {
-      *dupe_loc = Scanner::Location(
-          expression->position(), expression->position() + raw_name->length());
-      return false;
-    }
 
     // When the variable was seen, it was recorded as unresolved in the outer
     // scope. But it's really not unresolved.
     scope->outer_scope()->RemoveUnresolved(expression->AsVariableProxy());
 
-    scope->DeclareParameter(raw_name, VAR);
+    bool is_rest = false;
+    bool is_duplicate = false;
+    scope->DeclareParameter(raw_name, VAR, is_rest, &is_duplicate);
+    if (is_duplicate) {
+      locs->duplicate = Scanner::Location(
+          expression->position(), expression->position() + raw_name->length());
+      return false;
+    }
+
     ++(*num_params);
     return true;
   }
@@ -3782,9 +3784,9 @@ bool CheckAndDeclareArrowParameter(ParserTraits* traits, Expression* expression,
       return false;
 
     return CheckAndDeclareArrowParameter(traits, binop->left(), scope,
-                                         num_params, undefined_loc, dupe_loc) &&
+                                         num_params, locs) &&
            CheckAndDeclareArrowParameter(traits, binop->right(), scope,
-                                         num_params, undefined_loc, dupe_loc);
+                                         num_params, locs);
   }
 
   // Any other kind of expression is not a valid parameter list.
@@ -3793,15 +3795,15 @@ bool CheckAndDeclareArrowParameter(ParserTraits* traits, Expression* expression,
 
 
 int ParserTraits::DeclareArrowParametersFromExpression(
-    Expression* expression, Scope* scope, Scanner::Location* undefined_loc,
-    Scanner::Location* dupe_loc, bool* ok) {
+    Expression* expression, Scope* scope, FormalParameterErrorLocations* locs,
+    bool* ok) {
   int num_params = 0;
   // Always reset the flag: It only needs to be set for the first expression
   // parsed as arrow function parameter list, because only top-level functions
   // are parsed lazily.
   parser_->parsing_lazy_arrow_parameters_ = false;
-  *ok = CheckAndDeclareArrowParameter(this, expression, scope, &num_params,
-                                      undefined_loc, dupe_loc);
+  *ok =
+      CheckAndDeclareArrowParameter(this, expression, scope, &num_params, locs);
   return num_params;
 }
 
@@ -3875,8 +3877,7 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
   int materialized_literal_count = -1;
   int expected_property_count = -1;
   int handler_count = 0;
-  FunctionLiteral::ParameterFlag duplicate_parameters =
-      FunctionLiteral::kNoDuplicateParameters;
+  FormalParameterErrorLocations error_locs;
   FunctionLiteral::IsParenthesizedFlag parenthesized = parenthesized_function_
       ? FunctionLiteral::kIsParenthesized
       : FunctionLiteral::kNotParenthesized;
@@ -3901,77 +3902,17 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
       function_state.set_generator_object_variable(temp);
     }
 
-    //  FormalParameterList ::
-    //    '(' (Identifier)*[','] ')'
+    bool has_rest = false;
     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.
-    Scanner::Location eval_args_loc = Scanner::Location::invalid();
-    Scanner::Location dupe_loc = Scanner::Location::invalid();
-    Scanner::Location reserved_loc = Scanner::Location::invalid();
-
-    // 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);
-      }
-
-      const AstRawString* param_name =
-          ParseIdentifierOrStrictReservedWord(&is_strict_reserved, 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();
-      }
-
-      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
-        // conservative approximation necessary to account for parameters
-        // 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);
-      }
-    }
+    int start_position = scanner()->location().beg_pos;
+    scope_->set_start_position(start_position);
+    num_parameters =
+        ParseFormalParameterList(scope, &error_locs, &has_rest, CHECK_OK);
     Expect(Token::RPAREN, CHECK_OK);
+    int formals_end_position = scanner()->location().end_pos;
+
+    CheckArityRestrictions(num_parameters, arity_restriction, start_position,
+                           formals_end_position, CHECK_OK);
 
     Expect(Token::LBRACE, CHECK_OK);
 
@@ -4053,10 +3994,9 @@ 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);
-    CheckFunctionParameterNames(language_mode(), use_strict_params,
-                                eval_args_loc, undefined_loc, dupe_loc,
-                                reserved_loc, CHECK_OK);
+    const bool use_strict_params = has_rest || IsConciseMethod(kind);
+    CheckFunctionParameterNames(language_mode(), use_strict_params, error_locs,
+                                CHECK_OK);
 
     if (is_strict(language_mode())) {
       CheckStrictOctalLiteral(scope->start_position(), scope->end_position(),
@@ -4075,6 +4015,10 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     }
   }
 
+  FunctionLiteral::ParameterFlag duplicate_parameters =
+      error_locs.duplicate.IsValid() ? FunctionLiteral::kHasDuplicateParameters
+                                     : FunctionLiteral::kNoDuplicateParameters;
+
   FunctionLiteral* function_literal = factory()->NewFunctionLiteral(
       function_name, ast_value_factory(), scope, body,
       materialized_literal_count, expected_property_count, handler_count,

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 Scope FormalParameterScope;
     typedef ZoneList<v8::internal::Statement*>* StatementList;
 
     // For constructing objects returned by the traversing functions.
@@ -705,6 +707,7 @@ class ParserTraits {
   static ZoneList<Expression*>* NullExpressionList() {
     return NULL;
   }
+  static const AstRawString* EmptyFormalParameter() { return NULL; }
 
   // Non-NULL empty string.
   V8_INLINE const AstRawString* EmptyIdentifierString();
@@ -745,10 +748,22 @@ class ParserTraits {
 
   // Utility functions
   int DeclareArrowParametersFromExpression(Expression* expression, Scope* scope,
-                                           Scanner::Location* undefined_loc,
-                                           Scanner::Location* dupe_loc,
+                                           FormalParameterErrorLocations* locs,
                                            bool* ok);
 
+  bool DeclareFormalParameter(Scope* scope, const AstRawString* name,
+                              bool is_rest) {
+    bool is_duplicate = false;
+    Variable* var = scope->DeclareParameter(name, VAR, is_rest, &is_duplicate);
+    if (is_sloppy(scope->language_mode())) {
+      // TODO(sigurds) Mark every parameter as maybe assigned. This is a
+      // conservative approximation necessary to account for parameters
+      // that are assigned via the arguments array.
+      var->set_maybe_assigned();
+    }
+    return is_duplicate;
+  }
+
   // Temporary glue; these functions will move to ParserBase.
   Expression* ParseV8Intrinsic(bool* ok);
   FunctionLiteral* ParseFunctionLiteral(

src/preparser.cc

@@ -926,62 +926,23 @@ 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.
-  Scanner::Location eval_args_loc = Scanner::Location::invalid();
-  Scanner::Location dupe_loc = Scanner::Location::invalid();
-  Scanner::Location reserved_loc = Scanner::Location::invalid();
-
-  // Similarly for strong mode.
-  Scanner::Location undefined_loc = Scanner::Location::invalid();
+  FormalParameterErrorLocations error_locs;
 
   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;
+  function_scope->set_start_position(start_position);
+  int num_parameters;
+  {
+    DuplicateFinder duplicate_finder(scanner()->unicode_cache());
+    num_parameters = ParseFormalParameterList(&duplicate_finder, &error_locs,
+                                              &is_rest, CHECK_OK);
   }
   Expect(Token::RPAREN, CHECK_OK);
+  int formals_end_position = scanner()->location().end_pos;
+
+  CheckArityRestrictions(num_parameters, arity_restriction, start_position,
+                         formals_end_position, CHECK_OK);
 
   // See Parser::ParseFunctionLiteral for more information about lazy parsing
   // and lazy compilation.
@@ -1002,8 +963,8 @@ PreParser::Expression PreParser::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);
-  CheckFunctionParameterNames(language_mode(), use_strict_params, eval_args_loc,
-                              undefined_loc, dupe_loc, reserved_loc, CHECK_OK);
+  CheckFunctionParameterNames(language_mode(), use_strict_params, error_locs,
+                              CHECK_OK);
 
   if (is_strict(language_mode())) {
     int end_position = scanner()->location().end_pos;

src/preparser.h

@@ -17,6 +17,26 @@
 namespace v8 {
 namespace internal {
 
+
+// When parsing the formal parameters of a function, we usually don't yet know
+// if the function will be strict, so we cannot yet produce errors for
+// parameter names or duplicates.  Instead, we remember the locations of these
+// errors if they occur and produce the errors later.
+class FormalParameterErrorLocations BASE_EMBEDDED {
+ public:
+  FormalParameterErrorLocations()
+      : eval_or_arguments(Scanner::Location::invalid()),
+        undefined(Scanner::Location::invalid()),
+        duplicate(Scanner::Location::invalid()),
+        reserved(Scanner::Location::invalid()) {}
+
+  Scanner::Location eval_or_arguments;
+  Scanner::Location undefined;
+  Scanner::Location duplicate;
+  Scanner::Location reserved;
+};
+
+
 // Common base class shared between parser and pre-parser. Traits encapsulate
 // the differences between Parser and PreParser:
 
@@ -51,6 +71,8 @@ namespace internal {
 //     typedef Literal;
 //     typedef ExpressionList;
 //     typedef PropertyList;
+//     typedef FormalParameter;
+//     typedef FormalParameterScope;
 //     // For constructing objects returned by the traversing functions.
 //     typedef Factory;
 //   };
@@ -63,6 +85,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::FormalParameterScope FormalParameterScopeT;
   typedef typename Traits::Type::FunctionLiteral FunctionLiteralT;
   typedef typename Traits::Type::Literal LiteralT;
   typedef typename Traits::Type::ObjectLiteralProperty ObjectLiteralPropertyT;
@@ -495,31 +519,28 @@ class ParserBase : public Traits {
   // after parsing the function, since the function can declare itself strict.
   void CheckFunctionParameterNames(LanguageMode language_mode,
                                    bool strict_params,
-                                   const Scanner::Location& eval_args_loc,
-                                   const Scanner::Location& undefined_loc,
-                                   const Scanner::Location& dupe_loc,
-                                   const Scanner::Location& reserved_loc,
+                                   const FormalParameterErrorLocations& locs,
                                    bool* ok) {
     if (is_sloppy(language_mode) && !strict_params) return;
-    if (is_strict(language_mode) && eval_args_loc.IsValid()) {
-      Traits::ReportMessageAt(eval_args_loc, "strict_eval_arguments");
+    if (is_strict(language_mode) && locs.eval_or_arguments.IsValid()) {
+      Traits::ReportMessageAt(locs.eval_or_arguments, "strict_eval_arguments");
       *ok = false;
       return;
     }
-    if (is_strong(language_mode) && undefined_loc.IsValid()) {
-      Traits::ReportMessageAt(undefined_loc, "strong_undefined");
+    if (is_strong(language_mode) && locs.undefined.IsValid()) {
+      Traits::ReportMessageAt(locs.undefined, "strong_undefined");
       *ok = false;
       return;
     }
     // TODO(arv): When we add support for destructuring in setters we also need
     // to check for duplicate names.
-    if (dupe_loc.IsValid()) {
-      Traits::ReportMessageAt(dupe_loc, "strict_param_dupe");
+    if (locs.duplicate.IsValid()) {
+      Traits::ReportMessageAt(locs.duplicate, "strict_param_dupe");
       *ok = false;
       return;
     }
-    if (reserved_loc.IsValid()) {
-      Traits::ReportMessageAt(reserved_loc, "unexpected_strict_reserved");
+    if (locs.reserved.IsValid()) {
+      Traits::ReportMessageAt(locs.reserved, "unexpected_strict_reserved");
       *ok = false;
       return;
     }
@@ -606,6 +627,16 @@ class ParserBase : public Traits {
   void AddTemplateExpression(ExpressionT);
   ExpressionT ParseSuperExpression(bool is_new, bool* ok);
 
+  void ParseFormalParameter(FormalParameterScopeT* scope,
+                            FormalParameterErrorLocations* locs, bool is_rest,
+                            bool* ok);
+  int ParseFormalParameterList(FormalParameterScopeT* scope,
+                               FormalParameterErrorLocations* locs,
+                               bool* has_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.
@@ -911,7 +942,7 @@ class PreParserExpression {
     return IsIdentifier() || IsProperty();
   }
 
-  bool IsValidArrowParamList(Scanner::Location* undefined_loc) const {
+  bool IsValidArrowParamList(FormalParameterErrorLocations* locs) const {
     ValidArrowParam valid = ValidateArrowParams();
     if (ParenthesizationField::decode(code_) == kMultiParenthesizedExpression) {
       return false;
@@ -921,7 +952,7 @@ class PreParserExpression {
     } else if (valid == kInvalidStrongArrowParam) {
       // Return true for now regardless of strong mode for compatibility with
       // parser.
-      *undefined_loc = Scanner::Location();
+      locs->undefined = Scanner::Location();
       return true;
     } else {
       return false;
@@ -1040,20 +1071,24 @@ 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;
+
+
 class PreParserStatement {
  public:
   static PreParserStatement Default() {
@@ -1118,16 +1153,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 {
@@ -1292,6 +1318,8 @@ class PreParserTraits {
     typedef PreParserExpression Literal;
     typedef PreParserExpressionList ExpressionList;
     typedef PreParserExpressionList PropertyList;
+    typedef PreParserIdentifier FormalParameter;
+    typedef DuplicateFinder FormalParameterScope;
     typedef PreParserStatementList StatementList;
 
     // For constructing objects returned by the traversing functions.
@@ -1526,13 +1554,12 @@ class PreParserTraits {
                          FunctionKind kind, bool* ok);
 
   // Utility functions
-  int DeclareArrowParametersFromExpression(PreParserExpression expression,
-                                           Scope* scope,
-                                           Scanner::Location* undefined_loc,
-                                           Scanner::Location* dupe_loc,
-                                           bool* ok) {
-    // TODO(aperez): Detect duplicated identifiers in paramlists.
-    *ok = expression.IsValidArrowParamList(undefined_loc);
+  V8_INLINE int DeclareArrowParametersFromExpression(
+      PreParserExpression expression, Scope* scope,
+      FormalParameterErrorLocations* locs, bool* ok) {
+    // TODO(wingo): Detect duplicated identifiers in paramlists.  Detect eval or
+    // arguments.  Detect reserved words.
+    *ok = expression.IsValidArrowParamList(locs);
     return 0;
   }
 
@@ -1560,6 +1587,10 @@ class PreParserTraits {
     return !tag.IsNoTemplateTag();
   }
 
+  V8_INLINE bool DeclareFormalParameter(DuplicateFinder* scope,
+                                        PreParserIdentifier param,
+                                        bool is_rest);
+
   void CheckConflictingVarDeclarations(Scope* scope, bool* ok) {}
 
   // Temporary glue; these functions will move to ParserBase.
@@ -1748,6 +1779,13 @@ PreParserExpression PreParserTraits::SpreadCallNew(PreParserExpression function,
 }
 
 
+bool PreParserTraits::DeclareFormalParameter(
+    DuplicateFinder* duplicate_finder, PreParserIdentifier current_identifier,
+    bool is_rest) {
+  return pre_parser_->scanner()->FindSymbol(duplicate_finder, 1) != 0;
+}
+
+
 PreParserStatementList PreParser::ParseEagerFunctionBody(
     PreParserIdentifier function_name, int pos, Variable* fvar,
     Token::Value fvar_init_op, FunctionKind kind, bool* ok) {
@@ -3040,6 +3078,101 @@ ParserBase<Traits>::ParseMemberExpressionContinuation(ExpressionT expression,
 }
 
 
+template <class Traits>
+void ParserBase<Traits>::ParseFormalParameter(
+    FormalParameterScopeT* scope, FormalParameterErrorLocations* locs,
+    bool is_rest, bool* ok) {
+  // FormalParameter[Yield,GeneratorParameter] :
+  //   BindingElement[?Yield, ?GeneratorParameter]
+  bool is_strict_reserved;
+  IdentifierT name =
+      ParseIdentifierOrStrictReservedWord(&is_strict_reserved, ok);
+  if (!*ok) return;
+
+  // Store locations for possible future error reports.
+  if (!locs->eval_or_arguments.IsValid() && this->IsEvalOrArguments(name)) {
+    locs->eval_or_arguments = scanner()->location();
+  }
+  if (!locs->undefined.IsValid() && this->IsUndefined(name)) {
+    locs->undefined = scanner()->location();
+  }
+  if (!locs->reserved.IsValid() && is_strict_reserved) {
+    locs->reserved = scanner()->location();
+  }
+  bool was_declared = Traits::DeclareFormalParameter(scope, name, is_rest);
+  if (!locs->duplicate.IsValid() && was_declared) {
+    locs->duplicate = scanner()->location();
+  }
+}
+
+
+template <class Traits>
+int ParserBase<Traits>::ParseFormalParameterList(
+    FormalParameterScopeT* scope, FormalParameterErrorLocations* locs,
+    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]
+
+  int parameter_count = 0;
+
+  if (peek() != Token::RPAREN) {
+    do {
+      if (++parameter_count > Code::kMaxArguments) {
+        ReportMessage("too_many_parameters");
+        *ok = false;
+        return -1;
+      }
+      *is_rest = allow_harmony_rest_params() && Check(Token::ELLIPSIS);
+      ParseFormalParameter(scope, locs, *is_rest, ok);
+      if (!*ok) return -1;
+    } while (!*is_rest && Check(Token::COMMA));
+
+    if (*is_rest && peek() == Token::COMMA) {
+      ReportMessageAt(scanner()->peek_location(), "param_after_rest");
+      *ok = false;
+      return -1;
+    }
+  }
+
+  return parameter_count;
+}
+
+
+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,
@@ -3066,11 +3199,9 @@ ParserBase<Traits>::ParseArrowFunctionLiteral(int start_pos,
     typename Traits::Type::Factory function_factory(ast_value_factory());
     FunctionState function_state(&function_state_, &scope_, scope,
                                  kArrowFunction, &function_factory);
-    Scanner::Location undefined_loc = Scanner::Location::invalid();
-    Scanner::Location dupe_loc = Scanner::Location::invalid();
-    // TODO(arv): Pass in eval_args_loc and reserved_loc here.
+    FormalParameterErrorLocations error_locs;
     num_parameters = Traits::DeclareArrowParametersFromExpression(
-        params_ast, scope_, &undefined_loc, &dupe_loc, ok);
+        params_ast, scope_, &error_locs, ok);
     if (!*ok) {
       ReportMessageAt(
           Scanner::Location(start_pos, scanner()->location().beg_pos),
@@ -3078,10 +3209,10 @@ ParserBase<Traits>::ParseArrowFunctionLiteral(int start_pos,
       return this->EmptyExpression();
     }
 
-    if (undefined_loc.IsValid()) {
+    if (error_locs.undefined.IsValid()) {
       // Workaround for preparser not keeping track of positions.
-      undefined_loc = Scanner::Location(start_pos,
-                                        scanner()->location().end_pos);
+      error_locs.undefined =
+          Scanner::Location(start_pos, scanner()->location().end_pos);
     }
     if (num_parameters > Code::kMaxArguments) {
       ReportMessageAt(Scanner::Location(params_ast->position(), position()),
@@ -3127,15 +3258,13 @@ ParserBase<Traits>::ParseArrowFunctionLiteral(int start_pos,
     scope->set_start_position(start_pos);
     scope->set_end_position(scanner()->location().end_pos);
 
-    // Arrow function *parameter lists* are always checked as in strict mode.
-    // TODO(arv): eval_args_loc and reserved_loc needs to be set by
-    // DeclareArrowParametersFromExpression.
-    Scanner::Location eval_args_loc = Scanner::Location::invalid();
-    Scanner::Location reserved_loc = Scanner::Location::invalid();
+    // Arrow function formal parameters are parsed as StrictFormalParameterList,
+    // which is not the same as "parameters of a strict function"; it only means
+    // that duplicates are not allowed.  Of course, the arrow function may
+    // itself be strict as well.
     const bool use_strict_params = true;
     this->CheckFunctionParameterNames(language_mode(), use_strict_params,
-                                      eval_args_loc, undefined_loc, dupe_loc,
-                                      reserved_loc, CHECK_OK);
+                                      error_locs, CHECK_OK);
 
     // Validate strict mode.
     if (is_strict(language_mode())) {

src/scopes.cc

@@ -452,7 +452,7 @@ Variable* Scope::Lookup(const AstRawString* name) {
 
 
 Variable* Scope::DeclareParameter(const AstRawString* name, VariableMode mode,
-                                  bool is_rest) {
+                                  bool is_rest, bool* is_duplicate) {
   DCHECK(!already_resolved());
   DCHECK(is_function_scope());
   Variable* var = variables_.Declare(this, name, mode, Variable::NORMAL,
@@ -462,6 +462,8 @@ Variable* Scope::DeclareParameter(const AstRawString* name, VariableMode mode,
     rest_parameter_ = var;
     rest_index_ = num_parameters();
   }
+  // TODO(wingo): Avoid O(n^2) check.
+  *is_duplicate = IsDeclaredParameter(name);
   params_.Add(var, zone());
   return var;
 }

src/scopes.h

@@ -125,7 +125,7 @@ class Scope: public ZoneObject {
   // parameters the rightmost one 'wins'.  However, the implementation
   // expects all parameters to be declared and from left to right.
   Variable* DeclareParameter(const AstRawString* name, VariableMode mode,
-                             bool is_rest = false);
+                             bool is_rest, bool* is_duplicate);
 
   // Declare a local variable in this scope. If the variable has been
   // declared before, the previously declared variable is returned.

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

+// 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

+*%(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

+// 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

+*%(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

+// 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

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

test/message/strict-formal-parameters.js

+// 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

+*%(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