Rescope arrow-function parameter lists by moving the delta to the parameter scope

This replaces the AstVisitor approach for scope rewriting with a Scope-only
solution, using a new Scope::Snapshot object that keeps track of inner scopes,
unresolved variables, and temps.

The only use of the AstVisitor is now for parameter varblock scopes introduced
due to sloppy eval in parameters, which greatly simplifies the rewriter
as it no longer needs to handle temps. A future CL may be able to
eliminate it altogether by taking a snapshot per function argument.

Based on verwaest's https://codereview.chromium.org/2166023002/.

BUG=v8:5226

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

src/ast/scopes.cc

@@ -367,6 +367,51 @@ Scope* Scope::FinalizeBlockScope() {
   return NULL;
 }
 
+void Scope::Snapshot::Reparent(Scope* new_parent) const {
+  DCHECK_EQ(new_parent, outer_scope_->inner_scope_);
+  DCHECK_EQ(new_parent->outer_scope_, outer_scope_);
+  DCHECK_EQ(new_parent, new_parent->ClosureScope());
+  DCHECK_NULL(new_parent->inner_scope_);
+  DCHECK_NULL(new_parent->unresolved_);
+  DCHECK_EQ(0, new_parent->temps_.length());
+  Scope* inner_scope = new_parent->sibling_;
+  if (inner_scope != top_inner_scope_) {
+    for (; inner_scope->sibling() != top_inner_scope_;
+         inner_scope = inner_scope->sibling()) {
+      inner_scope->outer_scope_ = new_parent;
+      DCHECK_NE(inner_scope, new_parent);
+    }
+    inner_scope->outer_scope_ = new_parent;
+
+    new_parent->inner_scope_ = new_parent->sibling_;
+    inner_scope->sibling_ = nullptr;
+    // Reset the sibling rather than the inner_scope_ since we
+    // want to keep new_parent there.
+    new_parent->sibling_ = top_inner_scope_;
+  }
+
+  if (outer_scope_->unresolved_ != top_unresolved_) {
+    VariableProxy* last = outer_scope_->unresolved_;
+    while (last->next_unresolved() != top_unresolved_) {
+      last = last->next_unresolved();
+    }
+    last->set_next_unresolved(nullptr);
+    new_parent->unresolved_ = outer_scope_->unresolved_;
+    outer_scope_->unresolved_ = top_unresolved_;
+  }
+
+  if (outer_scope_->ClosureScope()->temps_.length() != top_temp_) {
+    ZoneList<Variable*>* temps = &outer_scope_->ClosureScope()->temps_;
+    for (int i = top_temp_; i < temps->length(); i++) {
+      Variable* temp = temps->at(i);
+      DCHECK_EQ(temp->scope(), temp->scope()->ClosureScope());
+      DCHECK_NE(temp->scope(), new_parent);
+      temp->set_scope(new_parent);
+      new_parent->AddTemporary(temp);
+    }
+    temps->Rewind(top_temp_);
+  }
+}
 
 void Scope::ReplaceOuterScope(Scope* outer) {
   DCHECK_NOT_NULL(outer);

src/ast/scopes.h

@@ -92,6 +92,23 @@ class Scope: public ZoneObject {
   Scope(Zone* zone, Scope* outer_scope, ScopeType scope_type,
         FunctionKind function_kind = kNormalFunction);
 
+  class Snapshot final BASE_EMBEDDED {
+   public:
+    explicit Snapshot(Scope* scope)
+        : outer_scope_(scope),
+          top_inner_scope_(scope->inner_scope_),
+          top_unresolved_(scope->unresolved_),
+          top_temp_(scope->ClosureScope()->temps_.length()) {}
+
+    void Reparent(Scope* new_parent) const;
+
+   private:
+    Scope* outer_scope_;
+    Scope* top_inner_scope_;
+    VariableProxy* top_unresolved_;
+    int top_temp_;
+  };
+
   // Compute top scope and allocate variables. For lazy compilation the top
   // scope only contains the single lazily compiled function, so this
   // doesn't re-allocate variables repeatedly.

src/parsing/parameter-initializer-rewriter.cc

@@ -4,10 +4,6 @@
 
 #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"
@@ -20,14 +16,9 @@ namespace {
 
 class Rewriter final : public AstExpressionVisitor {
  public:
-  Rewriter(uintptr_t stack_limit, Expression* initializer, Scope* old_scope,
-           Scope* new_scope)
+  Rewriter(uintptr_t stack_limit, Expression* initializer, Scope* param_scope)
       : AstExpressionVisitor(stack_limit, initializer),
-        old_scope_(old_scope),
-        new_scope_(new_scope),
-        old_scope_closure_(old_scope->ClosureScope()),
-        new_scope_closure_(new_scope->ClosureScope()) {}
-  ~Rewriter();
+        param_scope_(param_scope) {}
 
  private:
   void VisitExpression(Expression* expr) override {}
@@ -40,38 +31,16 @@ class Rewriter final : public AstExpressionVisitor {
   void VisitTryCatchStatement(TryCatchStatement* stmt) override;
   void VisitWithStatement(WithStatement* stmt) override;
 
-  Scope* old_scope_;
-  Scope* new_scope_;
-  Scope* old_scope_closure_;
-  Scope* new_scope_closure_;
-  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;
-  }
+  Scope* param_scope_;
 };
 
-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_closure_);
-      new_scope_closure_->AddTemporary(var_and_index.first);
-    }
-  }
-}
-
 void Rewriter::VisitFunctionLiteral(FunctionLiteral* function_literal) {
-  function_literal->scope()->ReplaceOuterScope(new_scope_);
+  function_literal->scope()->ReplaceOuterScope(param_scope_);
 }
 
 
 void Rewriter::VisitClassLiteral(ClassLiteral* class_literal) {
-  class_literal->scope()->ReplaceOuterScope(new_scope_);
+  class_literal->scope()->ReplaceOuterScope(param_scope_);
   if (class_literal->extends() != nullptr) {
     Visit(class_literal->extends());
   }
@@ -91,26 +60,21 @@ void Rewriter::VisitClassLiteral(ClassLiteral* class_literal) {
 
 
 void Rewriter::VisitVariableProxy(VariableProxy* proxy) {
-  if (proxy->is_resolved()) {
-    Variable* var = proxy->var();
-    if (var->mode() != TEMPORARY) return;
-    // 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_closure_) return;
-    int index = old_scope_closure_->RemoveTemporary(var);
-    if (index >= 0) {
-      temps_.push_back(std::make_pair(var, index));
+  if (!proxy->is_resolved()) {
+    if (param_scope_->outer_scope()->RemoveUnresolved(proxy)) {
+      param_scope_->AddUnresolved(proxy);
     }
-  } else if (old_scope_->RemoveUnresolved(proxy)) {
-    new_scope_->AddUnresolved(proxy);
+  } else {
+    // Ensure that temporaries we find are already in the correct scope.
+    DCHECK(proxy->var()->mode() != TEMPORARY ||
+           proxy->var()->scope() == param_scope_->ClosureScope());
   }
 }
 
 
 void Rewriter::VisitBlock(Block* stmt) {
   if (stmt->scope() != nullptr)
-    stmt->scope()->ReplaceOuterScope(new_scope_);
+    stmt->scope()->ReplaceOuterScope(param_scope_);
   else
     VisitStatements(stmt->statements());
 }
@@ -118,23 +82,28 @@ void Rewriter::VisitBlock(Block* stmt) {
 
 void Rewriter::VisitTryCatchStatement(TryCatchStatement* stmt) {
   Visit(stmt->try_block());
-  stmt->scope()->ReplaceOuterScope(new_scope_);
+  stmt->scope()->ReplaceOuterScope(param_scope_);
 }
 
 
 void Rewriter::VisitWithStatement(WithStatement* stmt) {
   Visit(stmt->expression());
-  stmt->scope()->ReplaceOuterScope(new_scope_);
+  stmt->scope()->ReplaceOuterScope(param_scope_);
 }
 
 
 }  // anonymous namespace
 
+void ReparentParameterExpressionScope(uintptr_t stack_limit, Expression* expr,
+                                      Scope* param_scope) {
+  // The only case that uses this code is block scopes for parameters containing
+  // sloppy eval.
+  DCHECK(param_scope->is_block_scope());
+  DCHECK(param_scope->is_declaration_scope());
+  DCHECK(param_scope->calls_sloppy_eval());
+  DCHECK(param_scope->outer_scope()->is_function_scope());
 
-void RewriteParameterInitializerScope(uintptr_t stack_limit,
-                                      Expression* initializer, Scope* old_scope,
-                                      Scope* new_scope) {
-  Rewriter rewriter(stack_limit, initializer, old_scope, new_scope);
+  Rewriter rewriter(stack_limit, expr, param_scope);
   rewriter.Run();
 }
 

src/parsing/parameter-initializer-rewriter.h

@@ -10,11 +10,15 @@
 namespace v8 {
 namespace internal {
 
-
-void RewriteParameterInitializerScope(uintptr_t stack_limit,
-                                      Expression* initializer, Scope* old_scope,
-                                      Scope* new_scope);
-
+// When an extra declaration scope needs to be inserted to account for
+// a sloppy eval in a default parameter or function body, the expressions
+// needs to be in that new inner scope which was added after initial
+// parsing.
+//
+// param_scope is the new inner scope, and its outer_scope() is assumed
+// to be the function scope which was used during the initial parse.
+void ReparentParameterExpressionScope(uintptr_t stack_limit, Expression* expr,
+                                      Scope* param_scope);
 
 }  // namespace internal
 }  // namespace v8

src/parsing/parser-base.h

@@ -2263,6 +2263,8 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
   ExpressionClassifier arrow_formals_classifier(this,
                                                 classifier->duplicate_finder());
 
+  Scope::Snapshot scope_snapshot(scope());
+
   bool is_async = allow_harmony_async_await() && peek() == Token::ASYNC &&
                   !scanner()->HasAnyLineTerminatorAfterNext() &&
                   IsValidArrowFormalParametersStart(PeekAhead());
@@ -2312,8 +2314,8 @@ ParserBase<Traits>::ParseAssignmentExpression(bool accept_IN,
 
     scope->set_start_position(lhs_beg_pos);
     Scanner::Location duplicate_loc = Scanner::Location::invalid();
-    this->ParseArrowFunctionFormalParameterList(&parameters, expression, loc,
-                                                &duplicate_loc, CHECK_OK);
+    this->ParseArrowFunctionFormalParameterList(
+        &parameters, expression, loc, &duplicate_loc, scope_snapshot, CHECK_OK);
     if (duplicate_loc.IsValid()) {
       arrow_formals_classifier.RecordDuplicateFormalParameterError(
           duplicate_loc);

src/parsing/parser.cc

@@ -4138,22 +4138,11 @@ void ParserTraits::ParseArrowFunctionFormalParameters(
   }
 
   Expression* initializer = nullptr;
-  if (expr->IsVariableProxy()) {
-    // When the formal parameter was originally seen, it was parsed as a
-    // VariableProxy and recorded as unresolved in the scope.  Here we undo that
-    // parse-time side-effect for parameters that are single-names (not
-    // patterns; for patterns that happens uniformly in
-    // PatternRewriter::VisitVariableProxy).
-    parser_->scope()->RemoveUnresolved(expr->AsVariableProxy());
-  } else if (expr->IsAssignment()) {
+  if (expr->IsAssignment()) {
     Assignment* assignment = expr->AsAssignment();
     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, end_pos, is_rest);
@@ -4233,16 +4222,17 @@ DoExpression* Parser::ParseDoExpression(bool* ok) {
   return expr;
 }
 
-
 void ParserTraits::ParseArrowFunctionFormalParameterList(
     ParserFormalParameters* parameters, Expression* expr,
-    const Scanner::Location& params_loc,
-    Scanner::Location* duplicate_loc, bool* ok) {
+    const Scanner::Location& params_loc, Scanner::Location* duplicate_loc,
+    const Scope::Snapshot& scope_snapshot, bool* ok) {
   if (expr->IsEmptyParentheses()) return;
 
   ParseArrowFunctionFormalParameters(parameters, expr, params_loc.end_pos,
                                      CHECK_OK_VOID);
 
+  scope_snapshot.Reparent(parameters->scope);
+
   if (parameters->Arity() > Code::kMaxArguments) {
     ReportMessageAt(params_loc, MessageTemplate::kMalformedArrowFunParamList);
     *ok = false;
@@ -4731,7 +4721,7 @@ Block* Parser::BuildParameterInitializationBlock(
       // rewrite inner initializers of the pattern to param_scope
       descriptor.scope = param_scope;
       // Rewrite the outer initializer to point to param_scope
-      RewriteParameterInitializerScope(stack_limit(), initial_value, scope(),
+      ReparentParameterExpressionScope(stack_limit(), initial_value,
                                        param_scope);
     }
 

src/parsing/parser.h

@@ -558,8 +558,8 @@ class ParserTraits {
                                           bool* ok);
   void ParseArrowFunctionFormalParameterList(
       ParserFormalParameters* parameters, Expression* params,
-      const Scanner::Location& params_loc,
-      Scanner::Location* duplicate_loc, bool* ok);
+      const Scanner::Location& params_loc, Scanner::Location* duplicate_loc,
+      const Scope::Snapshot& scope_snapshot, bool* ok);
 
   V8_INLINE Expression* ParseAsyncFunctionExpression(bool* ok);
 

src/parsing/pattern-rewriter.cc

@@ -380,36 +380,20 @@ void Parser::PatternRewriter::VisitRewritableExpression(
   return set_context(old_context);
 }
 
-// Two cases for scope rewriting the scope of default parameters:
-// - Eagerly parsed arrow functions are initially parsed as having
-//   expressions in the enclosing scope, but when the arrow is encountered,
-//   need to be in the scope of the function.
-// - When an extra declaration scope needs to be inserted to account for
-//   a sloppy eval in a default parameter or function body, the expressions
-//   needs to be in that new inner scope which was added after initial
-//   parsing.
-// Each of these cases can be handled by rewriting the contents of the
-// expression to the current scope. The source scope is typically the outer
-// scope when one case occurs; when both cases occur, both scopes need to
-// be included as the outer scope. (Both rewritings still need to be done
-// to account for lazily parsed arrow functions which hit the second case.)
-// TODO(littledan): Remove the outer_scope parameter of
-//                  RewriteParameterInitializerScope
+// When an extra declaration scope needs to be inserted to account for
+// a sloppy eval in a default parameter or function body, the expressions
+// needs to be in that new inner scope which was added after initial
+// parsing.
 void Parser::PatternRewriter::RewriteParameterScopes(Expression* expr) {
   if (!IsBindingContext()) return;
   if (descriptor_->declaration_kind != DeclarationDescriptor::PARAMETER) return;
-  if (!scope()->is_arrow_scope() && !scope()->is_block_scope()) return;
+  if (!scope()->is_block_scope()) return;
 
-  // Either this scope is an arrow scope or a declaration block scope.
   DCHECK(scope()->is_declaration_scope());
+  DCHECK(scope()->outer_scope()->is_function_scope());
+  DCHECK(scope()->calls_sloppy_eval());
 
-  if (scope()->outer_scope()->is_arrow_scope() && scope()->is_block_scope()) {
-    RewriteParameterInitializerScope(parser_->stack_limit(), expr,
-                                     scope()->outer_scope()->outer_scope(),
-                                     scope());
-  }
-  RewriteParameterInitializerScope(parser_->stack_limit(), expr,
-                                   scope()->outer_scope(), scope());
+  ReparentParameterExpressionScope(parser_->stack_limit(), expr, scope());
 }
 
 void Parser::PatternRewriter::VisitObjectLiteral(ObjectLiteral* pattern,

src/parsing/preparser.h

@@ -878,9 +878,9 @@ class PreParserTraits {
       FunctionLiteral::FunctionType function_type, bool* ok);
 
   V8_INLINE void ParseArrowFunctionFormalParameterList(
-      PreParserFormalParameters* parameters,
-      PreParserExpression expression, const Scanner::Location& params_loc,
-      Scanner::Location* duplicate_loc, bool* ok);
+      PreParserFormalParameters* parameters, PreParserExpression expression,
+      const Scanner::Location& params_loc, Scanner::Location* duplicate_loc,
+      const Scope::Snapshot& scope_snapshot, bool* ok);
 
   void ParseAsyncArrowSingleExpressionBody(
       PreParserStatementList body, bool accept_IN,
@@ -1192,11 +1192,10 @@ PreParserExpression PreParserTraits::SpreadCallNew(PreParserExpression function,
   return pre_parser_->factory()->NewCallNew(function, args, pos);
 }
 
-
 void PreParserTraits::ParseArrowFunctionFormalParameterList(
-    PreParserFormalParameters* parameters,
-    PreParserExpression params, const Scanner::Location& params_loc,
-    Scanner::Location* duplicate_loc, bool* ok) {
+    PreParserFormalParameters* parameters, PreParserExpression params,
+    const Scanner::Location& params_loc, Scanner::Location* duplicate_loc,
+    const Scope::Snapshot& scope_snapshot, bool* ok) {
   // TODO(wingo): Detect duplicated identifiers in paramlists.  Detect parameter
   // lists that are too long.
 }