[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 07e431bfc54790b0a65c081f9ea8021ab6fba84c..8f7764730257a1bc1a1ecac423fa177971a3dbc5 100644
--- a/src/preparser.h
+++ b/src/preparser.h
@@ -703,7 +703,12 @@ class ParserBase : public Traits {
                                              bool* ok);
 
   void ParseFormalParameter(FormalParametersT* parameters,
-                            ExpressionClassifier* classifier, bool* ok);
+                            ExpressionClassifier* classifier, int start_pos,
+                            bool* ok);
+  inline void ParseFormalParameter(FormalParametersT* parameters,

[adamk, 2015/08/28 23:26:37]

Is it really worth having two versions of this method? I don't think there are
so many callers that you couldn't just do the peek_position() at the callsites
and leave out the overload.

[caitp (gmail), 2015/08/28 23:58:01]

Sure, this way just seemed like less work than hunting down all the callers.
I'll make that change during the next round of cleanup

[caitp (gmail), 2015/08/29 15:51:59]

Method is gone, isn't needed without the AST node (might make errors slightly
uglier though)

+                                   ExpressionClassifier* classifier, bool* ok) {
+    ParseFormalParameter(parameters, classifier, peek_position(), ok);
+  }
   void ParseFormalParameterList(FormalParametersT* parameters,
                                 ExpressionClassifier* classifier, bool* ok);
   void CheckArityRestrictions(
@@ -917,7 +922,10 @@ class PreParserExpression {
   static PreParserExpression BinaryOperation(PreParserExpression left,
                                              Token::Value op,
                                              PreParserExpression right) {
-    return PreParserExpression(TypeField::encode(kBinaryOperationExpression));
+    return PreParserExpression(
+        TypeField::encode(kBinaryOperationExpression) |
+        BinaryOperationTokenField::encode(op) |

[adamk, 2015/08/28 23:26:37]

This seems overly general. Can you get away with just:

HasRestField::encode(op == Token::COMMA && right->IsSpreadExpression())

It doesn't look to me like you're using the token for anything else; see below
on HasRestParameter for the rest of this comment.

[caitp (gmail), 2015/08/28 23:58:01]

Acknowledged

+        HasRestField::encode(right->IsSpreadExpression()));
   }
 
   static PreParserExpression StringLiteral() {
@@ -972,6 +980,8 @@ class PreParserExpression {
     return TypeField::decode(code_) == kIdentifierExpression;
   }
 
+  PreParserExpression AsVariableProxy() const { return *this; }

[adamk, 2015/08/28 23:26:37]

If you end up needing this, it seems like it should have a
DCHECK(IsIdentifier())

[caitp (gmail), 2015/08/28 23:58:01]

If it only returns false for `this`, that's probably okay. Acknowledged

[caitp (gmail), 2015/08/29 15:51:59]

It's not needed without the new AST node, so it's gone anyways

+
   PreParserIdentifier AsIdentifier() const {
     DCHECK(IsIdentifier());
     return PreParserIdentifier(IdentifierTypeField::decode(code_));
@@ -1034,6 +1044,14 @@ class PreParserExpression {
     return TypeField::decode(code_) == kSpreadExpression;
   }
 
+  bool HasRestParameter() const {
+    // Preparser's way of determining if an arrow function has a rest parameter.
+    return IsSpreadExpression() ||
+           (IsBinaryOperation() &&
+            BinaryOperationTokenField::decode(code_) == Token::COMMA &&

[adamk, 2015/08/28 23:26:37]

I'm not sure that you're getting much from this token field. Wouldn't the
following expression also return true for HasRestParameter?

"b + c, ...d"

Which clearly isn't a valid parameter list. Given the narrow usage of this Token
field, it seems like it might make more sense to directly encode the bit you
care about.

[caitp (gmail), 2015/08/28 23:58:01]

It's important for making lazy parsing work with arrow functions :( The
pre-parser needs to be able to figure out that it needs to increase the
materialized literal count for the new function. Without it, you end up with a
segfault, which isn't ideal :(
I think (hope) that `b + c` as a parameter would cause the classifier to record
an arrow formal parameters error. The flag is used only after the parameters
have been determined to be valid, so this situation doesn't occur.

I could leave a comment indicating the conditions under which it can be used, if
that helps

[adamk, 2015/08/29 01:01:07]

I think I wasn't as clear as I could have been. I understand what you're trying
to detect here (and why it's necessary), I just don't think you need to encode
as much information as you're currently encoding.

Basically, the only binary op whose token you care about is the one whose RHS is
a Spread. So you should be able to handle the COMMA check when you're creating
that binary op.

The advantage of doing it that way is you don't need 8 bits of
PreParserExpression space.

[caitp (gmail), 2015/08/29 15:51:59]

Done.

+            HasRestField::decode(code_));
+  }
+
   PreParserExpression AsFunctionLiteral() { return *this; }
 
   bool IsBinaryOperation() const {
@@ -1082,6 +1100,8 @@ class PreParserExpression {
   typedef BitField<bool, IsUseStrictField::kNext, 1> IsUseStrongField;
   typedef BitField<PreParserIdentifier::Type, TypeField::kNext, 10>
       IdentifierTypeField;
+  typedef BitField<Token::Value, TypeField::kNext, 8> BinaryOperationTokenField;
+  typedef BitField<bool, BinaryOperationTokenField::kNext, 1> HasRestField;
 
   uint32_t code_;
 };
@@ -1225,6 +1245,10 @@ class PreParserFactory {
   PreParserExpression NewVariableProxy(void* variable) {
     return PreParserExpression::Default();
   }
+  PreParserExpression NewRestParameter(PreParserExpression param, int index,
+                                       int position) {
+    return PreParserExpression::Default();
+  }
   PreParserExpression NewProperty(PreParserExpression obj,
                                   PreParserExpression key,
                                   int pos) {
@@ -1854,6 +1878,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.HasRestParameter()) {
+    ++parameters->materialized_literals_count;
+    pre_parser_->function_state_->NextMaterializedLiteralIndex();
+  }
 }
 
 
@@ -2862,16 +2892,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,

[adamk, 2015/08/28 23:26:37]

So this had to move up because it now affects materialized_literals_count?

[caitp (gmail), 2015/08/28 23:58:01]

Yeah :( I tried to find a better way to do it, but storing this info in the
ExpressionClassifier was a bit problematic.

Would a comment help indicate the reasoning for moving it?

[adamk, 2015/08/29 01:01:07]

No need for a comment, as long as a test will fail if these are reordered again.
I was mostly checking that my assumption was correct; after this patch, this
order seems fine too.

                                                 &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;
@@ -3656,8 +3689,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,
+                                              int start_pos, bool* ok) {
   // FormalParameter[Yield,GeneratorParameter] :
   //   BindingElement[?Yield, ?GeneratorParameter]
   bool is_rest = parameters->has_rest;
@@ -3679,6 +3713,13 @@ void ParserBase<Traits>::ParseFormalParameter(
     classifier->RecordNonSimpleParameter();
   }
 
+  if (is_rest) {
+    pattern = factory()->NewRestParameter(
+        pattern->AsVariableProxy(),
+        function_state_->NextMaterializedLiteralIndex(), start_pos);
+    ++parameters->materialized_literals_count;
+  }
+
   ExpressionT initializer = Traits::EmptyExpression();
   if (!is_rest && allow_harmony_default_parameters() && Check(Token::ASSIGN)) {
     ExpressionClassifier init_classifier;
@@ -3713,6 +3754,7 @@ void ParserBase<Traits>::ParseFormalParameterList(
 
   DCHECK_EQ(0, parameters->Arity());
 
+  int parameter_start;
   if (peek() != Token::RPAREN) {
     do {
       if (parameters->Arity() > Code::kMaxArguments) {
@@ -3720,9 +3762,11 @@ void ParserBase<Traits>::ParseFormalParameterList(
         *ok = false;
         return;
       }
+      parameter_start = peek_position();
       parameters->has_rest =
           allow_harmony_rest_parameters() && Check(Token::ELLIPSIS);
-      ParseFormalParameter(parameters, classifier, ok);
+
+      ParseFormalParameter(parameters, classifier, parameter_start, ok);
       if (!*ok) return;
     } while (!parameters->has_rest && Check(Token::COMMA));
 
@@ -3817,6 +3861,12 @@ ParserBase<Traits>::ParseArrowFunctionLiteral(
         body = this->NewStatementList(0, zone());
         this->SkipLazyFunctionBody(&materialized_literal_count,
                                    &expected_property_count, CHECK_OK);
+
+        if (formal_parameters.has_rest) {
+          DCHECK(formal_parameters.materialized_literals_count > 0);

[adamk, 2015/08/28 23:26:37]

Would this ever be anything other than 1 in this case?

[caitp (gmail), 2015/08/28 23:58:01]

Yes --- I believe initializers with materialized literal values can increase the
value

In fact, it turns out that there's a bug in default parameters with lazy
parsing, so in fact we really need this to happen whenever
`materialized_literals_count > 0` and not just when has_rest is true.

I'll file a bug about that.

[adamk, 2015/08/29 01:01:07]

Ah, I meant to say "can this ever happen without default params", but I guess
we're pretty much living in that world already. Not surprised to hear there are
more bugs in that stuff, look forward to your report.

[caitp (gmail), 2015/08/29 15:51:59]

I've changed it to always add the formals materialized literals whether has_rest
is true or not, but as noted in the bug I filed
(https://code.google.com/p/v8/issues/detail?id=4400) it doesn't actually solve
the problem for default arguments, which is a bit more complicated.

+          materialized_literal_count +=
+              formal_parameters.materialized_literals_count;
+        }
       } else {
         body = this->ParseEagerFunctionBody(
             this->EmptyIdentifier(), RelocInfo::kNoPosition, formal_parameters,