[parser] Fix reindexing of functions inside classes

A class's fields can appear twice in the class AST, via the properties
array and the synthetised initializer method. This means that the
reindexer can end up visiting the same function literal twice, since the
T in AST is no longer a T but rather a DAG.

Now, we special case the class visitor in the reindexer to avoid these
double visits where appropriate. We know what kinds of fields can be
double visisted, so we don't need a visited set, but we now also have
one for debug builds to verify that each function is visited exactly
once.

Bug: chromium:974627
Change-Id: Ib531becc6e3f3c73f420b5fb49790fe4a2022d65
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1667003
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62282}

src/ast/ast-function-literal-id-reindexer.cc

@@ -17,13 +17,83 @@ AstFunctionLiteralIdReindexer::AstFunctionLiteralIdReindexer(size_t stack_limit,
 AstFunctionLiteralIdReindexer::~AstFunctionLiteralIdReindexer() = default;
 
 void AstFunctionLiteralIdReindexer::Reindex(Expression* pattern) {
+#ifdef DEBUG
+  visited_.clear();
+#endif
   Visit(pattern);
+  CheckVisited(pattern);
 }
 
 void AstFunctionLiteralIdReindexer::VisitFunctionLiteral(FunctionLiteral* lit) {
+  // Make sure we're not already in the visited set.
+  DCHECK(visited_.insert(lit).second);
+
   AstTraversalVisitor::VisitFunctionLiteral(lit);
   lit->set_function_literal_id(lit->function_literal_id() + delta_);
 }
 
+void AstFunctionLiteralIdReindexer::VisitClassLiteral(ClassLiteral* expr) {
+  // Manually visit the class literal so that we can change the property walk.
+  // This should be kept in-sync with AstTraversalVisitor::VisitClassLiteral.
+
+  if (expr->extends() != nullptr) {
+    Visit(expr->extends());
+  }
+  Visit(expr->constructor());
+  if (expr->static_fields_initializer() != nullptr) {
+    Visit(expr->static_fields_initializer());
+  }
+  if (expr->instance_members_initializer_function() != nullptr) {
+    Visit(expr->instance_members_initializer_function());
+  }
+  ZonePtrList<ClassLiteral::Property>* props = expr->properties();
+  for (int i = 0; i < props->length(); ++i) {
+    ClassLiteralProperty* prop = props->at(i);
+
+    // Private fields and public fields with computed names have both their key
+    // and value present in instance_members_initializer_function, so they will
+    // already have been visited.
+    if ((prop->is_computed_name() || prop->is_private()) &&
+        !prop->value()->IsFunctionLiteral()) {
+      if (!prop->key()->IsLiteral()) {
+        CheckVisited(prop->key());
+      }
+      CheckVisited(prop->value());
+    } else {
+      if (!prop->key()->IsLiteral()) {
+        Visit(prop->key());
+      }
+      Visit(prop->value());
+    }
+  }
+}
+
+#ifdef DEBUG
+namespace {
+
+class AstFunctionLiteralIdReindexChecker final
+    : public AstTraversalVisitor<AstFunctionLiteralIdReindexChecker> {
+ public:
+  AstFunctionLiteralIdReindexChecker(size_t stack_limit,
+                                     const std::set<FunctionLiteral*>* visited)
+      : AstTraversalVisitor(stack_limit), visited_(visited) {}
+
+  void VisitFunctionLiteral(FunctionLiteral* lit) {
+    // TODO(leszeks): It would be nice to print the unvisited function literal
+    // here, but that requires more advanced DCHECK support with formatting.
+    DCHECK(visited_->find(lit) != visited_->end());
+  }
+
+ private:
+  const std::set<FunctionLiteral*>* visited_;
+};
+
+}  // namespace
+
+void AstFunctionLiteralIdReindexer::CheckVisited(Expression* expr) {
+  AstFunctionLiteralIdReindexChecker(stack_limit(), &visited_).Visit(expr);
+}
+#endif
+
 }  // namespace internal
 }  // namespace v8

src/ast/ast-function-literal-id-reindexer.h

@@ -8,6 +8,10 @@
 #include "src/ast/ast-traversal-visitor.h"
 #include "src/base/macros.h"
 
+#ifdef DEBUG
+#include <set>
+#endif
+
 namespace v8 {
 namespace internal {
 
@@ -23,10 +27,22 @@ class AstFunctionLiteralIdReindexer final
 
   // AstTraversalVisitor implementation.
   void VisitFunctionLiteral(FunctionLiteral* lit);
+  void VisitClassLiteral(ClassLiteral* lit);
 
  private:
   int delta_;
 
+#ifdef DEBUG
+  // Visited set, only used in DCHECKs for verification.
+  std::set<FunctionLiteral*> visited_;
+
+  // Visit all function literals, checking if they have already been visited
+  // (are in the visited set).
+  void CheckVisited(Expression* expr);
+#else
+  void CheckVisited(Expression* expr) {}
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(AstFunctionLiteralIdReindexer);
 };
 

src/ast/ast.h

@@ -2767,6 +2767,9 @@ class AstVisitor {
     return false;                                           \
   }                                                         \
                                                             \
+ protected:                                                 \
+  uintptr_t stack_limit() const { return stack_limit_; }    \
+                                                            \
  private:                                                   \
   void InitializeAstVisitor(Isolate* isolate) {             \
     stack_limit_ = isolate->stack_guard()->real_climit();   \

test/mjsunit/reindexing-in-classes.js

@@ -0,0 +1,72 @@
+// Copyright 2019 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.
+
+// Test the reindexer visiting classes, avoiding repeat visits of the same
+// function.
+//
+// For each test, create function literals inside a class, where the functions
+// have to be reindexed due to the whole thing being inside an arrow head scope.
+
+((arg = (function wrapper() {
+  // Class with field that has computed property name with a function in the
+  // computation.
+  class g {
+    [{b: function in_computed_field_name() {}}]
+  }
+})) => {})();
+
+((arg = (function wrapper() {
+  // Class with initialized field that has computed property name with a
+  // function in the computation.
+  class g {
+    [{b: function in_computed_field_name_with_init() {}}] = ""
+  }
+})) => {})();
+
+((arg = (function wrapper() {
+  // Class with initialized field that has literal property name with a function
+  // in the initializer value.
+  class g {
+    b = (function in_init_value_of_field(){})()
+  }
+})) => {})();
+
+((arg = (function wrapper() {
+  // Class with initialized field that has private property name with a function
+  // in the initializer value.
+  class g {
+    #b = (function in_init_value_of_private_field(){})()
+  }
+})) => {})();
+
+((arg = (function wrapper() {
+  // Class with initialized field that has computed property name with a
+  // function in the initializer value.
+  class g {
+    ["b"] = (function in_init_value_of_computed_field_name(){})()
+  }
+})) => {})();
+
+((arg = (function wrapper() {
+  // Class with method that has computed property name with a function in the
+  // computation.
+  class g {
+    [{b: function in_computed_method_name() {}}] () {}
+  }
+})) => {})();
+
+((arg = (function wrapper() {
+  // Class with method that has an argument with a default function init.
+  class g {
+    b(arg = function in_method_arg_default_init() {}) {}
+  }
+})) => {})();
+
+((arg = (function wrapper() {
+  // Class with method that has a computed property name and an argument with a
+  // default function init.
+  class g {
+    ["b"] (arg = function in_computed_method_arg_default_init() {}) {}
+  }
+})) => {})();