[es6] Re-implement rest parameters via desugaring.

src/preparser.h

 // 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.
 
 #ifndef V8_PREPARSER_H
 #define V8_PREPARSER_H
 
 #include "src/bailout-reason.h"
 #include "src/expression-classifier.h"
 #include "src/func-name-inferrer.h"
@@ skipping 13 matching lines @@
   kFunctionNameValidityUnknown
 };
 
 
 struct FormalParametersBase {
   explicit FormalParametersBase(Scope* scope) : scope(scope) {}
   Scope* scope;
   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.

 };
 
 
 // Common base class shared between parser and pre-parser. Traits encapsulate
 // the differences between Parser and PreParser:
 
 // - Return types: For example, Parser functions return Expression* and
 // PreParser functions return PreParserExpression.
 
 // - Creating parse tree nodes: Parser generates an AST during the recursive
@@ skipping 890 matching lines @@
   }
 
   static PreParserExpression FromIdentifier(PreParserIdentifier id) {
     return PreParserExpression(TypeField::encode(kIdentifierExpression) |
                                IdentifierTypeField::encode(id.type_));
   }
 
   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() {
     return PreParserExpression(TypeField::encode(kStringLiteralExpression));
   }
 
   static PreParserExpression UseStrictStringLiteral() {
     return PreParserExpression(TypeField::encode(kStringLiteralExpression) |
                                IsUseStrictField::encode(true));
   }
@@ skipping 96 matching lines @@
 
   bool IsNoTemplateTag() const {
     return TypeField::decode(code_) == kExpression &&
            ExpressionTypeField::decode(code_) == kNoTemplateTagExpression;
   }
 
   bool IsSpreadExpression() const {
     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 {
     return TypeField::decode(code_) == kBinaryOperationExpression;
   }
 
   // Dummy implementation for making expression->somefunc() work in both Parser
   // and PreParser.
   PreParserExpression* operator->() { return this; }
 
@@ skipping 28 matching lines @@
   // The first three bits are for the Type.
   typedef BitField<Type, 0, 3> TypeField;
 
   // The rest of the bits are interpreted depending on the value
   // of the Type field, so they can share the storage.
   typedef BitField<ExpressionType, TypeField::kNext, 3> ExpressionTypeField;
   typedef BitField<bool, TypeField::kNext, 1> IsUseStrictField;
   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_;
 };
 
 
 // The pre-parser doesn't need to build lists of expressions, identifiers, or
 // the like.
 template <typename T>
 class PreParserList {
  public:
@@ skipping 752 matching lines @@
   return pre_parser_->factory()->NewCallNew(function, args, pos);
 }
 
 
 void PreParserTraits::ParseArrowFunctionFormalParameterList(
     PreParserFormalParameters* parameters,
     PreParserExpression params, const Scanner::Location& params_loc,
     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

 }
 
 
 PreParserStatementList PreParser::ParseEagerFunctionBody(
     PreParserIdentifier function_name, int pos,
     const PreParserFormalParameters& parameters, FunctionKind kind,
     FunctionLiteral::FunctionType function_type, bool* ok) {
   ParsingModeScope parsing_mode(this, PARSE_EAGERLY);
 
   ParseStatementList(Token::RBRACE, ok);
@@ skipping 518 matching lines @@
   //   AssignmentExpression
   //   Expression ',' AssignmentExpression
 
   ExpressionClassifier binding_classifier;
   ExpressionT result =
       this->ParseAssignmentExpression(accept_IN, &binding_classifier, CHECK_OK);
   classifier->Accumulate(binding_classifier,
                          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
       // which error to signal.
       classifier->RecordArrowFormalParametersError(
           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
       // as the formal parameters of'(x, y, ...z) => foo', and is not itself a
       // valid expression or binding pattern.
       ExpressionUnexpectedToken(classifier);
       BindingPatternUnexpectedToken(classifier);
       Consume(Token::ELLIPSIS);
       seen_rest = is_rest = true;
     }
     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,
                            ExpressionClassifier::AllProductions);
     result = factory()->NewBinaryOperation(Token::COMMA, result, right, pos);
   }
   if (!is_simple_parameter_list || seen_rest) {
     classifier->RecordNonSimpleParameter();
   }
   return result;
@@ skipping 428 matching lines @@
     ValidateArrowFormalParameters(&arrow_formals_classifier, expression,
                                   parenthesized_formals, CHECK_OK);
     Scanner::Location loc(lhs_beg_pos, scanner()->location().end_pos);
     Scope* scope =
         this->NewScope(scope_, ARROW_SCOPE, FunctionKind::kArrowFunction);
     FormalParametersT parameters(scope);
     if (!arrow_formals_classifier.is_simple_parameter_list()) {
       scope->SetHasNonSimpleParameters();
       parameters.is_simple = false;
     }
+
+    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);
-    Scanner::Location duplicate_loc = Scanner::Location::invalid();
-    this->ParseArrowFunctionFormalParameterList(&parameters, expression, loc,
-                                                &duplicate_loc, CHECK_OK);
     if (duplicate_loc.IsValid()) {
       arrow_formals_classifier.RecordDuplicateFormalParameterError(
           duplicate_loc);
     }
+
     expression = this->ParseArrowFunctionLiteral(
         parameters, arrow_formals_classifier, CHECK_OK);
     return expression;
   }
 
   // "expression" was not itself an arrow function parameter list, but it might
   // form part of one.  Propagate speculative formal parameter error locations.
   classifier->Accumulate(arrow_formals_classifier,
                          ExpressionClassifier::StandardProductions |
                              ExpressionClassifier::FormalParametersProductions);
@@ skipping 765 matching lines @@
       default:
         return expression;
     }
   }
   DCHECK(false);
   return this->EmptyExpression();
 }
 
 
 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;
 
   Token::Value next = peek();
   ExpressionT pattern = ParsePrimaryExpression(classifier, ok);
   if (!*ok) return;
 
   ValidateBindingPattern(classifier, ok);
   if (!*ok) return;
 
   if (!Traits::IsIdentifier(pattern)) {
     if (is_rest || !allow_harmony_destructuring()) {
       ReportUnexpectedToken(next);
       *ok = false;
       return;
     }
     parameters->is_simple = false;
     ValidateFormalParameterInitializer(classifier, ok);
     if (!*ok) return;
     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;
     initializer = ParseAssignmentExpression(true, &init_classifier, ok);
     if (!*ok) return;
     ValidateExpression(&init_classifier, ok);
     ValidateFormalParameterInitializer(&init_classifier, ok);
     if (!*ok) return;
     parameters->is_simple = false;
     classifier->RecordNonSimpleParameter();
@@ skipping 140 matching lines @@
 
     if (peek() == Token::LBRACE) {
       // Multiple statement body
       Consume(Token::LBRACE);
       bool is_lazily_parsed =
           (mode() == PARSE_LAZILY && scope_->AllowsLazyCompilation());
       if (is_lazily_parsed) {
         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,
             kArrowFunction, FunctionLiteral::ANONYMOUS_EXPRESSION, CHECK_OK);
         materialized_literal_count =
             function_state.materialized_literal_count();
         expected_property_count = function_state.expected_property_count();
       }
     } else {
       // Single-expression body
@@ skipping 246 matching lines @@
       *ok = false;
       return;
     }
     has_seen_constructor_ = true;
     return;
   }
 }
 } }  // v8::internal
 
 #endif  // V8_PREPARSER_H