Use ExpressionClassifier to identify valid arrow function formals

src/preparser.h

Index: src/preparser.h
diff --git a/src/preparser.h b/src/preparser.h
index 541da23af1acfb83b59426c34e664195844bb9d5..aef56a2610b4c5b7c2afa34104406b17ab50fe95 100644
--- a/src/preparser.h
+++ b/src/preparser.h
@@ -34,6 +34,15 @@ class FormalParameterErrorLocations BASE_EMBEDDED {
   Scanner::Location undefined;
   Scanner::Location duplicate;
   Scanner::Location reserved;
+
+  void Accumulate(const FormalParameterErrorLocations& inner) {
+    if (!eval_or_arguments.IsValid()) {
+      eval_or_arguments = inner.eval_or_arguments;
+    }
+    if (!undefined.IsValid()) undefined = inner.undefined;
+    if (!duplicate.IsValid()) duplicate = inner.duplicate;
+    if (!reserved.IsValid()) reserved = inner.reserved;
+  }
 };
 
 
@@ -603,7 +612,9 @@ class ParserBase : public Traits {
       bool HasError() const { return location.IsValid(); }
     };
 
-    ExpressionClassifier() {}
+    ExpressionClassifier() : formal_parameter_error_locs_(nullptr) {}
+    explicit ExpressionClassifier(FormalParameterErrorLocations* error_locs)
+        : formal_parameter_error_locs_(error_locs) {}
 
     bool is_valid_expression() const { return !expression_error_.HasError(); }
 
@@ -615,6 +626,10 @@ class ParserBase : public Traits {
       return !assignment_pattern_error_.HasError();
     }
 
+    bool is_valid_arrow_formal_parameters() const {
+      return !arrow_formal_parameters_error_.HasError();
+    }
+
     const Error& expression_error() const { return expression_error_; }
 
     const Error& binding_pattern_error() const {
@@ -625,6 +640,14 @@ class ParserBase : public Traits {
       return assignment_pattern_error_;
     }
 
+    const Error& arrow_formal_parameters_error() const {
+      return arrow_formal_parameters_error_;
+    }
+
+    FormalParameterErrorLocations* formal_parameter_error_locs() const {
+      return formal_parameter_error_locs_;
+    }
+
     void RecordExpressionError(const Scanner::Location& loc,
                                const char* message, const char* arg = nullptr) {
       if (!is_valid_expression()) return;
@@ -651,10 +674,80 @@ class ParserBase : public Traits {
       assignment_pattern_error_.arg = arg;
     }
 
+    void RecordArrowFormalParametersError(const Scanner::Location& loc,

[Dmitry Lomov (no reviews), 2015/05/12 15:12:20]

Instead of having all these methods, I suggest merging
FormalParameterErrorLocations into ExpressionClassifier and just use one common
mechanism to classify all expression-like constructs.

+                                          const char* message,
+                                          const char* arg = nullptr) {
+      if (!is_valid_arrow_formal_parameters()) return;
+      arrow_formal_parameters_error_.location = loc;
+      arrow_formal_parameters_error_.message = message;
+      arrow_formal_parameters_error_.arg = arg;
+    }
+
+    void RecordEvalOrArguments(const Scanner::Location& loc) {
+      if (!formal_parameter_error_locs_) return;
+      if (!formal_parameter_error_locs_->eval_or_arguments.IsValid()) {
+        formal_parameter_error_locs_->eval_or_arguments = loc;
+      }
+    }
+
+    void RecordUndefined(const Scanner::Location& loc) {
+      if (!formal_parameter_error_locs_) return;
+      if (!formal_parameter_error_locs_->undefined.IsValid()) {
+        formal_parameter_error_locs_->undefined = loc;
+      }
+    }
+
+    void RecordDuplicate(const Scanner::Location& loc) {
+      if (!formal_parameter_error_locs_) return;
+      if (!formal_parameter_error_locs_->duplicate.IsValid()) {
+        formal_parameter_error_locs_->duplicate = loc;
+      }
+    }
+
+    void RecordFutureStrictReserved(const Scanner::Location& loc) {
+      if (!formal_parameter_error_locs_) return;
+      if (!formal_parameter_error_locs_->reserved.IsValid()) {
+        formal_parameter_error_locs_->reserved = loc;
+      }
+    }
+
+    enum TargetProduction {
+      ExpressionProduction = 1 << 0,
+      BindingPatternProduction = 1 << 1,
+      AssignmentPatternProduction = 1 << 2,
+      ArrowFormalParametersProduction = 1 << 3,
+      StandardProductions = (ExpressionProduction | BindingPatternProduction |
+                             AssignmentPatternProduction),
+      AllProductions = StandardProductions | ArrowFormalParametersProduction
+    };
+
+    void Accumulate(const ExpressionClassifier& inner,
+                    unsigned productions = StandardProductions) {
+      if (productions & ExpressionProduction && is_valid_expression()) {
+        expression_error_ = inner.expression_error_;
+      }
+      if (productions & BindingPatternProduction &&
+          is_valid_binding_pattern()) {
+        binding_pattern_error_ = inner.binding_pattern_error_;
+      }
+      if (productions & AssignmentPatternProduction &&
+          is_valid_assignment_pattern()) {
+        assignment_pattern_error_ = inner.assignment_pattern_error_;
+      }
+      if (productions & ArrowFormalParametersProduction &&
+          is_valid_arrow_formal_parameters()) {
+        // The result continues to be a valid arrow formal parameters if the
+        // inner expression is a valid binding pattern.
+        arrow_formal_parameters_error_ = inner.binding_pattern_error_;
+      }
+    }
+
    private:
     Error expression_error_;
     Error binding_pattern_error_;
     Error assignment_pattern_error_;
+    Error arrow_formal_parameters_error_;
+    FormalParameterErrorLocations* formal_parameter_error_locs_;
   };
 
   void ReportClassifierError(
@@ -686,9 +779,29 @@ class ParserBase : public Traits {
     }
   }
 
+  void ValidateArrowFormalParameters(const ExpressionClassifier* classifier,
+                                     ExpressionT expr, bool* ok) {
+    if (classifier->is_valid_binding_pattern()) {
+      // A simple arrow formal parameter: IDENTIFIER => BODY.
+      if (!this->IsIdentifier(expr)) {
+        Traits::ReportMessageAt(scanner()->location(), "unexpected_token",
+                                Token::String(scanner()->current_token()));
+        *ok = false;
+      }
+    } else if (!classifier->is_valid_arrow_formal_parameters()) {
+      ReportClassifierError(classifier->arrow_formal_parameters_error());
+      *ok = false;
+    }
+  }
+
   void BindingPatternUnexpectedToken(ExpressionClassifier* classifier) {
     classifier->RecordBindingPatternError(
-        scanner()->location(), "unexpected_token", Token::String(peek()));
+        scanner()->peek_location(), "unexpected_token", Token::String(peek()));
+  }
+
+  void ArrowFormalParametersUnexpectedToken(ExpressionClassifier* classifier) {
+    classifier->RecordArrowFormalParametersError(
+        scanner()->peek_location(), "unexpected_token", Token::String(peek()));
   }
 
   // Recursive descent functions:
@@ -752,7 +865,7 @@ class ParserBase : public Traits {
       ExpressionT expression, ExpressionClassifier* classifier, bool* ok);
   ExpressionT ParseArrowFunctionLiteral(
       Scope* function_scope, const FormalParameterErrorLocations& error_locs,
-      bool has_rest, ExpressionClassifier* classifier, bool* ok);
+      bool has_rest, bool* ok);
   ExpressionT ParseTemplateLiteral(ExpressionT tag, int start,
                                    ExpressionClassifier* classifier, bool* ok);
   void AddTemplateExpression(ExpressionT);
@@ -973,14 +1086,7 @@ class PreParserExpression {
   static PreParserExpression BinaryOperation(PreParserExpression left,
                                              Token::Value op,
                                              PreParserExpression right) {
-    ValidArrowParam valid_arrow_param_list =
-        (op == Token::COMMA && !left.is_single_parenthesized() &&
-         !right.is_single_parenthesized())
-            ? std::min(left.ValidateArrowParams(), right.ValidateArrowParams())
-            : kInvalidArrowParam;
-    return PreParserExpression(
-        TypeField::encode(kBinaryOperationExpression) |
-        IsValidArrowParamListField::encode(valid_arrow_param_list));
+    return PreParserExpression(TypeField::encode(kBinaryOperationExpression));
   }
 
   static PreParserExpression StringLiteral() {
@@ -1073,30 +1179,6 @@ class PreParserExpression {
     return IsIdentifier() || IsProperty();
   }
 
-  bool IsValidArrowParamList(FormalParameterErrorLocations* locs,
-                             const Scanner::Location& params_loc) const {
-    ValidArrowParam valid = ValidateArrowParams();
-    if (ParenthesizationField::decode(code_) == kMultiParenthesizedExpression) {
-      return false;
-    }
-    switch (valid) {
-      case kInvalidArrowParam:
-        return false;
-      case kInvalidStrongArrowParam:
-        locs->undefined = params_loc;
-        return true;
-      case kInvalidStrictReservedArrowParam:
-        locs->reserved = params_loc;
-        return true;
-      case kInvalidStrictEvalArgumentsArrowParam:
-        locs->eval_or_arguments = params_loc;
-        return true;
-      default:
-        DCHECK_EQ(valid, kValidArrowParam);
-        return true;
-    }
-  }
-
   // At the moment PreParser doesn't track these expression types.
   bool IsFunctionLiteral() const { return false; }
   bool IsCallNew() const { return false; }
@@ -1160,43 +1242,9 @@ class PreParserExpression {
     kNoTemplateTagExpression
   };
 
-  // These validity constraints are ordered such that a value of N implies lack
-  // of errors M < N.
-  enum ValidArrowParam {
-    kInvalidArrowParam,
-    kInvalidStrictEvalArgumentsArrowParam,
-    kInvalidStrictReservedArrowParam,
-    kInvalidStrongArrowParam,
-    kValidArrowParam
-  };
-
   explicit PreParserExpression(uint32_t expression_code)
       : code_(expression_code) {}
 
-  V8_INLINE ValidArrowParam ValidateArrowParams() const {
-    if (IsBinaryOperation()) {
-      return IsValidArrowParamListField::decode(code_);
-    }
-    if (!IsIdentifier()) {
-      return kInvalidArrowParam;
-    }
-    PreParserIdentifier ident = AsIdentifier();
-    // In strict mode, eval and arguments are not valid formal parameter names.
-    if (ident.IsEval() || ident.IsArguments()) {
-      return kInvalidStrictEvalArgumentsArrowParam;
-    }
-    // In strict mode, future reserved words are not valid either, and as they
-    // produce different errors we allot them their own error code.
-    if (ident.IsFutureStrictReserved()) {
-      return kInvalidStrictReservedArrowParam;
-    }
-    // In strong mode, 'undefined' isn't a valid formal parameter name either.
-    if (ident.IsUndefined()) {
-      return kInvalidStrongArrowParam;
-    }
-    return kValidArrowParam;
-  }
-
   // The first five bits are for the Type and Parenthesization.
   typedef BitField<Type, 0, 3> TypeField;
   typedef BitField<Parenthesization, TypeField::kNext, 2> ParenthesizationField;
@@ -1207,8 +1255,6 @@ class PreParserExpression {
       ExpressionTypeField;
   typedef BitField<bool, ParenthesizationField::kNext, 1> IsUseStrictField;
   typedef BitField<bool, IsUseStrictField::kNext, 1> IsUseStrongField;
-  typedef BitField<ValidArrowParam, ParenthesizationField::kNext, 3>
-      IsValidArrowParamListField;
   typedef BitField<PreParserIdentifier::Type, ParenthesizationField::kNext, 10>
       IdentifierTypeField;
 
@@ -1932,11 +1978,6 @@ void PreParserTraits::ParseArrowFunctionFormalParameters(
     FormalParameterErrorLocations* error_locs, bool* is_rest, bool* ok) {
   // TODO(wingo): Detect duplicated identifiers in paramlists.  Detect parameter
   // lists that are too long.
-  if (!params.IsValidArrowParamList(error_locs, params_loc)) {
-    *ok = false;
-    ReportMessageAt(params_loc, "malformed_arrow_function_parameter_list");
-    return;
-  }
 }
 
 
@@ -2061,16 +2102,22 @@ ParserBase<Traits>::ParseAndClassifyIdentifier(ExpressionClassifier* classifier,
   Token::Value next = Next();
   if (next == Token::IDENTIFIER) {
     IdentifierT name = this->GetSymbol(scanner());
-    if (is_strict(language_mode()) && this->IsEvalOrArguments(name)) {
-      classifier->RecordBindingPatternError(scanner()->location(),
-                                            "strict_eval_arguments");
+    if (this->IsEvalOrArguments(name)) {
+      classifier->RecordEvalOrArguments(scanner()->location());
+      if (is_strict(language_mode())) {
+        classifier->RecordBindingPatternError(scanner()->location(),
+                                              "strict_eval_arguments");
+      }
     }
-    if (is_strong(language_mode()) && this->IsUndefined(name)) {
-      // TODO(dslomov): allow 'undefined' in nested patterns.
-      classifier->RecordBindingPatternError(scanner()->location(),
-                                            "strong_undefined");
-      classifier->RecordAssignmentPatternError(scanner()->location(),
-                                               "strong_undefined");
+    if (this->IsUndefined(name)) {
+      classifier->RecordUndefined(scanner()->location());
+      if (is_strong(language_mode())) {
+        // TODO(dslomov): allow 'undefined' in nested patterns.
+        classifier->RecordBindingPatternError(scanner()->location(),
+                                              "strong_undefined");
+        classifier->RecordAssignmentPatternError(scanner()->location(),
+                                                 "strong_undefined");
+      }
     }
     if (is_strong(language_mode()) && this->IsArguments(name)) {
       classifier->RecordExpressionError(scanner()->location(),
@@ -2082,6 +2129,7 @@ ParserBase<Traits>::ParseAndClassifyIdentifier(ExpressionClassifier* classifier,
              (next == Token::FUTURE_STRICT_RESERVED_WORD ||
               next == Token::LET || next == Token::STATIC ||
               (next == Token::YIELD && !is_generator()))) {
+    classifier->RecordFutureStrictReserved(scanner()->location());
     return this->GetSymbol(scanner());
   } else {
     this->ReportUnexpectedToken(next);
@@ -2270,24 +2318,41 @@ ParserBase<Traits>::ParsePrimaryExpression(ExpressionClassifier* classifier,
       break;
 
     case Token::LBRACK:
+      if (!allow_harmony_destructuring()) {
+        BindingPatternUnexpectedToken(classifier);
+      }
       result = this->ParseArrayLiteral(classifier, CHECK_OK);
       break;
 
     case Token::LBRACE:
+      if (!allow_harmony_destructuring()) {
+        BindingPatternUnexpectedToken(classifier);
+      }
       result = this->ParseObjectLiteral(classifier, CHECK_OK);
       break;
 
     case Token::LPAREN:
+      // Arrow function formal parameters are either a single identifier or a
+      // list of BindingPattern productions enclosed in parentheses.
+      // Parentheses are not valid on the LHS of a BindingPattern, so we use the
+      // is_valid_binding_pattern() check to detect multiple levels of
+      // parenthesization.
+      if (!classifier->is_valid_binding_pattern()) {
+        ArrowFormalParametersUnexpectedToken(classifier);
+      }
       BindingPatternUnexpectedToken(classifier);
       Consume(Token::LPAREN);
       if (allow_harmony_arrow_functions() && Check(Token::RPAREN)) {
         // As a primary expression, the only thing that can follow "()" is "=>".
+        classifier->RecordBindingPatternError(scanner()->location(),
+                                              "unexpected_token",
+                                              Token::String(Token::RPAREN));
         Scope* scope = this->NewScope(scope_, ARROW_SCOPE);
         scope->set_start_position(beg_pos);
         FormalParameterErrorLocations error_locs;
         bool has_rest = false;
         result = this->ParseArrowFunctionLiteral(scope, error_locs, has_rest,
-                                                 classifier, CHECK_OK);
+                                                 CHECK_OK);
       } else {
         // Heuristically try to detect immediately called functions before
         // seeing the call parentheses.
@@ -2364,13 +2429,19 @@ typename ParserBase<Traits>::ExpressionT ParserBase<Traits>::ParseExpression(
   //   AssignmentExpression
   //   Expression ',' AssignmentExpression
 
+  ExpressionClassifier binding_classifier(
+      classifier->formal_parameter_error_locs());
   ExpressionT result =
-      this->ParseAssignmentExpression(accept_IN, classifier, CHECK_OK);
+      this->ParseAssignmentExpression(accept_IN, &binding_classifier, CHECK_OK);
+  classifier->Accumulate(binding_classifier,
+                         ExpressionClassifier::AllProductions);
   while (peek() == Token::COMMA) {
     Expect(Token::COMMA, CHECK_OK);
     int pos = position();
-    ExpressionT right =
-        this->ParseAssignmentExpression(accept_IN, classifier, CHECK_OK);
+    ExpressionT right = this->ParseAssignmentExpression(
+        accept_IN, &binding_classifier, CHECK_OK);
+    classifier->Accumulate(binding_classifier,
+                           ExpressionClassifier::AllProductions);
     result = factory()->NewBinaryOperation(Token::COMMA, result, right, pos);
   }
   return result;
@@ -2756,29 +2827,45 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
 
   if (fni_ != NULL) fni_->Enter();
   ParserBase<Traits>::Checkpoint checkpoint(this);
-  ExpressionT expression =
-      this->ParseConditionalExpression(accept_IN, classifier, CHECK_OK);
+  FormalParameterErrorLocations formals_error_locs;
+  ExpressionClassifier arrow_formals_classifier(&formals_error_locs);
+  if (peek() != Token::LPAREN) {
+    ArrowFormalParametersUnexpectedToken(&arrow_formals_classifier);
+  }
+  ExpressionT expression = this->ParseConditionalExpression(
+      accept_IN, &arrow_formals_classifier, CHECK_OK);
+  classifier->Accumulate(arrow_formals_classifier);
 
   if (allow_harmony_arrow_functions() && peek() == Token::ARROW) {
     checkpoint.Restore();
-    FormalParameterErrorLocations error_locs;
+    BindingPatternUnexpectedToken(classifier);
+    ValidateArrowFormalParameters(&arrow_formals_classifier, expression,
+                                  CHECK_OK);
     Scanner::Location loc(lhs_location.beg_pos, scanner()->location().end_pos);
     bool has_rest = false;
     Scope* scope = this->NewScope(scope_, ARROW_SCOPE);
     scope->set_start_position(lhs_location.beg_pos);
-    this->ParseArrowFunctionFormalParameters(scope, expression, loc,
-                                             &error_locs, &has_rest, CHECK_OK);
-    expression = this->ParseArrowFunctionLiteral(scope, error_locs, has_rest,
-                                                 classifier, CHECK_OK);
+    this->ParseArrowFunctionFormalParameters(
+        scope, expression, loc, &formals_error_locs, &has_rest, CHECK_OK);
+    expression = this->ParseArrowFunctionLiteral(scope, formals_error_locs,
+                                                 has_rest, CHECK_OK);
     return expression;
   }
 
+  if (classifier->formal_parameter_error_locs()) {
+    classifier->formal_parameter_error_locs()->Accumulate(formals_error_locs);
+  }
+
   if (!Token::IsAssignmentOp(peek())) {
     if (fni_ != NULL) fni_->Leave();
     // Parsed conditional expression only (no assignment).
     return expression;
   }
 
+  if (!allow_harmony_destructuring()) {
+    BindingPatternUnexpectedToken(classifier);
+  }
+
   expression = this->CheckAndRewriteReferenceExpression(
       expression, lhs_location, "invalid_lhs_in_assignment", CHECK_OK);
   expression = this->MarkExpressionAsAssigned(expression);
@@ -2880,6 +2967,7 @@ ParserBase<Traits>::ParseConditionalExpression(bool accept_IN,
   ExpressionT expression =
       this->ParseBinaryExpression(4, accept_IN, classifier, CHECK_OK);
   if (peek() != Token::CONDITIONAL) return expression;
+  BindingPatternUnexpectedToken(classifier);
   Consume(Token::CONDITIONAL);
   // In parsing the first assignment expression in conditional
   // expressions we always accept the 'in' keyword; see ECMA-262,
@@ -2903,6 +2991,7 @@ ParserBase<Traits>::ParseBinaryExpression(int prec, bool accept_IN,
   for (int prec1 = Precedence(peek(), accept_IN); prec1 >= prec; prec1--) {
     // prec1 >= 4
     while (Precedence(peek(), accept_IN) == prec1) {
+      BindingPatternUnexpectedToken(classifier);
       Token::Value op = Next();
       Scanner::Location op_location = scanner()->location();
       int pos = position();
@@ -3568,7 +3657,7 @@ template <class Traits>
 typename ParserBase<Traits>::ExpressionT
 ParserBase<Traits>::ParseArrowFunctionLiteral(
     Scope* scope, const FormalParameterErrorLocations& error_locs,
-    bool has_rest, ExpressionClassifier* classifier, bool* ok) {
+    bool has_rest, bool* ok) {
   if (peek() == Token::ARROW && scanner_->HasAnyLineTerminatorBeforeNext()) {
     // ASI inserts `;` after arrow parameters if a line terminator is found.
     // `=> ...` is never a valid expression, so report as syntax error.
@@ -3590,9 +3679,6 @@ ParserBase<Traits>::ParseArrowFunctionLiteral(
     FunctionState function_state(&function_state_, &scope_, scope,
                                  kArrowFunction, &function_factory);
 
-    if (peek() == Token::ARROW) {
-      BindingPatternUnexpectedToken(classifier);
-    }
     Expect(Token::ARROW, CHECK_OK);
 
     if (peek() == Token::LBRACE) {
@@ -3617,8 +3703,10 @@ ParserBase<Traits>::ParseArrowFunctionLiteral(
       // Single-expression body
       int pos = position();
       parenthesized_function_ = false;
+      ExpressionClassifier classifier;
       ExpressionT expression =
-          ParseAssignmentExpression(true, classifier, CHECK_OK);
+          ParseAssignmentExpression(true, &classifier, CHECK_OK);
+      ValidateExpression(&classifier, CHECK_OK);
       body = this->NewStatementList(1, zone());
       body->Add(factory()->NewReturnStatement(expression, pos), zone());
       materialized_literal_count = function_state.materialized_literal_count();