ParameterInitializerRewriter must maintain temporary variable order

When the rewriter moves a temporary variable between scopes, it must
be sure to maintain the order, so that the rewritten order is the
same as it would have been without rewriting.

To expose the difference in behavior, this patch removes the superfluous
visitation of ForOfStatement::each() from AstExpressionVisitor, which
happened to be the only thing keeping all the temporaries in order
in mjsunit/harmony/regress/regress-crbug-578038.js. Without the proper
order, this test would fail under --stress-opt, because the ".for"
variable (behind the "each" proxy) would get two different positions
in the scope, one on first parse (with rewriting) and the other on
second parse (lazy parsing for optimization).

A follow-up patch will remove each() and iterable() from ForOfStatement
altogether, but I wanted to keep this patch small to highlight exactly
the bit of code needed to make the test pass when not visiting each().

BUG=v8:4791
LOG=n

Review-Url: https://codereview.chromium.org/1784893003
Cr-Commit-Position: refs/heads/master@{#36150}

src/ast/ast-expression-visitor.cc

@@ -171,7 +171,6 @@ void AstExpressionVisitor::VisitForInStatement(ForInStatement* stmt) {
 
 void AstExpressionVisitor::VisitForOfStatement(ForOfStatement* stmt) {
   RECURSE(Visit(stmt->iterable()));
-  RECURSE(Visit(stmt->each()));
   RECURSE(Visit(stmt->assign_iterator()));
   RECURSE(Visit(stmt->next_result()));
   RECURSE(Visit(stmt->result_done()));

src/ast/scopes.cc

@@ -558,17 +558,19 @@ Variable* Scope::NewTemporary(const AstRawString* name) {
   return var;
 }
 
-
-bool Scope::RemoveTemporary(Variable* var) {
+int Scope::RemoveTemporary(Variable* var) {
+  DCHECK_NOT_NULL(var);
   // Most likely (always?) any temporary variable we want to remove
   // was just added before, so we search backwards.
   for (int i = temps_.length(); i-- > 0;) {
     if (temps_[i] == var) {
-      temps_.Remove(i);
-      return true;
+      // Don't shrink temps_, as callers of this method expect
+      // the returned indices to be unique per-scope.
+      temps_[i] = nullptr;
+      return i;
     }
   }
-  return false;
+  return -1;
 }
 
 
@@ -635,6 +637,7 @@ void Scope::CollectStackAndContextLocals(ZoneList<Variable*>* stack_locals,
   // context as a whole has forced context allocation.
   for (int i = 0; i < temps_.length(); i++) {
     Variable* var = temps_[i];
+    if (var == nullptr) continue;
     if (var->is_used()) {
       if (var->IsContextSlot()) {
         DCHECK(has_forced_context_allocation());
@@ -984,9 +987,15 @@ void Scope::Print(int n) {
   }
 
   if (temps_.length() > 0) {
-    Indent(n1, "// temporary vars:\n");
+    bool printed_header = false;
     for (int i = 0; i < temps_.length(); i++) {
-      PrintVar(n1, temps_[i]);
+      if (temps_[i] != nullptr) {
+        if (!printed_header) {
+          printed_header = true;
+          Indent(n1, "// temporary vars:\n");
+        }
+        PrintVar(n1, temps_[i]);
+      }
     }
   }
 
@@ -1416,6 +1425,7 @@ void Scope::AllocateDeclaredGlobal(Isolate* isolate, Variable* var) {
 void Scope::AllocateNonParameterLocalsAndDeclaredGlobals(Isolate* isolate) {
   // All variables that have no rewrite yet are non-parameter locals.
   for (int i = 0; i < temps_.length(); i++) {
+    if (temps_[i] == nullptr) continue;
     AllocateNonParameterLocal(isolate, temps_[i]);
   }
 

src/ast/scopes.h

@@ -209,7 +209,9 @@ class Scope: public ZoneObject {
 
   // Remove a temporary variable. This is for adjusting the scope of
   // temporaries used when desugaring parameter initializers.
-  bool RemoveTemporary(Variable* var);
+  // Returns the index at which it was found in this scope, or -1 if
+  // it was not found.
+  int RemoveTemporary(Variable* var);
 
   // Adds a temporary variable in this scope's TemporaryScope. This is for
   // adjusting the scope of temporaries used when desugaring parameter
@@ -605,7 +607,9 @@ class Scope: public ZoneObject {
   // variables may be implicitly 'declared' by being used (possibly in
   // an inner scope) with no intervening with statements or eval calls.
   VariableMap variables_;
-  // Compiler-allocated (user-invisible) temporaries.
+  // Compiler-allocated (user-invisible) temporaries. Due to the implementation
+  // of RemoveTemporary(), may contain nulls, which must be skipped-over during
+  // allocation and printing.
   ZoneList<Variable*> temps_;
   // Parameter list in source order.
   ZoneList<Variable*> params_;

src/parsing/parameter-initializer-rewriter.cc

@@ -4,6 +4,10 @@
 
 #include "src/parsing/parameter-initializer-rewriter.h"
 
+#include <algorithm>
+#include <utility>
+#include <vector>
+
 #include "src/ast/ast.h"
 #include "src/ast/ast-expression-visitor.h"
 #include "src/ast/scopes.h"
@@ -21,6 +25,7 @@ class Rewriter final : public AstExpressionVisitor {
       : AstExpressionVisitor(stack_limit, initializer),
         old_scope_(old_scope),
         new_scope_(new_scope) {}
+  ~Rewriter();
 
  private:
   void VisitExpression(Expression* expr) override {}
@@ -35,8 +40,26 @@ class Rewriter final : public AstExpressionVisitor {
 
   Scope* old_scope_;
   Scope* new_scope_;
+  std::vector<std::pair<Variable*, int>> temps_;
+};
+
+struct LessThanSecond {
+  bool operator()(const std::pair<Variable*, int>& left,
+                  const std::pair<Variable*, int>& right) {
+    return left.second < right.second;
+  }
 };
 
+Rewriter::~Rewriter() {
+  if (!temps_.empty()) {
+    // Ensure that we add temporaries in the order they appeared in old_scope_.
+    std::sort(temps_.begin(), temps_.end(), LessThanSecond());
+    for (auto var_and_index : temps_) {
+      var_and_index.first->set_scope(new_scope_);
+      new_scope_->AddTemporary(var_and_index.first);
+    }
+  }
+}
 
 void Rewriter::VisitFunctionLiteral(FunctionLiteral* function_literal) {
   function_literal->scope()->ReplaceOuterScope(new_scope_);
@@ -67,9 +90,9 @@ void Rewriter::VisitVariableProxy(VariableProxy* proxy) {
   if (proxy->is_resolved()) {
     Variable* var = proxy->var();
     if (var->mode() != TEMPORARY) return;
-    if (old_scope_->RemoveTemporary(var)) {
-      var->set_scope(new_scope_);
-      new_scope_->AddTemporary(var);
+    int index = old_scope_->RemoveTemporary(var);
+    if (index >= 0) {
+      temps_.push_back(std::make_pair(var, index));
     }
   } else if (old_scope_->RemoveUnresolved(proxy)) {
     new_scope_->AddUnresolved(proxy);