Revert of [es6] Fix scoping for default parameters in arrow functions...

Revert of [es6] Fix scoping for default parameters in arrow functions (patchset #5 id:80001 of https://codereview.chromium.org/1405313002/ )

Reason for revert:
Breaks nosnap: http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%20debug%20-%202/builds/2407/steps/Check/logs/regress-4395

Original issue's description:
> [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
>
> Committed: https://crrev.com/cf72aad39e51de9b7074ea039377c1812f4a2c6b
> Cr-Commit-Position: refs/heads/master@{#31402}

TBR=rossberg@chromium.org,caitpotter88@gmail.com,adamk@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4395

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

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

BUILD.gn

@@ -1099,11 +1099,9 @@ source_set("v8_base") {
     "src/optimizing-compile-dispatcher.h",
     "src/ostreams.cc",
     "src/ostreams.h",
-    "src/parameter-initializer-rewriter.cc",
-    "src/parameter-initializer-rewriter.h",
+    "src/pattern-rewriter.cc",
     "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,20 +32,14 @@ namespace internal {
   } while (false)
 
 
-AstExpressionVisitor::AstExpressionVisitor(Isolate* isolate, Expression* root)
+AstExpressionVisitor::AstExpressionVisitor(Isolate* isolate,
+                                           FunctionLiteral* root)
     : root_(root), depth_(0) {
   InitializeAstVisitor(isolate);
 }
 
 
-AstExpressionVisitor::AstExpressionVisitor(uintptr_t stack_limit,
-                                           Expression* root)
-    : root_(root), depth_(0) {
-  InitializeAstVisitor(stack_limit);
-}
-
-
-void AstExpressionVisitor::Run() { RECURSE(Visit(root_)); }
+void AstExpressionVisitor::Run() { RECURSE(VisitFunctionLiteral(root_)); }
 
 
 void AstExpressionVisitor::VisitVariableDeclaration(VariableDeclaration* decl) {
@@ -229,9 +223,6 @@ 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()));
   }
 }
@@ -345,27 +336,10 @@ void AstExpressionVisitor::VisitDeclarations(ZoneList<Declaration*>* decls) {
 }
 
 
-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::VisitClassLiteral(ClassLiteral* expr) {}
 
 
-void AstExpressionVisitor::VisitSpread(Spread* expr) {
-  VisitExpression(expr);
-  RECURSE_EXPRESSION(Visit(expr->expression()));
-}
+void AstExpressionVisitor::VisitSpread(Spread* expr) {}
 
 
 void AstExpressionVisitor::VisitEmptyParentheses(EmptyParentheses* expr) {}

src/ast-expression-visitor.h

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

src/ast.h

@@ -3170,36 +3170,32 @@ 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) {                  \
-    InitializeAstVisitor(isolate->stack_guard()->real_climit()); \
-  }                                                              \
-                                                                 \
-  void InitializeAstVisitor(uintptr_t stack_limit) {             \
-    stack_limit_ = stack_limit;                                  \
-    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) {             \
+    stack_limit_ = isolate->stack_guard()->real_climit();   \
+    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,7 +14,6 @@
 #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"
@@ -4048,10 +4047,6 @@ 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,7 +4,6 @@
 
 #include "src/ast.h"
 #include "src/messages.h"
-#include "src/parameter-initializer-rewriter.h"
 #include "src/parser.h"
 
 namespace v8 {
@@ -360,16 +359,8 @@ 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, initializer, factory()->NewVariableProxy(temp),
+      is_undefined, node->value(), factory()->NewVariableProxy(temp),
       RelocInfo::kNoPosition);
   RecurseIntoSubpattern(node->target(), value);
 }

src/preparser.h

@@ -138,8 +138,6 @@ 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,16 +548,15 @@ Variable* Scope::DeclareDynamicGlobal(const AstRawString* name) {
 }
 
 
-bool Scope::RemoveUnresolved(VariableProxy* var) {
+void 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 true;
+      return;
     }
   }
-  return false;
 }
 
 

src/scopes.h

@@ -178,19 +178,13 @@ 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.
-  bool RemoveUnresolved(VariableProxy* var);
+  void 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.
@@ -470,7 +464,6 @@ 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,11 +865,9 @@
         '../../src/optimizing-compile-dispatcher.h',
         '../../src/ostreams.cc',
         '../../src/ostreams.h',
-        '../../src/parameter-initializer-rewriter.cc',
-        '../../src/parameter-initializer-rewriter.h',
+        '../../src/pattern-rewriter.cc',
         '../../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',