[es6] Fix scoping for default parameters in arrow functions

When eagerly parsing arrow functions, expressions in default
parameter initializers are parsed in the enclosing scope,
rather than in the function's scope (since that scope does not
yet exist). This leads to VariableProxies being added to the
wrong scope, and scope chains for FunctionLiterals being incorrect.

This patch addresses these problems by adding a subclass of
AstExpressionVisitor that moves VariableProxies to the proper
scope and fixes up scope chains of FunctionLiterals.

This is a revert of the revert https://crrev.com/e41614a058426fb6102e4ab2dd4f98997f00c0fc
with a much-improved (though not yet perfect) Scope::ResetOuterScope
method which properly fixes not only the outer_scope_ pointer but also
fixes the inner_scope_ list in the relevant outer_scopes.

More work likely still needs to be done to make this work completely,
but it's very close to correct.

BUG=v8:4395
LOG=y

Review URL: https://codereview.chromium.org/1414283002

Cr-Commit-Position: refs/heads/master@{#31435}

BUILD.gn

@@ -1110,9 +1110,11 @@ source_set("v8_base") {
     "src/optimizing-compile-dispatcher.h",
     "src/ostreams.cc",
     "src/ostreams.h",
-    "src/pattern-rewriter.cc",
+    "src/parameter-initializer-rewriter.cc",
+    "src/parameter-initializer-rewriter.h",
     "src/parser.cc",
     "src/parser.h",
+    "src/pattern-rewriter.cc",
     "src/pending-compilation-error-handler.cc",
     "src/pending-compilation-error-handler.h",
     "src/preparse-data-format.h",

src/ast-expression-visitor.cc

@@ -32,14 +32,20 @@ namespace internal {
   } while (false)
 
 
-AstExpressionVisitor::AstExpressionVisitor(Isolate* isolate,
-                                           FunctionLiteral* root)
+AstExpressionVisitor::AstExpressionVisitor(Isolate* isolate, Expression* root)
     : root_(root), depth_(0) {
   InitializeAstVisitor(isolate);
 }
 
 
-void AstExpressionVisitor::Run() { RECURSE(VisitFunctionLiteral(root_)); }
+AstExpressionVisitor::AstExpressionVisitor(uintptr_t stack_limit,
+                                           Expression* root)
+    : root_(root), depth_(0) {
+  InitializeAstVisitor(stack_limit);
+}
+
+
+void AstExpressionVisitor::Run() { RECURSE(Visit(root_)); }
 
 
 void AstExpressionVisitor::VisitVariableDeclaration(VariableDeclaration* decl) {
@@ -229,6 +235,9 @@ void AstExpressionVisitor::VisitObjectLiteral(ObjectLiteral* expr) {
   ZoneList<ObjectLiteralProperty*>* props = expr->properties();
   for (int i = 0; i < props->length(); ++i) {
     ObjectLiteralProperty* prop = props->at(i);
+    if (!prop->key()->IsLiteral()) {
+      RECURSE_EXPRESSION(Visit(prop->key()));
+    }
     RECURSE_EXPRESSION(Visit(prop->value()));
   }
 }
@@ -342,10 +351,27 @@ void AstExpressionVisitor::VisitDeclarations(ZoneList<Declaration*>* decls) {
 }
 
 
-void AstExpressionVisitor::VisitClassLiteral(ClassLiteral* expr) {}
+void AstExpressionVisitor::VisitClassLiteral(ClassLiteral* expr) {
+  VisitExpression(expr);
+  if (expr->extends() != nullptr) {
+    RECURSE_EXPRESSION(Visit(expr->extends()));
+  }
+  RECURSE_EXPRESSION(Visit(expr->constructor()));
+  ZoneList<ObjectLiteralProperty*>* props = expr->properties();
+  for (int i = 0; i < props->length(); ++i) {
+    ObjectLiteralProperty* prop = props->at(i);
+    if (!prop->key()->IsLiteral()) {
+      RECURSE_EXPRESSION(Visit(prop->key()));
+    }
+    RECURSE_EXPRESSION(Visit(prop->value()));
+  }
+}
 
 
-void AstExpressionVisitor::VisitSpread(Spread* expr) {}
+void AstExpressionVisitor::VisitSpread(Spread* expr) {
+  VisitExpression(expr);
+  RECURSE_EXPRESSION(Visit(expr->expression()));
+}
 
 
 void AstExpressionVisitor::VisitEmptyParentheses(EmptyParentheses* expr) {}

src/ast-expression-visitor.h

@@ -21,7 +21,8 @@ namespace internal {
 
 class AstExpressionVisitor : public AstVisitor {
  public:
-  AstExpressionVisitor(Isolate* isolate, FunctionLiteral* root);
+  AstExpressionVisitor(Isolate* isolate, Expression* root);
+  AstExpressionVisitor(uintptr_t stack_limit, Expression* root);
   void Run();
 
  protected:
@@ -38,7 +39,7 @@ class AstExpressionVisitor : public AstVisitor {
   AST_NODE_LIST(DECLARE_VISIT)
 #undef DECLARE_VISIT
 
-  FunctionLiteral* root_;
+  Expression* root_;
   int depth_;
 
   DISALLOW_COPY_AND_ASSIGN(AstExpressionVisitor);

src/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/v8.h"
+
+#include "src/parameter-initializer-rewriter.h"
+
+#include "src/ast.h"
+#include "src/ast-expression-visitor.h"
+#include "src/scopes.h"
+
+namespace v8 {
+namespace internal {
+
+namespace {
+
+
+class Rewriter final : public AstExpressionVisitor {
+ public:
+  Rewriter(uintptr_t stack_limit, Expression* initializer, Scope* old_scope,
+           Scope* new_scope)
+      : AstExpressionVisitor(stack_limit, initializer),
+        old_scope_(old_scope),
+        new_scope_(new_scope) {}
+
+ private:
+  void VisitExpression(Expression* expr) override {}
+
+  void VisitFunctionLiteral(FunctionLiteral* expr) override;
+  void VisitClassLiteral(ClassLiteral* expr) override;
+  void VisitVariableProxy(VariableProxy* expr) override;
+
+  Scope* old_scope_;
+  Scope* new_scope_;
+};
+
+
+void Rewriter::VisitFunctionLiteral(FunctionLiteral* function_literal) {
+  function_literal->scope()->ReplaceOuterScope(new_scope_);
+}
+
+
+void Rewriter::VisitClassLiteral(ClassLiteral* class_literal) {
+  class_literal->scope()->ReplaceOuterScope(new_scope_);
+  if (class_literal->extends() != nullptr) {
+    Visit(class_literal->extends());
+  }
+  // No need to visit the constructor since it will have the class
+  // scope on its scope chain.
+  ZoneList<ObjectLiteralProperty*>* props = class_literal->properties();
+  for (int i = 0; i < props->length(); ++i) {
+    ObjectLiteralProperty* prop = props->at(i);
+    if (!prop->key()->IsLiteral()) {
+      Visit(prop->key());
+    }
+    // No need to visit the values, since all values are functions with
+    // the class scope on their scope chain.
+    DCHECK(prop->value()->IsFunctionLiteral());
+  }
+}
+
+
+void Rewriter::VisitVariableProxy(VariableProxy* proxy) {
+  DCHECK(!proxy->is_resolved());
+  if (old_scope_->RemoveUnresolved(proxy)) {
+    new_scope_->AddUnresolved(proxy);
+  }
+}
+
+
+}  // anonymous namespace
+
+
+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

src/parameter-initializer-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_PARAMETER_EXPRESSION_REWRITER_H_
+#define V8_PARAMETER_EXPRESSION_REWRITER_H_
+
+#include "src/ast.h"
+
+namespace v8 {
+namespace internal {
+
+
+void RewriteParameterInitializerScope(uintptr_t stack_limit,
+                                      Expression* initializer, Scope* old_scope,
+                                      Scope* new_scope);
+
+
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_PARAMETER_EXPRESSION_REWRITER_H_

src/parser.cc

@@ -14,6 +14,7 @@
 #include "src/codegen.h"
 #include "src/compiler.h"
 #include "src/messages.h"
+#include "src/parameter-initializer-rewriter.h"
 #include "src/preparser.h"
 #include "src/rewriter.h"
 #include "src/runtime/runtime.h"
@@ -4049,6 +4050,10 @@ void ParserTraits::ParseArrowFunctionFormalParameters(
     DCHECK(!assignment->is_compound());
     initializer = assignment->value();
     expr = assignment->target();
+
+    // TODO(adamk): Only call this if necessary.
+    RewriteParameterInitializerScope(parser_->stack_limit(), initializer,
+                                     parser_->scope_, parameters->scope);
   }
 
   AddFormalParameter(parameters, expr, initializer, is_rest);

src/pattern-rewriter.cc

@@ -4,6 +4,7 @@
 
 #include "src/ast.h"
 #include "src/messages.h"
+#include "src/parameter-initializer-rewriter.h"
 #include "src/parser.h"
 
 namespace v8 {
@@ -359,8 +360,16 @@ void Parser::PatternRewriter::VisitAssignment(Assignment* node) {
       Token::EQ_STRICT, factory()->NewVariableProxy(temp),
       factory()->NewUndefinedLiteral(RelocInfo::kNoPosition),
       RelocInfo::kNoPosition);
+  Expression* initializer = node->value();
+  if (descriptor_->declaration_kind == DeclarationDescriptor::PARAMETER &&
+      descriptor_->scope->is_arrow_scope()) {
+    // TODO(adamk): Only call this if necessary.
+    RewriteParameterInitializerScope(
+        descriptor_->parser->stack_limit(), initializer,
+        descriptor_->scope->outer_scope(), descriptor_->scope);
+  }
   Expression* value = factory()->NewConditional(
-      is_undefined, node->value(), factory()->NewVariableProxy(temp),
+      is_undefined, initializer, factory()->NewVariableProxy(temp),
       RelocInfo::kNoPosition);
   RecurseIntoSubpattern(node->target(), value);
 }

src/scopes.cc

@@ -377,12 +377,7 @@ Scope* Scope::FinalizeBlockScope() {
   }
 
   // Remove this scope from outer scope.
-  for (int i = 0; i < outer_scope_->inner_scopes_.length(); i++) {
-    if (outer_scope_->inner_scopes_[i] == this) {
-      outer_scope_->inner_scopes_.Remove(i);
-      break;
-    }
-  }
+  outer_scope()->RemoveInnerScope(this);
 
   // Reparent inner scopes.
   for (int i = 0; i < inner_scopes_.length(); i++) {
@@ -395,6 +390,7 @@ Scope* Scope::FinalizeBlockScope() {
   }
 
   // Propagate usage flags to outer scope.
+  // TODO(adamk): Why doesn't this call PropagateScopeInfo()?
   if (uses_arguments()) outer_scope_->RecordArgumentsUsage();
   if (uses_super_property()) outer_scope_->RecordSuperPropertyUsage();
   if (scope_calls_eval_) outer_scope_->RecordEvalCall();
@@ -403,6 +399,15 @@ Scope* Scope::FinalizeBlockScope() {
 }
 
 
+void Scope::ReplaceOuterScope(Scope* outer_scope) {
+  DCHECK_NOT_NULL(outer_scope_);
+  outer_scope_->RemoveInnerScope(this);
+  outer_scope_ = outer_scope;
+  outer_scope_->AddInnerScope(this);
+  // TODO(adamk): Do we need to propagate usage flags here?
+}
+
+
 Variable* Scope::LookupLocal(const AstRawString* name) {
   Variable* result = variables_.Lookup(name);
   if (result != NULL || scope_info_.is_null()) {
@@ -548,15 +553,16 @@ Variable* Scope::DeclareDynamicGlobal(const AstRawString* name) {
 }
 
 
-void Scope::RemoveUnresolved(VariableProxy* var) {
+bool Scope::RemoveUnresolved(VariableProxy* var) {
   // Most likely (always?) any variable we want to remove
   // was just added before, so we search backwards.
   for (int i = unresolved_.length(); i-- > 0;) {
     if (unresolved_[i] == var) {
       unresolved_.Remove(i);
-      return;
+      return true;
     }
   }
+  return false;
 }
 
 

src/scopes.h

@@ -112,6 +112,11 @@ class Scope: public ZoneObject {
   // tree and its children are reparented.
   Scope* FinalizeBlockScope();
 
+  // Inserts outer_scope into this scope's scope chain (and removes this
+  // from the current outer_scope_'s inner_scopes_).
+  // Assumes outer_scope_ is non-null.
+  void ReplaceOuterScope(Scope* outer_scope);
+
   Zone* zone() const { return zone_; }
 
   // ---------------------------------------------------------------------------
@@ -178,13 +183,19 @@ class Scope: public ZoneObject {
     return proxy;
   }
 
+  void AddUnresolved(VariableProxy* proxy) {
+    DCHECK(!already_resolved());
+    DCHECK(!proxy->is_resolved());
+    unresolved_.Add(proxy, zone_);
+  }
+
   // Remove a unresolved variable. During parsing, an unresolved variable
   // may have been added optimistically, but then only the variable name
   // was used (typically for labels). If the variable was not declared, the
   // addition introduced a new unresolved variable which may end up being
   // allocated globally as a "ghost" variable. RemoveUnresolved removes
   // such a variable again if it was added; otherwise this is a no-op.
-  void RemoveUnresolved(VariableProxy* var);
+  bool RemoveUnresolved(VariableProxy* var);
 
   // Creates a new temporary variable in this scope's TemporaryScope.  The
   // name is only used for printing and cannot be used to find the variable.
@@ -813,6 +824,16 @@ class Scope: public ZoneObject {
     }
   }
 
+  void RemoveInnerScope(Scope* inner_scope) {
+    DCHECK_NOT_NULL(inner_scope);
+    for (int i = 0; i < inner_scopes_.length(); i++) {
+      if (inner_scopes_[i] == inner_scope) {
+        inner_scopes_.Remove(i);
+        break;
+      }
+    }
+  }
+
   void SetDefaults(ScopeType type, Scope* outer_scope,
                    Handle<ScopeInfo> scope_info,
                    FunctionKind function_kind = kNormalFunction);

test/mjsunit/harmony/regress/regress-4395.js

+// 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.
+//
+// Flags: --harmony-destructuring --harmony-default-parameters
+
+(function testExpressionTypes() {
+  "use strict";
+  ((x, y = x) => assertEquals(42, y))(42);
+
+  ((x, y = (x)) => assertEquals(42, y))(42);
+  ((x, y = `${x}`) => assertEquals("42", y))(42);
+  ((x, y = x = x + 1) => assertEquals(43, y))(42);
+  ((x, y = x()) => assertEquals(42, y))(() => 42);
+  ((x, y = new x()) => assertEquals(42, y.z))(function() { this.z = 42 });
+  ((x, y = -x) => assertEquals(-42, y))(42);
+  ((x, y = ++x) => assertEquals(43, y))(42);
+  ((x, y = x === 42) => assertTrue(y))(42);
+  ((x, y = (x == 42 ? x : 0)) => assertEquals(42, y))(42);
+
+  ((x, y = function() { return x }) => assertEquals(42, y()))(42);
+  ((x, y = () => x) => assertEquals(42, y()))(42);
+
+  // Literals
+  ((x, y = {z: x}) => assertEquals(42, y.z))(42);
+  ((x, y = {[x]: x}) => assertEquals(42, y[42]))(42);
+  ((x, y = [x]) => assertEquals(42, y[0]))(42);
+  ((x, y = [...x]) => assertEquals(42, y[0]))([42]);
+
+  ((x, y = class {
+    static [x]() { return x }
+  }) => assertEquals(42, y[42]()))(42);
+  ((x, y = (new class {
+    z() { return x }
+  })) => assertEquals(42, y.z()))(42);
+
+  ((x, y = (new class Y {
+    static [x]() { return x }
+    z() { return Y[42]() }
+  })) => assertEquals(42, y.z()))(42);
+
+  ((x, y = (new class {
+    constructor() { this.z = x }
+  })) => assertEquals(42, y.z))(42);
+  ((x, y = (new class Y {
+    constructor() { this.z = x }
+  })) => assertEquals(42, y.z))(42);
+
+  ((x, y = (new class extends x {
+  })) => assertEquals(42, y.z()))(class { z() { return 42 } });
+
+  // Defaults inside destructuring
+  ((x, {y = x}) => assertEquals(42, y))(42, {});
+  ((x, [y = x]) => assertEquals(42, y))(42, []);
+})();
+
+(function testMultiScopeCapture() {
+  "use strict";
+  var x = 1;
+  {
+    let y = 2;
+    ((x, y, a = x, b = y) => {
+      assertEquals(3, x);
+      assertEquals(3, a);
+      assertEquals(4, y);
+      assertEquals(4, b);
+    })(3, 4);
+  }
+})();

tools/gyp/v8.gyp

@@ -868,9 +868,11 @@
         '../../src/optimizing-compile-dispatcher.h',
         '../../src/ostreams.cc',
         '../../src/ostreams.h',
-        '../../src/pattern-rewriter.cc',
+        '../../src/parameter-initializer-rewriter.cc',
+        '../../src/parameter-initializer-rewriter.h',
         '../../src/parser.cc',
         '../../src/parser.h',
+        '../../src/pattern-rewriter.cc',
         '../../src/pending-compilation-error-handler.cc',
         '../../src/pending-compilation-error-handler.h',
         '../../src/preparse-data-format.h',