Separate Scope into DeclarationScope and Scope

src/ast/scopeinfo.cc

Index: src/ast/scopeinfo.cc
diff --git a/src/ast/scopeinfo.cc b/src/ast/scopeinfo.cc
index 20d343f17aa74239f08d53004646b812e8aced68..107f628a76d7dbca1bc2d465302c61576b41eb23 100644
--- a/src/ast/scopeinfo.cc
+++ b/src/ast/scopeinfo.cc
@@ -32,8 +32,9 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
 
   // Determine use and location of the "this" binding if it is present.
   VariableAllocationInfo receiver_info;
-  if (scope->has_this_declaration()) {
-    Variable* var = scope->receiver();
+  if (scope->is_declaration_scope() &&

[adamk, 2016/08/04 00:18:50]

Code like this reads strangely. "this" declarations don't have much to do with
"declaration scopes" conceptually, so it's weird that the caller has to know
things about the scope hierarchy before asking a question like "is there a
receiver" in this scope?

Did you consider doing something more like what you did for AstNodes and
teaching the base class more about its subclass?

[Toon Verwaest, 2016/08/04 12:01:17]

The reasoning there is quite different than here though. It's the base visitor
that knows about the actual visitor, and there's only one visitor that's doing
the work. The nodes are "virtual" and they didn't change at all. Here I'm
introducing distinction between Scopes just like ast nodes are distinct and need
to be handled as such by visitors.

[adamk, 2016/08/04 16:31:28]

What I'm saying is that "has_this_declaration()" is a question that doesn't
apply any more to varblock scopes than it does to block scopes, yet it works
just fine when called on varblocks but doesn't even exist (to return false) on
blocks.

[Toon Verwaest, 2016/08/05 08:41:08]

Ok, I do agree to that. But at least this is a step forward towards only having
it available where relevant :)

+      scope->AsDeclarationScope()->has_this_declaration()) {
+    Variable* var = scope->AsDeclarationScope()->receiver();
     if (!var->is_used()) {
       receiver_info = UNUSED;
     } else if (var->IsContextSlot()) {
@@ -46,13 +47,16 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
     receiver_info = NONE;
   }
 
-  bool has_new_target = scope->new_target_var() != nullptr;
+  bool has_new_target =
+      scope->is_declaration_scope() &&

[adamk, 2016/08/04 00:18:51]

Same here. I suspect I'd make this comment in a lot of places. Would be
interested to hear your thoughts. I like the memory savings, but making the
calling code strange is something I'd like to avoid.

[Toon Verwaest, 2016/08/04 12:01:17]

I agree that we shouldn't make calling code strange for unrelated implementation
details. However, I think it's important for callers to know what they are
doing. We shouldn't have tons of helper methods that figure out what they are
doing automagically, since it's confuses code. Not having a receiver() at all on
Scope makes it clear that it doesn't have a receiver, and that invariant is
clear from the design. I'd prefer coming back to this code later and more
clearly separate Scope from DeclarationScope handling.

[adamk, 2016/08/04 16:31:28]

Again, receiver()-having doesn't make any more sense for DeclarationScopes than
it does for Scopes at large.

+      scope->AsDeclarationScope()->new_target_var() != nullptr;
 
   // Determine use and location of the function variable if it is present.
   VariableAllocationInfo function_name_info;
   VariableMode function_variable_mode;
-  if (scope->is_function_scope() && scope->function() != NULL) {
-    Variable* var = scope->function()->proxy()->var();
+  if (scope->is_function_scope() &&
+      scope->AsDeclarationScope()->function() != nullptr) {
+    Variable* var = scope->AsDeclarationScope()->function()->proxy()->var();
     if (!var->is_used()) {
       function_name_info = UNUSED;
     } else if (var->IsContextSlot()) {
@@ -80,7 +84,12 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
   Handle<ScopeInfo> scope_info = factory->NewScopeInfo(length);
 
   bool has_simple_parameters =
-      scope->is_function_scope() && scope->has_simple_parameters();
+      scope->is_function_scope() &&
+      scope->AsDeclarationScope()->has_simple_parameters();
+  FunctionKind function_kind =
+      scope->is_declaration_scope()
+          ? scope->AsDeclarationScope()->function_kind()
+          : kNormalFunction;
 
   // Encode the flags.
   int flags = ScopeTypeField::encode(scope->scope_type()) |
@@ -94,7 +103,7 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
               AsmModuleField::encode(scope->asm_module()) |
               AsmFunctionField::encode(scope->asm_function()) |
               HasSimpleParametersField::encode(has_simple_parameters) |
-              FunctionKindField::encode(scope->function_kind());
+              FunctionKindField::encode(function_kind);
   scope_info->SetFlags(flags);
   scope_info->SetParameterCount(parameter_count);
   scope_info->SetStackLocalCount(stack_local_count);
@@ -104,8 +113,11 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
   int index = kVariablePartIndex;
   // Add parameters.
   DCHECK(index == scope_info->ParameterEntriesIndex());
-  for (int i = 0; i < parameter_count; ++i) {
-    scope_info->set(index++, *scope->parameter(i)->name());
+  if (scope->is_declaration_scope()) {
+    for (int i = 0; i < parameter_count; ++i) {
+      scope_info->set(index++,
+                      *scope->AsDeclarationScope()->parameter(i)->name());
+    }
   }
 
   // Add stack locals' names. We are assuming that the stack locals'
@@ -171,7 +183,7 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
   // If the receiver is allocated, add its index.
   DCHECK(index == scope_info->ReceiverEntryIndex());
   if (has_receiver) {
-    int var_index = scope->receiver()->index();
+    int var_index = scope->AsDeclarationScope()->receiver()->index();
     scope_info->set(index++, Smi::FromInt(var_index));
     // ?? DCHECK(receiver_info != CONTEXT || var_index ==
     // scope_info->ContextLength() - 1);
@@ -180,8 +192,10 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone,
   // If present, add the function variable name and its index.
   DCHECK(index == scope_info->FunctionNameEntryIndex());
   if (has_function_name) {
-    int var_index = scope->function()->proxy()->var()->index();
-    scope_info->set(index++, *scope->function()->proxy()->name());
+    int var_index =
+        scope->AsDeclarationScope()->function()->proxy()->var()->index();
+    scope_info->set(index++,
+                    *scope->AsDeclarationScope()->function()->proxy()->name());
     scope_info->set(index++, Smi::FromInt(var_index));
     DCHECK(function_name_info != CONTEXT ||
            var_index == scope_info->ContextLength() - 1);