[es6] Re-implement rest parameters via desugaring.

src/parser.cc

Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 319d468e4d61db43f529e8424139842d7bede8ba..b08292ddc5228c0e62d9f52e8a9496d4217b523e 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -3949,9 +3949,12 @@ void ParserTraits::ParseArrowFunctionFormalParameters(
   if (is_rest) {
     expr = expr->AsSpread()->expression();
     parameters->has_rest = true;
+    parameters->rest_array_literal_index =
+        parser_->function_state_->NextMaterializedLiteralIndex();
+    ++parameters->materialized_literals_count;
   }
   if (parameters->is_simple) {
-    parameters->is_simple = !is_rest && expr->IsVariableProxy();
+    parameters->is_simple = expr->IsVariableProxy();
   }
 
   Expression* initializer = nullptr;
@@ -4005,6 +4008,11 @@ void ParserTraits::ReindexLiterals(const ParserFormalParameters& parameters) {
     for (const auto p : parameters.params) {
       if (p.pattern != nullptr) reindexer.Reindex(p.pattern);
     }
+
+    if (parameters.has_rest) {
+      parameters.rest_array_literal_index = reindexer.NextIndex();
+    }
+
     DCHECK(reindexer.count() <=
            parser_->function_state_->materialized_literal_count());
   }
@@ -4174,6 +4182,10 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
                            &expected_property_count, /*CHECK_OK*/ ok,
                            maybe_bookmark);
 
+      if (formals.materialized_literals_count > 0) {
+        materialized_literal_count += formals.materialized_literals_count;
+      }

[wingo, 2015/09/01 14:41:20]

No need for the if, just execute the body unconditionally, no?

Aside: it's pretty confusing that one field is "materialized_literal_count" and
the other is "materialized_literals_count".  Probably need to rename the latter
to the former, because the former has been around for longer.

+
       if (bookmark.HasBeenReset()) {
         // Trigger eager (re-)parsing, just below this block.
         is_lazily_parsed = false;
@@ -4238,11 +4250,6 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
   if (should_be_used_once_hint)
     function_literal->set_should_be_used_once_hint();
 
-  if (scope->has_rest_parameter()) {
-    // TODO(caitp): enable optimization of functions with rest params
-    function_literal->set_dont_optimize_reason(kRestParameter);
-  }
-
   if (fni_ != NULL && should_infer_name) fni_->AddFunction(function_literal);
   return function_literal;
 }
@@ -4377,8 +4384,6 @@ Block* Parser::BuildParameterInitializationBlock(
       factory()->NewBlock(NULL, 1, true, RelocInfo::kNoPosition);
   for (int i = 0; i < parameters.params.length(); ++i) {
     auto parameter = parameters.params[i];
-    // TODO(caitp,rossberg): Remove special handling for rest once desugared.
-    if (parameter.is_rest) break;
     DeclarationDescriptor descriptor;
     descriptor.declaration_kind = DeclarationDescriptor::PARAMETER;
     descriptor.parser = this;
@@ -4395,6 +4400,7 @@ Block* Parser::BuildParameterInitializationBlock(
         factory()->NewVariableProxy(parameters.scope->parameter(i));
     if (parameter.initializer != nullptr) {
       // IS_UNDEFINED($param) ? initializer : $param
+      DCHECK(!parameter.is_rest);
       auto condition = factory()->NewCompareOperation(
           Token::EQ_STRICT,
           factory()->NewVariableProxy(parameters.scope->parameter(i)),
@@ -4404,6 +4410,83 @@ Block* Parser::BuildParameterInitializationBlock(
           condition, parameter.initializer, initial_value,
           RelocInfo::kNoPosition);
       descriptor.initialization_pos = parameter.initializer->position();
+    } else if (parameter.is_rest) {
+      // $rest = [];
+      // for (var $argument_index = $rest_index;

[Benedikt Meurer, 2015/09/01 03:09:10]

I guess there was some discussion about this desugaring; so do we have any plans
how to make this work with inlining? esp. inlining in TurboFan?

[caitp (gmail), 2015/09/01 03:55:02]

I don't think there are plans per se (but you might have seen there was some TF
code previously for this, not sure how complete it was in terms of inlining).

There are at least some ideas about how the runtime call could be changed into
something relatively quick. The spec'd algorithm uses CreateDataProperty(O, P,
V) to add the element to the array. For rest parameters, we can make a lot of
assumptions which lend themselves well to inlining:

- the rest array's own "length" property is always writable, never configurable
- the rest array is always extensible
- the rest array's own indexed properties are always writable and configurable
(they aren't present until the rest parameter algorithm sets them up)

Based on this, yes, the current implementation probably isn't doing the right
thing, but it's pretty close.

I guess there are also statistical guesses about the size of the array to
allocate, and whether or not to perform the loop at all, after a bit of
profiling. But maybe compiler-folks are better for talking about that stuff.

+      //      $argument_index < %_ArgumentsLength();
+      //      ++$argument_index) {
+      //   %AppendElement($rest, %_Arguments($argument_index));
+      // }
+      // let <param> = $rest;
+      DCHECK(parameter.pattern->IsVariableProxy());
+      DCHECK_EQ(i, parameters.params.length() - 1);
+
+      int pos = parameter.pattern->position();
+      auto var = parameters.scope->parameter(i);

[Benedikt Meurer, 2015/09/01 03:09:10]

Nit: Wow, this heavy autofication. I'm not sure what the convention for the
parser is, but I'd prefer to reduce the use of auto, esp. when it's used in
place of a pointer type.

[caitp (gmail), 2015/09/01 03:55:02]

My feeling is that it's alright for AST nodes (because of the `New<NodeType>`
methods providing all the missing info), which is the bulk of the auto use. If
folks want explicit typenames used everywhere, we can do that too.

How important is this, given that the majority of this is AST nodes?

[wingo, 2015/09/01 14:41:20]

I had this same thought when I read this FWIW; but it was only on this one in
particular (parameters.scope->parameter(i)).  I don't know what the type of var
is; is it a proxy or a variable?  I guess the latter but since it's not a
constructor a type would be appreciated.

+      auto empty_values = new (zone()) ZoneList<Expression*>(0, zone());
+      auto empty_array = factory()->NewArrayLiteral(
+          empty_values, parameters.rest_array_literal_index,
+          is_strong(language_mode()), RelocInfo::kNoPosition);
+
+      auto init_array = factory()->NewAssignment(
+          Token::INIT_VAR, factory()->NewVariableProxy(var), empty_array,
+          RelocInfo::kNoPosition);
+
+      auto loop = factory()->NewForStatement(NULL, RelocInfo::kNoPosition);
+
+      auto argument_index =
+          parameters.scope->NewTemporary(ast_value_factory()->empty_string());
+      auto init = factory()->NewExpressionStatement(
+          factory()->NewAssignment(
+              Token::INIT_VAR, factory()->NewVariableProxy(argument_index),
+              factory()->NewSmiLiteral(i, RelocInfo::kNoPosition),
+              RelocInfo::kNoPosition),
+          RelocInfo::kNoPosition);
+
+      auto empty_arguments = new (zone()) ZoneList<Expression*>(0, zone());
+
+      // $arguments_index < arguments.length
+      auto cond = factory()->NewCompareOperation(
+          Token::LT, factory()->NewVariableProxy(argument_index),
+          factory()->NewCallRuntime(Runtime::kInlineArgumentsLength,
+                                    empty_arguments, RelocInfo::kNoPosition),
+          RelocInfo::kNoPosition);
+
+      // ++argument_index
+      auto next = factory()->NewExpressionStatement(
+          factory()->NewCountOperation(
+              Token::INC, true, factory()->NewVariableProxy(argument_index),
+              RelocInfo::kNoPosition),
+          RelocInfo::kNoPosition);
+
+      // %_Arguments($arguments_index)
+      auto arguments_args = new (zone()) ZoneList<Expression*>(1, zone());
+      arguments_args->Add(factory()->NewVariableProxy(argument_index), zone());
+
+      // %AppendElement($rest, %_Arguments($arguments_index))
+      auto append_element_args = new (zone()) ZoneList<Expression*>(2, zone());
+
+      append_element_args->Add(factory()->NewVariableProxy(var), zone());
+      append_element_args->Add(
+          factory()->NewCallRuntime(Runtime::kInlineArguments, arguments_args,
+                                    RelocInfo::kNoPosition),
+          zone());
+
+      auto body = factory()->NewExpressionStatement(
+          factory()->NewCallRuntime(Runtime::kAppendElement,
+                                    append_element_args,
+                                    RelocInfo::kNoPosition),
+          RelocInfo::kNoPosition);
+
+      loop->Initialize(init, cond, next, body);
+
+      init_block->AddStatement(
+          factory()->NewExpressionStatement(init_array, RelocInfo::kNoPosition),
+          zone());
+
+      init_block->AddStatement(loop, zone());
+
+      descriptor.initialization_pos = pos;
     }
 
     Scope* param_scope = scope_;