Merge DeclarationScope::temps_ and Scope::ordered_variables_ into Scope::locals_

BUG=v8:5209

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

src/ast/scopes.cc

@@ -41,13 +41,27 @@ Variable* VariableMap::Declare(Zone* zone, Scope* scope,
   if (added) *added = p->value == nullptr;
   if (p->value == nullptr) {
     // The variable has not been declared yet -> insert it.
-    DCHECK(p->key == name);
+    DCHECK_EQ(name, p->key);
     p->value = new (zone) Variable(scope, name, mode, kind, initialization_flag,
                                    maybe_assigned_flag);
   }
   return reinterpret_cast<Variable*>(p->value);
 }
 
+void VariableMap::Remove(Variable* var) {
+  const AstRawString* name = var->raw_name();
+  ZoneHashMap::Remove(const_cast<AstRawString*>(name), name->hash());
+}
+
+void VariableMap::Add(Zone* zone, Variable* var) {
+  const AstRawString* name = var->raw_name();
+  Entry* p =
+      ZoneHashMap::LookupOrInsert(const_cast<AstRawString*>(name), name->hash(),
+                                  ZoneAllocationPolicy(zone));
+  DCHECK_NULL(p->value);
+  DCHECK_EQ(name, p->key);
+  p->value = var;
+}
 
 Variable* VariableMap::Lookup(const AstRawString* name) {
   Entry* p = ZoneHashMap::Lookup(const_cast<AstRawString*>(name), name->hash());
@@ -81,7 +95,7 @@ Scope::Scope(Zone* zone, ScopeType scope_type)
     : zone_(zone),
       outer_scope_(nullptr),
       variables_(zone),
-      ordered_variables_(4, zone),
+      locals_(4, zone),
       decls_(4, zone),
       scope_type_(scope_type) {
   DCHECK(scope_type == SCRIPT_SCOPE || scope_type == WITH_SCOPE);
@@ -97,7 +111,7 @@ Scope::Scope(Zone* zone, Scope* outer_scope, ScopeType scope_type)
     : zone_(zone),
       outer_scope_(outer_scope),
       variables_(zone),
-      ordered_variables_(4, zone),
+      locals_(4, zone),
       decls_(4, zone),
       scope_type_(scope_type) {
   DCHECK_NE(SCRIPT_SCOPE, scope_type);
@@ -112,12 +126,12 @@ Scope::Snapshot::Snapshot(Scope* scope)
     : outer_scope_(scope),
       top_inner_scope_(scope->inner_scope_),
       top_unresolved_(scope->unresolved_),
-      top_temp_(scope->GetClosureScope()->temps()->length()) {}
+      top_local_(scope->GetClosureScope()->locals_.length()),
+      top_decl_(scope->GetClosureScope()->decls_.length()) {}
 
 DeclarationScope::DeclarationScope(Zone* zone)
     : Scope(zone),
       function_kind_(kNormalFunction),
-      temps_(4, zone),
       params_(4, zone),
       sloppy_block_function_map_(zone) {
   SetDefaults();
@@ -128,7 +142,6 @@ DeclarationScope::DeclarationScope(Zone* zone, Scope* outer_scope,
                                    FunctionKind function_kind)
     : Scope(zone, outer_scope, scope_type),
       function_kind_(function_kind),
-      temps_(4, zone),
       params_(4, zone),
       sloppy_block_function_map_(zone) {
   SetDefaults();
@@ -147,7 +160,7 @@ Scope::Scope(Zone* zone, ScopeType scope_type, Handle<ScopeInfo> scope_info)
     : zone_(zone),
       outer_scope_(nullptr),
       variables_(zone),
-      ordered_variables_(0, zone),
+      locals_(0, zone),
       decls_(0, zone),
       scope_info_(scope_info),
       scope_type_(scope_type) {
@@ -166,7 +179,6 @@ DeclarationScope::DeclarationScope(Zone* zone, ScopeType scope_type,
                                    Handle<ScopeInfo> scope_info)
     : Scope(zone, scope_type, scope_info),
       function_kind_(scope_info->function_kind()),
-      temps_(0, zone),
       params_(0, zone),
       sloppy_block_function_map_(zone) {
   SetDefaults();
@@ -176,7 +188,7 @@ Scope::Scope(Zone* zone, const AstRawString* catch_variable_name)
     : zone_(zone),
       outer_scope_(nullptr),
       variables_(zone),
-      ordered_variables_(0, zone),
+      locals_(0, zone),
       decls_(0, zone),
       scope_type_(CATCH_SCOPE) {
   SetDefaults();
@@ -530,7 +542,7 @@ void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
   DCHECK_EQ(new_parent, new_parent->GetClosureScope());
   DCHECK_NULL(new_parent->inner_scope_);
   DCHECK_NULL(new_parent->unresolved_);
-  DCHECK_EQ(0, new_parent->temps()->length());
+  DCHECK_EQ(0, new_parent->locals_.length());
   Scope* inner_scope = new_parent->sibling_;
   if (inner_scope != top_inner_scope_) {
     for (; inner_scope->sibling() != top_inner_scope_;
@@ -557,17 +569,25 @@ void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
     outer_scope_->unresolved_ = top_unresolved_;
   }
 
-  if (outer_scope_->GetClosureScope()->temps()->length() != top_temp_) {
-    ZoneList<Variable*>* temps = outer_scope_->GetClosureScope()->temps();
-    for (int i = top_temp_; i < temps->length(); i++) {
-      Variable* temp = temps->at(i);
-      DCHECK_EQ(temp->scope(), temp->scope()->GetClosureScope());
-      DCHECK_NE(temp->scope(), new_parent);
-      temp->set_scope(new_parent);
-      new_parent->AddTemporary(temp);
+  // TODO(verwaest): This currently only moves do-expression declared variables
+  // in default arguments that weren't already previously declared with the same
+  // name in the closure-scope. See
+  // test/mjsunit/harmony/default-parameter-do-expression.js.
+  DeclarationScope* outer_closure = outer_scope_->GetClosureScope();
+  for (int i = top_local_; i < outer_closure->locals_.length(); i++) {
+    Variable* local = outer_closure->locals_.at(i);
+    DCHECK(local->mode() == TEMPORARY || local->mode() == VAR);
+    DCHECK_EQ(local->scope(), local->scope()->GetClosureScope());
+    DCHECK_NE(local->scope(), new_parent);
+    local->set_scope(new_parent);
+    new_parent->AddLocal(local);
+    if (local->mode() == VAR) {
+      outer_closure->variables_.Remove(local);
+      new_parent->variables_.Add(new_parent->zone(), local);
     }
-    temps->Rewind(top_temp_);
   }
+  outer_closure->locals_.Rewind(top_local_);
+  outer_closure->decls_.Rewind(top_decl_);
 }
 
 void Scope::ReplaceOuterScope(Scope* outer) {
@@ -744,7 +764,7 @@ Variable* Scope::NewTemporary(const AstRawString* name) {
                                        TEMPORARY,
                                        Variable::NORMAL,
                                        kCreatedInitialized);
-  scope->AddTemporary(var);
+  scope->AddLocal(var);
   return var;
 }
 
@@ -806,31 +826,13 @@ Declaration* Scope::CheckLexDeclarationsConflictingWith(
 void Scope::CollectStackAndContextLocals(ZoneList<Variable*>* stack_locals,
                                          ZoneList<Variable*>* context_locals,
                                          ZoneList<Variable*>* context_globals) {
-  DCHECK(stack_locals != NULL);
-  DCHECK(context_locals != NULL);
-  DCHECK(context_globals != NULL);
+  // TODO(verwaest): Just pass out locals_ directly and walk it?
+  DCHECK_NOT_NULL(stack_locals);
+  DCHECK_NOT_NULL(context_locals);
+  DCHECK_NOT_NULL(context_globals);
 
-  // Collect temporaries which are always allocated on the stack, unless the
-  // context as a whole has forced context allocation.
-  if (is_declaration_scope()) {
-    ZoneList<Variable*>* temps = AsDeclarationScope()->temps();
-    for (int i = 0; i < temps->length(); i++) {
-      Variable* var = (*temps)[i];
-      if (var->is_used()) {
-        if (var->IsContextSlot()) {
-          DCHECK(has_forced_context_allocation());
-          context_locals->Add(var, zone());
-        } else if (var->IsStackLocal()) {
-          stack_locals->Add(var, zone());
-        } else {
-          DCHECK(var->IsParameter());
-        }
-      }
-    }
-  }
-
-  for (int i = 0; i < ordered_variables_.length(); i++) {
-    Variable* var = ordered_variables_[i];
+  for (int i = 0; i < locals_.length(); i++) {
+    Variable* var = locals_[i];
     if (var->IsStackLocal()) {
       stack_locals->Add(var, zone());
     } else if (var->IsContextSlot()) {
@@ -1157,18 +1159,6 @@ void Scope::Print(int n) {
     PrintVar(n1, function);
   }
 
-  if (is_declaration_scope()) {
-    bool printed_header = false;
-    ZoneList<Variable*>* temps = AsDeclarationScope()->temps();
-    for (int i = 0; i < temps->length(); i++) {
-      if (!printed_header) {
-        printed_header = true;
-        Indent(n1, "// temporary vars:\n");
-      }
-      PrintVar(n1, (*temps)[i]);
-    }
-  }
-
   if (variables_.Start() != NULL) {
     Indent(n1, "// local vars:\n");
     PrintMap(n1, &variables_, true);
@@ -1550,21 +1540,13 @@ void Scope::AllocateDeclaredGlobal(Variable* var) {
 }
 
 void Scope::AllocateNonParameterLocalsAndDeclaredGlobals() {
-  // All variables that have no rewrite yet are non-parameter locals.
-  if (is_declaration_scope()) {
-    ZoneList<Variable*>* temps = AsDeclarationScope()->temps();
-    for (int i = 0; i < temps->length(); i++) {
-      AllocateNonParameterLocal((*temps)[i]);
-    }
-  }
-
-  for (int i = 0; i < ordered_variables_.length(); i++) {
-    AllocateNonParameterLocal(ordered_variables_[i]);
+  for (int i = 0; i < locals_.length(); i++) {
+    AllocateNonParameterLocal(locals_[i]);
   }
 
   if (FLAG_global_var_shortcuts) {
-    for (int i = 0; i < ordered_variables_.length(); i++) {
-      AllocateDeclaredGlobal(ordered_variables_[i]);
+    for (int i = 0; i < locals_.length(); i++) {
+      AllocateDeclaredGlobal(locals_[i]);
     }
   }
 

src/ast/scopes.h

@@ -27,6 +27,8 @@ class VariableMap: public ZoneHashMap {
                     bool* added = nullptr);
 
   Variable* Lookup(const AstRawString* name);
+  void Remove(Variable* var);
+  void Add(Zone* zone, Variable* var);
 };
 
 
@@ -84,7 +86,8 @@ class Scope: public ZoneObject {
     Scope* outer_scope_;
     Scope* top_inner_scope_;
     VariableProxy* top_unresolved_;
-    int top_temp_;
+    int top_local_;
+    int top_decl_;
   };
 
   // Compute top scope and allocate variables. For lazy compilation the top
@@ -437,7 +440,7 @@ class Scope: public ZoneObject {
     Variable* var =
         variables_.Declare(zone, scope, name, mode, kind, initialization_flag,
                            maybe_assigned_flag, &added);
-    if (added) ordered_variables_.Add(var, zone);
+    if (added) locals_.Add(var, zone);
     return var;
   }
   Zone* zone_;
@@ -456,7 +459,7 @@ class Scope: public ZoneObject {
   // In case of non-scopeinfo-backed scopes, this contains the variables of the
   // map above in order of addition.
   // TODO(verwaest): Thread through Variable.
-  ZoneList<Variable*> ordered_variables_;
+  ZoneList<Variable*> locals_;
   // Unresolved variables referred to from this scope. The proxies themselves
   // form a linked list of all unresolved proxies.
   VariableProxy* unresolved_;
@@ -729,18 +732,16 @@ class DeclarationScope : public Scope {
     return this_function_;
   }
 
-  // Adds a temporary variable in this scope's TemporaryScope. This is for
-  // adjusting the scope of temporaries used when desugaring parameter
+  // Adds a local variable in this scope's locals list. This is for adjusting
+  // the scope of temporaries and do-expression vars when desugaring parameter
   // initializers.
-  void AddTemporary(Variable* var) {
+  void AddLocal(Variable* var) {
     DCHECK(!already_resolved_);
     // Temporaries are only placed in ClosureScopes.
     DCHECK_EQ(GetClosureScope(), this);
-    temps_.Add(var, zone());
+    locals_.Add(var, zone());
   }
 
-  ZoneList<Variable*>* temps() { return &temps_; }
-
   void DeclareSloppyBlockFunction(const AstRawString* name,
                                   SloppyBlockFunctionStatement* statement) {
     sloppy_block_function_map_.Declare(zone(), name, statement);
@@ -817,8 +818,6 @@ class DeclarationScope : public Scope {
 
   // Info about the parameter list of a function.
   int arity_;
-  // Compiler-allocated (user-invisible) temporaries.
-  ZoneList<Variable*> temps_;
   // Parameter list in source order.
   ZoneList<Variable*> params_;
   // Map of function names to lists of functions defined in sloppy blocks

test/cctest/interpreter/bytecode_expectations/DoExpression.golden

@@ -59,10 +59,10 @@ bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   34 E> */ B(StackCheck),
   /*   56 S> */ B(LdaSmi), U8(10),
-                B(Star), R(1),
+                B(Star), R(0),
   /*   69 S> */ B(Inc), U8(1),
-                B(Star), R(1),
                 B(Star), R(0),
+                B(Star), R(1),
   /*   74 S> */ B(Jump), U8(2),
                 B(LdaUndefined),
   /*   94 S> */ B(Return),

test/cctest/interpreter/bytecode_expectations/ForIn.golden

@@ -69,7 +69,7 @@ bytecode array length: 44
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   42 S> */ B(LdaConstant), U8(0),
-                B(Star), R(1),
+                B(Star), R(0),
   /*   68 S> */ B(JumpIfUndefined), U8(37),
                 B(JumpIfNull), U8(35),
                 B(ToObject), R(3),
@@ -80,7 +80,7 @@ bytecodes: [
                 B(JumpIfTrue), U8(22),
                 B(ForInNext), R(3), R(7), R(4), U8(1),
                 B(JumpIfUndefined), U8(9),
-                B(Star), R(0),
+                B(Star), R(1),
   /*   54 E> */ B(StackCheck),
                 B(Star), R(2),
   /*   73 S> */ B(Nop),
@@ -108,7 +108,7 @@ bytecode array length: 55
 bytecodes: [
   /*   30 E> */ B(StackCheck),
   /*   42 S> */ B(LdaZero),
-                B(Star), R(1),
+                B(Star), R(0),
   /*   59 S> */ B(CreateArrayLiteral), U8(0), U8(0), U8(3),
                 B(JumpIfUndefined), U8(45),
                 B(JumpIfNull), U8(43),
@@ -120,13 +120,13 @@ bytecodes: [
                 B(JumpIfTrue), U8(30),
                 B(ForInNext), R(3), R(7), R(4), U8(2),
                 B(JumpIfUndefined), U8(17),
-                B(Star), R(0),
+                B(Star), R(1),
   /*   45 E> */ B(StackCheck),
                 B(Star), R(2),
-  /*   70 S> */ B(Ldar), R(0),
-  /*   75 E> */ B(Add), R(1), U8(1),
-                B(Mov), R(1), R(8),
-                B(Star), R(1),
+  /*   70 S> */ B(Ldar), R(1),
+  /*   75 E> */ B(Add), R(0), U8(1),
+                B(Mov), R(0), R(8),
+                B(Star), R(0),
   /*   72 E> */ B(ForInStep), R(7),
                 B(Star), R(7),
                 B(Jump), U8(-31),

test/mjsunit/harmony/default-parameter-do-expression.js

+// Copyright 2016 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-do-expressions --nolazy
+
+function hoist_unique_do_var() {
+  var f = (y = do { var unique = 3 }) => unique;
+  assertEquals(3, f());
+  assertThrows(() => unique, ReferenceError);
+}
+hoist_unique_do_var();
+
+function hoist_duplicate_do_var() {
+  var duplicate = 100;
+  var f = (y = do { var duplicate = 3 }) => duplicate;
+  assertEquals(3, f());
+  // TODO(verwaest): The {duplicate} declarations were invalidly merged.
+  assertEquals(3, duplicate);
+}
+hoist_duplicate_do_var();