[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.

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/1405313002

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

BUILD.gn

@@ -1099,9 +1099,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) {
@@ -223,6 +229,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()));
   }
 }
@@ -336,10 +345,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/ast.h

@@ -3170,32 +3170,36 @@ class AstVisitor BASE_EMBEDDED {
 };
 
 
-#define DEFINE_AST_VISITOR_SUBCLASS_MEMBERS()               \
- public:                                                    \
-  void Visit(AstNode* node) final {                         \
-    if (!CheckStackOverflow()) node->Accept(this);          \
-  }                                                         \
-                                                            \
-  void SetStackOverflow() { stack_overflow_ = true; }       \
-  void ClearStackOverflow() { stack_overflow_ = false; }    \
-  bool HasStackOverflow() const { return stack_overflow_; } \
-                                                            \
-  bool CheckStackOverflow() {                               \
-    if (stack_overflow_) return true;                       \
-    if (GetCurrentStackPosition() < stack_limit_) {         \
-      stack_overflow_ = true;                               \
-      return true;                                          \
-    }                                                       \
-    return false;                                           \
-  }                                                         \
-                                                            \
- private:                                                   \
-  void InitializeAstVisitor(Isolate* isolate) {             \
-    stack_limit_ = isolate->stack_guard()->real_climit();   \
-    stack_overflow_ = false;                                \
-  }                                                         \
-                                                            \
-  uintptr_t stack_limit_;                                   \
+#define DEFINE_AST_VISITOR_SUBCLASS_MEMBERS()                    \
+ public:                                                         \
+  void Visit(AstNode* node) final {                              \
+    if (!CheckStackOverflow()) node->Accept(this);               \
+  }                                                              \
+                                                                 \
+  void SetStackOverflow() { stack_overflow_ = true; }            \
+  void ClearStackOverflow() { stack_overflow_ = false; }         \
+  bool HasStackOverflow() const { return stack_overflow_; }      \
+                                                                 \
+  bool CheckStackOverflow() {                                    \
+    if (stack_overflow_) return true;                            \
+    if (GetCurrentStackPosition() < stack_limit_) {              \
+      stack_overflow_ = true;                                    \
+      return true;                                               \
+    }                                                            \
+    return false;                                                \
+  }                                                              \
+                                                                 \
+ private:                                                        \
+  void InitializeAstVisitor(Isolate* isolate) {                  \
+    InitializeAstVisitor(isolate->stack_guard()->real_climit()); \
+  }                                                              \
+                                                                 \
+  void InitializeAstVisitor(uintptr_t stack_limit) {             \
+    stack_limit_ = stack_limit;                                  \
+    stack_overflow_ = false;                                     \
+  }                                                              \
+                                                                 \
+  uintptr_t stack_limit_;                                        \
   bool stack_overflow_
 
 

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()->set_outer_scope(new_scope_);
+}
+
+
+void Rewriter::VisitClassLiteral(ClassLiteral* class_literal) {
+  class_literal->scope()->set_outer_scope(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/runtime/runtime.h"
 #include "src/scanner-character-streams.h"
@@ -4047,6 +4048,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/preparser.h

@@ -138,6 +138,8 @@ class ParserBase : public Traits {
   ALLOW_ACCESSORS(legacy_const);
 #undef ALLOW_ACCESSORS
 
+  uintptr_t stack_limit() const { return stack_limit_; }
+
  protected:
   enum AllowRestrictedIdentifiers {
     kAllowRestrictedIdentifiers,

src/scopes.cc

@@ -548,15 +548,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

@@ -178,13 +178,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.
@@ -464,6 +470,7 @@ class Scope: public ZoneObject {
 
   // The scope immediately surrounding this scope, or NULL.
   Scope* outer_scope() const { return outer_scope_; }
+  void set_outer_scope(Scope* outer_scope) { outer_scope_ = outer_scope; }
 
   // The ModuleDescriptor for this scope; only for module scopes.
   ModuleDescriptor* module() const { return module_descriptor_; }

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

@@ -865,9 +865,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',