[es6] Re-implement rest parameters via desugaring.

src/preparser.h

Index: src/preparser.h
diff --git a/src/preparser.h b/src/preparser.h
index cc5a23d031dc762a8a49b66448e17a835012f22a..a8cfbb5fad2b831287fdf005c065cbd193fd966c 100644
--- a/src/preparser.h
+++ b/src/preparser.h
@@ -31,6 +31,7 @@ struct FormalParametersBase {
   bool has_rest = false;
   bool is_simple = true;
   int materialized_literals_count = 0;
+  mutable int rest_array_literal_index = -1;

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

Is mutable needed?  I think not, according to uses in this file.

[caitp (gmail), 2015/09/01 15:17:24]

It is needed by the pattern rewriter, because I was asked to remove the AST node
(although bmeurer seems to prefer the AST node --- I'm getting confused by
conflicting requests D:).

PatternRewriter touches mutable AST nodes, but because this (rest parameters)
isn't (currently) represented as an AST node, the Parser's ReindexLiterals
method needs to be able to update the literal index appropriately.

[adamk, 2015/09/01 15:43:12]

To be clear about the conflicting requests: I only objected to the use of an AST
node given the use of that node. If we go down the road of supporting this in
codegen (again), then clearly an AST node is required.

 };
 
 
@@ -941,7 +942,10 @@ class PreParserExpression {
   static PreParserExpression BinaryOperation(PreParserExpression left,
                                              Token::Value op,
                                              PreParserExpression right) {
-    return PreParserExpression(TypeField::encode(kBinaryOperationExpression));
+    return PreParserExpression(
+        TypeField::encode(kBinaryOperationExpression) |
+        HasRestField::encode(op == Token::COMMA &&
+                             right->IsSpreadExpression()));
   }
 
   static PreParserExpression StringLiteral() {
@@ -1058,6 +1062,13 @@ class PreParserExpression {
     return TypeField::decode(code_) == kSpreadExpression;
   }
 
+  bool IsArrowFunctionFormalParametersWithRestParameter() const {
+    // Preparser's way of determining if an arrow function has a rest parameter.
+    // Depends on `ExpressionClassifier::is_valid_arrow_formal_parameters()`

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

I apologize for nitpicking but can we have sentences with full stops please.  I
also find "depends on" to be a bit vague in this context.  A possibility:

"If (and only if) the expression classifier has determined that this expression
is a valid arrow formal parameter list, return true if the formal parameter list
has a rest parameter."

[caitp (gmail), 2015/09/01 15:17:24]

Done.

+    return IsSpreadExpression() ||
+           (IsBinaryOperation() && HasRestField::decode(code_));
+  }
+
   PreParserExpression AsFunctionLiteral() { return *this; }
 
   bool IsBinaryOperation() const {
@@ -1106,6 +1117,7 @@ class PreParserExpression {
   typedef BitField<bool, IsUseStrictField::kNext, 1> IsUseStrongField;
   typedef BitField<PreParserIdentifier::Type, TypeField::kNext, 10>
       IdentifierTypeField;
+  typedef BitField<bool, TypeField::kNext, 1> HasRestField;
 
   uint32_t code_;
 };
@@ -1878,6 +1890,12 @@ void PreParserTraits::ParseArrowFunctionFormalParameterList(
     Scanner::Location* duplicate_loc, bool* ok) {
   // TODO(wingo): Detect duplicated identifiers in paramlists.  Detect parameter
   // lists that are too long.
+
+  // Accomodate array literal for rest parameter.
+  if (params.IsArrowFunctionFormalParametersWithRestParameter()) {
+    ++parameters->materialized_literals_count;
+    pre_parser_->function_state_->NextMaterializedLiteralIndex();
+  }

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

I am OK with this I guess.  Is it possible, possibly in a followup, to separate
out a desurgaring phase which runs before parsing the function body, in all
cases?  "Parse" should produce a FormalParametersT, and ideally the desugaring
could be in ParserBase.  If you have to make an array literal node for the rest
array, the side effect on the materialized literal count comes out naturally. 
Dunno, I recognize that the parser is a gnarly thing onto which we keep
hammering new things and we can't always get it right, but fudging about with
the materialized literal count in four places smells wrong to me.

[caitp (gmail), 2015/09/01 15:17:25]

I think we'd be better off getting rid of FormalParametersBase's
materialized_literal_count entirely, and solving this problem properly. This
will hopefully be cleaned up as a part of a fix for v8:4400, which has been
discussed on IRC a bit

 }
 
 
@@ -2416,6 +2434,7 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseExpression(
                          ExpressionClassifier::AllProductions);
   bool is_simple_parameter_list = this->IsIdentifier(result);
   bool seen_rest = false;
+
   while (peek() == Token::COMMA) {
     if (seen_rest) {
       // At this point the production can't possibly be valid, but we don't know
@@ -2424,6 +2443,8 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseExpression(
           scanner()->peek_location(), MessageTemplate::kParamAfterRest);
     }
     Consume(Token::COMMA);
+    Token::Value next = peek();
+    Scanner::Location next_location = scanner()->peek_location();
     bool is_rest = false;
     if (allow_harmony_rest_parameters() && peek() == Token::ELLIPSIS) {
       // 'x, y, ...z' in CoverParenthesizedExpressionAndArrowParameterList only
@@ -2437,7 +2458,15 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseExpression(
     int pos = position();
     ExpressionT right = this->ParseAssignmentExpression(
         accept_IN, &binding_classifier, CHECK_OK);
-    if (is_rest) right = factory()->NewSpread(right, pos);
+    if (is_rest) {
+      if (!this->IsIdentifier(right)) {
+        ReportUnexpectedTokenAt(next_location, next);

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

Why is this here?  I can't reproduce this bug but I also don't see where this
error case would be caught.

[caitp (gmail), 2015/09/01 15:17:24]

It produces a better error message --- it was moved as part of the AST node
patch set to produce the same message as the full parser, but can be removed
now.

Reverted this change

+        *ok = false;
+        return this->EmptyExpression();
+      }
+      right = factory()->NewSpread(right, pos);
+    }
+
     is_simple_parameter_list =
         is_simple_parameter_list && this->IsIdentifier(right);
     classifier->Accumulate(binding_classifier,
@@ -2886,16 +2915,19 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
       scope->SetHasNonSimpleParameters();
       parameters.is_simple = false;
     }
-    checkpoint.Restore(&parameters.materialized_literals_count);
 
-    scope->set_start_position(lhs_beg_pos);
     Scanner::Location duplicate_loc = Scanner::Location::invalid();
     this->ParseArrowFunctionFormalParameterList(&parameters, expression, loc,

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

Yeah moving this before the checkpoint just sounds like a bad idea to me.  I
would prefer that the reservation of the literals index for the array happened
while parsing the function, within its scope and literals count, and before
parsing the function's body.  Is that not possible?

[caitp (gmail), 2015/09/01 15:17:24]

It's not currently possible with the FormalParametersT + FunctionState scheme. I
agree that this would be better, but we need a better way to track these in
order to make that work.

                                                 &duplicate_loc, CHECK_OK);
+
+    checkpoint.Restore(&parameters.materialized_literals_count);
+
+    scope->set_start_position(lhs_beg_pos);
     if (duplicate_loc.IsValid()) {
       arrow_formals_classifier.RecordDuplicateFormalParameterError(
           duplicate_loc);
     }
+
     expression = this->ParseArrowFunctionLiteral(
         parameters, arrow_formals_classifier, CHECK_OK);
     return expression;
@@ -3681,8 +3713,9 @@ ParserBase<Traits>::ParseMemberExpressionContinuation(
 
 
 template <class Traits>
-void ParserBase<Traits>::ParseFormalParameter(
-    FormalParametersT* parameters, ExpressionClassifier* classifier, bool* ok) {
+void ParserBase<Traits>::ParseFormalParameter(FormalParametersT* parameters,
+                                              ExpressionClassifier* classifier,
+                                              bool* ok) {
   // FormalParameter[Yield,GeneratorParameter] :
   //   BindingElement[?Yield, ?GeneratorParameter]
   bool is_rest = parameters->has_rest;
@@ -3706,6 +3739,12 @@ void ParserBase<Traits>::ParseFormalParameter(
     classifier->RecordNonSimpleParameter();
   }
 
+  if (is_rest) {
+    parameters->rest_array_literal_index =
+        function_state_->NextMaterializedLiteralIndex();
+    ++parameters->materialized_literals_count;
+  }
+
   ExpressionT initializer = Traits::EmptyExpression();
   if (!is_rest && allow_harmony_default_parameters() && Check(Token::ASSIGN)) {
     ExpressionClassifier init_classifier;
@@ -3866,6 +3905,11 @@ ParserBase<Traits>::ParseArrowFunctionLiteral(
         body = this->NewStatementList(0, zone());
         this->SkipLazyFunctionBody(&materialized_literal_count,
                                    &expected_property_count, CHECK_OK);
+
+        if (formal_parameters.materialized_literals_count > 0) {
+          materialized_literal_count +=
+              formal_parameters.materialized_literals_count;
+        }
       } else {
         body = this->ParseEagerFunctionBody(
             this->EmptyIdentifier(), RelocInfo::kNoPosition, formal_parameters,