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

src/scopes.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/v8.h"
 
 #include "src/accessors.h"
 #include "src/bootstrapper.h"
 #include "src/messages.h"
 #include "src/parser.h"
@@ skipping 14 matching lines @@
 
 VariableMap::VariableMap(Zone* zone)
     : ZoneHashMap(ZoneHashMap::PointersMatch, 8, ZoneAllocationPolicy(zone)),
       zone_(zone) {}
 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.
   Entry* p =
       ZoneHashMap::LookupOrInsert(const_cast<AstRawString*>(name), name->hash(),
                                   ZoneAllocationPolicy(zone()));
   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);
 }
 
 
 Variable* VariableMap::Lookup(const AstRawString* name) {
   Entry* p = ZoneHashMap::Lookup(const_cast<AstRawString*>(name), name->hash());
   if (p != NULL) {
     DCHECK(reinterpret_cast<const AstRawString*>(p->key) == name);
     DCHECK(p->value != NULL);
@@ skipping 12 matching lines @@
       variables_(zone),
       internals_(4, zone),
       temps_(4, zone),
       params_(4, zone),
       unresolved_(16, zone),
       decls_(4, zone),
       module_descriptor_(
           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.
   DCHECK(scope_type == SCRIPT_SCOPE || outer_scope != NULL);
   DCHECK(!HasIllegalRedeclaration());
 }
 
 
 Scope::Scope(Zone* zone, Scope* inner_scope, ScopeType scope_type,
              Handle<ScopeInfo> scope_info, AstValueFactory* value_factory)
     : inner_scopes_(4, zone),
       variables_(zone),
       internals_(4, zone),
       temps_(4, zone),
       params_(4, zone),
       unresolved_(16, zone),
       decls_(4, zone),
       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();
   }
   // Ensure at least MIN_CONTEXT_SLOTS to indicate a materialized context.
   num_heap_slots_ = Max(num_heap_slots_,
                         static_cast<int>(Context::MIN_CONTEXT_SLOTS));
   AddInnerScope(inner_scope);
 }
 
 
 Scope::Scope(Zone* zone, Scope* inner_scope,
              const AstRawString* catch_variable_name,
              AstValueFactory* value_factory)
     : inner_scopes_(1, zone),
       variables_(zone),
       internals_(0, zone),
       temps_(0, zone),
       params_(0, zone),
       unresolved_(0, zone),
       decls_(0, zone),
       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_;
   num_heap_slots_ = Context::MIN_CONTEXT_SLOTS;
   Variable* variable = variables_.Declare(this,
                                           catch_variable_name,
                                           VAR,
                                           Variable::NORMAL,
                                           kCreatedInitialized);
   AllocateHeapSlot(variable);
@@ skipping 326 matching lines @@
     rest_parameter_ = var;
     rest_index_ = num_parameters();
   }
   params_.Add(var, zone());
   return var;
 }
 
 
 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
   // explicitly, and TEMPORARY variables are allocated via NewTemporary().
   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);
 }
 
 
 Variable* Scope::DeclareDynamicGlobal(const AstRawString* name) {
   DCHECK(is_script_scope());
   return variables_.Declare(this,
                             name,
                             DYNAMIC_GLOBAL,
                             Variable::NORMAL,
                             kCreatedInitialized);
@@ skipping 641 matching lines @@
 
   // Allow referencing the class name from methods of that class, even though
   // the initializer position for class names is only after the body.
   Scope* scope = this;
   while (scope) {
     if (scope->ClassVariableForMethod() == var) return true;
     scope = scope->outer_scope();
   }
 
   // Allow references from methods to classes declared later, if we detect no
-  // problematic dependency cycles.
+  // 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.
 
-  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:
 
-    // 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
 
-    // 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.
 
-    // 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
   // indirectly), or one of them is, we need to check whether they are inside
   // the same eval scope or different ones.
 
   // TODO(marja,rossberg): Detect errors across different evals (depends on the
   // future of eval in strong mode).
   const Scope* eval_for_use = NearestOuterEvalScope();
   const Scope* eval_for_declaration = var->scope()->NearestOuterEvalScope();
 
   if (proxy->position() != RelocInfo::kNoPosition &&
       proxy->position() < var->initializer_position() && !var->is_function() &&
       eval_for_use == eval_for_declaration) {
     DCHECK(proxy->end_position() != RelocInfo::kNoPosition);
     ReportMessage(proxy->position(), proxy->end_position(),
                   "strong_use_before_declaration", proxy->raw_name());
     return false;
   }
   return true;
 }
 
 
 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_) &&
       !IsAccessorFunction(function_kind_)) {
     return nullptr;
   }
   DCHECK_NOT_NULL(outer_scope_);
   DCHECK(outer_scope_->is_class_scope());
   // The class scope contains at most one variable, the class name.
   DCHECK(outer_scope_->variables_.occupancy() <= 1);
@@ skipping 291 matching lines @@
       (function_ != NULL && function_->proxy()->var()->IsStackLocal() ? 1 : 0);
 }
 
 
 int Scope::ContextLocalCount() const {
   if (num_heap_slots() == 0) return 0;
   return num_heap_slots() - Context::MIN_CONTEXT_SLOTS -
       (function_ != NULL && function_->proxy()->var()->IsContextSlot() ? 1 : 0);
 }
 } }  // namespace v8::internal