Re-apply formal argument, arrow function parsing refactors

src/parser.cc

 // 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.
 
 #include "src/v8.h"
 
 #include "src/api.h"
 #include "src/ast.h"
 #include "src/bailout-reason.h"
 #include "src/base/platform/platform.h"
@@ skipping 769 matching lines @@
   return NULL;
 }
 
 
 Expression* ParserTraits::ExpressionFromIdentifier(const AstRawString* name,
                                                    int start_position,
                                                    int end_position,
                                                    Scope* scope,
                                                    AstNodeFactory* factory) {
   if (parser_->fni_ != NULL) parser_->fni_->PushVariableName(name);
-
-  // 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);
+  return scope->NewUnresolved(factory, name, start_position, end_position);
 }
 
 
 Expression* ParserTraits::ExpressionFromString(int pos, Scanner* scanner,
                                                AstNodeFactory* factory) {
   const AstRawString* symbol = GetSymbol(scanner);
   if (parser_->fni_ != NULL) parser_->fni_->PushLiteralName(symbol);
   return factory->NewStringLiteral(symbol, pos);
 }
 
@@ skipping 44 matching lines @@
 Parser::Parser(ParseInfo* info)
     : ParserBase<ParserTraits>(info->zone(), &scanner_, info->stack_limit(),
                                info->extension(), info->ast_value_factory(),
                                NULL, this),
       scanner_(info->unicode_cache()),
       reusable_preparser_(NULL),
       original_scope_(NULL),
       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) {
   // Even though we were passed ParseInfo, we should not store it in
   // Parser - this makes sure that Isolate is not accidentally accessed via
   // ParseInfo during background parsing.
   DCHECK(!info->script().is_null() || info->source_stream() != NULL);
   set_allow_lazy(info->allow_lazy_parsing());
   set_allow_natives(FLAG_allow_natives_syntax || info->is_native());
   set_allow_harmony_modules(!info->is_native() && FLAG_harmony_modules);
@@ skipping 260 matching lines @@
     DCHECK(info->language_mode() == shared_info->language_mode());
     scope->SetLanguageMode(shared_info->language_mode());
     FunctionLiteral::FunctionType function_type = shared_info->is_expression()
         ? (shared_info->is_anonymous()
               ? FunctionLiteral::ANONYMOUS_EXPRESSION
               : FunctionLiteral::NAMED_EXPRESSION)
         : FunctionLiteral::DECLARATION;
     bool ok = true;
 
     if (shared_info->is_arrow()) {
-      // 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);
+      Scope* scope = NewScope(scope_, ARROW_SCOPE);
+      scope->set_start_position(shared_info->start_position());
+      FormalParameterErrorLocations error_locs;
+      bool has_rest = false;
+      if (Check(Token::LPAREN)) {
+        // '(' StrictFormalParameters ')'
+        ParseFormalParameterList(scope, &error_locs, &has_rest, &ok);
+        if (ok) ok = Check(Token::RPAREN);
+      } else {
+        // BindingIdentifier
+        DuplicateFinder* null_duplicate_finder = nullptr;
+        const AstRawString* single_param =
+            ParseFormalParameter(null_duplicate_finder, &error_locs, &ok);
+        ParserTraits::DeclareFormalParameter(scope, single_param, has_rest);
+      }
+
       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 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;
+        Expression* expression =
+            ParseArrowFunctionLiteral(scope, 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;
+          }
         }
       }
     } else if (shared_info->is_default_constructor()) {
       result = DefaultConstructor(IsSubclassConstructor(shared_info->kind()),
                                   scope, shared_info->start_position(),
                                   shared_info->end_position());
     } else {
       result = ParseFunctionLiteral(raw_name, Scanner::Location::invalid(),
                                     false,  // Strict mode name already checked.
                                     shared_info->kind(), RelocInfo::kNoPosition,
@@ skipping 2549 matching lines @@
   Smi* literal_type = Smi::cast(value->get(kLiteralTypeSlot));
   return static_cast<LiteralType>(literal_type->value());
 }
 
 
 Handle<FixedArray> CompileTimeValue::GetElements(Handle<FixedArray> value) {
   return Handle<FixedArray>(FixedArray::cast(value->get(kElementsSlot)));
 }
 
 
-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 (expression->is_multi_parenthesized()) return false;
-
-  // 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());
-    }
-    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);
-    ++(*num_params);
-    return true;
+void ParserTraits::DeclareArrowFunctionParameters(
+    Scope* scope, Expression* expr, const Scanner::Location& params_loc,
+    FormalParameterErrorLocations* error_locs, bool* ok) {
+  if (scope->num_parameters() >= Code::kMaxArguments) {
+    ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
+    *ok = false;
+    return;
   }
 
-  // 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 CheckAndDeclareArrowParameter(traits, binop->left(), scope,
-                                         num_params, undefined_loc, dupe_loc) &&
-           CheckAndDeclareArrowParameter(traits, binop->right(), scope,
-                                         num_params, undefined_loc, dupe_loc);
+  // ArrowFunctionFormals ::
+  //    Binary(Token::COMMA, ArrowFunctionFormals, VariableProxy)
+  //    VariableProxy
+  //
+  // As we need to visit the parameters in left-to-right order, we recurse on
+  // the left-hand side of comma expressions.
+  //
+  // 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.
+  if (expr->IsBinaryOperation()) {
+    BinaryOperation* binop = expr->AsBinaryOperation();
+    if (binop->op() != Token::COMMA) {
+      ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
+      *ok = false;
+      return;
+    }
+    Expression* left = binop->left();
+    Expression* right = binop->right();
+    if (left->is_parenthesized() || right->is_parenthesized()) {
+      ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
+      *ok = false;
+      return;
+    }
+    DeclareArrowFunctionParameters(scope, left, params_loc, error_locs, ok);
+    if (!*ok) return;
+    // LHS of comma expression should be unparenthesized.
+    expr = right;
   }
 
-  // Any other kind of expression is not a valid parameter list.
-  return false;
+  // TODO(wingo): Support rest parameters.
+  if (!expr->IsVariableProxy()) {
+    ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
+    *ok = false;
+    return;
+  }
+
+  const AstRawString* raw_name = expr->AsVariableProxy()->raw_name();
+  Scanner::Location param_location(expr->position(),
+                                   expr->position() + raw_name->length());
+
+  if (expr->AsVariableProxy()->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;
+
+  // 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.
+  if (scope->IsDeclared(raw_name)) {
+    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(expr->AsVariableProxy());
+
+  bool is_rest = false;
+  scope->DeclareParameter(raw_name, VAR, is_rest);
 }
 
 
-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;
+void ParserTraits::ParseArrowFunctionFormalParameters(
+    Scope* scope, Expression* params, const Scanner::Location& params_loc,
+    FormalParameterErrorLocations* error_locs, bool* is_rest, bool* ok) {
+  // 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;
+  }
+
+  DeclareArrowFunctionParameters(scope, params, params_loc, error_locs, ok);
 }
 
 
 FunctionLiteral* Parser::ParseFunctionLiteral(
     const AstRawString* function_name, Scanner::Location function_name_location,
     bool name_is_strict_reserved, FunctionKind kind, int function_token_pos,
     FunctionLiteral::FunctionType function_type,
     FunctionLiteral::ArityRestriction arity_restriction, bool* ok) {
   // Function ::
   //   '(' FormalParameterList? ')' '{' FunctionBody '}'
@@ skipping 78 matching lines @@
       scope_->ForceContextAllocation();
 
       // Calling a generator returns a generator object.  That object is stored
       // in a temporary variable, a definition that is used by "yield"
       // expressions. This also marks the FunctionState as a generator.
       Variable* temp = scope_->DeclarationScope()->NewTemporary(
           ast_value_factory()->dot_generator_object_string());
       function_state.set_generator_object_variable(temp);
     }
 
-    //  FormalParameterList ::
-    //    '(' (Identifier)*[','] ')'
+    FormalParameterErrorLocations error_locs;
+    bool has_rest = false;
     Expect(Token::LPAREN, CHECK_OK);
-    scope->set_start_position(scanner()->location().beg_pos);
+    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;
 
-    // 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();
+    CheckArityRestrictions(num_parameters, arity_restriction, start_position,
+                           formals_end_position, CHECK_OK);
 
-    // 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);
-      }
+    if (error_locs.duplicate.IsValid()) {

[marja, 2015/04/20 12:31:02]

Nit: duplicate_parameters is really far from its use now. Can you move it
closer... (also the initial declaration is unnecessarily far away in a C-like
manner).

[wingo, 2015/04/20 13:26:54]

I did so, though I had to move the error_locs decl out of the block, because of
the FunctionState RAII thing.  Let me know if this was the wrong thing.

+      duplicate_parameters = FunctionLiteral::kHasDuplicateParameters;
     }
-    Expect(Token::RPAREN, CHECK_OK);
 
     Expect(Token::LBRACE, CHECK_OK);
 
     // If we have a named function expression, we add a local variable
     // declaration to the body of the function with the name of the
     // function and let it refer to the function itself (closure).
     // NOTE: We create a proxy and resolve it here so that in the
     // future we can change the AST to only refer to VariableProxies
     // instead of Variables and Proxis as is the case now.
     Variable* fvar = NULL;
@@ skipping 61 matching lines @@
       materialized_literal_count = function_state.materialized_literal_count();
       expected_property_count = function_state.expected_property_count();
       handler_count = function_state.handler_count();
     }
 
     // Validate name and parameter names. We can do this only after parsing the
     // function, since the function can declare itself strict.
     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(),
                               CHECK_OK);
     }
     if (is_strict(language_mode())) {
       CheckConflictingVarDeclarations(scope, CHECK_OK);
     }
     if (is_strong(language_mode()) && IsSubclassConstructor(kind)) {
       if (!function_state.super_call_location().IsValid()) {
@@ skipping 1709 matching lines @@
 
 Expression* Parser::SpreadCallNew(Expression* function,
                                   ZoneList<v8::internal::Expression*>* args,
                                   int pos) {
   args->InsertAt(0, function, zone());
 
   return factory()->NewCallRuntime(
       ast_value_factory()->reflect_construct_string(), NULL, args, pos);
 }
 } }  // namespace v8::internal