[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 685 matching lines @@
   void AddTemplateExpression(ExpressionT);
   ExpressionT ParseSuperExpression(bool is_new,
                                    ExpressionClassifier* classifier, bool* ok);
   ExpressionT ParseNewTargetExpression(bool* ok);
   ExpressionT ParseStrongInitializationExpression(
       ExpressionClassifier* classifier, bool* ok);
   ExpressionT ParseStrongSuperCallExpression(ExpressionClassifier* classifier,
                                              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(
       int param_count, FunctionLiteral::ArityRestriction arity_restriction,
       bool has_rest, int formals_start_pos, int formals_end_pos, bool* ok);
 
   // Checks if the expression is a valid reference expression (e.g., on the
   // left-hand side of assignments). Although ruled out by ECMA as early errors,
   // we allow calls for web compatibility and rewrite them to a runtime throw.
   ExpressionT CheckAndRewriteReferenceExpression(
@@ skipping 193 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) |
+        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() {
     return PreParserExpression(TypeField::encode(kStringLiteralExpression));
   }
 
   static PreParserExpression UseStrictStringLiteral() {
     return PreParserExpression(TypeField::encode(kStringLiteralExpression) |
                                IsUseStrictField::encode(true));
   }
@@ skipping 34 matching lines @@
   static PreParserExpression NoTemplateTag() {
     return PreParserExpression(
         TypeField::encode(kExpression) |
         ExpressionTypeField::encode(kNoTemplateTagExpression));
   }
 
   bool IsIdentifier() const {
     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_));
   }
 
   bool IsStringLiteral() const {
     return TypeField::decode(code_) == kStringLiteralExpression;
   }
 
   bool IsUseStrictLiteral() const {
@@ skipping 42 matching lines @@
 
   bool IsNoTemplateTag() const {
     return TypeField::decode(code_) == kExpression &&
            ExpressionTypeField::decode(code_) == kNoTemplateTagExpression;
   }
 
   bool IsSpreadExpression() const {
     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 {
     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<Token::Value, TypeField::kNext, 8> BinaryOperationTokenField;
+  typedef BitField<bool, BinaryOperationTokenField::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 123 matching lines @@
                                        int literal_index,
                                        int boilerplate_properties,
                                        bool has_function,
                                        bool is_strong,
                                        int pos) {
     return PreParserExpression::Default();
   }
   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) {
     if (obj.IsThis()) {
       return PreParserExpression::ThisProperty();
     }
     return PreParserExpression::Property();
   }
   PreParserExpression NewUnaryOperation(Token::Value op,
                                         PreParserExpression expression,
@@ skipping 609 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.HasRestParameter()) {
+    ++parameters->materialized_literals_count;
+    pre_parser_->function_state_->NextMaterializedLiteralIndex();
+  }
 }
 
 
 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 988 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,

[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);
-    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 764 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,
+                                              int start_pos, 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;
     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;
     initializer = ParseAssignmentExpression(true, &init_classifier, ok);
     if (!*ok) return;
     ValidateExpression(&init_classifier, ok);
     if (!*ok) return;
     parameters->is_simple = false;
     classifier->RecordNonSimpleParameter();
   }
@@ skipping 14 matching lines @@
   //   FormalsList[?Yield, ?GeneratorParameter]
   //   FormalsList[?Yield, ?GeneratorParameter] , FunctionRestParameter[?Yield]
   //
   // FormalsList[Yield,GeneratorParameter] :
   //   FormalParameter[?Yield, ?GeneratorParameter]
   //   FormalsList[?Yield, ?GeneratorParameter] ,
   //     FormalParameter[?Yield,?GeneratorParameter]
 
   DCHECK_EQ(0, parameters->Arity());
 
+  int parameter_start;
   if (peek() != Token::RPAREN) {
     do {
       if (parameters->Arity() > Code::kMaxArguments) {
         ReportMessage(MessageTemplate::kTooManyParameters);
         *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));
 
     if (parameters->has_rest) {
       parameters->is_simple = false;
       classifier->RecordNonSimpleParameter();
       if (peek() == Token::COMMA) {
         ReportMessageAt(scanner()->peek_location(),
                       MessageTemplate::kParamAfterRest);
         *ok = false;
@@ skipping 74 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.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,
             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