Add function-var to variables_ so LookupRecursive doesn't need to special-case it

src/ast/scopes.cc

Index: src/ast/scopes.cc
diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc
index 2a41c7d47a2c1a458422e4f1225b24bd080b8567..53ddb93678a7919f45530daaece83b1c3ed4c5a3 100644
--- a/src/ast/scopes.cc
+++ b/src/ast/scopes.cc
@@ -476,8 +476,15 @@ 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);
+  bool preexists = variables_.Lookup(name);
+  if (preexists || calls_sloppy_eval()) {

[adamk, 2016/08/24 18:15:49]

This logic is pretty tricky (I find it trickier than the old lookup logic).

+    function_ = new (zone())

[adamk, 2016/08/24 20:22:48]

Maybe we should just skip creating the function_ variable if it's definitely
shadowed in the function? The parser already has to know a lot about how this
stuff works...what if it reacted to a nullptr return value of DeclareFunctionVar
by not creating the assignment?

That lets the calls_sloppy_eval use the Declare() path, with the NonLocal
becoming a DYNAMIC_LOCAL.

WDYT?

+        Variable(this, name, mode, Variable::NORMAL, kCreatedInitialized);
+    if (!preexists) NonLocal(name, DYNAMIC);
+  } else {
+    function_ = variables_.Declare(zone(), this, name, mode, Variable::NORMAL,
+                                   kCreatedInitialized);
+  }
   return function_;
 }
 
@@ -618,7 +625,14 @@ Variable* Scope::LookupLocal(const AstRawString* name) {
   if (index < 0) {
     // Check parameters.
     index = scope_info_->ParameterIndex(*name_handle);
-    if (index < 0) return NULL;
+    if (index < 0) {
+      index = scope_info_->FunctionContextSlotIndex(*name_handle, &mode);
+      if (index < 0) return nullptr;
+      Variable* var = AsDeclarationScope()->DeclareFunctionVar(name);
+      DCHECK_EQ(mode, var->mode());
+      var->AllocateTo(VariableLocation::CONTEXT, index);
+      return variables_.Lookup(name);
+    }
 
     mode = DYNAMIC;
     location = VariableLocation::LOOKUP;
@@ -647,24 +661,6 @@ Variable* Scope::LookupLocal(const AstRawString* name) {
   return 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 nullptr;
-    Variable* var = DeclareFunctionVar(name);
-    DCHECK_EQ(mode, var->mode());
-    var->AllocateTo(VariableLocation::CONTEXT, index);
-    return var;
-  } else {
-    return nullptr;
-  }
-}
-
-
 Variable* Scope::Lookup(const AstRawString* name) {
   for (Scope* scope = this;
        scope != NULL;
@@ -1245,16 +1241,6 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy, bool declare_free,
   // remains the same.)
   if (var != nullptr) return var;
 
-  // We did not find a variable locally. Check against the function variable, if
-  // any.
-  if (is_function_scope()) {
-    var = AsDeclarationScope()->LookupFunctionVar(proxy->raw_name());
-    if (var != nullptr) {
-      if (calls_sloppy_eval()) return NonLocal(proxy->raw_name(), DYNAMIC);
-      return var;
-    }
-  }
-
   if (outer_scope_ == outer_scope_end) {
     if (!declare_free) return nullptr;
     DCHECK(is_script_scope());