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..c5bc88f3c4f4ca094f4e3910023e53ad9927a9ca 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);

[arv (Not doing code reviews), 2015/04/13 18:26:38]

Nice.

+        if (ok) ok = Check(Token::RPAREN);
+      } else {
+        // BindingIdentifier
+        params = NewFormalParameterList(1, zone());
+        DuplicateFinder duplicate_finder(scanner()->unicode_cache());

[arv (Not doing code reviews), 2015/04/13 18:26:38]

Is it worth refactoring to not create a duplicate_finder here?

[marja, 2015/04/14 07:38:26]

+1, this is unnecessary...

[wingo, 2015/04/14 08:23:28]

Done by passing a null pointer.

+        const AstRawString* single_param =
+            ParseFormalParameter(&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 statement bodies, since there

[arv (Not doing code reviews), 2015/04/13 18:26:38]

single statement bodies

->

single expression bodies

+          // 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,143 @@ Handle<FixedArray> CompileTimeValue::GetElements(Handle<FixedArray> value) {
 }
 
 
-bool CheckAndDeclareArrowParameter(ParserTraits* traits, Expression* expression,
-                                   Scope* scope, int* num_params,
-                                   FormalParameterErrorLocations* locs) {
+void ParserTraits::RecordArrowFunctionParameter(
+    ZoneList<const v8::internal::AstRawString*>* result, 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 (IsEvalOrArguments(raw_name) && !error_locs->eval_or_arguments_.IsValid())

[arv (Not doing code reviews), 2015/04/13 18:26:38]

Maybe check IsValid first?

+    error_locs->eval_or_arguments_ = param_location;
+  if (IsFutureStrictReserved(raw_name) && !error_locs->reserved_.IsValid())
+    error_locs->reserved_ = param_location;
+  if (IsUndefined(raw_name) && !error_locs->undefined_.IsValid())
+    error_locs->undefined_ = param_location;
+
+  // TODO(wingo): Fix quadratic check.  (Scope::IsDeclaredParameter has the same
+  // 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 outer scope. But it's
+  // really not unresolved.
+  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 v8::internal::AstRawString*>*
+ParserTraits::ParseArrowFunctionFormalParameterList(
+    Expression* params, const Scanner::Location& params_loc,
+    FormalParameterErrorLocations* error_locs, bool* is_rest, bool* ok) {
+  ZoneList<const v8::internal::AstRawString*>* result =
+      NewFormalParameterList(4, parser_->zone());
+
   // Case for empty parameter lists:
   //   () => ...
-  if (expression == NULL) return true;
+  if (params == NULL) return result;
 
   // 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 != nullptr && params->is_multi_parenthesized()) {

[arv (Not doing code reviews), 2015/04/13 18:26:38]

Change params != nullptr to DCHECK_NOT_NULL.

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

[arv (Not doing code reviews), 2015/04/13 18:26:38]

An idea to fixing the preparser would be to:

 - Parse as we do today
 - If there was a parse error,
   - go back and parse again (with a flag or parse as StrictFormalParamers) and
report the first error with a location

That way only the error case gets slower.

(if that seems feasible we can do that in a follow up CL)

+  // 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,
+                                          bool* ok) {

[marja, 2015/04/14 07:38:25]

The bool ok seems unnecessary now as this cannot fail. Does it make sense to
remove it, or is it ever imaginable this stage could fail?

[wingo, 2015/04/14 08:23:28]

Good catch, thanks.

+  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 +3956,12 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
 
     scope->set_start_position(start_position);
 
-    num_parameters = params->length();
+    num_parameters =
+        DeclareFormalParameters(params, scope_, has_rest, CHECK_OK);
     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);