Revert "Factor formal argument parsing into ParserBase"

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 5f794c23c999a740ad1e0f0c9bb2c60286bf8a6e..53aa2ee2462ba2b55f4d150df5118d3a711097cf 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -787,7 +787,16 @@ Expression* ParserTraits::ExpressionFromIdentifier(const AstRawString* name,
                                                    Scope* scope,
                                                    AstNodeFactory* factory) {
   if (parser_->fni_ != NULL) parser_->fni_->PushVariableName(name);
-  return scope->NewUnresolved(factory, name, start_position, end_position);
+
+  // Arrow function parameters are parsed as an expression. When
+  // parsing lazily, it is enough to create a VariableProxy in order
+  // for Traits::DeclareArrowParametersFromExpression() to be able to
+  // pick the names of the parameters.
+  return parser_->parsing_lazy_arrow_parameters_
+             ? factory->NewVariableProxy(name, Variable::NORMAL, start_position,
+                                         end_position)
+             : scope->NewUnresolved(factory, name, start_position,
+                                    end_position);
 }
 
 
@@ -852,6 +861,7 @@ Parser::Parser(ParseInfo* info)
       target_stack_(NULL),
       compile_options_(info->compile_options()),
       cached_parse_data_(NULL),
+      parsing_lazy_arrow_parameters_(false),
       total_preparse_skipped_(0),
       pre_parse_timer_(NULL),
       parsing_on_main_thread_(true) {
@@ -1132,40 +1142,27 @@ FunctionLiteral* Parser::ParseLazy(Isolate* isolate, ParseInfo* info,
     bool ok = true;
 
     if (shared_info->is_arrow()) {
-      FormalParameterErrorLocations error_locs;
-      bool has_rest = false;
-      ZoneList<const AstRawString*>* params;
-      if (Check(Token::LPAREN)) {
-        // '(' StrictFormalParameters ')'
-        params = ParseFormalParameterList(&error_locs, &has_rest, &ok);
-        if (ok) ok = Check(Token::RPAREN);
-      } else {
-        // BindingIdentifier
-        params = NewFormalParameterList(1, zone());
-        DuplicateFinder* null_duplicate_finder = nullptr;
-        const AstRawString* single_param =
-            ParseFormalParameter(null_duplicate_finder, &error_locs, &ok);
-        if (ok) params->Add(single_param, zone());
-      }
-
+      // The first expression being parsed is the parameter list of the arrow
+      // function. Setting this avoids prevents ExpressionFromIdentifier()
+      // from creating unresolved variables in already-resolved scopes.
+      parsing_lazy_arrow_parameters_ = true;
+      Expression* expression = ParseExpression(false, &ok);
       if (ok) {
-        Expression* expression = ParseArrowFunctionLiteral(
-            shared_info->start_position(), params, error_locs, has_rest, &ok);
-        if (ok) {
-          // Scanning must end at the same position that was recorded
-          // previously. If not, parsing has been interrupted due to a stack
-          // overflow, at which point the partially parsed arrow function
-          // concise body happens to be a valid expression. This is a problem
-          // only for arrow functions with single expression bodies, since there
-          // is no end token such as "}" for normal functions.
-          if (scanner()->location().end_pos == shared_info->end_position()) {
-            // The pre-parser saw an arrow function here, so the full parser
-            // must produce a FunctionLiteral.
-            DCHECK(expression->IsFunctionLiteral());
-            result = expression->AsFunctionLiteral();
-          } else {
-            ok = false;
-          }
+        // Scanning must end at the same position that was recorded
+        // previously. If not, parsing has been interrupted due to a
+        // stack overflow, at which point the partially parsed arrow
+        // function concise body happens to be a valid expression. This
+        // is a problem only for arrow functions with single statement
+        // bodies, since there is no end token such as "}" for normal
+        // functions.
+        if (scanner()->location().end_pos == shared_info->end_position()) {
+          // The pre-parser saw an arrow function here, so the full parser
+          // must produce a FunctionLiteral.
+          DCHECK(expression->IsFunctionLiteral());
+          result = expression->AsFunctionLiteral();
+        } else {
+          result = NULL;
+          ok = false;
         }
       }
     } else if (shared_info->is_default_constructor()) {
@@ -3737,141 +3734,77 @@ Handle<FixedArray> CompileTimeValue::GetElements(Handle<FixedArray> value) {
 }
 
 
-void ParserTraits::RecordArrowFunctionParameter(
-    ZoneList<const AstRawString*>* params, VariableProxy* proxy,
-    FormalParameterErrorLocations* error_locs, bool* ok) {
-  const AstRawString* raw_name = proxy->raw_name();
-  Scanner::Location param_location(proxy->position(),
-                                   proxy->position() + raw_name->length());
-
-  if (proxy->is_this()) {
-    ReportMessageAt(param_location, "this_formal_parameter");
-    *ok = false;
-    return;
-  }
-
-  if (!error_locs->eval_or_arguments.IsValid() && IsEvalOrArguments(raw_name))
-    error_locs->eval_or_arguments = param_location;
-  if (!error_locs->reserved.IsValid() && IsFutureStrictReserved(raw_name))
-    error_locs->reserved = param_location;
-  if (!error_locs->undefined.IsValid() && IsUndefined(raw_name))
-    error_locs->undefined = param_location;
-
-  // TODO(wingo): Fix quadratic check.  (Scope::IsDeclaredParameter has the same
-  // issue.)
-  for (int i = 0; i < params->length(); i++) {
-    // Eagerly report the error here; duplicate formal parameter names are never
-    // allowed in arrow functions.
-    if (raw_name == params->at(i)) {
-      ReportMessageAt(param_location,
-                      "duplicate_arrow_function_formal_parameter");
-      *ok = false;
-      return;
-    }
-  }
-
-  // When the formal parameter was originally seen, it was parsed as a
-  // VariableProxy and recorded as unresolved in the scope.  Here we undo that
-  // parse-time side-effect.
-  parser_->scope_->RemoveUnresolved(proxy);
-
-  params->Add(raw_name, parser_->zone());
-}
-
-
-// Arrow function parameter lists are parsed as StrictFormalParameters, which
-// means that they cannot have duplicates.  Note that this is a subset of the
-// restrictions placed on parameters to functions whose body is strict.
-ZoneList<const AstRawString*>*
-ParserTraits::ParseArrowFunctionFormalParameterList(
-    Expression* params, const Scanner::Location& params_loc,
-    FormalParameterErrorLocations* error_locs, bool* is_rest, bool* ok) {
-  ZoneList<const AstRawString*>* result =
-      NewFormalParameterList(4, parser_->zone());
-
-  DCHECK_NOT_NULL(params);
+bool CheckAndDeclareArrowParameter(ParserTraits* traits, Expression* expression,
+                                   Scope* scope, int* num_params,
+                                   Scanner::Location* undefined_loc,
+                                   Scanner::Location* dupe_loc) {
+  // Case for empty parameter lists:
+  //   () => ...
+  if (expression == NULL) return true;
 
   // Too many parentheses around expression:
   //   (( ... )) => ...
-  if (params->is_multi_parenthesized()) {
-    // TODO(wingo): Make a better message.
-    ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
-    *ok = false;
-    return NULL;
-  }
+  if (expression->is_multi_parenthesized()) return false;
 
-  // ArrowFunctionFormals ::
-  //    VariableProxy
-  //    Binary(Token::COMMA, ArrowFunctionFormals, VariableProxy)
-  //
-  // To stay iterative we'll process arguments in right-to-left order, then
-  // reverse the list in place.
-  //
-  // Sadly, for the various malformed_arrow_function_parameter_list errors, we
-  // can't be more specific on the error message or on the location because we
-  // need to match the pre-parser's behavior.
-  while (params->IsBinaryOperation()) {
-    BinaryOperation* binop = params->AsBinaryOperation();
-    Expression* left = binop->left();
-    Expression* right = binop->right();
-    if (binop->op() != Token::COMMA) {
-      ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
-      *ok = false;
-      return NULL;
-    }
-    // RHS of comma expression should be an unparenthesized variable proxy.
-    if (right->is_parenthesized() || !right->IsVariableProxy()) {
-      ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
-      *ok = false;
-      return NULL;
+  // Case for a single parameter:
+  //   (foo) => ...
+  //   foo => ...
+  if (expression->IsVariableProxy()) {
+    if (expression->AsVariableProxy()->is_this()) return false;
+
+    const AstRawString* raw_name = expression->AsVariableProxy()->raw_name();
+    if (traits->IsEvalOrArguments(raw_name) ||
+        traits->IsFutureStrictReserved(raw_name))
+      return false;
+    if (traits->IsUndefined(raw_name) && !undefined_loc->IsValid()) {
+      *undefined_loc = Scanner::Location(
+          expression->position(), expression->position() + raw_name->length());
     }
-    RecordArrowFunctionParameter(result, right->AsVariableProxy(), error_locs,
-                                 CHECK_OK);
-    // LHS of comma expression should be unparenthesized.
-    params = left;
-    if (params->is_parenthesized()) {
-      ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
-      *ok = false;
-      return NULL;
+    if (scope->IsDeclared(raw_name)) {
+      *dupe_loc = Scanner::Location(
+          expression->position(), expression->position() + raw_name->length());
+      return false;
     }
 
-    if (result->length() > Code::kMaxArguments) {
-      ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
-      *ok = false;
-      return NULL;
-    }
-  }
+    // 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());
 
-  if (params->IsVariableProxy()) {
-    RecordArrowFunctionParameter(result, params->AsVariableProxy(), error_locs,
-                                 CHECK_OK);
-  } else {
-    ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
-    *ok = false;
-    return NULL;
+    scope->DeclareParameter(raw_name, VAR);
+    ++(*num_params);
+    return true;
   }
 
-  // Reverse in place.
-  std::reverse(result->begin(), result->end());
+  // Case for more than one parameter:
+  //   (foo, bar [, ...]) => ...
+  if (expression->IsBinaryOperation()) {
+    BinaryOperation* binop = expression->AsBinaryOperation();
+    if (binop->op() != Token::COMMA || binop->left()->is_parenthesized() ||
+        binop->right()->is_parenthesized())
+      return false;
 
-  return result;
+    return CheckAndDeclareArrowParameter(traits, binop->left(), scope,
+                                         num_params, undefined_loc, dupe_loc) &&
+           CheckAndDeclareArrowParameter(traits, binop->right(), scope,
+                                         num_params, undefined_loc, dupe_loc);
+  }
+
+  // Any other kind of expression is not a valid parameter list.
+  return false;
 }
 
 
-int ParserTraits::DeclareFormalParameters(ZoneList<const AstRawString*>* params,
-                                          Scope* scope, bool has_rest) {
-  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
-      // conservative approximation necessary to account for parameters
-      // that are assigned via the arguments array.
-      var->set_maybe_assigned();
-    }
-  }
-  return params->length();
+int ParserTraits::DeclareArrowParametersFromExpression(
+    Expression* expression, Scope* scope, Scanner::Location* undefined_loc,
+    Scanner::Location* dupe_loc, 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);
+  return num_params;
 }
 
 
@@ -3970,25 +3903,77 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
       function_state.set_generator_object_variable(temp);
     }
 
-    FormalParameterErrorLocations error_locs;
-    bool has_rest = false;
+    //  FormalParameterList ::
+    //    '(' (Identifier)*[','] ')'
     Expect(Token::LPAREN, CHECK_OK);
-    int start_position = scanner()->location().beg_pos;
-    ZoneList<const AstRawString*>* params =
-        ParseFormalParameterList(&error_locs, &has_rest, CHECK_OK);
-    Expect(Token::RPAREN, CHECK_OK);
-    int formals_end_position = scanner()->location().end_pos;
+    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);
+      }
 
-    CheckArityRestrictions(params->length(), arity_restriction, start_position,
-                           formals_end_position, CHECK_OK);
+      const AstRawString* param_name =
+          ParseIdentifierOrStrictReservedWord(&is_strict_reserved, CHECK_OK);
 
-    scope->set_start_position(start_position);
+      // 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();
+      }
 
-    num_parameters = DeclareFormalParameters(params, scope_, has_rest);
-    if (error_locs.duplicate.IsValid()) {
-      duplicate_parameters = FunctionLiteral::kHasDuplicateParameters;
-    }
+      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);
+      }
+    }
+    Expect(Token::RPAREN, CHECK_OK);
 
     Expect(Token::LBRACE, CHECK_OK);
 
@@ -4070,9 +4055,10 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
     CheckFunctionName(language_mode(), kind, function_name,
                       name_is_strict_reserved, function_name_location,
                       CHECK_OK);
-    const bool use_strict_params = has_rest || IsConciseMethod(kind);
-    CheckFunctionParameterNames(language_mode(), use_strict_params, error_locs,
-                                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);
 
     if (is_strict(language_mode())) {
       CheckStrictOctalLiteral(scope->start_position(), scope->end_position(),