Remove Scope::scope_contains_with_ bit

This part of Scope has existed since V8's initial check in, but from what
I can tell it's not required to implement "with". The only tests that
depend upon it are tests of the debugger and the Scope mirrors, but the
resulting test behavior after removing the bit still seems perfectly
reasonable to me. In fact, with the included fix for scope name collection,
the scope mirror is actually improved with this change.

As a bi-product, this fixes the attached bug, about the contains_with
bit having inconsistent values in some arrow function compilation
scenarios.

BUG=chromium:592353
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1804783002

Cr-Commit-Position: refs/heads/master@{#34802}

src/ast/scopes.cc

@@ -171,7 +171,6 @@ void Scope::SetDefaults(ScopeType scope_type, Scope* outer_scope,
   this_function_ = nullptr;
   illegal_redecl_ = nullptr;
   scope_inside_with_ = false;
-  scope_contains_with_ = false;
   scope_calls_eval_ = false;
   scope_uses_arguments_ = false;
   scope_uses_super_property_ = false;
@@ -210,7 +209,6 @@ Scope* Scope::DeserializeScopeChain(Isolate* isolate, Zone* zone,
   // Reconstruct the outer scope chain from a closure's context chain.
   Scope* current_scope = NULL;
   Scope* innermost_scope = NULL;
-  bool contains_with = false;
   while (!context->IsNativeContext()) {
     if (context->IsWithContext()) {
       Scope* with_scope = new (zone)
@@ -218,7 +216,6 @@ Scope* Scope::DeserializeScopeChain(Isolate* isolate, Zone* zone,
                 script_scope->ast_value_factory_);
       current_scope = with_scope;
       // All the inner scopes are inside a with.
-      contains_with = true;
       for (Scope* s = innermost_scope; s != NULL; s = s->outer_scope()) {
         s->scope_inside_with_ = true;
       }
@@ -252,13 +249,7 @@ Scope* Scope::DeserializeScopeChain(Isolate* isolate, Zone* zone,
           script_scope->ast_value_factory_->GetString(Handle<String>(name)),
           script_scope->ast_value_factory_);
     }
-    if (contains_with) current_scope->RecordWithStatement();
     if (innermost_scope == NULL) innermost_scope = current_scope;
-
-    // Forget about a with when we move to a context for a different function.
-    if (context->previous()->closure() != context->closure()) {
-      contains_with = false;
-    }
     context = context->previous();
   }
 
@@ -392,7 +383,6 @@ void Scope::PropagateUsageFlagsToScope(Scope* other) {
   if (uses_arguments()) other->RecordArgumentsUsage();
   if (uses_super_property()) other->RecordSuperPropertyUsage();
   if (calls_eval()) other->RecordEvalCall();
-  if (scope_contains_with_) other->RecordWithStatement();
 }
 
 
@@ -982,7 +972,6 @@ void Scope::Print(int n) {
     Indent(n1, "// strict mode scope\n");
   }
   if (scope_inside_with_) Indent(n1, "// scope inside 'with'\n");
-  if (scope_contains_with_) Indent(n1, "// scope contains 'with'\n");
   if (scope_calls_eval_) Indent(n1, "// scope calls 'eval'\n");
   if (scope_uses_arguments_) Indent(n1, "// scope uses 'arguments'\n");
   if (scope_uses_super_property_)
@@ -1254,8 +1243,8 @@ bool Scope::MustAllocate(Variable* var) {
   // visible name.
   if ((var->is_this() || !var->raw_name()->IsEmpty()) &&
       (var->has_forced_context_allocation() || scope_calls_eval_ ||
-       inner_scope_calls_eval_ || scope_contains_with_ || is_catch_scope() ||
-       is_block_scope() || is_module_scope() || is_script_scope())) {
+       inner_scope_calls_eval_ || is_catch_scope() || is_block_scope() ||
+       is_module_scope() || is_script_scope())) {
     var->set_is_used();
     if (scope_calls_eval_ || inner_scope_calls_eval_) var->set_maybe_assigned();
   }
@@ -1278,10 +1267,8 @@ bool Scope::MustAllocateInContext(Variable* var) {
   if (var->mode() == TEMPORARY) return false;
   if (is_catch_scope() || is_module_scope()) return true;
   if (is_script_scope() && IsLexicalVariableMode(var->mode())) return true;
-  return var->has_forced_context_allocation() ||
-      scope_calls_eval_ ||
-      inner_scope_calls_eval_ ||
-      scope_contains_with_;
+  return var->has_forced_context_allocation() || scope_calls_eval_ ||
+         inner_scope_calls_eval_;
 }
 
 

src/ast/scopes.h

@@ -245,9 +245,6 @@ class Scope: public ZoneObject {
   // ---------------------------------------------------------------------------
   // Scope-specific info.
 
-  // Inform the scope that the corresponding code contains a with statement.
-  void RecordWithStatement() { scope_contains_with_ = true; }
-
   // Inform the scope that the corresponding code contains an eval call.
   void RecordEvalCall() { scope_calls_eval_ = true; }
 
@@ -646,8 +643,6 @@ class Scope: public ZoneObject {
   //
   // This scope is inside a 'with' of some outer scope.
   bool scope_inside_with_;
-  // This scope contains a 'with' statement.
-  bool scope_contains_with_;
   // This scope or a nested catch scope or with scope contain an 'eval' call. At
   // the 'eval' call site this scope is the declaration scope.
   bool scope_calls_eval_;

src/debug/debug-scopes.cc

@@ -135,11 +135,6 @@ MUST_USE_RESULT MaybeHandle<JSObject> ScopeIterator::MaterializeScopeDetails() {
   Handle<JSFunction> js_function = HasContext()
                                        ? handle(CurrentContext()->closure())
                                        : Handle<JSFunction>::null();
-  if (!js_function.is_null()) {
-    Handle<String> closure_name = JSFunction::GetDebugName(js_function);
-    if (!closure_name.is_null() && (closure_name->length() != 0))
-      details->set(kScopeDetailsNameIndex, *closure_name);
-  }
   if (Type() == ScopeTypeGlobal || Type() == ScopeTypeScript) {
     return isolate_->factory()->NewJSArrayWithElements(details);
   }
@@ -156,6 +151,10 @@ MUST_USE_RESULT MaybeHandle<JSObject> ScopeIterator::MaterializeScopeDetails() {
   }
 
   if (!js_function.is_null()) {
+    Handle<String> closure_name = JSFunction::GetDebugName(js_function);
+    if (!closure_name.is_null() && closure_name->length() != 0) {
+      details->set(kScopeDetailsNameIndex, *closure_name);
+    }
     details->set(kScopeDetailsStartPositionIndex, Smi::FromInt(start_position));
     details->set(kScopeDetailsEndPositionIndex, Smi::FromInt(end_position));
     details->set(kScopeDetailsFunctionIndex, *js_function);

src/parsing/parser.cc

@@ -2711,7 +2711,6 @@ Statement* Parser::ParseWithStatement(ZoneList<const AstRawString*>* labels,
   Expression* expr = ParseExpression(true, CHECK_OK);
   Expect(Token::RPAREN, CHECK_OK);
 
-  scope_->DeclarationScope()->RecordWithStatement();
   Scope* with_scope = NewScope(scope_, WITH_SCOPE);
   Statement* body;
   { BlockState block_state(&scope_, with_scope);

test/cctest/interpreter/bytecode_expectations/WithStatement.golden

@@ -11,27 +11,19 @@ wrap: yes
 snippet: "
   with ({x:42}) { return x; }
 "
-frame size: 5
+frame size: 4
 parameter count: 1
-bytecode array length: 45
+bytecode array length: 24
 bytecodes: [
-  B(CallRuntime), U16(Runtime::kNewFunctionContext), R(closure), U8(1),
-  B(PushContext), R(0),
-  B(Ldar), R(this),
-  B(StaContextSlot), R(context), U8(4),
-  B(CreateMappedArguments),
-  B(StaContextSlot), R(context), U8(5),
-  B(Ldar), R(new_target),
-  B(StaContextSlot), R(context), U8(6),
   B(StackCheck),
   B(CreateObjectLiteral), U8(0), U8(0), U8(5),
-  B(Star), R(2),
+  B(Star), R(1),
   B(ToObject),
-  B(Star), R(3),
+  B(Star), R(2),
   B(Ldar), R(closure),
-  B(Star), R(4),
-  B(CallRuntime), U16(Runtime::kPushWithContext), R(3), U8(2),
-  B(PushContext), R(1),
+  B(Star), R(3),
+  B(CallRuntime), U16(Runtime::kPushWithContext), R(2), U8(2),
+  B(PushContext), R(0),
   B(LdaLookupSlot), U8(1),
   B(Return),
 ]

test/mjsunit/debug-evaluate-locals.js

@@ -89,18 +89,17 @@ function f() {
 }
 
 function checkFrame2(frame) {
-  // Frame 2 (f) has normal variables a and b (and arguments).
+  // Frame 2 (f) has normal variables a and b.
   var count = frame.localCount();
-  assertEquals(3, count);
+  assertEquals(2, count);
   for (var i = 0; i < count; ++i) {
     var name = frame.localName(i);
     var value = frame.localValue(i).value();
     if (name == 'a') {
       assertEquals(5, value);
-    } else if (name == 'b') {
-      assertEquals(0, value);
     } else {
-      assertEquals('arguments', name);
+      assertEquals('b', name);
+      assertEquals(0, value);
     }
   }
 }

test/mjsunit/debug-function-scopes.js

@@ -73,7 +73,7 @@ assertEquals(6, mirror.scopeCount());
 
 CheckScope(mirror.scope(0), { a: 4, b: 5 }, ScopeType.Closure);
 CheckScope(mirror.scope(1), { w: 5, v: "Capybara" }, ScopeType.With);
-CheckScope(mirror.scope(2), { y: 17, z: 22 }, ScopeType.Closure);
+CheckScope(mirror.scope(2), { z: 22 }, ScopeType.Closure);
 CheckScope(mirror.scope(3), { x: 5 }, ScopeType.Closure);
 CheckScope(mirror.scope(4), {}, ScopeType.Script);
 CheckScope(mirror.scope(5), {}, ScopeType.Global);

test/mjsunit/debug-scopes.js

@@ -152,7 +152,7 @@ function CheckScopeChainNames(names, exec_state) {
   for (var i = 0; i < names.length; i++) {
     var scope = exec_state.frame().scope(i);
     assertTrue(scope.isScope());
-    assertEquals(scope.details().name(), names[i])
+    assertEquals(names[i], scope.details().name())
   }
 }
 
@@ -544,7 +544,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({a:1}, 1, exec_state);
-  CheckScopeChainNames([undefined, "closure_1", undefined, undefined], exec_state)
+  CheckScopeChainNames(["f", "closure_1", undefined, undefined], exec_state)
 };
 closure_1(1)();
 EndTest();
@@ -571,7 +571,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({a:1,x:3}, 1, exec_state);
-  CheckScopeChainNames([undefined, "closure_2", undefined, undefined], exec_state)
+  CheckScopeChainNames(["f", "closure_2", undefined, undefined], exec_state)
 };
 closure_2(1, 2)();
 EndTest();
@@ -599,7 +599,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({a:1,b:2,x:3,y:4}, 1, exec_state);
-  CheckScopeChainNames([undefined, "closure_3", undefined, undefined], exec_state)
+  CheckScopeChainNames(["f", "closure_3", undefined, undefined], exec_state)
 };
 closure_3(1, 2)();
 EndTest();
@@ -630,7 +630,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({a:1,b:2,x:3,y:4,f:function(){}}, 1, exec_state);
-  CheckScopeChainNames([undefined, "closure_4", undefined, undefined], exec_state)
+  CheckScopeChainNames(["f", "closure_4", undefined, undefined], exec_state)
 };
 closure_4(1, 2)();
 EndTest();
@@ -748,7 +748,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({x: 2}, 0, exec_state);
-  CheckScopeChainNames([undefined, undefined, undefined], exec_state)
+  CheckScopeChainNames(["inner", undefined, undefined], exec_state)
 };
 closure_8();
 EndTest();
@@ -770,7 +770,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Closure,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
-  CheckScopeChainNames([undefined, "closure_9", undefined, undefined], exec_state)
+  CheckScopeChainNames(["inner", "closure_9", undefined, undefined], exec_state)
 };
 closure_9();
 EndTest();
@@ -837,10 +837,10 @@ function closure_in_with_1() {
 listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.Local,
                    debug.ScopeType.With,
-                   debug.ScopeType.Closure,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({x: 2}, 0, exec_state);
+  CheckScopeContent({x: 1}, 1, exec_state);
 };
 closure_in_with_1();
 EndTest();
@@ -861,13 +861,12 @@ listener_delegate = function(exec_state) {
   CheckScopeChain([debug.ScopeType.With,
                    debug.ScopeType.Local,
                    debug.ScopeType.With,
-                   debug.ScopeType.Closure,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({x: 3}, 0, exec_state);
   CheckScopeContent({x: 2}, 1, exec_state);
   CheckScopeContent({x: 1}, 2, exec_state);
-  CheckScopeChainNames(["inner", "inner", "closure_in_with_2", "closure_in_with_2", undefined, undefined], exec_state)
+  CheckScopeChainNames(["inner", "inner", "closure_in_with_2", undefined, undefined], exec_state)
 };
 closure_in_with_2();
 EndTest();
@@ -949,7 +948,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({e:'Exception'}, 0, exec_state);
-  CheckScopeChainNames(["catch_block_1", undefined, undefined, undefined], exec_state)
+  CheckScopeChainNames(["catch_block_1", "catch_block_1", undefined, undefined], exec_state)
 };
 catch_block_1();
 EndTest();
@@ -1095,7 +1094,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({e:'Exception'}, 0, exec_state);
-  CheckScopeChainNames(["catch_block_7", undefined, undefined, undefined], exec_state)
+  CheckScopeChainNames(["catch_block_7", "catch_block_7", undefined, undefined], exec_state)
 };
 catch_block_7();
 EndTest();
@@ -1109,7 +1108,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeContent({}, 1, exec_state);
-  CheckScopeChainNames([undefined, undefined, undefined], exec_state)
+  CheckScopeChainNames(["m", undefined, undefined], exec_state)
 };
 
 (function() {

test/mjsunit/regress/regress-592353.js

+// Copyright 2016 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.
+//
+// Flags: --allow-natives-syntax --no-lazy
+
+with ({}) {}
+f = ({x}) => { };
+%OptimizeFunctionOnNextCall(f);
+f({});