Fix bug with re-scoping arrow function parameter initializers

src/parsing/parameter-initializer-rewriter.cc

 // 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.
 
 #include "src/parsing/parameter-initializer-rewriter.h"
 
 #include <algorithm>
 #include <utility>
 #include <vector>
 
@@ skipping 37 matching lines @@
                   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);
+      Scope* scope = new_scope_->ClosureScope();
+      var_and_index.first->set_scope(scope);
+      scope->AddTemporary(var_and_index.first);
     }
   }
 }
 
 void Rewriter::VisitFunctionLiteral(FunctionLiteral* function_literal) {
   function_literal->scope()->ReplaceOuterScope(new_scope_);
 }
 
 
 void Rewriter::VisitClassLiteral(ClassLiteral* class_literal) {
@@ skipping 13 matching lines @@
     // the class scope on their scope chain.
     DCHECK(prop->value()->IsFunctionLiteral());
   }
 }
 
 
 void Rewriter::VisitVariableProxy(VariableProxy* proxy) {
   if (proxy->is_resolved()) {
     Variable* var = proxy->var();
     if (var->mode() != TEMPORARY) return;
-    // For rewriting inside the same ClosureScope (e.g., putting default
-    // parameter values in their own inner scope in certain cases), refrain
-    // from invalidly moving temporaries to a block scope.
-    if (var->scope()->ClosureScope() == new_scope_->ClosureScope()) return;
-    int index = old_scope_->RemoveTemporary(var);
+    // Temporaries are only placed in ClosureScopes.
+    DCHECK_EQ(var->scope(), var->scope()->ClosureScope());
+    // If the temporary is already where it should be, return quickly.
+    if (var->scope() == new_scope_->ClosureScope()) return;

[nickie, 2016/06/24 13:39:51]

The condition of this "if" statement is identical to the previous one, given the
DCHECK_EQ above.  I was confused by Dan's original comment and changed it in the
way that I understand it.

Even if the "if" statement in line 97 is completely removed, everything works
alright now.  However, I think it's better to leave the "if" statement there as
an optimization --- no need to remove a temporary and then put it back in the
same scope.

[Dan Ehrenberg, 2016/06/24 16:58:44]

Yeah, this seems like a valid change. I was writing the code imagining that we
might eventually extend the scope rewriter to work between arbitrary block
scopes, but there are other changes that we would need to this method (and
probably others) to enable that.

+    int index = old_scope_->ClosureScope()->RemoveTemporary(var);
     if (index >= 0) {
       temps_.push_back(std::make_pair(var, index));
     }
   } else if (old_scope_->RemoveUnresolved(proxy)) {
     new_scope_->AddUnresolved(proxy);
   }
 }
 
 
 void Rewriter::VisitBlock(Block* stmt) {
@@ skipping 22 matching lines @@
 void RewriteParameterInitializerScope(uintptr_t stack_limit,
                                       Expression* initializer, Scope* old_scope,
                                       Scope* new_scope) {
   Rewriter rewriter(stack_limit, initializer, old_scope, new_scope);
   rewriter.Run();
 }
 
 
 }  // namespace internal
 }  // namespace v8