Accurately record eval calls in arrow parameter lists

Previously, we over-approximated Scope::scope_calls_eval_ in
arrow functions: if either the outer scope or the arrow function
parameters had a direct eval call, we marked both scopes as calling
eval. This over-approximation kept getting us into trouble, though,
especially when eager or lazy parsing would disagree about the
"calls eval" bit.

This patch instead tracks eval calls accurately, using a boolean on
Scope::Snapshot that is reset as appropriately depending on whether
a particular AssignmentExpression turned out to be an arrow parameter
list or not.

BUG=chromium:691687

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

src/ast/scopes.cc

@@ -155,7 +155,18 @@ Scope::Snapshot::Snapshot(Scope* scope)
       top_inner_scope_(scope->inner_scope_),
       top_unresolved_(scope->unresolved_),
       top_local_(scope->GetClosureScope()->locals_.end()),
-      top_decl_(scope->GetClosureScope()->decls_.end()) {}
+      top_decl_(scope->GetClosureScope()->decls_.end()),
+      outer_scope_calls_eval_(scope->scope_calls_eval_) {
+  // Reset in order to record eval calls during this Snapshot's lifetime.
+  outer_scope_->scope_calls_eval_ = false;
+}
+
+Scope::Snapshot::~Snapshot() {
+  // Restore previous calls_eval bit if needed.
+  if (outer_scope_calls_eval_) {
+    outer_scope_->scope_calls_eval_ = true;
+  }
+}
 
 DeclarationScope::DeclarationScope(Zone* zone,
                                    AstValueFactory* ast_value_factory)
@@ -778,7 +789,9 @@ Scope* Scope::FinalizeBlockScope() {
     unresolved_ = nullptr;
   }
 
-  PropagateUsageFlagsToScope(outer_scope_);
+  if (scope_calls_eval_) outer_scope()->scope_calls_eval_ = true;
+  if (inner_scope_calls_eval_) outer_scope()->inner_scope_calls_eval_ = true;
+
   // This block does not need a context.
   num_heap_slots_ = 0;
 
@@ -820,10 +833,15 @@ void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
     for (; inner_scope->sibling() != top_inner_scope_;
          inner_scope = inner_scope->sibling()) {
       inner_scope->outer_scope_ = new_parent;
+      if (inner_scope->inner_scope_calls_eval_) {
+        new_parent->inner_scope_calls_eval_ = true;
+      }
       DCHECK_NE(inner_scope, new_parent);
     }
     inner_scope->outer_scope_ = new_parent;
-
+    if (inner_scope->inner_scope_calls_eval_) {
+      new_parent->inner_scope_calls_eval_ = true;
+    }
     new_parent->inner_scope_ = new_parent->sibling_;
     inner_scope->sibling_ = nullptr;
     // Reset the sibling rather than the inner_scope_ since we
@@ -860,6 +878,14 @@ void Scope::Snapshot::Reparent(DeclarationScope* new_parent) const {
   }
   outer_closure->locals_.Rewind(top_local_);
   outer_closure->decls_.Rewind(top_decl_);
+
+  // Move eval calls since Snapshot's creation into new_parent.
+  if (outer_scope_->scope_calls_eval_) {
+    new_parent->scope_calls_eval_ = true;
+  }
+  // Reset the outer_scope's eval state. It will be restored to its
+  // original value as necessary in the destructor of this class.
+  outer_scope_->scope_calls_eval_ = false;
 }
 
 void Scope::ReplaceOuterScope(Scope* outer) {
@@ -871,15 +897,6 @@ void Scope::ReplaceOuterScope(Scope* outer) {
   outer_scope_ = outer;
 }
 
-
-void Scope::PropagateUsageFlagsToScope(Scope* other) {
-  DCHECK_NOT_NULL(other);
-  DCHECK(!already_resolved_);
-  DCHECK(!other->already_resolved_);
-  if (calls_eval()) other->RecordEvalCall();
-  if (inner_scope_calls_eval_) other->inner_scope_calls_eval_ = true;
-}
-
 Variable* Scope::LookupInScopeInfo(const AstRawString* name) {
   Handle<String> name_handle = name->string();
   // The Scope is backed up by ScopeInfo. This means it cannot operate in a

src/ast/scopes.h

@@ -112,6 +112,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   class Snapshot final BASE_EMBEDDED {
    public:
     explicit Snapshot(Scope* scope);
+    ~Snapshot();
 
     void Reparent(DeclarationScope* new_parent) const;
 
@@ -121,6 +122,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
     VariableProxy* top_unresolved_;
     ThreadedList<Variable>::Iterator top_local_;
     ThreadedList<Declaration>::Iterator top_decl_;
+    const bool outer_scope_calls_eval_;
   };
 
   enum class DeserializationMode { kIncludingVariables, kScopesOnly };
@@ -146,10 +148,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // Assumes outer_scope_ is non-null.
   void ReplaceOuterScope(Scope* outer_scope);
 
-  // Propagates any eagerly-gathered scope usage flags (such as calls_eval())
-  // to the passed-in scope.
-  void PropagateUsageFlagsToScope(Scope* other);
-
   Zone* zone() const { return zone_; }
 
   // ---------------------------------------------------------------------------

src/parsing/parse-info.cc

@@ -59,6 +59,7 @@ ParseInfo::ParseInfo(Handle<SharedFunctionInfo> shared)
   set_language_mode(shared->language_mode());
   set_shared_info(shared);
   set_module(shared->kind() == FunctionKind::kModule);
+  set_scope_info_is_empty(shared->scope_info() == ScopeInfo::Empty(isolate_));
 
   Handle<Script> script(Script::cast(shared->script()));
   set_script(script);
@@ -121,6 +122,7 @@ ParseInfo* ParseInfo::AllocateWithoutScript(Handle<SharedFunctionInfo> shared) {
   p->set_language_mode(shared->language_mode());
   p->set_shared_info(shared);
   p->set_module(shared->kind() == FunctionKind::kModule);
+  p->set_scope_info_is_empty(shared->scope_info() == ScopeInfo::Empty(isolate));
 
   // BUG(5946): This function exists as a workaround until we can
   // get rid of %SetCode in our native functions. The ParseInfo

src/parsing/parse-info.h

@@ -73,6 +73,7 @@ class V8_EXPORT_PRIVATE ParseInfo {
   FLAG_ACCESSOR(kCallsEval, calls_eval, set_calls_eval)
   FLAG_ACCESSOR(kDebug, is_debug, set_is_debug)
   FLAG_ACCESSOR(kSerializing, will_serialize, set_will_serialize)
+  FLAG_ACCESSOR(kScopeInfoIsEmpty, scope_info_is_empty, set_scope_info_is_empty)
 
 #undef FLAG_ACCESSOR
 
@@ -247,8 +248,9 @@ class V8_EXPORT_PRIVATE ParseInfo {
     kCallsEval = 1 << 9,
     kDebug = 1 << 10,
     kSerializing = 1 << 11,
+    kScopeInfoIsEmpty = 1 << 12,
     // ---------- Output flags --------------------------
-    kAstValueFactoryOwned = 1 << 12
+    kAstValueFactoryOwned = 1 << 13
   };
 
   //------------- Inputs to parsing and scope analysis -----------------------

src/parsing/parser-base.h

@@ -2726,11 +2726,9 @@ ParserBase<Impl>::ParseAssignmentExpression(bool accept_IN, bool* ok) {
     DeclarationScope* scope =
         NewFunctionScope(is_async ? FunctionKind::kAsyncArrowFunction
                                   : FunctionKind::kArrowFunction);
-    // Because the arrow's parameters were parsed in the outer scope, any
-    // usage flags that might have been triggered there need to be copied
-    // to the arrow scope.
-    this->scope()->PropagateUsageFlagsToScope(scope);
 
+    // Because the arrow's parameters were parsed in the outer scope,
+    // we need to fix up the scope chain appropriately.
     scope_snapshot.Reparent(scope);
 
     FormalParametersT parameters(scope);

src/parsing/parser.cc

@@ -868,13 +868,8 @@ FunctionLiteral* Parser::DoParseFunction(ParseInfo* info,
       // TODO(adamk): We should construct this scope from the ScopeInfo.
       DeclarationScope* scope = NewFunctionScope(kind);
 
-      // These two bits only need to be explicitly set because we're
+      // This bit only needs to be explicitly set because we're
       // not passing the ScopeInfo to the Scope constructor.
-      // TODO(adamk): Remove these calls once the above NewScope call
-      // passes the ScopeInfo.
-      if (info->calls_eval()) {
-        scope->RecordEvalCall();
-      }
       SetLanguageMode(scope, info->language_mode());
 
       scope->set_start_position(info->start_position());
@@ -951,6 +946,8 @@ FunctionLiteral* Parser::DoParseFunction(ParseInfo* info,
   DCHECK_NULL(target_stack_);
   DCHECK_IMPLIES(result,
                  info->function_literal_id() == result->function_literal_id());
+  DCHECK_IMPLIES(!info->scope_info_is_empty() && result,
+                 info->calls_eval() == result->scope()->calls_eval());
   return result;
 }
 

test/mjsunit/regress/regress-crbug-691687.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.
+//
+// Flags: --always-opt --no-lazy --turbo-filter=whatever
+
+function g() { eval() }
+with ({}) { }
+f = ({x}) => x;
+assertEquals(42, f({x: 42}));