[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..1a53691dfb2857773c30ad5828ee2a377d4a96ff 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_batch_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_batch_start);
   }
   return reinterpret_cast<Variable*>(p->value);
 }
@@ -469,7 +471,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_batch_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 +480,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_batch_start);
 }
 
 
@@ -1140,8 +1144,8 @@ 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()) {
+  Variable* class_var = 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
@@ -1157,9 +1161,29 @@ bool Scope::CheckStrongModeDeclaration(VariableProxy* proxy, Variable* var) {
     // time dependency (computed property name or extends clause) from C to
     // something that depends on this class directly or transitively.
 
-    // TODO(marja,rossberg): implement these checks. Here we undershoot the
-    // target and allow referring to any class.
-    return true;
+    // This is needed because a class normally creates two scopes (outer and

[rossberg, 2015/04/20 11:15:25]

Hm, a class only introduces a single scope. Do you mean it creates two bindings?

[marja, 2015/04/20 15:58:22]

Yes, s/two scopes/two bindings/ was what I meant; fixed.

+    // inner); the reference is coming from the inner one, but classes in the
+    // same consectutive class declaration batch are declared in the outer

[rossberg, 2015/04/20 11:15:25]

Nit: "consectutive class declaration batch" -> "declaration group" (here and
elsewhere)

[marja, 2015/04/20 15:58:22]

Done.

+    // one. We also need to compare the scopes to distinguish between two
+    // consecutive classes from a class which has another class inside its
+    // computed property name. This is pretty complicated.
+    if (class_var->scope()->outer_scope()) {
+      Variable* maybe_outer_class_var =
+          class_var->scope()->outer_scope()->LookupLocal(class_var->raw_name());
+      if (maybe_outer_class_var) {

[rossberg, 2015/04/20 11:15:25]

This may be too liberal. I suppose you somehow need to verify that this var is
in fact a class variable, for the same class. Otherwise you might incorrectly
capture

  let A = class C { ... }
  let C = class D { ... }

Unfortunately, we don't distinguish that in the declaration. Maybe some trick
with positions would work? If not, a TODO is fine for this CL.

[marja, 2015/04/20 15:58:22]

Ahh, true. The breaking case is this:

let A = class B {
  m() { C; }
}
;;;;
class C {}
class B {}

(Should produce an error, doesn't.)

Fixed that and added a test.

+        class_var = maybe_outer_class_var;
+      }
+    }
+
+    // TODO(marja,rossberg): implement the dependency cycle detection. Here we
+    // undershoot the target and allow referring to any class in the same
+    // consectuive declaration batch.
+    if (class_var->scope() == var->scope() &&
+        class_var->consecutive_declaration_batch_start() ==
+            var->consecutive_declaration_batch_start() &&
+        class_var->consecutive_declaration_batch_start() >= 0) {
+      return true;
+    }
   }
 
   // If both the use and the declaration are inside an eval scope (possibly