Function name variable does not need a VariableDeclaration

This changes Scope::function_ (for holding the name binding
for named function expression) from a VariableDeclaration
to a Variable. No work is done when visiting this declaration,
since it's kCreatedInitialized, so we can treat it like
other function-specific variables.

This simplifies a wide variety of code, and centralizes
the logic for constructing the variable inside scopes.cc.
This may one day make it easier to eliminate the CONST_LEGACY
VariableMode.

R=neis@chromium.org, verwaest@chromium.org
BUG=v8:5209

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

src/ast/scopeinfo.cc

@@ -55,8 +55,8 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
   VariableAllocationInfo function_name_info;
   VariableMode function_variable_mode;
   if (scope->is_function_scope() &&
-      scope->AsDeclarationScope()->function() != nullptr) {
-    Variable* var = scope->AsDeclarationScope()->function()->proxy()->var();
+      scope->AsDeclarationScope()->function_var() != nullptr) {
+    Variable* var = scope->AsDeclarationScope()->function_var();
     if (!var->is_used()) {
       function_name_info = UNUSED;
     } else if (var->IsContextSlot()) {
@@ -192,10 +192,9 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
   // If present, add the function variable name and its index.
   DCHECK(index == scope_info->FunctionNameEntryIndex());
   if (has_function_name) {
-    int var_index =
-        scope->AsDeclarationScope()->function()->proxy()->var()->index();
+    int var_index = scope->AsDeclarationScope()->function_var()->index();
     scope_info->set(index++,
-                    *scope->AsDeclarationScope()->function()->proxy()->name());
+                    *scope->AsDeclarationScope()->function_var()->name());
     scope_info->set(index++, Smi::FromInt(var_index));
     DCHECK(function_name_info != CONTEXT ||
            var_index == scope_info->ContextLength() - 1);

src/ast/scopes.cc

@@ -333,18 +333,13 @@ void Scope::DeserializeScopeInfo(Isolate* isolate,
 
   // Internalize function proxy for this scope.
   if (scope_info_->HasFunctionName()) {
-    AstNodeFactory factory(ast_value_factory);
     Handle<String> name_handle(scope_info_->FunctionName(), isolate);
     const AstRawString* name = ast_value_factory->GetString(name_handle);
     VariableMode mode;
     int index = scope_info_->FunctionContextSlotIndex(*name_handle, &mode);
     if (index >= 0) {
-      Variable* result = new (zone())
-          Variable(this, name, mode, Variable::NORMAL, kCreatedInitialized);
-      VariableProxy* proxy = factory.NewVariableProxy(result);
-      VariableDeclaration* declaration =
-          factory.NewVariableDeclaration(proxy, this, kNoSourcePosition);
-      AsDeclarationScope()->DeclareFunctionVar(declaration);
+      Variable* result = AsDeclarationScope()->DeclareFunctionVar(name);
+      DCHECK_EQ(mode, result->mode());
       result->AllocateTo(VariableLocation::CONTEXT, index);
     }
   }
@@ -430,6 +425,14 @@ void DeclarationScope::DeclareDefaultFunctionVariables(
   }
 }
 
+Variable* DeclarationScope::DeclareFunctionVar(const AstRawString* name) {
+  DCHECK(is_function_scope());
+  DCHECK_NULL(function_);
+  VariableMode mode = is_strict(language_mode()) ? CONST : CONST_LEGACY;
+  function_ = new (zone())
+      Variable(this, name, mode, Variable::NORMAL, kCreatedInitialized);
+  return function_;
+}
 
 Scope* Scope::FinalizeBlockScope() {
   DCHECK(is_block_scope());
@@ -597,27 +600,20 @@ Variable* Scope::LookupLocal(const AstRawString* name) {
   return var;
 }
 
-Variable* DeclarationScope::LookupFunctionVar(const AstRawString* name,
-                                              AstNodeFactory* factory) {
-  if (function_ != NULL && function_->proxy()->raw_name() == name) {
-    return function_->proxy()->var();
+Variable* DeclarationScope::LookupFunctionVar(const AstRawString* name) {
+  if (function_ != nullptr && function_->raw_name() == name) {
+    return function_;
   } else if (!scope_info_.is_null()) {
     // If we are backed by a scope info, try to lookup the variable there.
     VariableMode mode;
     int index = scope_info_->FunctionContextSlotIndex(*(name->string()), &mode);
-    if (index < 0) return NULL;
-    Variable* var = new (zone())
-        Variable(this, name, mode, Variable::NORMAL, kCreatedInitialized);
-    DCHECK_NOT_NULL(factory);
-    VariableProxy* proxy = factory->NewVariableProxy(var);
-    VariableDeclaration* declaration =
-        factory->NewVariableDeclaration(proxy, this, kNoSourcePosition);
-    DCHECK_EQ(factory->zone(), zone());
-    DeclareFunctionVar(declaration);
+    if (index < 0) return nullptr;
+    Variable* var = DeclareFunctionVar(name);
+    DCHECK_EQ(mode, var->mode());
     var->AllocateTo(VariableLocation::CONTEXT, index);
     return var;
   } else {
-    return NULL;
+    return nullptr;
   }
 }
 
@@ -745,11 +741,6 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
   for (int i = 0; i < length; i++) {
     Declaration* decl = decls_[i];
     VariableMode mode = decl->proxy()->var()->mode();
-    // We don't create a separate scope to hold the function name of a function
-    // expression, so we have to make sure not to consider it when checking for
-    // conflicts (since it's conceptually "outside" the declaration scope).
-    if (is_function_scope() && decl == AsDeclarationScope()->function())
-      continue;
     if (IsLexicalVariableMode(mode) && !is_block_scope()) continue;
 
     // Iterate through all scopes until and including the declaration scope.
@@ -1146,10 +1137,10 @@ void Scope::Print(int n) {
   }
 
   // Print parameters, if any.
-  VariableDeclaration* function = nullptr;
+  Variable* function = nullptr;
   if (is_function_scope()) {
     AsDeclarationScope()->PrintParameters();
-    function = AsDeclarationScope()->function();
+    function = AsDeclarationScope()->function_var();
   }
 
   PrintF(" { // (%d, %d)\n", start_position(), end_position());
@@ -1157,7 +1148,7 @@ void Scope::Print(int n) {
   // Function name, if any (named function literals, only).
   if (function != nullptr) {
     Indent(n1, "// (local) function name: ");
-    PrintName(function->proxy()->raw_name());
+    PrintName(function->raw_name());
     PrintF("\n");
   }
 
@@ -1191,7 +1182,7 @@ void Scope::Print(int n) {
   // Print locals.
   if (function != nullptr) {
     Indent(n1, "// function var:\n");
-    PrintVar(n1, function->proxy()->var());
+    PrintVar(n1, function);
   }
 
   if (is_declaration_scope()) {
@@ -1293,10 +1284,9 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy,
   // We did not find a variable locally. Check against the function variable, if
   // any.
   *binding_kind = UNBOUND;
-  var =
-      is_function_scope()
-          ? AsDeclarationScope()->LookupFunctionVar(proxy->raw_name(), factory)
-          : nullptr;
+  var = is_function_scope()
+            ? AsDeclarationScope()->LookupFunctionVar(proxy->raw_name())
+            : nullptr;
   if (var != NULL) {
     *binding_kind = BOUND;
   } else if (outer_scope_ != nullptr && this != max_outer_scope) {
@@ -1697,7 +1687,7 @@ void DeclarationScope::AllocateLocals(AstValueFactory* ast_value_factory) {
   // because of the current ScopeInfo implementation (see
   // ScopeInfo::ScopeInfo(FunctionScope* scope) constructor).
   if (function_ != nullptr) {
-    AllocateNonParameterLocal(function_->proxy()->var(), ast_value_factory);
+    AllocateNonParameterLocal(function_, ast_value_factory);
   }
 
   if (rest_parameter_ != nullptr) {
@@ -1776,19 +1766,19 @@ void Scope::AllocateVariablesRecursively(AstValueFactory* ast_value_factory) {
 
 
 int Scope::StackLocalCount() const {
-  VariableDeclaration* function =
-      is_function_scope() ? AsDeclarationScope()->function() : nullptr;
+  Variable* function =
+      is_function_scope() ? AsDeclarationScope()->function_var() : nullptr;
   return num_stack_slots() -
-         (function != NULL && function->proxy()->var()->IsStackLocal() ? 1 : 0);
+         (function != nullptr && function->IsStackLocal() ? 1 : 0);
 }
 
 
 int Scope::ContextLocalCount() const {
   if (num_heap_slots() == 0) return 0;
-  VariableDeclaration* function =
-      is_function_scope() ? AsDeclarationScope()->function() : nullptr;
+  Variable* function =
+      is_function_scope() ? AsDeclarationScope()->function_var() : nullptr;
   bool is_function_var_in_context =
-      function != NULL && function->proxy()->var()->IsContextSlot();
+      function != nullptr && function->IsContextSlot();
   return num_heap_slots() - Context::MIN_CONTEXT_SLOTS - num_global_slots() -
          (is_function_var_in_context ? 1 : 0);
 }

src/ast/scopes.h

@@ -689,19 +689,12 @@ class DeclarationScope : public Scope {
   // between this scope and the outer scope. (ECMA-262, 3rd., requires that
   // the name of named function literal is kept in an intermediate scope
   // in between this scope and the next outer scope.)
-  Variable* LookupFunctionVar(const AstRawString* name,
-                              AstNodeFactory* factory);
+  Variable* LookupFunctionVar(const AstRawString* name);
 
   // Declare the function variable for a function literal. This variable
   // is in an intermediate scope between this function scope and the the
   // outer scope. Only possible for function scopes; at most one variable.
-  void DeclareFunctionVar(VariableDeclaration* declaration) {
-    DCHECK(is_function_scope());
-    // Handle implicit declaration of the function name in named function
-    // expressions before other declarations.
-    declarations()->InsertAt(0, declaration, zone());
-    function_ = declaration;
-  }
+  Variable* DeclareFunctionVar(const AstRawString* name);
 
   // Declare a parameter in this scope.  When there are duplicated
   // parameters the rightmost one 'wins'.  However, the implementation
@@ -735,7 +728,7 @@ class DeclarationScope : public Scope {
 
   // The variable holding the function literal for named function
   // literals, or NULL.  Only valid for function scopes.
-  VariableDeclaration* function() const {
+  Variable* function_var() const {
     DCHECK(is_function_scope());
     return function_;
   }
@@ -889,7 +882,7 @@ class DeclarationScope : public Scope {
   // Convenience variable.
   Variable* receiver_;
   // Function variable, if any; function scopes only.
-  VariableDeclaration* function_;
+  Variable* function_;
   // new.target variable, function scopes only.
   Variable* new_target_;
   // Convenience variable; function scopes only.

src/parsing/parser.cc

@@ -4932,19 +4932,8 @@ ZoneList<Statement*>* Parser::ParseEagerFunctionBody(
   if (function_type == FunctionLiteral::kNamedExpression) {
     // Now that we know the language mode, we can create the const assignment
     // in the previously reserved spot.
-    // NOTE: We create a proxy and resolve it here so that in the
-    // future we can change the AST to only refer to VariableProxies
-    // instead of Variables and Proxies as is the case now.
     DCHECK_EQ(function_scope, scope());
-    VariableMode fvar_mode = is_strict(language_mode()) ? CONST : CONST_LEGACY;
-    Variable* fvar = new (zone())
-        Variable(scope(), function_name, fvar_mode, Variable::NORMAL,
-                 kCreatedInitialized, kNotAssigned);
-    VariableProxy* proxy = factory()->NewVariableProxy(fvar);
-    VariableDeclaration* fvar_declaration =
-        factory()->NewVariableDeclaration(proxy, scope(), kNoSourcePosition);
-    function_scope->DeclareFunctionVar(fvar_declaration);
-
+    Variable* fvar = function_scope->DeclareFunctionVar(function_name);
     VariableProxy* fproxy = factory()->NewVariableProxy(fvar);
     result->Set(kFunctionNameAssignmentIndex,
                 factory()->NewExpressionStatement(

test/cctest/test-parsing.cc

@@ -1495,8 +1495,8 @@ TEST(DiscardFunctionBody) {
         AsCall()->expression()->AsFunctionLiteral();
     i::Scope* inner_scope = inner->scope();
     i::FunctionLiteral* fun = nullptr;
-    if (inner_scope->declarations()->length() > 1) {
-      fun = inner_scope->declarations()->at(1)->AsFunctionDeclaration()->fun();
+    if (inner_scope->declarations()->length() > 0) {
+      fun = inner_scope->declarations()->at(0)->AsFunctionDeclaration()->fun();
     } else {
       // TODO(conradw): This path won't be hit until the other test cases can be
       // uncommented.