[strong] Stricter check for referring to other classes inside methods.

src/scopes.cc

Index: src/scopes.cc
diff --git a/src/scopes.cc b/src/scopes.cc
index 364f2f1cf7ce88ca08e207172c1cba1613dbb3f7..59d2aee30b62a095f7ff70a2d3594ef981c60af1 100644
--- a/src/scopes.cc
+++ b/src/scopes.cc
@@ -32,7 +32,8 @@ VariableMap::~VariableMap() {}
 Variable* VariableMap::Declare(Scope* scope, const AstRawString* name,
                                VariableMode mode, Variable::Kind kind,
                                InitializationFlag initialization_flag,
-                               MaybeAssignedFlag maybe_assigned_flag) {
+                               MaybeAssignedFlag maybe_assigned_flag,
+                               int consecutive_declaration_group_start) {
   // AstRawStrings are unambiguous, i.e., the same string is always represented
   // by the same AstRawString*.
   // FIXME(marja): fix the type of Lookup.
@@ -42,8 +43,9 @@ Variable* VariableMap::Declare(Scope* scope, const AstRawString* name,
   if (p->value == NULL) {
     // The variable has not been declared yet -> insert it.
     DCHECK(p->key == name);
-    p->value = new (zone()) Variable(scope, name, mode, kind,
-                                     initialization_flag, maybe_assigned_flag);
+    p->value = new (zone())
+        Variable(scope, name, mode, kind, initialization_flag,
+                 maybe_assigned_flag, consecutive_declaration_group_start);
   }
   return reinterpret_cast<Variable*>(p->value);
 }
@@ -76,7 +78,8 @@ Scope::Scope(Zone* zone, Scope* outer_scope, ScopeType scope_type,
           scope_type == MODULE_SCOPE ? ModuleDescriptor::New(zone) : NULL),
       already_resolved_(false),
       ast_value_factory_(ast_value_factory),
-      zone_(zone) {
+      zone_(zone),
+      consecutive_class_declaration_group_start_(-1) {
   SetDefaults(scope_type, outer_scope, Handle<ScopeInfo>::null(),
               function_kind);
   // The outermost scope must be a script scope.
@@ -97,7 +100,8 @@ Scope::Scope(Zone* zone, Scope* inner_scope, ScopeType scope_type,
       module_descriptor_(NULL),
       already_resolved_(true),
       ast_value_factory_(value_factory),
-      zone_(zone) {
+      zone_(zone),
+      consecutive_class_declaration_group_start_(-1) {
   SetDefaults(scope_type, NULL, scope_info);
   if (!scope_info.is_null()) {
     num_heap_slots_ = scope_info_->ContextLength();
@@ -122,7 +126,8 @@ Scope::Scope(Zone* zone, Scope* inner_scope,
       module_descriptor_(NULL),
       already_resolved_(true),
       ast_value_factory_(value_factory),
-      zone_(zone) {
+      zone_(zone),
+      consecutive_class_declaration_group_start_(-1) {
   SetDefaults(CATCH_SCOPE, NULL, Handle<ScopeInfo>::null());
   AddInnerScope(inner_scope);
   ++num_var_or_const_;
@@ -469,7 +474,8 @@ Variable* Scope::DeclareParameter(const AstRawString* name, VariableMode mode,
 
 Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode,
                               InitializationFlag init_flag, Variable::Kind kind,
-                              MaybeAssignedFlag maybe_assigned_flag) {
+                              MaybeAssignedFlag maybe_assigned_flag,
+                              int consecutive_declaration_group_start) {
   DCHECK(!already_resolved());
   // This function handles VAR, LET, and CONST modes.  DYNAMIC variables are
   // introduces during variable allocation, INTERNAL variables are allocated
@@ -477,7 +483,8 @@ Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode,
   DCHECK(IsDeclaredVariableMode(mode));
   ++num_var_or_const_;
   return variables_.Declare(this, name, mode, kind, init_flag,
-                            maybe_assigned_flag);
+                            maybe_assigned_flag,
+                            consecutive_declaration_group_start);
 }
 
 
@@ -1139,27 +1146,47 @@ bool Scope::CheckStrongModeDeclaration(VariableProxy* proxy, Variable* var) {
   }
 
   // Allow references from methods to classes declared later, if we detect no
-  // problematic dependency cycles.
-
-  if (ClassVariableForMethod() && var->is_class()) {
-    // A method is referring to some other class, possibly declared
-    // later. Referring to a class declared earlier is always OK and covered by
-    // the code outside this if. Here we only need to allow special cases for
-    // referring to a class which is declared later.
-
-    // Referring to a class C declared later is OK under the following
-    // circumstances:
-
-    // 1. The class declarations are in a consecutive group with no other
-    // declarations or statements in between, and
-
-    // 2. There is no dependency cycle where the first edge is an initialization
-    // time dependency (computed property name or extends clause) from C to
-    // something that depends on this class directly or transitively.
+  // problematic dependency cycles. Note that we can be inside multiple methods
+  // at the same time, and it's enough if we find one where the reference is
+  // allowed.
+  scope = this;
+  while (scope) {

[rossberg, 2015/04/21 13:30:02]

A couple of observations:
- I shouldn't need to loop further than var->scope().
- You only need to look for the class variable if you encounter a class scope.
- You don't need to check the group start at each level, only once you've
reached the right scope.
- I think you can skip this whole part if var is not part of a declaration
group.

What we want to find is an outer_class_var that is bound in var->scope(), and
that corresponds to a class_var in an inner class scope surrounding us. So I
think something like this should work:

if (var->class_declaration_group_start() != -1) {
  Variable* class_var = nullptr;
  for (Scope* scope = this; scope != var->scope(); scope = scope->outer_scope())
{
    if (scope->is_class_scope()) {
      if (Variable* inner_class_var = scope->ClassVariable()) {
        class_var = inner_var->corresponding_outer_class_variable();
      }
    }
  }

  if (var->class_declaration_group_start() ==
class_var->class_declaration_group_start()) {
    return true;
  }
}

where Scope::ClassVariable can be factored out of the current
GetClassVariableForMethod.

[marja, 2015/04/23 09:52:37]

I rewrote the code so that it's closer to what you suggested, but not quite
(some details, like, 1) we're not necessarily in a method even if scope is a
class scope; we want this check to only apply to variables inside methods, 2)
there might not be an outer class var).

+    Variable* class_var = scope->ClassVariableForMethod();
+    if (class_var && var->is_class()) {
+      // A method is referring to some other class, possibly declared
+      // later. Referring to a class declared earlier is always OK and covered
+      // by the code outside this if. Here we only need to allow special cases
+      // for referring to a class which is declared later.
+
+      // Referring to a class C declared later is OK under the following
+      // circumstances:
+
+      // 1. The class declarations are in a consecutive group with no other
+      // declarations or statements in between, and
+
+      // 2. There is no dependency cycle where the first edge is an
+      // initialization time dependency (computed property name or extends
+      // clause) from C to something that depends on this class directly or
+      // transitively.
+
+      // This is needed because a class ("class Name { }") creates two bindings
+      // (one in the outer scope, and one in the class scope). The method is a
+      // function scope inside the inner scope (class scope). The consecutive
+      // class declarations are in the outer scope.
+      if (class_var->corresponding_outer_class_variable()) {
+        class_var = class_var->corresponding_outer_class_variable();
+      }
 
-    // TODO(marja,rossberg): implement these checks. Here we undershoot the
-    // target and allow referring to any class.
-    return true;
+      // TODO(marja,rossberg): implement the dependency cycle detection. Here we
+      // undershoot the target and allow referring to any class in the same
+      // consectuive declaration group.
+      if (class_var->consecutive_declaration_group_start() ==
+              var->consecutive_declaration_group_start() &&
+          class_var->consecutive_declaration_group_start() >= 0) {
+        return true;
+      }
+    }
+    scope = scope->outer_scope();
   }
 
   // If both the use and the declaration are inside an eval scope (possibly
@@ -1184,6 +1211,10 @@ bool Scope::CheckStrongModeDeclaration(VariableProxy* proxy, Variable* var) {
 
 
 Variable* Scope::ClassVariableForMethod() const {
+  // TODO(marja, rossberg): This fails to find a class variable in the following
+  // cases:
+  // let A = class { ... }
+  // It needs to be investigated whether this causes any practical problems.
   if (!is_function_scope()) return nullptr;
   if (IsInObjectLiteral(function_kind_)) return nullptr;
   if (!IsConciseMethod(function_kind_) && !IsConstructor(function_kind_) &&