[parser] Change and fix how we MarkLoopVariableAsAssigned

Keep track of loop nesting depth on FunctionState and use that to decide
whether to mark var as assigned. That also fixes the weird cornercase where a
loop body can have multiple expressions due to multiple declarations with
independent initializers in a single var-statement.

Change-Id: Ia24affde29e22e9464448fd390062f6dd983faf2
Reviewed-on: https://chromium-review.googlesource.com/c/1405037
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58707}

src/parsing/parser-base.h

@@ -421,6 +421,21 @@ class ParserBase {
       PointerWithPayload<FunctionState, bool, 1> state_and_prev_value_;
     };
 
+    class LoopScope {
+     public:
+      explicit LoopScope(FunctionState* function_state)
+          : function_state_(function_state) {
+        function_state_->loop_nesting_depth_++;
+      }
+
+      ~LoopScope() { function_state_->loop_nesting_depth_--; }
+
+     private:
+      FunctionState* function_state_;
+    };
+
+    int loop_nesting_depth() const { return loop_nesting_depth_; }
+
    private:
     // Properties count estimation.
     int expected_property_count_;
@@ -428,6 +443,9 @@ class ParserBase {
     // How many suspends are needed for this function.
     int suspend_count_;
 
+    // How deeply nested we currently are in this function.
+    int loop_nesting_depth_ = 0;
+
     FunctionState** function_state_stack_;
     FunctionState* outer_function_state_;
     DeclarationScope* scope_;
@@ -450,11 +468,10 @@ class ParserBase {
   };
 
   struct DeclarationDescriptor {
-    enum Kind { NORMAL, PARAMETER, FOR_EACH };
     VariableMode mode;
+    VariableKind kind;
     int declaration_pos;
     int initialization_pos;
-    Kind declaration_kind;
   };
 
   struct DeclarationParsingResult {
@@ -1152,21 +1169,13 @@ class ParserBase {
   //   let i = 10;
   //   do { var x = i } while (i--):
   //
-  // As a simple and very conservative approximation of this, we explicitly mark
-  // as maybe-assigned any non-lexical variable whose initializing "declaration"
-  // does not syntactically occur in the function scope.  (In the example above,
-  // it occurs in a block scope.)
-  //
   // Note that non-lexical variables include temporaries, which may also get
   // assigned inside a loop due to the various rewritings that the parser
   // performs.
   //
   // This also handles marking of loop variables in for-in and for-of loops,
-  // as determined by declaration_kind.
-  //
-  static void MarkLoopVariableAsAssigned(
-      Scope* scope, Variable* var,
-      typename DeclarationDescriptor::Kind declaration_kind);
+  // as determined by loop-nesting-depth.
+  void MarkLoopVariableAsAssigned(Variable* var);
 
   FunctionKind FunctionKindForImpl(bool is_method, ParseFunctionFlags flags) {
     static const FunctionKind kFunctionKinds[][2][2] = {
@@ -3399,7 +3408,7 @@ void ParserBase<Impl>::ParseVariableDeclarations(
   // declaration syntax.
 
   DCHECK_NOT_NULL(parsing_result);
-  parsing_result->descriptor.declaration_kind = DeclarationDescriptor::NORMAL;
+  parsing_result->descriptor.kind = NORMAL_VARIABLE;
   parsing_result->descriptor.declaration_pos = peek_position();
   parsing_result->descriptor.initialization_pos = peek_position();
 
@@ -4931,6 +4940,8 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseDoWhileStatement(
     ZonePtrList<const AstRawString>* own_labels) {
   // DoStatement ::
   //   'do' Statement 'while' '(' Expression ')' ';'
+  typename FunctionState::LoopScope loop_scope(function_state_);
+
   auto loop =
       factory()->NewDoWhileStatement(labels, own_labels, peek_position());
   TargetT target(this, loop);
@@ -4969,6 +4980,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseWhileStatement(
     ZonePtrList<const AstRawString>* own_labels) {
   // WhileStatement ::
   //   'while' '(' Expression ')' Statement
+  typename FunctionState::LoopScope loop_scope(function_state_);
 
   auto loop = factory()->NewWhileStatement(labels, own_labels, peek_position());
   TargetT target(this, loop);
@@ -5205,6 +5217,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
   //
   // We parse a declaration/expression after the 'for (' and then read the first
   // expression/declaration before we know if this is a for or a for-each.
+  typename FunctionState::LoopScope loop_scope(function_state_);
 
   int stmt_pos = peek_position();
   ForInfo for_info(this);
@@ -5344,10 +5357,6 @@ ParserBase<Impl>::ParseForEachStatementWithDeclarations(
     return impl()->NullStatement();
   }
 
-  // Reset the declaration_kind to ensure proper processing during declaration.
-  for_info->parsing_result.descriptor.declaration_kind =
-      DeclarationDescriptor::FOR_EACH;
-
   BlockT init_block = impl()->RewriteForVarInLegacy(*for_info);
 
   auto loop = factory()->NewForEachStatement(for_info->mode, labels, own_labels,
@@ -5529,12 +5538,9 @@ typename ParserBase<Impl>::ForStatementT ParserBase<Impl>::ParseStandardForLoop(
 }
 
 template <typename Impl>
-void ParserBase<Impl>::MarkLoopVariableAsAssigned(
-    Scope* scope, Variable* var,
-    typename DeclarationDescriptor::Kind declaration_kind) {
+void ParserBase<Impl>::MarkLoopVariableAsAssigned(Variable* var) {
   if (!IsLexicalVariableMode(var->mode()) &&
-      (!scope->is_function_scope() ||
-       declaration_kind == DeclarationDescriptor::FOR_EACH)) {
+      function_state_->loop_nesting_depth() > 0) {
     var->set_maybe_assigned();
   }
 }
@@ -5545,6 +5551,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForAwaitStatement(
     ZonePtrList<const AstRawString>* own_labels) {
   // for await '(' ForDeclaration of AssignmentExpression ')'
   DCHECK(is_async_function());
+  typename FunctionState::LoopScope loop_scope(function_state_);
 
   int stmt_pos = peek_position();
 

src/parsing/parser.cc

@@ -1577,7 +1577,7 @@ Statement* Parser::RewriteSwitchStatement(SwitchStatement* switch_statement,
 Block* Parser::RewriteCatchPattern(CatchInfo* catch_info) {
   DCHECK_NOT_NULL(catch_info->pattern);
   DeclarationDescriptor descriptor;
-  descriptor.declaration_kind = DeclarationDescriptor::NORMAL;
+  descriptor.kind = NORMAL_VARIABLE;
   descriptor.mode = VariableMode::kLet;
   descriptor.declaration_pos = catch_info->pattern->position();
   descriptor.initialization_pos = catch_info->pattern->position();
@@ -2788,7 +2788,7 @@ Block* Parser::BuildParameterInitializationBlock(
   int index = 0;
   for (auto parameter : parameters.params) {
     DeclarationDescriptor descriptor;
-    descriptor.declaration_kind = DeclarationDescriptor::PARAMETER;
+    descriptor.kind = PARAMETER_VARIABLE;
     descriptor.mode = VariableMode::kLet;
     descriptor.declaration_pos = parameter->pattern->position();
     // The position that will be used by the AssignmentExpression

src/parsing/pattern-rewriter.cc

@@ -130,8 +130,7 @@ void PatternRewriter::InitializeVariables(
   PatternRewriter rewriter(parser, declaration_descriptor, names,
                            declaration->initializer != nullptr,
                            declaration->initializer_position,
-                           declaration_descriptor->declaration_kind ==
-                                   DeclarationDescriptor::PARAMETER &&
+                           declaration_descriptor->kind == PARAMETER_VARIABLE &&
                                parser->scope()->is_block_scope());
 
   rewriter.RecurseIntoSubpattern(declaration->pattern);
@@ -197,12 +196,8 @@ void PatternRewriter::VisitVariableProxy(VariableProxy* proxy) {
                                         proxy->position());
   }
 
-  VariableKind kind =
-      descriptor_->declaration_kind == DeclarationDescriptor::PARAMETER
-          ? PARAMETER_VARIABLE
-          : NORMAL_VARIABLE;
   parser_->DeclareVariable(
-      proxy, kind, descriptor_->mode,
+      proxy, descriptor_->kind, descriptor_->mode,
       Variable::DefaultInitializationFlag(descriptor_->mode), target_scope,
       descriptor_->declaration_pos);
 
@@ -225,8 +220,7 @@ void PatternRewriter::VisitVariableProxy(VariableProxy* proxy) {
   // If there's no initializer, we're done.
   if (!has_initializer_) return;
 
-  Parser::MarkLoopVariableAsAssigned(var_init_scope, proxy->var(),
-                                     descriptor_->declaration_kind);
+  parser_->MarkLoopVariableAsAssigned(proxy->var());
 }
 
 // When an extra declaration scope needs to be inserted to account for

src/parsing/preparser.cc

@@ -425,8 +425,7 @@ void PreParser::InitializeVariables(
         ReportUnidentifiableError();
         return;
       }
-      MarkLoopVariableAsAssigned(scope(), var,
-                                 declaration_descriptor->declaration_kind);
+      MarkLoopVariableAsAssigned(var);
       // This is only necessary if there is an initializer, but we don't have
       // that information here.  Consequently, the preparser sometimes says
       // maybe-assigned where the parser (correctly) says never-assigned.

test/mjsunit/regress/regress-loop-var-assign-without-block-scope.js

@@ -0,0 +1,14 @@
+// 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: --allow-natives-syntax
+function f() {
+  // Loop with a body that's not wrapped in a block.
+  for (i = 0; i < 2; i++)
+    var x = i, // var x that's assigned on each iteration
+        y = y||(()=>x), // single arrow function that returns x
+        z = (%OptimizeFunctionOnNextCall(y), y()); // optimize y on first iteration
+  return y()
+};
+assertEquals(1, f())