[parser] Greatly simplify Array spread rewriting code

Since each Array literal containing a spread is individually queued for
rewriting, there's no need for an AstVisitor here: a simple linear
pass through the queue is sufficient.

This patch deletes AstExpressionRewriter and all the machinery supporting
it in the AST. This code was built with the idea of using it as
a general expression rewriting mechanism in the parser, but those use
cases never materialized, and Array spread remains the only thing
that used this code.

Bug: v8:6092
Change-Id: I754c4883099e840881b005f20216f86e57721d5a
Reviewed-on: https://chromium-review.googlesource.com/765051Reviewed-by: Marja Hölttä <marja@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49337}

BUILD.gn

@@ -1205,8 +1205,6 @@ v8_source_set("v8_base") {
     "src/assembler.h",
     "src/assert-scope.cc",
     "src/assert-scope.h",
-    "src/ast/ast-expression-rewriter.cc",
-    "src/ast/ast-expression-rewriter.h",
     "src/ast/ast-function-literal-id-reindexer.cc",
     "src/ast/ast-function-literal-id-reindexer.h",
     "src/ast/ast-numbering.cc",

src/ast/ast-expression-rewriter.h

-// Copyright 2015 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_AST_AST_EXPRESSION_REWRITER_H_
-#define V8_AST_AST_EXPRESSION_REWRITER_H_
-
-#include "src/allocation.h"
-#include "src/ast/ast.h"
-#include "src/ast/scopes.h"
-#include "src/zone/zone.h"
-
-namespace v8 {
-namespace internal {
-
-// A rewriting Visitor over a CompilationInfo's AST that invokes
-// VisitExpression on each expression node.
-
-// This AstVistor is not final, and provides the AstVisitor methods as virtual
-// methods so they can be specialized by subclasses.
-class AstExpressionRewriter : public AstVisitor<AstExpressionRewriter> {
- public:
-  explicit AstExpressionRewriter(Isolate* isolate) {
-    InitializeAstRewriter(isolate);
-  }
-  explicit AstExpressionRewriter(uintptr_t stack_limit) {
-    InitializeAstRewriter(stack_limit);
-  }
-  virtual ~AstExpressionRewriter() {}
-
-  virtual void VisitDeclarations(Declaration::List* declarations);
-  virtual void VisitStatements(ZoneList<Statement*>* statements);
-  virtual void VisitExpressions(ZoneList<Expression*>* expressions);
-
-  virtual void VisitLiteralProperty(LiteralProperty* property);
-
- protected:
-  virtual bool RewriteExpression(Expression* expr) = 0;
-
- private:
-  DEFINE_AST_REWRITER_SUBCLASS_MEMBERS();
-
-#define DECLARE_VISIT(type) virtual void Visit##type(type* node);
-  AST_NODE_LIST(DECLARE_VISIT)
-#undef DECLARE_VISIT
-
-  DISALLOW_COPY_AND_ASSIGN(AstExpressionRewriter);
-};
-
-}  // namespace internal
-}  // namespace v8
-
-#endif  // V8_AST_AST_EXPRESSION_REWRITER_H_

src/parsing/expression-classifier.h

@@ -424,7 +424,7 @@ class ExpressionClassifier {
   typename Types::Base* base_;
   ExpressionClassifier* previous_;
   Zone* zone_;
-  ZoneList<typename Types::Expression>* non_patterns_to_rewrite_;
+  ZoneList<typename Types::RewritableExpression>* non_patterns_to_rewrite_;
   ZoneList<Error>* reported_errors_;
   DuplicateFinder* duplicate_finder_;
   // The uint16_t for non_pattern_begin_ will not be enough in the case,

src/parsing/parser-base.h

@@ -236,6 +236,7 @@ class ParserBase {
   typedef typename Types::ObjectLiteralProperty ObjectLiteralPropertyT;
   typedef typename Types::ClassLiteralProperty ClassLiteralPropertyT;
   typedef typename Types::Suspend SuspendExpressionT;
+  typedef typename Types::RewritableExpression RewritableExpressionT;
   typedef typename Types::ExpressionList ExpressionListT;
   typedef typename Types::FormalParameters FormalParametersT;
   typedef typename Types::Statement StatementT;
@@ -413,7 +414,7 @@ class ParserBase {
       return &reported_errors_;
     }
 
-    ZoneList<ExpressionT>* non_patterns_to_rewrite() {
+    ZoneList<RewritableExpressionT>* non_patterns_to_rewrite() {
       return &non_patterns_to_rewrite_;
     }
 
@@ -458,11 +459,12 @@ class ParserBase {
       destructuring_assignments_to_rewrite_.Add(pair, scope_->zone());
     }
 
-    void AddNonPatternForRewriting(ExpressionT expr, bool* ok) {
+    void AddNonPatternForRewriting(RewritableExpressionT expr, bool* ok) {
       non_patterns_to_rewrite_.Add(expr, scope_->zone());
       if (non_patterns_to_rewrite_.length() >=
-          std::numeric_limits<uint16_t>::max())
+          std::numeric_limits<uint16_t>::max()) {
         *ok = false;
+      }
     }
 
     // Properties count estimation.
@@ -473,7 +475,7 @@ class ParserBase {
     DeclarationScope* scope_;
 
     ZoneList<DestructuringAssignment> destructuring_assignments_to_rewrite_;
-    ZoneList<ExpressionT> non_patterns_to_rewrite_;
+    ZoneList<RewritableExpressionT> non_patterns_to_rewrite_;
 
     ZoneList<typename ExpressionClassifier::Error> reported_errors_;
 
@@ -2064,8 +2066,8 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseArrayLiteral(
   ExpressionT result =
       factory()->NewArrayLiteral(values, first_spread_index, pos);
   if (first_spread_index >= 0) {
-    result = factory()->NewRewritableExpression(result);
-    impl()->QueueNonPatternForRewriting(result, ok);
+    auto rewritable = factory()->NewRewritableExpression(result);
+    impl()->QueueNonPatternForRewriting(rewritable, ok);
     if (!*ok) {
       // If the non-pattern rewriting mechanism is used in the future for
       // rewriting other things than spreads, this error message will have
@@ -2074,6 +2076,7 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseArrayLiteral(
       ReportMessage(MessageTemplate::kTooManySpreads);
       return impl()->NullExpression();
     }
+    result = rewritable;
   }
   return result;
 }

src/parsing/parser.cc

@@ -8,7 +8,6 @@
 #include <memory>
 
 #include "src/api.h"
-#include "src/ast/ast-expression-rewriter.h"
 #include "src/ast/ast-function-literal-id-reindexer.h"
 #include "src/ast/ast-traversal-visitor.h"
 #include "src/ast/ast.h"
@@ -3826,53 +3825,18 @@ void Parser::RewriteAsyncFunctionBody(ZoneList<Statement*>* body, Block* block,
   body->Add(block, zone());
 }
 
-class NonPatternRewriter : public AstExpressionRewriter {
- public:
-  NonPatternRewriter(uintptr_t stack_limit, Parser* parser)
-      : AstExpressionRewriter(stack_limit), parser_(parser) {}
-  ~NonPatternRewriter() override {}
-
- private:
-  bool RewriteExpression(Expression* expr) override {
-    if (expr->IsRewritableExpression()) return true;
-    // Rewrite only what could have been a pattern but is not.
-    if (expr->IsArrayLiteral()) {
-      // Spread rewriting in array literals.
-      ArrayLiteral* lit = expr->AsArrayLiteral();
-      VisitExpressions(lit->values());
-      replacement_ = parser_->RewriteSpreads(lit);
-      return false;
-    }
-    if (expr->IsObjectLiteral()) {
-      return true;
-    }
-    if (expr->IsBinaryOperation() &&
-        expr->AsBinaryOperation()->op() == Token::COMMA) {
-      return true;
-    }
-    // Everything else does not need rewriting.
-    return false;
-  }
-
-  void VisitLiteralProperty(LiteralProperty* property) override {
-    if (property == nullptr) return;
-    // Do not rewrite (computed) key expressions
-    AST_REWRITE_PROPERTY(Expression, property, value);
-  }
-
-  Parser* parser_;
-};
-
 void Parser::RewriteNonPattern(bool* ok) {
   ValidateExpression(CHECK_OK_VOID);
   auto non_patterns_to_rewrite = function_state_->non_patterns_to_rewrite();
   int begin = classifier()->GetNonPatternBegin();
   int end = non_patterns_to_rewrite->length();
   if (begin < end) {
-    NonPatternRewriter rewriter(stack_limit_, this);
     for (int i = begin; i < end; i++) {
-      DCHECK(non_patterns_to_rewrite->at(i)->IsRewritableExpression());
-      rewriter.Rewrite(non_patterns_to_rewrite->at(i));
+      RewritableExpression* expr = non_patterns_to_rewrite->at(i);
+      // TODO(adamk): Make this more typesafe.
+      DCHECK(expr->expression()->IsArrayLiteral());
+      ArrayLiteral* lit = expr->expression()->AsArrayLiteral();
+      expr->Rewrite(RewriteSpreads(lit));
     }
     non_patterns_to_rewrite->Rewind(begin);
   }
@@ -4045,8 +4009,7 @@ void Parser::QueueDestructuringAssignmentForRewriting(Expression* expr) {
       DestructuringAssignment(expr, scope()));
 }
 
-void Parser::QueueNonPatternForRewriting(Expression* expr, bool* ok) {
-  DCHECK(expr->IsRewritableExpression());
+void Parser::QueueNonPatternForRewriting(RewritableExpression* expr, bool* ok) {
   function_state_->AddNonPatternForRewriting(expr, ok);
 }
 

src/parsing/parser.h

@@ -167,6 +167,7 @@ struct ParserTypes<Parser> {
   typedef ObjectLiteral::Property* ObjectLiteralProperty;
   typedef ClassLiteral::Property* ClassLiteralProperty;
   typedef v8::internal::Suspend* Suspend;
+  typedef v8::internal::RewritableExpression* RewritableExpression;
   typedef ZoneList<v8::internal::Expression*>* ExpressionList;
   typedef ZoneList<ObjectLiteral::Property*>* ObjectPropertyList;
   typedef ZoneList<ClassLiteral::Property*>* ClassPropertyList;
@@ -557,7 +558,6 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   V8_INLINE Expression* RewriteAssignExponentiation(Expression* left,
                                                     Expression* right, int pos);
 
-  friend class NonPatternRewriter;
   V8_INLINE Expression* RewriteSpreads(ArrayLiteral* lit);
 
   // Rewrite expressions that are not used as patterns
@@ -565,7 +565,8 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
 
   V8_INLINE void QueueDestructuringAssignmentForRewriting(
       Expression* assignment);
-  V8_INLINE void QueueNonPatternForRewriting(Expression* expr, bool* ok);
+  V8_INLINE void QueueNonPatternForRewriting(RewritableExpression* expr,
+                                             bool* ok);
 
   friend class InitializerRewriter;
   void RewriteParameterInitializer(Expression* expr);
@@ -996,7 +997,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
     return function_state_->GetReportedErrorList();
   }
 
-  V8_INLINE ZoneList<Expression*>* GetNonPatternList() const {
+  V8_INLINE ZoneList<RewritableExpression*>* GetNonPatternList() const {
     return function_state_->non_patterns_to_rewrite();
   }
 

src/parsing/preparser.h

@@ -831,6 +831,7 @@ struct ParserTypes<PreParser> {
   typedef PreParserExpression ObjectLiteralProperty;
   typedef PreParserExpression ClassLiteralProperty;
   typedef PreParserExpression Suspend;
+  typedef PreParserExpression RewritableExpression;
   typedef PreParserExpressionList ExpressionList;
   typedef PreParserExpressionList ObjectPropertyList;
   typedef PreParserExpressionList ClassPropertyList;

src/v8.gyp

@@ -581,8 +581,6 @@
         'assembler-inl.h',
         'assert-scope.h',
         'assert-scope.cc',
-        'ast/ast-expression-rewriter.cc',
-        'ast/ast-expression-rewriter.h',
         'ast/ast-function-literal-id-reindexer.cc',
         'ast/ast-function-literal-id-reindexer.h',
         'ast/ast-numbering.cc',