Reland of [parsing] Fix maybe-assigned for loop variables. (patchset #1 id:1...

Reland of [parsing] Fix maybe-assigned for loop variables. (patchset #1 id:1 of https://codereview.chromium.org/2679263002/ )

Reason for revert:
False alarm, bot hiccup

Original issue's description:
> Revert of [parsing] Fix maybe-assigned for loop variables. (patchset #3 id:40001 of https://codereview.chromium.org/2673403003/ )
>
> Reason for revert:
> Speculative revert because of https://codereview.chromium.org/2679163002/.
>
> Original issue's description:
> > [parsing] Fix maybe-assigned for loop variables.
> >
> > Due to hoisting, the value of a 'var'-declared variable may actually change even
> > if the code contains only the "initial" assignment, namely when that assignment
> > occurs inside a loop.  For example:
> >
> >   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 "declaration" does not
> > syntactically occur in the function scope.  (In the example above, it
> > occurs in a block scope.)
> >
> > BUG=v8:5636
> >
> > Review-Url: https://codereview.chromium.org/2673403003
> > Cr-Commit-Position: refs/heads/master@{#42989}
> > Committed: https://chromium.googlesource.com/v8/v8/+/a33fcd663b28b8846e12b97c30d6e7d837767f86
>
> TBR=marja@chromium.org,adamk@chromium.org,neis@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=v8:5636
>
> Review-Url: https://codereview.chromium.org/2679263002
> Cr-Commit-Position: refs/heads/master@{#43010}
> Committed: https://chromium.googlesource.com/v8/v8/+/f3ae5ccf57690d8c2d87c4fe1d10b103ad6a4ab3

TBR=marja@chromium.org,adamk@chromium.org,neis@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:5636

Review-Url: https://codereview.chromium.org/2686663002
Cr-Commit-Position: refs/heads/master@{#43013}

src/ast/scopes.h

@@ -181,6 +181,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
                             bool* sloppy_mode_block_scope_function_redefinition,
                             bool* ok);
 
+  // The return value is meaningful only if FLAG_preparser_scope_analysis is on.
   Variable* DeclareVariableName(const AstRawString* name, VariableMode mode);
 
   // Declarations list.

src/parsing/parser-base.h

@@ -1347,6 +1347,24 @@ class ParserBase {
     return expression->IsObjectLiteral() || expression->IsArrayLiteral();
   }
 
+  // Due to hoisting, the value of a 'var'-declared variable may actually change
+  // even if the code contains only the "initial" assignment, namely when that
+  // assignment occurs inside a loop.  For example:
+  //
+  //   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.
+  //
+  static void MarkLoopVariableAsAssigned(Scope* scope, Variable* var);
+
   // Keep track of eval() calls since they disable all local variable
   // optimizations. This checks if expression is an eval call, and if yes,
   // forwards the information to scope.
@@ -5698,8 +5716,12 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseStandardForLoop(
   return loop;
 }
 
-#undef CHECK_OK
-#undef CHECK_OK_CUSTOM
+template <typename Impl>
+void ParserBase<Impl>::MarkLoopVariableAsAssigned(Scope* scope, Variable* var) {
+  if (!IsLexicalVariableMode(var->mode()) && !scope->is_function_scope()) {
+    var->set_maybe_assigned();
+  }
+}
 
 template <typename Impl>
 void ParserBase<Impl>::ObjectLiteralChecker::CheckDuplicateProto(
@@ -5751,6 +5773,8 @@ void ParserBase<Impl>::ClassLiteralChecker::CheckClassMethodName(
   }
 }
 
+#undef CHECK_OK
+#undef CHECK_OK_CUSTOM
 #undef CHECK_OK_VOID
 
 }  // namespace internal

src/parsing/pattern-rewriter.cc

@@ -177,6 +177,8 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
   // If there's no initializer, we're done.
   if (value == nullptr) return;
 
+  MarkLoopVariableAsAssigned(var_init_scope, proxy->var());
+
   // A declaration of the form:
   //
   //    var v = x;

src/parsing/preparser.cc

@@ -310,8 +310,14 @@ void PreParser::DeclareAndInitializeVariables(
     DCHECK(track_unresolved_variables_);
     for (auto variable : *(declaration->pattern.variables_)) {
       declaration_descriptor->scope->RemoveUnresolved(variable);
-      scope()->DeclareVariableName(variable->raw_name(),
-                                   declaration_descriptor->mode);
+      Variable* var = scope()->DeclareVariableName(
+          variable->raw_name(), declaration_descriptor->mode);
+      if (FLAG_preparser_scope_analysis) {
+        MarkLoopVariableAsAssigned(declaration_descriptor->scope, 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.
+      }
       if (names) {
         names->Add(variable->raw_name(), zone());
       }

test/cctest/scope-test-helper.h

@@ -32,7 +32,7 @@ class ScopeTestHelper {
   }
 
   static void CompareScopeToData(Scope* scope, const PreParsedScopeData* data,
-                                 size_t& index) {
+                                 size_t& index, bool precise_maybe_assigned) {
     CHECK_EQ(data->backing_store_[index++], scope->scope_type());
     CHECK_EQ(data->backing_store_[index++], scope->start_position());
     CHECK_EQ(data->backing_store_[index++], scope->end_position());
@@ -70,14 +70,19 @@ class ScopeTestHelper {
         }
 #endif
         CHECK_EQ(data->backing_store_[index++], local->location());
-        CHECK_EQ(data->backing_store_[index++], local->maybe_assigned());
+        if (precise_maybe_assigned) {
+          CHECK_EQ(data->backing_store_[index++], local->maybe_assigned());
+        } else {
+          STATIC_ASSERT(kMaybeAssigned > kNotAssigned);
+          CHECK_GE(data->backing_store_[index++], local->maybe_assigned());
+        }
       }
     }
 
     for (Scope* inner = scope->inner_scope(); inner != nullptr;
          inner = inner->sibling()) {
       if (!ScopeTreeIsHidden(inner)) {
-        CompareScopeToData(inner, data, index);
+        CompareScopeToData(inner, data, index, precise_maybe_assigned);
       }
     }
   }

test/mjsunit/regress/regress-5636-1.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 --function-context-specialization
+
+function f(n) {
+  var a = [];
+  function g() { return x }
+  for (var i = 0; i < n; ++i) {
+    var x = i;
+    a[i] = g;
+    %OptimizeFunctionOnNextCall(g);
+    g();
+  }
+  return a;
+}
+var a = f(3);
+assertEquals(3, a.length);
+assertEquals(2, a[0]());
+assertEquals(2, a[1]());
+assertEquals(2, a[2]());

test/mjsunit/regress/regress-5636-2.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
+
+function f(n) {
+  "use asm";
+  var a = [];
+  function g() { return x }
+  for (var i = 0; i < n; ++i) {
+    var x = i;
+    a[i] = g;
+    %OptimizeFunctionOnNextCall(g);
+    g();
+  }
+  return a;
+}
+var a = f(3);
+assertEquals(3, a.length);
+assertEquals(2, a[0]());
+assertEquals(2, a[1]());
+assertEquals(2, a[2]());