Implement proper scoping for "this" in arrow functions

src/scopes.cc

Index: src/scopes.cc
diff --git a/src/scopes.cc b/src/scopes.cc
index 7687a4cbb6dc69fe12dcf5e6285a4321100d5052..3c36dc3c0c54181ae58b6cde3bb49068ab2a66a2 100644
--- a/src/scopes.cc
+++ b/src/scopes.cc
@@ -163,7 +163,6 @@ void Scope::SetDefaults(ScopeType scope_type,
   scope_uses_arguments_ = false;
   scope_uses_super_property_ = false;
   scope_uses_super_constructor_call_ = false;
-  scope_uses_this_ = false;
   asm_module_ = false;
   asm_function_ = outer_scope != NULL && outer_scope->asm_module_;
   // Inherit the strict mode from the parent scope.
@@ -171,7 +170,6 @@ void Scope::SetDefaults(ScopeType scope_type,
   outer_scope_calls_sloppy_eval_ = false;
   inner_scope_calls_eval_ = false;
   inner_scope_uses_arguments_ = false;
-  inner_scope_uses_this_ = false;
   inner_scope_uses_super_property_ = false;
   inner_scope_uses_super_constructor_call_ = false;
   force_eager_compilation_ = false;
@@ -310,7 +308,7 @@ void Scope::Initialize() {
   // 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_declaration_scope()) {
+  if (has_this_declaration()) {
     Variable* var =
         variables_.Declare(this,
                            ast_value_factory_->this_string(),
@@ -318,11 +316,7 @@ void Scope::Initialize() {
                            false,
                            Variable::THIS,
                            kCreatedInitialized);
-    var->AllocateTo(Variable::PARAMETER, -1);
     receiver_ = var;
-  } else {
-    DCHECK(outer_scope() != NULL);
-    receiver_ = outer_scope()->receiver();
   }
 
   if (is_function_scope()) {
@@ -370,7 +364,6 @@ Scope* Scope::FinalizeBlockScope() {
   if (uses_super_property()) outer_scope_->RecordSuperPropertyUsage();
   if (uses_super_constructor_call())
     outer_scope_->RecordSuperConstructorCallUsage();
-  if (uses_this()) outer_scope_->RecordThisUsage();
 
   return NULL;
 }
@@ -894,7 +887,6 @@ void Scope::Print(int n) {
     Indent(n1, "// scope uses 'super' property\n");
   if (scope_uses_super_constructor_call_)
     Indent(n1, "// scope uses 'super' constructor\n");
-  if (scope_uses_this_) Indent(n1, "// scope uses 'this'\n");
   if (inner_scope_uses_arguments_) {
     Indent(n1, "// inner scope uses 'arguments'\n");
   }
@@ -903,7 +895,6 @@ void Scope::Print(int n) {
   if (inner_scope_uses_super_constructor_call_) {
     Indent(n1, "// inner scope uses 'super' constructor\n");
   }
-  if (inner_scope_uses_this_) Indent(n1, "// inner scope uses 'this'\n");
   if (outer_scope_calls_sloppy_eval_) {
     Indent(n1, "// outer scope calls 'eval' in sloppy context\n");
   }
@@ -1183,10 +1174,8 @@ void Scope::PropagateScopeInfo(bool outer_scope_calls_sloppy_eval ) {
           inner->inner_scope_uses_super_constructor_call_) {
         inner_scope_uses_super_constructor_call_ = true;
       }
-      if (inner->scope_uses_this_ || inner->inner_scope_uses_this_) {
-        inner_scope_uses_this_ = true;
-      }
     }
+

[wingo, 2015/02/04 10:02:13]

extra whitespace

[aperez, 2015/02/04 21:00:42]

Acknowledged.

     if (inner->force_eager_compilation_) {
       force_eager_compilation_ = true;
     }
@@ -1213,8 +1202,9 @@ bool Scope::MustAllocate(Variable* var) {
     var->set_is_used();
     if (scope_calls_eval_ || inner_scope_calls_eval_) var->set_maybe_assigned();
   }
-  // Global variables do not need to be allocated.
-  return !var->IsGlobalObjectProperty() && var->is_used();
+  // Global variables do not need to be allocated, except the receiver
+  // which is not preallocated.

[wingo, 2015/02/04 10:02:13]

Confusing comment.  Also  I think the conditional below should be reassociated:
if var->is_this(), then certainly var->is_used() (per the if block above).

i.e. return var->is_this() || (var->is_used() &&
!var->IsGlobalObjectProperty());

[aperez, 2015/02/04 21:00:42]

Acknowledged.

+  return (var->is_this() || !var->IsGlobalObjectProperty()) && var->is_used();
 }
 
 
@@ -1299,18 +1289,34 @@ void Scope::AllocateParameterLocals() {
       // Force context allocation of the parameter.
       var->ForceContextAllocation();
     }
+    AllocateParameter(var, i);
+  }
+}
 
-    if (MustAllocate(var)) {
-      if (MustAllocateInContext(var)) {
-        DCHECK(var->IsUnallocated() || var->IsContextSlot());
-        if (var->IsUnallocated()) {
-          AllocateHeapSlot(var);
-        }
-      } else {
-        DCHECK(var->IsUnallocated() || var->IsParameter());
-        if (var->IsUnallocated()) {
-          var->AllocateTo(Variable::PARAMETER, i);
-        }
+
+void Scope::AllocateReceiver() {
+  DCHECK(receiver());
+  DCHECK(receiver()->scope() == this);

[wingo, 2015/02/04 10:02:13]

the second DCHECK is sufficient, I think, if we add the non-null DCHECK to
receiver() itself

[aperez, 2015/02/04 21:00:42]

Acknowledged.

+
+  if (has_forced_context_allocation()) {
+    // Force context allocation of the receiver.
+    receiver()->ForceContextAllocation();
+  }
+  AllocateParameter(receiver(), -1);
+}
+
+
+void Scope::AllocateParameter(Variable* var, int index) {
+  if (MustAllocate(var)) {
+    if (MustAllocateInContext(var)) {
+      DCHECK(var->IsUnallocated() || var->IsContextSlot());
+      if (var->IsUnallocated()) {
+        AllocateHeapSlot(var);
+      }
+    } else {
+      DCHECK(var->IsUnallocated() || var->IsParameter());
+      if (var->IsUnallocated()) {
+        var->AllocateTo(Variable::PARAMETER, index);
       }
     }
   }
@@ -1379,6 +1385,7 @@ void Scope::AllocateVariablesRecursively() {
 
   // Allocate variables for this scope.
   // Parameters must be allocated first, if any.
+  if (has_this_declaration()) AllocateReceiver();
   if (is_function_scope()) AllocateParameterLocals();
   AllocateNonParameterLocals();