Fix parsing of arrow function formal parameters

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 2970 matching lines @@
                                                bool* ok) {
   // ConditionalExpression ::
   //   LogicalOrExpression
   //   LogicalOrExpression '?' AssignmentExpression ':' AssignmentExpression
 
   int pos = peek_position();
   // We start using the binary expression parser for prec >= 4 only!
   ExpressionT expression =
       this->ParseBinaryExpression(4, accept_IN, classifier, CHECK_OK);
   if (peek() != Token::CONDITIONAL) return expression;
+  ArrowFormalParametersUnexpectedToken(classifier);
   BindingPatternUnexpectedToken(classifier);
   Consume(Token::CONDITIONAL);
   // In parsing the first assignment expression in conditional
   // expressions we always accept the 'in' keyword; see ECMA-262,
   // section 11.12, page 58.
   ExpressionT left = ParseAssignmentExpression(true, classifier, CHECK_OK);
   Expect(Token::COLON, CHECK_OK);
   ExpressionT right =
       ParseAssignmentExpression(accept_IN, classifier, CHECK_OK);
   return factory()->NewConditional(expression, left, right, pos);
 }
 
 
 // Precedence >= 4
 template <class Traits>
 typename ParserBase<Traits>::ExpressionT
 ParserBase<Traits>::ParseBinaryExpression(int prec, bool accept_IN,
                                           ExpressionClassifier* classifier,
                                           bool* ok) {
   DCHECK(prec >= 4);
   ExpressionT x = this->ParseUnaryExpression(classifier, CHECK_OK);
   for (int prec1 = Precedence(peek(), accept_IN); prec1 >= prec; prec1--) {
     // prec1 >= 4
     while (Precedence(peek(), accept_IN) == prec1) {
       BindingPatternUnexpectedToken(classifier);
+      ArrowFormalParametersUnexpectedToken(classifier);
       Token::Value op = Next();
       Scanner::Location op_location = scanner()->location();
       int pos = position();
       ExpressionT y =
           ParseBinaryExpression(prec1 + 1, accept_IN, classifier, CHECK_OK);
 
       if (this->ShortcutNumericLiteralBinaryExpression(&x, y, op, pos,
                                                        factory())) {
         continue;
       }
@@ skipping 42 matching lines @@
   //   '++' UnaryExpression
   //   '--' UnaryExpression
   //   '+' UnaryExpression
   //   '-' UnaryExpression
   //   '~' UnaryExpression
   //   '!' UnaryExpression
 
   Token::Value op = peek();
   if (Token::IsUnaryOp(op)) {
     BindingPatternUnexpectedToken(classifier);
+    ArrowFormalParametersUnexpectedToken(classifier);
 
     op = Next();
     int pos = position();
     ExpressionT expression = ParseUnaryExpression(classifier, CHECK_OK);
 
     if (op == Token::DELETE && is_strict(language_mode())) {
       if (is_strong(language_mode())) {
         ReportMessage(MessageTemplate::kStrongDelete);
         *ok = false;
         return this->EmptyExpression();
       } else if (this->IsIdentifier(expression)) {
         // "delete identifier" is a syntax error in strict mode.
         ReportMessage(MessageTemplate::kStrictDelete);
         *ok = false;
         return this->EmptyExpression();
       }
     }
 
     // Allow Traits do rewrite the expression.
     return this->BuildUnaryExpression(expression, op, pos, factory());
   } else if (Token::IsCountOp(op)) {
     BindingPatternUnexpectedToken(classifier);
+    ArrowFormalParametersUnexpectedToken(classifier);
     op = Next();
     int beg_pos = peek_position();
     ExpressionT expression = this->ParseUnaryExpression(classifier, CHECK_OK);
     expression = this->CheckAndRewriteReferenceExpression(
         expression, beg_pos, scanner()->location().end_pos,
         MessageTemplate::kInvalidLhsInPrefixOp, CHECK_OK);
     this->MarkExpressionAsAssigned(expression);
 
     return factory()->NewCountOperation(op,
                                         true /* prefix */,
@@ skipping 12 matching lines @@
                                            bool* ok) {
   // PostfixExpression ::
   //   LeftHandSideExpression ('++' | '--')?
 
   int lhs_beg_pos = peek_position();
   ExpressionT expression =
       this->ParseLeftHandSideExpression(classifier, CHECK_OK);
   if (!scanner()->HasAnyLineTerminatorBeforeNext() &&
       Token::IsCountOp(peek())) {
     BindingPatternUnexpectedToken(classifier);
+    ArrowFormalParametersUnexpectedToken(classifier);
 
     expression = this->CheckAndRewriteReferenceExpression(
         expression, lhs_beg_pos, scanner()->location().end_pos,
         MessageTemplate::kInvalidLhsInPostfixOp, CHECK_OK);
     expression = this->MarkExpressionAsAssigned(expression);
 
     Token::Value next = Next();
     expression =
         factory()->NewCountOperation(next,
                                      false /* postfix */,
@@ skipping 11 matching lines @@
   // LeftHandSideExpression ::
   //   (NewExpression | MemberExpression) ...
 
   ExpressionT result =
       this->ParseMemberWithNewPrefixesExpression(classifier, CHECK_OK);
 
   while (true) {
     switch (peek()) {
       case Token::LBRACK: {
         BindingPatternUnexpectedToken(classifier);
+        ArrowFormalParametersUnexpectedToken(classifier);
         Consume(Token::LBRACK);
         int pos = position();
         ExpressionT index = ParseExpression(true, classifier, CHECK_OK);
         result = factory()->NewProperty(result, index, pos);
         Expect(Token::RBRACK, CHECK_OK);
         break;
       }
 
       case Token::LPAREN: {
         BindingPatternUnexpectedToken(classifier);
+        ArrowFormalParametersUnexpectedToken(classifier);
 
         if (is_strong(language_mode()) && this->IsIdentifier(result) &&
             this->IsEval(this->AsIdentifier(result))) {
           ReportMessage(MessageTemplate::kStrongDirectEval);
           *ok = false;
           return this->EmptyExpression();
         }
         int pos;
         if (scanner()->current_token() == Token::IDENTIFIER) {
           // For call of an identifier we want to report position of
@@ skipping 41 matching lines @@
           result = factory()->NewAssignment(Token::INIT_CONST, this_expr,
                                             result, pos);
         }
 
         if (fni_ != NULL) fni_->RemoveLastFunction();
         break;
       }
 
       case Token::PERIOD: {
         BindingPatternUnexpectedToken(classifier);
+        ArrowFormalParametersUnexpectedToken(classifier);
         Consume(Token::PERIOD);
         int pos = position();
         IdentifierT name = ParseIdentifierName(CHECK_OK);
         result = factory()->NewProperty(
             result, factory()->NewStringLiteral(name, pos), pos);
         if (fni_ != NULL) this->PushLiteralName(fni_, name);
         break;
       }
 
       case Token::TEMPLATE_SPAN:
       case Token::TEMPLATE_TAIL: {
         BindingPatternUnexpectedToken(classifier);
+        ArrowFormalParametersUnexpectedToken(classifier);
         result = ParseTemplateLiteral(result, position(), classifier, CHECK_OK);
         break;
       }
 
       default:
         return result;
     }
   }
 }
 
@@ skipping 17 matching lines @@
   // Examples of new expression:
   // new foo.bar().baz means (new (foo.bar)()).baz
   // new foo()() means (new foo())()
   // new new foo()() means (new (new foo())())
   // new new foo means new (new foo)
   // new new foo() means new (new foo())
   // new new foo().bar().baz means (new (new foo()).bar()).baz
 
   if (peek() == Token::NEW) {
     BindingPatternUnexpectedToken(classifier);
+    ArrowFormalParametersUnexpectedToken(classifier);
     Consume(Token::NEW);
     int new_pos = position();
     ExpressionT result = this->EmptyExpression();
     if (peek() == Token::SUPER) {
       const bool is_new = true;
       result = ParseSuperExpression(is_new, classifier, CHECK_OK);
     } else if (allow_harmony_new_target() && peek() == Token::PERIOD) {
       return ParseNewTargetExpression(CHECK_OK);
     } else {
       result = this->ParseMemberWithNewPrefixesExpression(classifier, CHECK_OK);
@@ skipping 33 matching lines @@
   //     ('[' Expression ']' | '.' Identifier | Arguments | TemplateLiteral)*
 
   // The '[' Expression ']' and '.' Identifier parts are parsed by
   // ParseMemberExpressionContinuation, and the Arguments part is parsed by the
   // caller.
 
   // Parse the initial primary or function expression.
   ExpressionT result = this->EmptyExpression();
   if (peek() == Token::FUNCTION) {
     BindingPatternUnexpectedToken(classifier);
+    ArrowFormalParametersUnexpectedToken(classifier);
 
     Consume(Token::FUNCTION);
     int function_token_position = position();
     bool is_generator = Check(Token::MUL);
     IdentifierT name = this->EmptyIdentifier();
     bool is_strict_reserved_name = false;
     Scanner::Location function_name_location = Scanner::Location::invalid();
     FunctionLiteral::FunctionType function_type =
         FunctionLiteral::ANONYMOUS_EXPRESSION;
     if (peek_any_identifier()) {
@@ skipping 223 matching lines @@
 template <class Traits>
 typename ParserBase<Traits>::ExpressionT
 ParserBase<Traits>::ParseMemberExpressionContinuation(
     ExpressionT expression, ExpressionClassifier* classifier, bool* ok) {
   // Parses this part of MemberExpression:
   // ('[' Expression ']' | '.' Identifier | TemplateLiteral)*
   while (true) {
     switch (peek()) {
       case Token::LBRACK: {
         BindingPatternUnexpectedToken(classifier);
+        ArrowFormalParametersUnexpectedToken(classifier);
 
         Consume(Token::LBRACK);
         int pos = position();
         ExpressionT index = this->ParseExpression(true, classifier, CHECK_OK);
         expression = factory()->NewProperty(expression, index, pos);
         if (fni_ != NULL) {
           this->PushPropertyName(fni_, index);
         }
         Expect(Token::RBRACK, CHECK_OK);
         break;
       }
       case Token::PERIOD: {
         BindingPatternUnexpectedToken(classifier);
+        ArrowFormalParametersUnexpectedToken(classifier);

[caitp (gmail), 2015/08/20 15:22:57]

probably want to make sure destructuring still works properly, `({ x: x =
foo.bar }) => {}` for example. I don't expect `({ x = foo.bar }) => {}` to work
properly because CoverInitializedName parsing is pretty messed up in
maybe-expressions, might be worth marking that as a TODO though in the test case

This also applies to other tokens which can legally occur in binding patterns
within arrow formals, I guess.

If this is covered by other tests, then that's ok

[wingo, 2015/08/20 15:47:57]

Thanks for having a look!

I'm pretty sure that destructuring should still work properly, and the reasoning
is a bit convoluted: an assignmentexpression is a valid arrow formal parameter
list if it is a bare identifier, or if it starts with a parenthesis, only
contains binding patterns separated by commas, possibly ending in a rest pattern
(which, oddly, can't contain patterns).  OK.  That is what was already the case
before and is covered by DestructuringPositiveTests and
DestructuringNegativeTests in test-parser.cc.

The way the above classifier works is that when parsing a parenthesized list in
ParsePrimaryExpression, the expression as a whole remains a valid
ArrowFormalParameterList iff the inner productions are valid BindingPatterns. 
Specifically the invalid-ArrowFormalParameterList *does not* accumulate out from
the individual arguments -- instead it's the BindingPattern flag that propagates
out.  See the gnarly special case in ExpressionClassifier::Accumulate.  So I
don't think additional tests are necessary for existing destructuring foo.

What we missed is that the classifier would accept things as valid
ArrowFormalParameterLists that weren't valid.  That's what this patch fixes. 
These additional flags won't cause problems for arrow function formals because
they are explicitly ignored there.

 
         Consume(Token::PERIOD);
         int pos = position();
         IdentifierT name = ParseIdentifierName(CHECK_OK);
         expression = factory()->NewProperty(
             expression, factory()->NewStringLiteral(name, pos), pos);
         if (fni_ != NULL) {
           this->PushLiteralName(fni_, name);
         }
         break;
       }
       case Token::TEMPLATE_SPAN:
       case Token::TEMPLATE_TAIL: {
         BindingPatternUnexpectedToken(classifier);
+        ArrowFormalParametersUnexpectedToken(classifier);
         int pos;
         if (scanner()->current_token() == Token::IDENTIFIER) {
           pos = position();
         } else {
           pos = peek_position();
           if (expression->IsFunctionLiteral() && mode() == PARSE_EAGERLY) {
             // If the tag function looks like an IIFE, set_parenthesized() to
             // force eager compilation.
             expression->AsFunctionLiteral()->set_should_eager_compile();
           }
@@ skipping 430 matching lines @@
       *ok = false;
       return;
     }
     has_seen_constructor_ = true;
     return;
   }
 }
 } }  // v8::internal
 
 #endif  // V8_PREPARSER_H