Introduce with scope and rework variable resolution.

src/scopes.cc

Index: src/scopes.cc
diff --git a/src/scopes.cc b/src/scopes.cc
index d5a7a9f9ca8d7b86361856fa92bde29bbf796489..4edb4ff0c6bde59dbcddb5d65cb7a4ae71f21f47 100644
--- a/src/scopes.cc
+++ b/src/scopes.cc
@@ -156,9 +156,8 @@ Scope::Scope(Scope* inner_scope,
       unresolved_(16),
       decls_(4),
       already_resolved_(true) {
-  ASSERT(!scope_info.is_null());
   SetDefaults(type, NULL, scope_info);
-  if (scope_info->HasHeapAllocatedLocals()) {
+  if (!scope_info.is_null() && scope_info->HasHeapAllocatedLocals()) {
     num_heap_slots_ = scope_info_->NumberOfContextSlots();
   }
   AddInnerScope(inner_scope);
@@ -202,10 +201,8 @@ void Scope::SetDefaults(Type type,
   scope_calls_eval_ = false;
   // Inherit the strict mode from the parent scope.
   strict_mode_ = (outer_scope != NULL) && outer_scope->strict_mode_;
-  outer_scope_calls_eval_ = false;
   outer_scope_calls_non_strict_eval_ = false;
   inner_scope_calls_eval_ = false;
-  outer_scope_is_eval_scope_ = false;
   force_eager_compilation_ = false;
   num_var_or_const_ = 0;
   num_stack_slots_ = 0;
@@ -224,30 +221,31 @@ Scope* Scope::DeserializeScopeChain(CompilationInfo* info,
   bool contains_with = false;
   while (!context->IsGlobalContext()) {
     if (context->IsWithContext()) {
+      Scope* with_scope = new Scope(current_scope, WITH_SCOPE,
+                                    Handle<SerializedScopeInfo>::null());
+      current_scope = with_scope;
       // All the inner scopes are inside a with.
       contains_with = true;
       for (Scope* s = innermost_scope; s != NULL; s = s->outer_scope()) {
         s->scope_inside_with_ = true;
       }
+    } else if (context->IsFunctionContext()) {
+      SerializedScopeInfo* scope_info =
+          context->closure()->shared()->scope_info();
+      current_scope = new Scope(current_scope, FUNCTION_SCOPE,
+                                Handle<SerializedScopeInfo>(scope_info));
+    } else if (context->IsBlockContext()) {
+      SerializedScopeInfo* scope_info =
+          SerializedScopeInfo::cast(context->extension());
+      current_scope = new Scope(current_scope, BLOCK_SCOPE,
+                                Handle<SerializedScopeInfo>(scope_info));
     } else {
-      if (context->IsFunctionContext()) {
-        SerializedScopeInfo* scope_info =
-            context->closure()->shared()->scope_info();
-        current_scope = new Scope(current_scope, FUNCTION_SCOPE,
-            Handle<SerializedScopeInfo>(scope_info));
-      } else if (context->IsBlockContext()) {
-        SerializedScopeInfo* scope_info =
-            SerializedScopeInfo::cast(context->extension());
-        current_scope = new Scope(current_scope, BLOCK_SCOPE,
-            Handle<SerializedScopeInfo>(scope_info));
-      } else {
-        ASSERT(context->IsCatchContext());
-        String* name = String::cast(context->extension());
-        current_scope = new Scope(current_scope, Handle<String>(name));
-      }
-      if (contains_with) current_scope->RecordWithStatement();
-      if (innermost_scope == NULL) innermost_scope = current_scope;
+      ASSERT(context->IsCatchContext());
+      String* name = String::cast(context->extension());
+      current_scope = new Scope(current_scope, Handle<String>(name));
     }
+    if (contains_with) current_scope->RecordWithStatement();
+    if (innermost_scope == NULL) innermost_scope = current_scope;
 
     // Forget about a with when we move to a context for a different function.
     if (context->previous()->closure() != context->closure()) {
@@ -281,15 +279,15 @@ bool Scope::Analyze(CompilationInfo* info) {
 }
 
 
-void Scope::Initialize(bool inside_with) {
+void Scope::Initialize() {
   ASSERT(!already_resolved());
 
   // Add this scope as a new inner scope of the outer scope.
   if (outer_scope_ != NULL) {
     outer_scope_->inner_scopes_.Add(this);
-    scope_inside_with_ = outer_scope_->scope_inside_with_ || inside_with;
+    scope_inside_with_ = outer_scope_->scope_inside_with_ || is_with_scope();
   } else {
-    scope_inside_with_ = inside_with;
+    scope_inside_with_ = is_with_scope();
   }
 
   // Declare convenience variables.
@@ -300,10 +298,7 @@ void Scope::Initialize(bool inside_with) {
   // instead load them directly from the stack. Currently, the only
   // such parameter is 'this' which is passed on the stack when
   // invoking scripts
-  if (is_catch_scope() || is_block_scope()) {
-    ASSERT(outer_scope() != NULL);
-    receiver_ = outer_scope()->receiver();
-  } else {
+  if (is_declaration_scope()) {
     ASSERT(is_function_scope() ||

[Kevin Millikin (Chromium), 2011/09/16 12:58:19]

I don't think this ASSERT is necessary any more.

[Steven, 2011/09/16 18:29:52]

Done.

            is_global_scope() ||
            is_eval_scope());
@@ -315,6 +310,9 @@ void Scope::Initialize(bool inside_with) {
                            Variable::THIS);
     var->AllocateTo(Variable::PARAMETER, -1);
     receiver_ = var;
+  } else {
+    ASSERT(outer_scope() != NULL);
+    receiver_ = outer_scope()->receiver();
   }
 
   if (is_function_scope()) {
@@ -443,15 +441,13 @@ Variable* Scope::DeclareGlobal(Handle<String> name) {
 }
 
 
-VariableProxy* Scope::NewUnresolved(Handle<String> name,
-                                    bool inside_with,
-                                    int position) {
+VariableProxy* Scope::NewUnresolved(Handle<String> name, int position) {
   // Note that we must not share the unresolved variables with
   // the same name because they may be removed selectively via
   // RemoveUnresolved().
   ASSERT(!already_resolved());
   VariableProxy* proxy = new(isolate_->zone()) VariableProxy(
-      isolate_, name, false, inside_with, position);
+      isolate_, name, false, position);
   unresolved_.Add(proxy);
   return proxy;
 }
@@ -507,17 +503,19 @@ Declaration* Scope::CheckConflictingVarDeclarations() {
     Declaration* decl = decls_[i];
     if (decl->mode() != Variable::VAR) continue;
     Handle<String> name = decl->proxy()->name();
-    bool cond = true;
-    for (Scope* scope = decl->scope(); cond ; scope = scope->outer_scope_) {
+
+    // Iterate through all scopes until and including the declaration scope.
+    Scope* previous = NULL;
+    Scope* current = decl->scope();
+    do {
       // There is a conflict if there exists a non-VAR binding.
-      Variable* other_var = scope->variables_.Lookup(name);
+      Variable* other_var = current->variables_.Lookup(name);
       if (other_var != NULL && other_var->mode() != Variable::VAR) {
         return decl;
       }
-
-      // Include declaration scope in the iteration but stop after.
-      if (!scope->is_block_scope() && !scope->is_catch_scope()) cond = false;
-    }
+      previous = current;
+      current = current->outer_scope_;
+    } while (!previous->is_declaration_scope());
   }
   return NULL;
 }
@@ -565,16 +563,11 @@ void Scope::AllocateVariables(Handle<Context> context) {
   // this information in the ScopeInfo and then use it here (by traversing
   // the call chain stack, at compile time).
 
-  bool eval_scope = is_eval_scope();
-  bool outer_scope_calls_eval = false;
   bool outer_scope_calls_non_strict_eval = false;
   if (!is_global_scope()) {
-    context->ComputeEvalScopeInfo(&outer_scope_calls_eval,
-                                  &outer_scope_calls_non_strict_eval);
+    context->ComputeEvalScopeInfo(&outer_scope_calls_non_strict_eval);
   }
-  PropagateScopeInfo(outer_scope_calls_eval,
-                     outer_scope_calls_non_strict_eval,
-                     eval_scope);
+  PropagateScopeInfo(outer_scope_calls_non_strict_eval);
 
   // 2) Resolve variables.
   Scope* global_scope = NULL;
@@ -627,8 +620,7 @@ int Scope::ContextChainLength(Scope* scope) {
 
 Scope* Scope::DeclarationScope() {
   Scope* scope = this;
-  while (scope->is_catch_scope() ||
-         scope->is_block_scope()) {
+  while (!scope->is_declaration_scope()) {
     scope = scope->outer_scope();
   }
   return scope;
@@ -651,6 +643,7 @@ static const char* Header(Scope::Type type) {
     case Scope::GLOBAL_SCOPE: return "global";
     case Scope::CATCH_SCOPE: return "catch";
     case Scope::BLOCK_SCOPE: return "block";
+    case Scope::WITH_SCOPE: return "with";
   }
   UNREACHABLE();
   return NULL;
@@ -750,14 +743,10 @@ void Scope::Print(int n) {
   if (scope_inside_with_) Indent(n1, "// scope inside 'with'\n");
   if (scope_contains_with_) Indent(n1, "// scope contains 'with'\n");
   if (scope_calls_eval_) Indent(n1, "// scope calls 'eval'\n");
-  if (outer_scope_calls_eval_) Indent(n1, "// outer scope calls 'eval'\n");
   if (outer_scope_calls_non_strict_eval_) {
     Indent(n1, "// outer scope calls 'eval' in non-strict context\n");
   }
   if (inner_scope_calls_eval_) Indent(n1, "// inner scope calls 'eval'\n");
-  if (outer_scope_is_eval_scope_) {
-    Indent(n1, "// outer scope is 'eval' scope\n");
-  }
   if (num_stack_slots_ > 0) { Indent(n1, "// ");
   PrintF("%d stack slots\n", num_stack_slots_); }
   if (num_heap_slots_ > 0) { Indent(n1, "// ");
@@ -811,75 +800,60 @@ Variable* Scope::NonLocal(Handle<String> name, Variable::Mode mode) {
 }
 
 
-// Lookup a variable starting with this scope. The result is either
-// the statically resolved variable belonging to an outer scope, or
-// NULL. It may be NULL because a) we couldn't find a variable, or b)
-// because the variable is just a guess (and may be shadowed by
-// another variable that is introduced dynamically via an 'eval' call
-// or a 'with' statement).
-Variable* Scope::LookupRecursive(Handle<String> name,
-                                 bool from_inner_scope,
-                                 Variable** invalidated_local) {
-  // If we find a variable, but the current scope calls 'eval', the found
-  // variable may not be the correct one (the 'eval' may introduce a
-  // property with the same name). In that case, remember that the variable
-  // found is just a guess.
-  bool guess = scope_calls_eval_;
-
+Scope::LookupResult Scope::LookupRecursive(Handle<String> name,
+                                           Handle<Context> context,
+                                           Variable** var) {
   // Try to find the variable in this scope.
-  Variable* var = LocalLookup(name);
-
-  if (var != NULL) {
-    // We found a variable. If this is not an inner lookup, we are done.
-    // (Even if there is an 'eval' in this scope which introduces the
-    // same variable again, the resulting variable remains the same.
-    // Note that enclosing 'with' statements are handled at the call site.)
-    if (!from_inner_scope)
-      return var;
+  *var = LocalLookup(name);

[Kevin Millikin (Chromium), 2011/09/16 12:58:19]

For some reason I prefer returning the variable and having the lookup kind be
the output parameter.  Just saying.

[Steven, 2011/09/16 18:29:52]

Yeah, after all me too.
On 2011/09/16 12:58:19, Kevin Millikin wrote:

 
-  } else {
-    // We did not find a variable locally. Check against the function variable,
-    // if any. We can do this for all scopes, since the function variable is
-    // only present - if at all - for function scopes.
-    //
-    // This lookup corresponds to a lookup in the "intermediate" scope sitting
-    // 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.)
-    if (function_ != NULL && function_->name().is_identical_to(name)) {
-      var = function_->var();
-
-    } else if (outer_scope_ != NULL) {
-      var = outer_scope_->LookupRecursive(name, true, invalidated_local);
-      // We may have found a variable in an outer scope. However, if
-      // the current scope is inside a 'with', the actual variable may
-      // be a property introduced via the 'with' statement. Then, the
-      // variable we may have found is just a guess.
-      if (scope_inside_with_)
-        guess = true;
-    }
+  // We found a variable and we are done. (Even if there is an 'eval' in
+  // this scope which introduces the same variable again, the resulting
+  // variable remains the same.)
+  if (*var != NULL) return BOUND;
 
-    // If we did not find a variable, we are done.
-    if (var == NULL)
-      return NULL;
-  }
-
-  ASSERT(var != NULL);
-
-  // If this is a lookup from an inner scope, mark the variable.
-  if (from_inner_scope) {
-    var->MarkAsAccessedFromInnerScope();
+  // We did not find a variable locally. Check against the function variable,
+  // if any. We can do this for all scopes, since the function variable is
+  // only present - if at all - for function scopes.
+  //
+  // This lookup corresponds to a lookup in the "intermediate" scope sitting
+  // 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.)
+  LookupResult result = UNBOUND;
+  if (function_ != NULL && function_->name().is_identical_to(name)) {

[Kevin Millikin (Chromium), 2011/09/16 12:58:19]

I wonder if we could make Scope::LocalLookup check the function name after
checking the variable map, and get rid of this here.

It seems like it Scope::LocalLookup already does that for scopes backed by a
ScopeInfo.  That seems fishy, even if it's not an actual bug.

[Steven, 2011/09/16 18:29:52]

Unfortunately we can't. The function name is still in an 'implicit' outer scope
and might be shadowed by eval introduced variables, i.e. something like
function y( eval("var y = 1;"); return y; )
Thus the lookup for the y reference should produce BOUND_EVAL_SHADOWED. But if
we let LocalLookup do this check it will be BOUND.

Yes it is fishy that the function name of the SerializedScopeInfo is checked.
Removing it does not break any tests, but I left it in there for another cleanup
CL. It is for example used in another fishy function Scope::Lookup which in turn
is used in the parser to detect direct 'eval' calls.
On 2011/09/16 12:58:19, Kevin Millikin wrote:

+    *var = function_->var();
+    result = BOUND;
+  } else if (outer_scope_ != NULL) {
+    result = outer_scope_->LookupRecursive(name, context, var);
+    if (result == BOUND) (*var)->MarkAsAccessedFromInnerScope();
   }
 
-  // If the variable we have found is just a guess, invalidate the
-  // result. If the found variable is local, record that fact so we
-  // can generate fast code to get it if it is not shadowed by eval.
-  if (guess) {
-    if (!var->is_global()) *invalidated_local = var;
-    var = NULL;
+  if (is_with_scope()) {
+    // The current scope is a with scope, so the variable binding can not be
+    // statically resolved. However, note that it was necessary to do a lookup
+    // in the outer scope anyway, because if a binding exists in an outer scope,
+    // the associated variable has to be marked as potentially being accessed
+    // from inside of an inner with scope (the property may not be in the 'with'
+    // object).
+    return DYNAMIC_LOOKUP;

[Kevin Millikin (Chromium), 2011/09/16 12:58:19]

And I'd prefer setting var to NULL (or returning NULL if you take the suggestion
above) in the cases where it's meaningless/should not be used/will not be used.

[Steven, 2011/09/16 18:29:52]

Done.

+  } else if (is_eval_scope()) {
+    // No local binding was found, no 'with' statements have been encountered
+    // and the code is executed as part of a call to 'eval'. The calling context
+    // contains scope information that we can use to determine if the variable
+    // is global, i.e. the calling context chain does not contain a binding and
+    // no 'with' contexts.
+    ASSERT(result == UNBOUND);
+    return context->GlobalIfNotShadowedByEval(name)
+        ? UNBOUND_EVAL_SHADOWED : DYNAMIC_LOOKUP;
+  } else if (calls_non_strict_eval()) {
+    // A variable binding may have been found in an outer scope, but the current
+    // scope makes a non-strict 'eval' call, so the found variable may not be
+    // the correct one (the 'eval' may introduce a binding with the same name).
+    // In that case, change the lookup result to reflect this situation.
+    if (result == BOUND) return BOUND_EVAL_SHADOWED;
+    if (result == UNBOUND) return UNBOUND_EVAL_SHADOWED;
   }
-
-  return var;
+  return result;
 }
 
 
@@ -893,71 +867,43 @@ void Scope::ResolveVariable(Scope* global_scope,
   if (proxy->var() != NULL) return;
 
   // Otherwise, try to resolve the variable.
-  Variable* invalidated_local = NULL;
-  Variable* var = LookupRecursive(proxy->name(), false, &invalidated_local);
-
-  if (proxy->inside_with()) {
-    // If we are inside a local 'with' statement, all bets are off
-    // and we cannot resolve the proxy to a local variable even if
-    // we found an outer matching variable.
-    // Note that we must do a lookup anyway, because if we find one,
-    // we must mark that variable as potentially accessed from this
-    // inner scope (the property may not be in the 'with' object).
-    var = NonLocal(proxy->name(), Variable::DYNAMIC);
-
-  } else {
-    // We are not inside a local 'with' statement.
-
-    if (var == NULL) {
-      // We did not find the variable. We have a global variable
-      // if we are in the global scope (we know already that we
-      // are outside a 'with' statement) or if there is no way
-      // that the variable might be introduced dynamically (through
-      // a local or outer eval() call, or an outer 'with' statement),
-      // or we don't know about the outer scope (because we are
-      // in an eval scope).
-      if (is_global_scope() ||
-          !(scope_inside_with_ || outer_scope_is_eval_scope_ ||
-            scope_calls_eval_ || outer_scope_calls_eval_)) {
-        // We must have a global variable.
-        ASSERT(global_scope != NULL);
-        var = global_scope->DeclareGlobal(proxy->name());
-
-      } else if (scope_inside_with_) {
-        // If we are inside a with statement we give up and look up
-        // the variable at runtime.
-        var = NonLocal(proxy->name(), Variable::DYNAMIC);
-
-      } else if (invalidated_local != NULL) {
-        // No with statements are involved and we found a local
-        // variable that might be shadowed by eval introduced
-        // variables.
-        var = NonLocal(proxy->name(), Variable::DYNAMIC_LOCAL);
-        var->set_local_if_not_shadowed(invalidated_local);
-
-      } else if (outer_scope_is_eval_scope_) {
-        // No with statements and we did not find a local and the code
-        // is executed with a call to eval.  The context contains
-        // scope information that we can use to determine if the
-        // variable is global if it is not shadowed by eval-introduced
-        // variables.
-        if (context->GlobalIfNotShadowedByEval(proxy->name())) {
-          var = NonLocal(proxy->name(), Variable::DYNAMIC_GLOBAL);
-
-        } else {
-          var = NonLocal(proxy->name(), Variable::DYNAMIC);
-        }
+  Variable* var = NULL;
+  switch (LookupRecursive(proxy->name(), context, &var)) {
+    case BOUND:
+      // We found a variable binding.
+      break;
 
-      } else {
-        // No with statements and we did not find a local and the code
-        // is not executed with a call to eval.  We know that this
-        // variable is global unless it is shadowed by eval-introduced
-        // variables.
+    case BOUND_EVAL_SHADOWED:
+      // We found a variable variable binding that might be shadowed
+      // by 'eval' introduced variable bindings.
+      if (var->is_global()) {
         var = NonLocal(proxy->name(), Variable::DYNAMIC_GLOBAL);
+      } else {
+        Variable* invalidated = var;
+        var = NonLocal(proxy->name(), Variable::DYNAMIC_LOCAL);
+        var->set_local_if_not_shadowed(invalidated);
       }
-    }
+      break;
+
+    case UNBOUND:
+      // No binding has been found. Declare a variable in global scope.
+      ASSERT(global_scope != NULL);
+      var = global_scope->DeclareGlobal(proxy->name());
+      break;
+
+    case UNBOUND_EVAL_SHADOWED:
+      // No binding has been found. But some scope makes a
+      // non-strict 'eval' call.
+      var = NonLocal(proxy->name(), Variable::DYNAMIC_GLOBAL);
+      break;
+
+    case DYNAMIC_LOOKUP:
+      // The variable could not be resolved statically.
+      var = NonLocal(proxy->name(), Variable::DYNAMIC);
+      break;
   }
 
+  ASSERT(var != NULL);
   proxy->BindTo(var);
 }
 
@@ -978,31 +924,17 @@ void Scope::ResolveVariablesRecursively(Scope* global_scope,
 }
 
 
-bool Scope::PropagateScopeInfo(bool outer_scope_calls_eval,
-                               bool outer_scope_calls_non_strict_eval,
-                               bool outer_scope_is_eval_scope) {
-  if (outer_scope_calls_eval) {
-    outer_scope_calls_eval_ = true;
-  }
-
+bool Scope::PropagateScopeInfo(bool outer_scope_calls_non_strict_eval ) {
   if (outer_scope_calls_non_strict_eval) {
     outer_scope_calls_non_strict_eval_ = true;
   }
 
-  if (outer_scope_is_eval_scope) {
-    outer_scope_is_eval_scope_ = true;
-  }
-
-  bool calls_eval = scope_calls_eval_ || outer_scope_calls_eval_;
-  bool is_eval = is_eval_scope() || outer_scope_is_eval_scope_;
   bool calls_non_strict_eval =
       (scope_calls_eval_ && !is_strict_mode()) ||
       outer_scope_calls_non_strict_eval_;
   for (int i = 0; i < inner_scopes_.length(); i++) {
     Scope* inner_scope = inner_scopes_[i];
-    if (inner_scope->PropagateScopeInfo(calls_eval,
-                                        calls_non_strict_eval,
-                                        is_eval)) {
+    if (inner_scope->PropagateScopeInfo(calls_non_strict_eval)) {
       inner_scope_calls_eval_ = true;
     }
     if (inner_scope->force_eager_compilation_) {