[parser] Mark maybe_assigned recursively for shadowing vars

The previous fix for this bug (crrev.com/c/1678365) pessimistically
would mark all shadowed variables as maybe_assigned. Unfortunately,
this doesn't work across a parse/preparse boundary, where the shadowing
variable is found via Scope::AnalyzePartially while the shadowed
variable is outside of the preparser entry point. In those cases, the
referencing proxy is copied to the outer scope, in which case the
dynamicness of the original lookup is lost and the maybe_assigned
pessimisation no longer applies.

This means that maybe_assigned status of a variable is dependent on
which function is being parsed. In particular, it can cause bytecode
to change on recompilation, causing issues for lazy source positions.

This patch allows SetMaybeAssigned to walk its shadowed variables,
and recursively set them to maybe_assigned too. Checking for
maybe_assigned changing prevents this recursion from having a
quadratic performance failure mode.

Bug: v8:8510
Bug: v8:9394
Change-Id: Id19fe1fad5ec8f0f9aa03b00eb24497f88f71216
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1677265
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62458}

src/ast/ast.cc

@@ -178,7 +178,7 @@ void VariableProxy::BindTo(Variable* var) {
   set_var(var);
   set_is_resolved();
   var->set_is_used();
-  if (is_assigned()) var->set_maybe_assigned();
+  if (is_assigned()) var->SetMaybeAssigned();
 }
 
 Assignment::Assignment(NodeType node_type, Token::Value op, Expression* target,

src/ast/ast.h

@@ -1509,7 +1509,7 @@ class VariableProxy final : public Expression {
   void set_is_assigned() {
     bit_field_ = IsAssignedField::update(bit_field_, true);
     if (is_resolved()) {
-      var()->set_maybe_assigned();
+      var()->SetMaybeAssigned();
     }
   }
 

src/ast/scopes.cc

@@ -508,8 +508,9 @@ void DeclarationScope::HoistSloppyBlockFunctions(AstNodeFactory* factory) {
       DCHECK(is_being_lazily_parsed_);
       bool was_added;
       Variable* var = DeclareVariableName(name, VariableMode::kVar, &was_added);
-      if (sloppy_block_function->init() == Token::ASSIGN)
-        var->set_maybe_assigned();
+      if (sloppy_block_function->init() == Token::ASSIGN) {
+        var->SetMaybeAssigned();
+      }
     }
   }
 }
@@ -893,7 +894,7 @@ Variable* Scope::DeclareLocal(const AstRawString* name, VariableMode mode,
   // assigned because they might be accessed by a lazily parsed top-level
   // function, which, for efficiency, we preparse without variable tracking.
   if (is_script_scope() || is_module_scope()) {
-    if (mode != VariableMode::kConst) var->set_maybe_assigned();
+    if (mode != VariableMode::kConst) var->SetMaybeAssigned();
     var->set_is_used();
   }
 
@@ -942,7 +943,7 @@ Variable* Scope::DeclareVariable(
       DCHECK(*was_added);
     }
   } else {
-    var->set_maybe_assigned();
+    var->SetMaybeAssigned();
     if (V8_UNLIKELY(IsLexicalVariableMode(mode) ||
                     IsLexicalVariableMode(var->mode()))) {
       // The name was declared in this scope before; check for conflicting
@@ -1013,7 +1014,7 @@ Variable* Scope::DeclareVariableName(const AstRawString* name,
       }
       // Sloppy block function redefinition.
     }
-    var->set_maybe_assigned();
+    var->SetMaybeAssigned();
   }
   var->set_is_used();
   return var;
@@ -1067,7 +1068,7 @@ Variable* Scope::NewTemporary(const AstRawString* name,
   Variable* var = new (zone()) Variable(scope, name, VariableMode::kTemporary,
                                         NORMAL_VARIABLE, kCreatedInitialized);
   scope->AddLocal(var);
-  if (maybe_assigned == kMaybeAssigned) var->set_maybe_assigned();
+  if (maybe_assigned == kMaybeAssigned) var->SetMaybeAssigned();
   return var;
 }
 
@@ -1405,7 +1406,7 @@ void Scope::AnalyzePartially(DeclarationScope* max_outer_scope,
         }
       } else {
         var->set_is_used();
-        if (proxy->is_assigned()) var->set_maybe_assigned();
+        if (proxy->is_assigned()) var->SetMaybeAssigned();
       }
     }
 
@@ -1887,11 +1888,14 @@ Variable* Scope::LookupWith(VariableProxy* proxy, Scope* scope,
     DCHECK(!scope->already_resolved_);
     var->set_is_used();
     var->ForceContextAllocation();
-    var->set_maybe_assigned();
+    if (proxy->is_assigned()) var->SetMaybeAssigned();
   }
   if (entry_point != nullptr) entry_point->variables_.Remove(var);
   Scope* target = entry_point == nullptr ? scope : entry_point;
-  return target->NonLocal(proxy->raw_name(), VariableMode::kDynamic);
+  Variable* dynamic =
+      target->NonLocal(proxy->raw_name(), VariableMode::kDynamic);
+  dynamic->set_local_if_not_shadowed(var);
+  return dynamic;
 }
 
 Variable* Scope::LookupSloppyEval(VariableProxy* proxy, Scope* scope,
@@ -1920,7 +1924,7 @@ Variable* Scope::LookupSloppyEval(VariableProxy* proxy, Scope* scope,
   // script scope are always dynamic.
   if (var->IsGlobalObjectProperty()) {
     Scope* target = entry_point == nullptr ? scope : entry_point;
-    return target->NonLocal(proxy->raw_name(), VariableMode::kDynamicGlobal);
+    var = target->NonLocal(proxy->raw_name(), VariableMode::kDynamicGlobal);
   }
 
   if (var->is_dynamic()) return var;
@@ -2018,7 +2022,7 @@ void Scope::ResolvePreparsedVariable(VariableProxy* proxy, Scope* scope,
       var->set_is_used();
       if (!var->is_dynamic()) {
         var->ForceContextAllocation();
-        if (proxy->is_assigned()) var->set_maybe_assigned();
+        if (proxy->is_assigned()) var->SetMaybeAssigned();
         return;
       }
     }
@@ -2062,7 +2066,7 @@ bool Scope::MustAllocate(Variable* var) {
   if (!var->raw_name()->IsEmpty() &&
       (inner_scope_calls_eval_ || is_catch_scope() || is_script_scope())) {
     var->set_is_used();
-    if (inner_scope_calls_eval_) var->set_maybe_assigned();
+    if (inner_scope_calls_eval_) var->SetMaybeAssigned();
   }
   DCHECK(!var->has_forced_context_allocation() || var->is_used());
   // Global variables do not need to be allocated.
@@ -2132,7 +2136,7 @@ void DeclarationScope::AllocateParameterLocals() {
     DCHECK_EQ(this, var->scope());
     if (has_mapped_arguments) {
       var->set_is_used();
-      var->set_maybe_assigned();
+      var->SetMaybeAssigned();
       var->ForceContextAllocation();
     }
     AllocateParameter(var, i);

src/ast/variables.h

@@ -71,8 +71,18 @@ class Variable final : public ZoneObject {
   MaybeAssignedFlag maybe_assigned() const {
     return MaybeAssignedFlagField::decode(bit_field_);
   }
-  void set_maybe_assigned() {
-    bit_field_ = MaybeAssignedFlagField::update(bit_field_, kMaybeAssigned);
+  void SetMaybeAssigned() {
+    // If this variable is dynamically shadowing another variable, then that
+    // variable could also be assigned (in the non-shadowing case).
+    if (has_local_if_not_shadowed()) {
+      // Avoid repeatedly marking the same tree of variables by only recursing
+      // when this variable's maybe_assigned status actually changes.
+      if (!maybe_assigned()) {
+        local_if_not_shadowed()->SetMaybeAssigned();
+      }
+      DCHECK(local_if_not_shadowed()->maybe_assigned());
+    }
+    set_maybe_assigned();
   }
 
   RequiresBrandCheckFlag get_requires_brand_check_flag() const {
@@ -158,11 +168,16 @@ class Variable final : public ZoneObject {
   }
 
   Variable* local_if_not_shadowed() const {
-    DCHECK(mode() == VariableMode::kDynamicLocal &&
-           local_if_not_shadowed_ != nullptr);
+    DCHECK((mode() == VariableMode::kDynamicLocal ||
+            mode() == VariableMode::kDynamic) &&
+           has_local_if_not_shadowed());
     return local_if_not_shadowed_;
   }
 
+  bool has_local_if_not_shadowed() const {
+    return local_if_not_shadowed_ != nullptr;
+  }
+
   void set_local_if_not_shadowed(Variable* local) {
     local_if_not_shadowed_ = local;
   }
@@ -215,15 +230,19 @@ class Variable final : public ZoneObject {
   const AstRawString* name_;
 
   // If this field is set, this variable references the stored locally bound
-  // variable, but it might be shadowed by variable bindings introduced by
-  // sloppy 'eval' calls between the reference scope (inclusive) and the
-  // binding scope (exclusive).
+  // variable, but it might be shadowed by variable bindings introduced by with
+  // blocks or sloppy 'eval' calls between the reference scope (inclusive) and
+  // the binding scope (exclusive).
   Variable* local_if_not_shadowed_;
   Variable* next_;
   int index_;
   int initializer_position_;
   uint16_t bit_field_;
 
+  void set_maybe_assigned() {
+    bit_field_ = MaybeAssignedFlagField::update(bit_field_, kMaybeAssigned);
+  }
+
   class VariableModeField : public BitField16<VariableMode, 0, 3> {};
   class VariableKindField
       : public BitField16<VariableKind, VariableModeField::kNext, 3> {};

src/parsing/expression-scope.h

@@ -330,7 +330,7 @@ class VariableDeclarationParsingScope : public ExpressionScope<Types> {
         // This also handles marking of loop variables in for-in and for-of
         // loops, as determined by loop-nesting-depth.
         DCHECK_NOT_NULL(var);
-        var->set_maybe_assigned();
+        var->SetMaybeAssigned();
       }
     }
     return var;

src/parsing/parser-base.h

@@ -3347,7 +3347,7 @@ typename ParserBase<Impl>::ExpressionT ParserBase<Impl>::ParseSuperExpression(
       // TODO(rossberg): This might not be the correct FunctionState for the
       // method here.
       expression_scope()->RecordThisUse();
-      UseThis()->set_maybe_assigned();
+      UseThis()->SetMaybeAssigned();
       return impl()->NewSuperCallReference(pos);
     }
   }

src/parsing/preparse-data.cc

@@ -645,7 +645,7 @@ void BaseConsumedPreparseData<Data>::RestoreDataForVariable(Variable* var) {
 #endif
   uint8_t variable_data = scope_data_->ReadQuarter();
   if (VariableMaybeAssignedField::decode(variable_data)) {
-    var->set_maybe_assigned();
+    var->SetMaybeAssigned();
   }
   if (VariableContextAllocatedField::decode(variable_data)) {
     var->set_is_used();

test/mjsunit/regress/regress-v8-9394-2.js

+// Copyright 2019 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: --enable-lazy-source-positions --stress-lazy-source-positions
+
+function test() {
+  function f() {
+    with ({}) {
+      // This is a non-assigning shadowing access to value. If both f and test
+      // are fully parsed or both are preparsed, then this is resolved during
+      // scope analysis to the outer value, and the outer value knows it can be
+      // shadowed. If test is fully parsed and f is preparsed, value here
+      // doesn't resolve to anything during partial analysis, and the outer
+      // value does not know it can be shadowed.
+      return value;
+    }
+  }
+  var value = 2;
+  var status = f();
+  return value;
+}
+test();