[scopes] Clarify and narrow when scopes care about an eval() call

There are two reasons for Scopes to need information about eval calls
inside them:

  - Eval in a scope, or any of its inner scopes, turns off a bunch of
    scope analysis optimizations (e.g., all variables have to be treated
    as "used" and context-allocated).
  - Eval in a sloppy declaration scope means allows runtime addition
    of var declarations.

This patch aims to make the code better-reflect this reality.
It's meant as a pure cleanup, with no expected change in behavior.

Change-Id: I744c5051bb7a90b11420930e9596e5d6c35eb440
Reviewed-on: https://chromium-review.googlesource.com/602848
Commit-Queue: Adam Klein <adamk@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47257}

src/ast/scopes.cc

@@ -259,7 +259,7 @@ Scope::Scope(Zone* zone, ScopeType scope_type, Handle<ScopeInfo> scope_info)
 #ifdef DEBUG
   already_resolved_ = true;
 #endif
-  if (scope_info->CallsEval()) RecordEvalCall();
+  if (scope_info->CallsSloppyEval()) scope_calls_eval_ = true;
   set_language_mode(scope_info->language_mode());
   num_heap_slots_ = scope_info->ContextLength();
   DCHECK_LE(Context::MIN_CONTEXT_SLOTS, num_heap_slots_);
@@ -791,7 +791,7 @@ Scope* Scope::FinalizeBlockScope() {
   DCHECK(!HasBeenRemoved());
 
   if (variables_.occupancy() > 0 ||
-      (is_declaration_scope() && calls_sloppy_eval())) {
+      (is_declaration_scope() && AsDeclarationScope()->calls_sloppy_eval())) {
     return this;
   }
 
@@ -824,9 +824,13 @@ Scope* Scope::FinalizeBlockScope() {
     unresolved_ = nullptr;
   }
 
-  if (scope_calls_eval_) outer_scope()->scope_calls_eval_ = true;
   if (inner_scope_calls_eval_) outer_scope()->inner_scope_calls_eval_ = true;
 
+  // No need to propagate scope_calls_eval_, since if it was relevant to
+  // this scope we would have had to bail out at the top.
+  DCHECK(!scope_calls_eval_ || !is_declaration_scope() ||
+         !is_sloppy(language_mode()));
+
   // This block does not need a context.
   num_heap_slots_ = 0;
 
@@ -1373,7 +1377,10 @@ int Scope::ContextChainLengthUntilOutermostSloppyEval() const {
   for (const Scope* s = this; s != nullptr; s = s->outer_scope()) {
     if (!s->NeedsContext()) continue;
     length++;
-    if (s->calls_sloppy_eval()) result = length;
+    if (s->is_declaration_scope() &&
+        s->AsDeclarationScope()->calls_sloppy_eval()) {
+      result = length;
+    }
   }
 
   return result;
@@ -1706,7 +1713,9 @@ void Scope::Print(int n) {
   }
   if (IsAsmModule()) Indent(n1, "// scope is an asm module\n");
   if (IsAsmFunction()) Indent(n1, "// scope is an asm function\n");
-  if (scope_calls_eval_) Indent(n1, "// scope calls 'eval'\n");
+  if (is_declaration_scope() && AsDeclarationScope()->calls_sloppy_eval()) {
+    Indent(n1, "// scope calls sloppy 'eval'\n");
+  }
   if (is_declaration_scope() && AsDeclarationScope()->uses_super_property()) {
     Indent(n1, "// scope uses 'super' property\n");
   }
@@ -1867,7 +1876,7 @@ Variable* Scope::LookupRecursive(VariableProxy* proxy, Scope* outer_scope_end) {
     return NonLocal(proxy->raw_name(), DYNAMIC);
   }
 
-  if (calls_sloppy_eval() && is_declaration_scope()) {
+  if (is_declaration_scope() && AsDeclarationScope()->calls_sloppy_eval()) {
     // A variable binding may have been found in an outer scope, but the current
     // scope makes a sloppy 'eval' call, so the found variable may not be the
     // correct one (the 'eval' may introduce a binding with the same name). In
@@ -2283,8 +2292,9 @@ void Scope::AllocateVariablesRecursively() {
   // Likewise for modules and function scopes representing asm.js modules.
   bool must_have_context =
       is_with_scope() || is_module_scope() || IsAsmModule() ||
-      (is_function_scope() && calls_sloppy_eval()) ||
-      (is_block_scope() && is_declaration_scope() && calls_sloppy_eval());
+      (is_function_scope() && AsDeclarationScope()->calls_sloppy_eval()) ||
+      (is_block_scope() && is_declaration_scope() &&
+       AsDeclarationScope()->calls_sloppy_eval());
 
   // If we didn't allocate any locals in the local context, then we only
   // need the minimal number of slots if we must have a context.

src/ast/scopes.h

@@ -264,7 +264,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // eval call.
   void RecordEvalCall() {
     scope_calls_eval_ = true;
-    RecordInnerScopeEvalCall();
   }
 
   void RecordInnerScopeEvalCall() {
@@ -363,11 +362,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   bool is_with_scope() const { return scope_type_ == WITH_SCOPE; }
   bool is_declaration_scope() const { return is_declaration_scope_; }
 
-  // Information about which scopes calls eval.
-  bool calls_eval() const { return scope_calls_eval_; }
-  bool calls_sloppy_eval() const {
-    return scope_calls_eval_ && is_sloppy(language_mode());
-  }
   bool inner_scope_calls_eval() const { return inner_scope_calls_eval_; }
   bool IsAsmModule() const;
   bool IsAsmFunction() const;
@@ -693,6 +687,10 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope {
                                         IsClassConstructor(function_kind())));
   }
 
+  bool calls_sloppy_eval() const {
+    return scope_calls_eval_ && is_sloppy(language_mode());
+  }
+
   bool was_lazily_parsed() const { return was_lazily_parsed_; }
 
 #ifdef DEBUG

src/objects/scope-info.cc

@@ -145,6 +145,7 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
   bool has_simple_parameters = false;
   bool asm_module = false;
   bool asm_function = false;
+  bool calls_sloppy_eval = false;
   if (scope->is_function_scope()) {
     DeclarationScope* function_scope = scope->AsDeclarationScope();
     has_simple_parameters = function_scope->has_simple_parameters();
@@ -154,12 +155,13 @@ Handle<ScopeInfo> ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope,
   FunctionKind function_kind = kNormalFunction;
   if (scope->is_declaration_scope()) {
     function_kind = scope->AsDeclarationScope()->function_kind();
+    calls_sloppy_eval = scope->AsDeclarationScope()->calls_sloppy_eval();
   }
 
   // Encode the flags.
   int flags =
       ScopeTypeField::encode(scope->scope_type()) |
-      CallsEvalField::encode(scope->calls_eval()) |
+      CallsSloppyEvalField::encode(calls_sloppy_eval) |
       LanguageModeField::encode(scope->language_mode()) |
       DeclarationScopeField::encode(scope->is_declaration_scope()) |
       ReceiverVariableField::encode(receiver_info) |
@@ -299,7 +301,7 @@ Handle<ScopeInfo> ScopeInfo::CreateForWithScope(
 
   // Encode the flags.
   int flags =
-      ScopeTypeField::encode(WITH_SCOPE) | CallsEvalField::encode(false) |
+      ScopeTypeField::encode(WITH_SCOPE) | CallsSloppyEvalField::encode(false) |
       LanguageModeField::encode(SLOPPY) | DeclarationScopeField::encode(false) |
       ReceiverVariableField::encode(NONE) | HasNewTargetField::encode(false) |
       FunctionVariableField::encode(NONE) | AsmModuleField::encode(false) |
@@ -351,16 +353,17 @@ Handle<ScopeInfo> ScopeInfo::CreateGlobalThisBinding(Isolate* isolate) {
   Handle<ScopeInfo> scope_info = factory->NewScopeInfo(length);
 
   // Encode the flags.
-  int flags =
-      ScopeTypeField::encode(SCRIPT_SCOPE) | CallsEvalField::encode(false) |
-      LanguageModeField::encode(SLOPPY) | DeclarationScopeField::encode(true) |
-      ReceiverVariableField::encode(receiver_info) |
-      FunctionVariableField::encode(function_name_info) |
-      AsmModuleField::encode(false) | AsmFunctionField::encode(false) |
-      HasSimpleParametersField::encode(has_simple_parameters) |
-      FunctionKindField::encode(FunctionKind::kNormalFunction) |
-      HasOuterScopeInfoField::encode(has_outer_scope_info) |
-      IsDebugEvaluateScopeField::encode(false);
+  int flags = ScopeTypeField::encode(SCRIPT_SCOPE) |
+              CallsSloppyEvalField::encode(false) |
+              LanguageModeField::encode(SLOPPY) |
+              DeclarationScopeField::encode(true) |
+              ReceiverVariableField::encode(receiver_info) |
+              FunctionVariableField::encode(function_name_info) |
+              AsmModuleField::encode(false) | AsmFunctionField::encode(false) |
+              HasSimpleParametersField::encode(has_simple_parameters) |
+              FunctionKindField::encode(FunctionKind::kNormalFunction) |
+              HasOuterScopeInfoField::encode(has_outer_scope_info) |
+              IsDebugEvaluateScopeField::encode(false);
   scope_info->SetFlags(flags);
   scope_info->SetParameterCount(parameter_count);
   scope_info->SetStackLocalCount(stack_local_count);
@@ -404,8 +407,12 @@ ScopeType ScopeInfo::scope_type() {
   return ScopeTypeField::decode(Flags());
 }
 
-bool ScopeInfo::CallsEval() {
-  return length() > 0 && CallsEvalField::decode(Flags());
+bool ScopeInfo::CallsSloppyEval() {
+  bool calls_sloppy_eval =
+      length() > 0 && CallsSloppyEvalField::decode(Flags());
+  DCHECK_IMPLIES(calls_sloppy_eval, is_sloppy(language_mode()));
+  DCHECK_IMPLIES(calls_sloppy_eval, is_declaration_scope());
+  return calls_sloppy_eval;
 }
 
 LanguageMode ScopeInfo::language_mode() {

src/objects/scope-info.h

@@ -37,9 +37,6 @@ class ScopeInfo : public FixedArray {
   // Return the type of this scope.
   ScopeType scope_type();
 
-  // Does this scope call eval?
-  bool CallsEval();
-
   // Return the language mode of this scope.
   LanguageMode language_mode();
 
@@ -47,7 +44,7 @@ class ScopeInfo : public FixedArray {
   bool is_declaration_scope();
 
   // Does this scope make a sloppy eval call?
-  bool CallsSloppyEval() { return CallsEval() && is_sloppy(language_mode()); }
+  bool CallsSloppyEval();
 
   // Return the total number of locals allocated on the stack and in the
   // context. This includes the parameters that are allocated in the context.
@@ -301,10 +298,11 @@ class ScopeInfo : public FixedArray {
 
   // Properties of scopes.
   class ScopeTypeField : public BitField<ScopeType, 0, 4> {};
-  class CallsEvalField : public BitField<bool, ScopeTypeField::kNext, 1> {};
+  class CallsSloppyEvalField : public BitField<bool, ScopeTypeField::kNext, 1> {
+  };
   STATIC_ASSERT(LANGUAGE_END == 2);
   class LanguageModeField
-      : public BitField<LanguageMode, CallsEvalField::kNext, 1> {};
+      : public BitField<LanguageMode, CallsSloppyEvalField::kNext, 1> {};
   class DeclarationScopeField
       : public BitField<bool, LanguageModeField::kNext, 1> {};
   class ReceiverVariableField

src/parsing/expression-scope-reparenter.cc

@@ -97,7 +97,7 @@ void ReparentExpressionScope(uintptr_t stack_limit, Expression* expr,
   // sloppy eval.
   DCHECK(scope->is_block_scope());
   DCHECK(scope->is_declaration_scope());
-  DCHECK(scope->calls_sloppy_eval());
+  DCHECK(scope->AsDeclarationScope()->calls_sloppy_eval());
   DCHECK(scope->outer_scope()->is_function_scope());
 
   Reparenter r(stack_limit, expr, scope);

src/parsing/parser-base.h

@@ -1323,13 +1323,20 @@ class ParserBase {
                                            Scope* scope) {
     if (impl()->IsIdentifier(expression) &&
         impl()->IsEval(impl()->AsIdentifier(expression))) {
-      scope->RecordEvalCall();
+      scope->RecordInnerScopeEvalCall();
       function_state_->RecordFunctionOrEvalCall();
       if (is_sloppy(scope->language_mode())) {
         // For sloppy scopes we also have to record the call at function level,
         // in case it includes declarations that will be hoisted.
         scope->GetDeclarationScope()->RecordEvalCall();
       }
+
+      // This call is only necessary to track evals that may be
+      // inside arrow function parameter lists. In that case,
+      // Scope::Snapshot::Reparent will move this bit down into
+      // the arrow function's scope.
+      scope->RecordEvalCall();
+
       return Call::IS_POSSIBLY_EVAL;
     }
     return Call::NOT_EVAL;

src/parsing/parser.cc

@@ -3032,7 +3032,8 @@ Block* Parser::BuildParameterInitializationBlock(
 
     Scope* param_scope = scope();
     Block* param_block = init_block;
-    if (!parameter->is_simple() && scope()->calls_sloppy_eval()) {
+    if (!parameter->is_simple() &&
+        scope()->AsDeclarationScope()->calls_sloppy_eval()) {
       param_scope = NewVarblockScope();
       param_scope->set_start_position(descriptor.initialization_pos);
       param_scope->set_end_position(parameter->initializer_end_position);

src/parsing/pattern-rewriter.cc

@@ -297,8 +297,8 @@ bool Parser::PatternRewriter::DeclaresParameterContainingSloppyEval() const {
       // And only when scope is a block scope;
       // without eval, it is a function scope.
       scope()->is_block_scope()) {
-    DCHECK(scope()->calls_sloppy_eval());
     DCHECK(scope()->is_declaration_scope());
+    DCHECK(scope()->AsDeclarationScope()->calls_sloppy_eval());
     DCHECK(scope()->outer_scope()->is_function_scope());
     return true;
   }

src/parsing/preparsed-scope-data.cc

@@ -16,9 +16,9 @@ namespace internal {
 
 namespace {
 
-class ScopeCallsEvalField : public BitField<bool, 0, 1> {};
+class ScopeCallsSloppyEvalField : public BitField<bool, 0, 1> {};
 class InnerScopeCallsEvalField
-    : public BitField<bool, ScopeCallsEvalField::kNext, 1> {};
+    : public BitField<bool, ScopeCallsSloppyEvalField::kNext, 1> {};
 
 class VariableIsUsedField : public BitField16<bool, 0, 1> {};
 class VariableMaybeAssignedField
@@ -314,7 +314,9 @@ void ProducedPreParsedScopeData::SaveDataForScope(Scope* scope) {
 #endif
 
   uint32_t eval =
-      ScopeCallsEvalField::encode(scope->calls_eval()) |
+      ScopeCallsSloppyEvalField::encode(
+          scope->is_declaration_scope() &&
+          scope->AsDeclarationScope()->calls_sloppy_eval()) |
       InnerScopeCallsEvalField::encode(scope->inner_scope_calls_eval());
   backing_store_.push_back(eval);
 
@@ -482,7 +484,7 @@ void ConsumedPreParsedScopeData::RestoreData(Scope* scope,
   DCHECK_EQ(scope_data->get(index_++), scope->scope_type());
 
   uint32_t eval = scope_data->get(index_++);
-  if (ScopeCallsEvalField::decode(eval)) {
+  if (ScopeCallsSloppyEvalField::decode(eval)) {
     scope->RecordEvalCall();
   }
   if (InnerScopeCallsEvalField::decode(eval)) {

src/parsing/preparser.cc

@@ -386,8 +386,9 @@ PreParser::LazyParsingResult PreParser::ParseStatementListAndLogFunction(
 PreParserStatement PreParser::BuildParameterInitializationBlock(
     const PreParserFormalParameters& parameters, bool* ok) {
   DCHECK(!parameters.is_simple);
+  DCHECK(scope()->is_function_scope());
   if (FLAG_experimental_preparser_scope_analysis &&
-      scope()->calls_sloppy_eval()) {
+      scope()->AsDeclarationScope()->calls_sloppy_eval()) {
     DCHECK_NOT_NULL(produced_preparsed_scope_data_);
     // We cannot replicate the Scope structure constructed by the Parser,
     // because we've lost information whether each individual parameter was

test/cctest/test-parsing.cc

@@ -865,7 +865,10 @@ TEST(ScopeUsesArgumentsSuperThis) {
         CHECK(
             scope->Lookup(info.ast_value_factory()->this_string())->is_used());
       }
-      CHECK_EQ((source_data[i].expected & EVAL) != 0, scope->calls_eval());
+      if (is_sloppy(scope->language_mode())) {
+        CHECK_EQ((source_data[i].expected & EVAL) != 0,
+                 scope->AsDeclarationScope()->calls_sloppy_eval());
+      }
     }
   }
 }