Reland "Fix "this" value in lazily-parsed module functions."

This is a reland of c3bd741e
Original change's description:
> Fix "this" value in lazily-parsed module functions.
>
> When preparsing top-level functions in a module, we didn't track
> unresolved variables. Consequently, "this" ended up referencing
> the global "this", which has the wrong value (in a module "this"
> is supposed to be the undefined value).
>
> This patch fixes that. This also lets us stop forcing context
> allocation of all variables in module scopes, which the patch
> takes care of as well.
>
> Bug: chromium:791334
> Change-Id: Ifac1f1adc033f3facfb3d29dd4bca32ee27bffcf
> Reviewed-on: https://chromium-review.googlesource.com/808938
> Reviewed-by: Marja Hölttä <marja@chromium.org>
> Reviewed-by: Adam Klein <adamk@chromium.org>
> Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
> Commit-Queue: Georg Neis <neis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#50025}

TBR=adamk@chromium.org
TBR=kozyatinskiy@chromium.org

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel

Bug: chromium:791334
Change-Id: I57acc7b84a345565b36cbb55924fa2ff9b449eec
Reviewed-on: https://chromium-review.googlesource.com/822341
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50045}

src/ast/scopes.cc

@@ -147,8 +147,6 @@ Scope::Scope(Zone* zone, Scope* outer_scope, ScopeType scope_type)
   DCHECK_NE(SCRIPT_SCOPE, scope_type);
   SetDefaults();
   set_language_mode(outer_scope->language_mode());
-  force_context_allocation_ =
-      !is_function_scope() && outer_scope->has_forced_context_allocation();
   outer_scope_->AddInnerScope(this);
 }
 
@@ -1370,12 +1368,8 @@ bool Scope::AllowsLazyParsingWithoutUnresolvedVariables(
     if (s->is_catch_scope()) continue;
     // With scopes do not introduce variables that need allocation.
     if (s->is_with_scope()) continue;
-    // Module scopes context-allocate all variables, and have no
-    // {this} or {arguments} variables whose existence depends on
-    // references to them.
-    if (s->is_module_scope()) continue;
-    // Only block scopes and function scopes should disallow preparsing.
-    DCHECK(s->is_block_scope() || s->is_function_scope());
+    DCHECK(s->is_module_scope() || s->is_block_scope() ||
+           s->is_function_scope());
     return false;
   }
   return true;
@@ -1734,9 +1728,6 @@ void Scope::Print(int n) {
     if (scope->was_lazily_parsed()) Indent(n1, "// lazily parsed\n");
     if (scope->ShouldEagerCompile()) Indent(n1, "// will be compiled\n");
   }
-  if (has_forced_context_allocation()) {
-    Indent(n1, "// forces context allocation\n");
-  }
   if (num_stack_slots_ > 0) {
     Indent(n1, "// ");
     PrintF("%d stack slots\n", num_stack_slots_);
@@ -2111,11 +2102,8 @@ bool Scope::MustAllocateInContext(Variable* var) {
   // an eval() call or a runtime with lookup), it must be allocated in the
   // context.
   //
-  // Exceptions: If the scope as a whole has forced context allocation, all
-  // variables will have context allocation, even temporaries.  Otherwise
-  // temporary variables are always stack-allocated.  Catch-bound variables are
+  // Temporary variables are always stack-allocated.  Catch-bound variables are
   // always context-allocated.
-  if (has_forced_context_allocation()) return true;
   if (var->mode() == TEMPORARY) return false;
   if (is_catch_scope()) return true;
   if ((is_script_scope() || is_eval_scope()) &&

src/ast/scopes.h

@@ -334,14 +334,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   bool is_hidden() const { return is_hidden_; }
   void set_is_hidden() { is_hidden_ = true; }
 
-  // In some cases we want to force context allocation for a whole scope.
-  void ForceContextAllocation() {
-    DCHECK(!already_resolved_);
-    force_context_allocation_ = true;
-  }
-  bool has_forced_context_allocation() const {
-    return force_context_allocation_;
-  }
   void ForceContextAllocationForParameters() {
     DCHECK(!already_resolved_);
     force_context_allocation_for_parameters_ = true;

src/parsing/parser.cc

@@ -690,7 +690,6 @@ FunctionLiteral* Parser::DoParseProgram(ParseInfo* info) {
       var->AllocateTo(VariableLocation::PARAMETER, 0);
 
       PrepareGeneratorVariables();
-      scope->ForceContextAllocation();
       Expression* initial_yield =
           BuildInitialYield(kNoSourcePosition, kGeneratorFunction);
       body->Add(

test/cctest/test-parsing.cc

@@ -6224,9 +6224,9 @@ TEST(ModuleParsingInternals) {
 
   CHECK(declarations->AtForTest(8)->proxy()->raw_name()->IsOneByteEqualTo(
       "nonexport"));
-  CHECK(declarations->AtForTest(8)->proxy()->var()->binding_needs_init());
-  CHECK(declarations->AtForTest(8)->proxy()->var()->location() !=
-        i::VariableLocation::MODULE);
+  CHECK(!declarations->AtForTest(8)->proxy()->var()->binding_needs_init());
+  CHECK(declarations->AtForTest(8)->proxy()->var()->location() ==
+        i::VariableLocation::LOCAL);
 
   CHECK(
       declarations->AtForTest(9)->proxy()->raw_name()->IsOneByteEqualTo("mm"));

test/debugger/debug/debug-modules-set-variable-value.js

@@ -271,7 +271,7 @@ let salad = 12;
   function listener(event, exec_state) {
     if (event == Debug.DebugEvent.Break) {
       let scope_count = exec_state.frame().scopeCount();
-      let module_scope = exec_state.frame().scope(2);
+      let module_scope = exec_state.frame().scope(1);
       assertEquals(debug.ScopeType.Module, module_scope.scopeType());
       module_scope.setVariableValue('salad', 42);
     }
@@ -311,7 +311,7 @@ export let ham = 1;
   function listener(event, exec_state) {
     if (event == Debug.DebugEvent.Break) {
       let scope_count = exec_state.frame().scopeCount();
-      let module_scope = exec_state.frame().scope(2);
+      let module_scope = exec_state.frame().scope(1);
       assertEquals(debug.ScopeType.Module, module_scope.scopeType());
       module_scope.setVariableValue('ham', 2);
     }

test/debugger/debug/harmony/modules-debug-scopes2.js

@@ -139,10 +139,10 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent(
-      {local_var: undefined, exported_var: undefined, imported_var: undefined},
+      {exported_var: undefined, imported_var: undefined},
       0, exec_state);
   CheckScopeDoesNotHave(
-      ["doesnotexist", "local_let", "exported_let", "imported_let"],
+      ["local_var", "doesntexist", "local_let", "exported_let", "imported_let"],
       0, exec_state);
 };
 debugger;
@@ -161,8 +161,9 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent(
-      {local_let: 1, local_var: 2, exported_let: 3, exported_var: 4,
+      {exported_let: 3, exported_var: 4,
        imported_let: 3, imported_var: 4}, 0, exec_state);
+  CheckScopeDoesNotHave(["local_var", "local_let"], 0, exec_state);
 };
 debugger;
 EndTest();
@@ -178,8 +179,9 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent(
-      {local_let: 11, local_var: 12, exported_let: 13, exported_var: 14,
+      {exported_let: 13, exported_var: 14,
        imported_let: 13, imported_var: 14}, 0, exec_state);
+  CheckScopeDoesNotHave(["local_var", "local_let"], 0, exec_state);
 };
 debugger;
 EndTest();

test/inspector/debugger/evaluate-on-call-frame-in-module-expected.txt

@@ -12,9 +12,8 @@ local:foo1
 module
 [
     [0] : a1 = 10
-    [1] : g1 = 1
-    [2] : b1 = 11
-    [3] : foo1 = function foo1() {   let c1 = 12;   let g1 = 2;   debugger;   return a1 + b1 + c1 + g1; }
+    [1] : b1 = 11
+    [2] : foo1 = function foo1() {   let c1 = 12;   let g1 = 2;   debugger;   return a1 + b1 + c1 + g1; }
 ]
 global
 [
@@ -180,9 +179,8 @@ foo2 =
 }
 module
 [
-    [0] : a3 = 30
-    [1] : foo2 = function foo2() {   let c2 = 22;   return foo1() + a2 + b2 + c2; }
-    [2] : b3 = 31
+    [0] : foo2 = function foo2() {   let c2 = 22;   return foo1() + a2 + b2 + c2; }
+    [1] : b3 = 31
 ]
 global
 [
@@ -200,20 +198,6 @@ Array =
     objectId : <objectId>
     type : function
 }
-a3 = 
-{
-    description : 30
-    type : number
-    value : 30
-}
-Evaluating: ++a3
-updated a3 = 
-{
-    description : 31
-    type : number
-    value : 31
-}
-Evaluating: --a3
 foo2 = 
 {
     className : Function
@@ -247,12 +231,6 @@ closure:bar
 [
     [0] : a = 0
 ]
-module
-[
-    [0] : a = 1
-    [1] : b = 2
-    [2] : bar = function bar() {   let a = 0;   (() => {a; debugger;})(); }
-]
 global
 [
   ...
@@ -283,37 +261,10 @@ updated a =
     value : 1
 }
 Evaluating: --a
-b = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: ++b
-updated b = 
-{
-    description : 3
-    type : number
-    value : 3
-}
-Evaluating: --b
-bar = 
-{
-    className : Function
-    description : function bar() {   let a = 0;   (() => {a; debugger;})(); }
-    objectId : <objectId>
-    type : function
-}
 local:bar
 [
     [0] : a = 0
 ]
-module
-[
-    [0] : a = 1
-    [1] : b = 2
-    [2] : bar = function bar() {   let a = 0;   (() => {a; debugger;})(); }
-]
 global
 [
   ...
@@ -344,33 +295,6 @@ updated a =
     value : 1
 }
 Evaluating: --a
-b = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: ++b
-updated b = 
-{
-    description : 3
-    type : number
-    value : 3
-}
-Evaluating: --b
-bar = 
-{
-    className : Function
-    description : function bar() {   let a = 0;   (() => {a; debugger;})(); }
-    objectId : <objectId>
-    type : function
-}
-module
-[
-    [0] : a = 1
-    [1] : b = 2
-    [2] : bar = function bar() {   let a = 0;   (() => {a; debugger;})(); }
-]
 global
 [
   ...
@@ -386,41 +310,6 @@ Array =
     objectId : <objectId>
     type : function
 }
-a = 
-{
-    description : 1
-    type : number
-    value : 1
-}
-Evaluating: ++a
-updated a = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: --a
-b = 
-{
-    description : 2
-    type : number
-    value : 2
-}
-Evaluating: ++b
-updated b = 
-{
-    description : 3
-    type : number
-    value : 3
-}
-Evaluating: --b
-bar = 
-{
-    className : Function
-    description : function bar() {   let a = 0;   (() => {a; debugger;})(); }
-    objectId : <objectId>
-    type : function
-}
 
 Running test: testDifferentModuleVariables
 (anonymous) (module5:5:0)
@@ -503,3 +392,112 @@ updated c =
     value : 1
 }
 Evaluating: --c
+
+Running test: testCapturedLocalVariable
+(anonymous) (module6:2:25)
+(anonymous) (module6:2:37)
+local
+[
+    [0] : y = 5
+]
+module
+[
+    [0] : x = 5
+]
+global
+[
+  ...
+]
+Check variables in frame#0
+let x = 5;
+(function() { let y = x; #debugger; })()
+
+
+Array = 
+{
+    className : Function
+    description : function Array() { [native code] }
+    objectId : <objectId>
+    type : function
+}
+y = 
+{
+    description : 5
+    type : number
+    value : 5
+}
+Evaluating: ++y
+updated y = 
+{
+    description : 6
+    type : number
+    value : 6
+}
+Evaluating: --y
+x = 
+{
+    description : 5
+    type : number
+    value : 5
+}
+Evaluating: ++x
+updated x = 
+{
+    description : 6
+    type : number
+    value : 6
+}
+Evaluating: --x
+module
+[
+    [0] : x = 5
+]
+global
+[
+  ...
+]
+Check variables in frame#1
+let x = 5;
+(function() { let y = x; debugger; })#()
+
+
+Array = 
+{
+    className : Function
+    description : function Array() { [native code] }
+    objectId : <objectId>
+    type : function
+}
+x = 
+{
+    description : 5
+    type : number
+    value : 5
+}
+Evaluating: ++x
+updated x = 
+{
+    description : 6
+    type : number
+    value : 6
+}
+Evaluating: --x
+
+Running test: testLocalVariableToplevel
+(anonymous) (module7:2:0)
+global
+[
+  ...
+]
+Check variables in frame#0
+let x = 5;
+#debugger;
+
+
+Array = 
+{
+    className : Function
+    description : function Array() { [native code] }
+    objectId : <objectId>
+    type : function
+}

test/inspector/debugger/evaluate-on-call-frame-in-module.js

@@ -49,6 +49,16 @@ export var c = 0;
 debugger;
 `;
 
+var module6 = `
+let x = 5;
+(function() { let y = x; debugger; })()
+`;
+
+var module7 = `
+let x = 5;
+debugger;
+`;
+
 InspectorTest.runAsyncTestSuite([
   async function testTotal() {
     session.setupScriptMap();
@@ -82,6 +92,26 @@ InspectorTest.runAsyncTestSuite([
       await checkFrame(callFrames[i], i);
     }
     await Protocol.Debugger.resume();
+  },
+
+  async function testCapturedLocalVariable() {
+    contextGroup.addModule(module6, 'module6');
+    let {params:{callFrames}} = (await Protocol.Debugger.oncePaused());
+    session.logCallFrames(callFrames);
+    for (let i = 0; i < callFrames.length; ++i) {
+      await checkFrame(callFrames[i], i);
+    }
+    await Protocol.Debugger.resume();
+  },
+
+  async function testLocalVariableToplevel() {
+    contextGroup.addModule(module7, 'module7');
+    let {params:{callFrames}} = (await Protocol.Debugger.oncePaused());
+    session.logCallFrames(callFrames);
+    for (let i = 0; i < callFrames.length; ++i) {
+      await checkFrame(callFrames[i], i);
+    }
+    await Protocol.Debugger.resume();
   }
 ]);
 

test/mjsunit/regress/regress-791334.js

+// Copyright 2017 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.
+
+// MODULE
+
+let foo = () => { return this };
+assertEquals(undefined, foo());