Allow eval/arguments in arrow functions

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 4e3ca8bcc7d05d36645339665511dd5199ea2c3e..a6526b8d1a2e490e3a34fb2bd968204d4835a59a 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -787,16 +787,7 @@ Expression* ParserTraits::ExpressionFromIdentifier(const AstRawString* name,
                                                    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);
 }
 
 
@@ -861,7 +852,6 @@ 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) {
@@ -1140,27 +1130,40 @@ FunctionLiteral* Parser::ParseLazy(Isolate* isolate, ParseInfo* info,
     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);
+      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());
+      }
+
       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(
+            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;
+          }
         }
       }
     } else if (shared_info->is_default_constructor()) {
@@ -3704,76 +3707,141 @@ Handle<FixedArray> CompileTimeValue::GetElements(Handle<FixedArray> value) {
 }
 
 
-bool CheckAndDeclareArrowParameter(ParserTraits* traits, Expression* expression,
-                                   Scope* scope, int* num_params,
-                                   FormalParameterErrorLocations* locs) {
-  // Case for empty parameter lists:
-  //   () => ...
-  if (expression == NULL) return true;
+void ParserTraits::RecordArrowFunctionParameter(
+    ZoneList<const AstRawString*>* result, VariableProxy* proxy,

[marja, 2015/04/14 11:59:48]

"result" is not a very good name, could you change it to something more
descriptive?

We use results e.g., in this sense:
FunctionLiteral* result = nullptr;
...
result = ParseFunctionLiteral(...);

but here the "result" it's not really the "result" of this operation (recording
an arrow function param). declared_params?

[wingo, 2015/04/14 15:05:42]

Done.

+    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

[marja, 2015/04/14 11:59:48]

Nit: Can't you easily use DuplicateFinder one level up? Or, the caller could
pass it here... This way you wouldn't even need to iterate over "result" here...

(No need to fix in this CL necessarily.)

[wingo, 2015/04/14 15:05:42]

ACK.  Some sort of hash map might work just as well though, given that
DuplicateFinder doesn't operate natively on AstRawString.

+  // issue.)
+  for (int i = 0; i < result->length(); i++) {
+    // Eagerly report the error here; duplicate formal parameter names are never
+    // allowed in arrow functions.
+    if (raw_name == result->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);
+
+  result->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);
 
   // 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;
+  if (params->is_multi_parenthesized()) {
+    // TODO(wingo): Make a better message.
+    ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
+    *ok = false;
+    return NULL;
+  }
 
-    const AstRawString* raw_name = expression->AsVariableProxy()->raw_name();
-    if (traits->IsEvalOrArguments(raw_name) ||
-        traits->IsFutureStrictReserved(raw_name))
-      return false;
-    if (traits->IsUndefined(raw_name) && !locs->undefined_.IsValid()) {
-      locs->undefined_ = Scanner::Location(
-          expression->position(), expression->position() + raw_name->length());
+  // 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;
     }
-    if (scope->IsDeclared(raw_name)) {
-      locs->duplicate_ = Scanner::Location(
-          expression->position(), expression->position() + raw_name->length());
-      return false;
+    // 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;
+    }
+    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;
     }
 
-    // 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;
+    if (result->length() > Code::kMaxArguments) {
+      ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
+      *ok = false;
+      return NULL;
+    }
   }
 
-  // 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, locs) &&
-           CheckAndDeclareArrowParameter(traits, binop->right(), scope,
-                                         num_params, locs);
+  if (params->IsVariableProxy()) {
+    RecordArrowFunctionParameter(result, params->AsVariableProxy(), error_locs,
+                                 CHECK_OK);
+  } else {
+    ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
+    *ok = false;
+    return NULL;
   }
 
-  // Any other kind of expression is not a valid parameter list.
-  return false;
+  // Reverse in place.
+  std::reverse(result->begin(), result->end());
+
+  return result;
 }
 
 
-int ParserTraits::DeclareArrowParametersFromExpression(
-    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, locs);
-  return num_params;
+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();
 }
 
 
@@ -3886,22 +3954,11 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
 
     scope->set_start_position(start_position);
 
-    num_parameters = params->length();
+    num_parameters = DeclareFormalParameters(params, scope_, has_rest);
     if (error_locs.duplicate_.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
-        // conservative approximation necessary to account for parameters
-        // that are assigned via the arguments array.
-        var->set_maybe_assigned();
-      }
-    }
 
     Expect(Token::LBRACE, CHECK_OK);