[parser] avoid complex for-loop desugaring when possible

let/const declarations in "standard" C-style for-loops have
some complex desugaring to accommodate the case where loop
loop variables may be captured. This slows down the baseline
performance of for-loops with let variables.

This change attempts to avoid this desugaring if it's known that
the loop variable is not captured at any point. A side effect of
this change is that let/const loop variables, when not captured
within the loop body, are not necessarily shown in the debugger,
similar to other stack-allocated vars.

BUG=v8:4762, v8:5460
R=marja@chromium.org, adamk@chromium.org, yangguo@chromium.org

Change-Id: I8dbe545a12c086f675972bdba60c94998268311a
Reviewed-on: https://chromium-review.googlesource.com/472247
Commit-Queue: Caitlin Potter <caitp@igalia.com>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44731}

src/parsing/parser-base.h

@@ -451,6 +451,30 @@ class ParserBase {
       next_function_is_likely_called_ = true;
     }
 
+    void RecordFunctionOrEvalCall() { contains_function_or_eval_ = true; }
+    bool contains_function_or_eval() const {
+      return contains_function_or_eval_;
+    }
+
+    class FunctionOrEvalRecordingScope {
+     public:
+      explicit FunctionOrEvalRecordingScope(FunctionState* state)
+          : state_(state) {
+        prev_value_ = state->contains_function_or_eval_;
+        state->contains_function_or_eval_ = false;
+      }
+      ~FunctionOrEvalRecordingScope() {
+        bool found = state_->contains_function_or_eval_;
+        if (!found) {
+          state_->contains_function_or_eval_ = prev_value_;
+        }
+      }
+
+     private:
+      FunctionState* state_;
+      bool prev_value_;
+    };
+
    private:
     void AddDestructuringAssignment(DestructuringAssignment pair) {
       destructuring_assignments_to_rewrite_.Add(pair, scope_->zone());
@@ -485,6 +509,9 @@ class ParserBase {
     bool next_function_is_likely_called_;
     bool previous_function_was_likely_called_;
 
+    // Track if a function or eval occurs within this FunctionState
+    bool contains_function_or_eval_;
+
     friend Impl;
   };
 
@@ -656,6 +683,10 @@ class ParserBase {
     if (target_zone == nullptr) target_zone = zone();
     DeclarationScope* result = new (target_zone)
         DeclarationScope(zone(), scope(), FUNCTION_SCOPE, kind);
+
+    // Record presence of an inner function scope
+    function_state_->RecordFunctionOrEvalCall();
+
     // TODO(verwaest): Move into the DeclarationScope constructor.
     if (!IsArrowFunction(kind)) {
       result->DeclareDefaultFunctionVariables(ast_value_factory());
@@ -1340,6 +1371,7 @@ class ParserBase {
     if (impl()->IsIdentifier(expression) &&
         impl()->IsEval(impl()->AsIdentifier(expression))) {
       scope->RecordEvalCall();
+      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.
@@ -1534,7 +1566,8 @@ ParserBase<Impl>::FunctionState::FunctionState(
       non_patterns_to_rewrite_(0, scope->zone()),
       reported_errors_(16, scope->zone()),
       next_function_is_likely_called_(false),
-      previous_function_was_likely_called_(false) {
+      previous_function_was_likely_called_(false),
+      contains_function_or_eval_(false) {
   *function_state_stack = this;
   if (outer_function_state_) {
     outer_function_state_->previous_function_was_likely_called_ =
@@ -5471,6 +5504,8 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseTryStatement(
 template <typename Impl>
 typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
     ZoneList<const AstRawString*>* labels, bool* ok) {
+  typename FunctionState::FunctionOrEvalRecordingScope recording_scope(
+      function_state_);
   int stmt_pos = peek_position();
   ForInfo for_info(this);
   bool bound_names_are_lexical = false;
@@ -5480,7 +5515,6 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseForStatement(
   Expect(Token::FOR, CHECK_OK);
   Expect(Token::LPAREN, CHECK_OK);
   scope()->set_start_position(scanner()->location().beg_pos);
-  scope()->set_is_hidden();
 
   StatementT init = impl()->NullStatement();
 
@@ -5538,6 +5572,7 @@ typename ParserBase<Impl>::StatementT
 ParserBase<Impl>::ParseForEachStatementWithDeclarations(
     int stmt_pos, ForInfo* for_info, ZoneList<const AstRawString*>* labels,
     bool* ok) {
+  scope()->set_is_hidden();
   // Just one declaration followed by in/of.
   if (for_info->parsing_result.declarations.length() != 1) {
     impl()->ReportMessageAt(for_info->parsing_result.bindings_loc,
@@ -5618,6 +5653,7 @@ typename ParserBase<Impl>::StatementT
 ParserBase<Impl>::ParseForEachStatementWithoutDeclarations(
     int stmt_pos, ExpressionT expression, int lhs_beg_pos, int lhs_end_pos,
     ForInfo* for_info, ZoneList<const AstRawString*>* labels, bool* ok) {
+  scope()->set_is_hidden();
   // Initializer is reference followed by in/of.
   if (!expression->IsArrayLiteral() && !expression->IsObjectLiteral()) {
     expression = impl()->CheckAndRewriteReferenceExpression(
@@ -5701,15 +5737,15 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseStandardForLoop(
     body = ParseStatement(nullptr, CHECK_OK);
   }
 
-  if (bound_names_are_lexical && for_info->bound_names.length() > 0) {
-    auto result = impl()->DesugarLexicalBindingsInForStatement(
+  scope()->set_end_position(scanner()->location().end_pos);
+  inner_scope->set_end_position(scanner()->location().end_pos);
+  if (bound_names_are_lexical && for_info->bound_names.length() > 0 &&
+      (is_resumable() || function_state_->contains_function_or_eval())) {
+    scope()->set_is_hidden();
+    return impl()->DesugarLexicalBindingsInForStatement(
         loop, init, cond, next, body, inner_scope, *for_info, CHECK_OK);
-    scope()->set_end_position(scanner()->location().end_pos);
-    inner_scope->set_end_position(scanner()->location().end_pos);
-    return result;
   }
 
-  scope()->set_end_position(scanner()->location().end_pos);
   Scope* for_scope = scope()->FinalizeBlockScope();
   if (for_scope != nullptr) {
     // Rewrite a for statement of the form
@@ -5733,6 +5769,7 @@ typename ParserBase<Impl>::StatementT ParserBase<Impl>::ParseStandardForLoop(
     BlockT block = factory()->NewBlock(nullptr, 2, false, kNoSourcePosition);
     if (!impl()->IsNullStatement(init)) {
       block->statements()->Add(init, zone());
+      init = impl()->NullStatement();
     }
     block->statements()->Add(loop, zone());
     block->set_scope(for_scope);

test/cctest/interpreter/test-bytecode-generator.cc

@@ -2507,6 +2507,108 @@ TEST(ForAwaitOf) {
   i::FLAG_harmony_async_iteration = old_flag;
 }
 
+TEST(StandardForLoop) {
+  InitializedIgnitionHandleScope scope;
+  BytecodeExpectationsPrinter printer(CcTest::isolate());
+  printer.set_wrap(false);
+  printer.set_test_function_name("f");
+
+  const char* snippets[] = {
+      "function f() {\n"
+      "  for (let x = 0; x < 10; ++x) { let y = x; }\n"
+      "}\n"
+      "f();\n",
+
+      "function f() {\n"
+      "  for (let x = 0; x < 10; ++x) { eval('1'); }\n"
+      "}\n"
+      "f();\n",
+
+      "function f() {\n"
+      "  for (let x = 0; x < 10; ++x) { (function() { return x; })(); }\n"
+      "}\n"
+      "f();\n",
+
+      "function f() {\n"
+      "  for (let { x, y } = { x: 0, y: 3 }; y > 0; --y) { let z = x + y; }\n"
+      "}\n"
+      "f();\n",
+
+      "function* f() {\n"
+      "  for (let x = 0; x < 10; ++x) { let y = x; }\n"
+      "}\n"
+      "f();\n",
+
+      "function* f() {\n"
+      "  for (let x = 0; x < 10; ++x) yield x;\n"
+      "}\n"
+      "f();\n",
+
+      "async function f() {\n"
+      "  for (let x = 0; x < 10; ++x) { let y = x; }\n"
+      "}\n"
+      "f();\n",
+
+      "async function f() {\n"
+      "  for (let x = 0; x < 10; ++x) await x;\n"
+      "}\n"
+      "f();\n"};
+
+  CHECK(CompareTexts(BuildActual(printer, snippets),
+                     LoadGolden("StandardForLoop.golden")));
+}
+
+TEST(ForOfLoop) {
+  InitializedIgnitionHandleScope scope;
+  BytecodeExpectationsPrinter printer(CcTest::isolate());
+  printer.set_wrap(false);
+  printer.set_test_function_name("f");
+
+  const char* snippets[] = {
+      "function f(arr) {\n"
+      "  for (let x of arr) { let y = x; }\n"
+      "}\n"
+      "f([1, 2, 3]);\n",
+
+      "function f(arr) {\n"
+      "  for (let x of arr) { eval('1'); }\n"
+      "}\n"
+      "f([1, 2, 3]);\n",
+
+      "function f(arr) {\n"
+      "  for (let x of arr) { (function() { return x; })(); }\n"
+      "}\n"
+      "f([1, 2, 3]);\n",
+
+      "function f(arr) {\n"
+      "  for (let { x, y } of arr) { let z = x + y; }\n"
+      "}\n"
+      "f([{ x: 0, y: 3 }, { x: 1, y: 9 }, { x: -12, y: 17 }]);\n",
+
+      "function* f(arr) {\n"
+      "  for (let x of arr) { let y = x; }\n"
+      "}\n"
+      "f([1, 2, 3]);\n",
+
+      "function* f(arr) {\n"
+      "  for (let x of arr) yield x;\n"
+      "}\n"
+      "f([1, 2, 3]);\n",
+
+      "async function f(arr) {\n"
+      "  for (let x of arr) { let y = x; }\n"
+      "}\n"
+      "f([1, 2, 3]);\n",
+
+      "async function f(arr) {\n"
+      "  for (let x of arr) await x;\n"
+      "}\n"
+      "f([1, 2, 3]);\n"};
+
+  CHECK(CompareTexts(BuildActual(printer, snippets),
+                     LoadGolden("ForOfLoop.golden")));
+}
+
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8

test/cctest/test-parsing.cc

@@ -10161,3 +10161,188 @@ TEST(AsyncGeneratorErrors) {
   RunParserSyncTest(context_data, statement_data, kError, NULL, 0, always_flags,
                     arraysize(always_flags));
 }
+
+TEST(LexicalLoopVariable) {
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::HandleScope scope(isolate);
+  LocalContext env;
+  typedef std::function<void(const i::ParseInfo& info, i::DeclarationScope*)>
+      TestCB;
+  auto TestProgram = [isolate](const char* program, TestCB test) {
+    i::Factory* const factory = isolate->factory();
+    i::Handle<i::String> source =
+        factory->NewStringFromUtf8(i::CStrVector(program)).ToHandleChecked();
+    i::Handle<i::Script> script = factory->NewScript(source);
+    i::ParseInfo info(script);
+
+    info.set_allow_lazy_parsing(false);
+    CHECK(i::parsing::ParseProgram(&info, isolate));
+    CHECK(i::Rewriter::Rewrite(&info, isolate));
+    i::DeclarationScope::Analyze(&info, isolate, i::AnalyzeMode::kRegular);
+    CHECK(info.literal() != NULL);
+
+    i::DeclarationScope* script_scope = info.literal()->scope();
+    CHECK(script_scope->is_script_scope());
+
+    test(info, script_scope);
+  };
+
+  // Check `let` loop variables is a stack local when not captured by an eval
+  // or closure within the area of the loop body.
+  const char* local_bindings[] = {
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < 10; ++loop_var) {"
+      "  }"
+      "  eval('0');"
+      "}",
+
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < 10; ++loop_var) {"
+      "  }"
+      "  function foo() {}"
+      "  foo();"
+      "}",
+  };
+  for (const char* source : local_bindings) {
+    TestProgram(source, [=](const i::ParseInfo& info, i::DeclarationScope* s) {
+      i::Scope* fn = s->inner_scope();
+      CHECK(fn->is_function_scope());
+
+      i::Scope* loop_block = fn->inner_scope();
+      if (loop_block->is_function_scope()) loop_block = loop_block->sibling();
+      CHECK(loop_block->is_block_scope());
+
+      const i::AstRawString* var_name =
+          info.ast_value_factory()->GetOneByteString("loop_var");
+      i::Variable* loop_var = loop_block->LookupLocal(var_name);
+      CHECK_NOT_NULL(loop_var);
+      CHECK(loop_var->IsStackLocal());
+      CHECK_EQ(loop_block->ContextLocalCount(), 0);
+      CHECK_EQ(loop_block->inner_scope()->ContextLocalCount(), 0);
+    });
+  }
+
+  // Check `let` loop variable is not a stack local, and is duplicated in the
+  // loop body to ensure capturing can work correctly.
+  // In this version of the test, the inner loop block's duplicate `loop_var`
+  // binding is not captured, and is a local.
+  const char* context_bindings1[] = {
+      "function loop() {"
+      "  for (let loop_var = eval('0'); loop_var < 10; ++loop_var) {"
+      "  }"
+      "}",
+
+      "function loop() {"
+      "  for (let loop_var = (() => (loop_var, 0))(); loop_var < 10;"
+      "       ++loop_var) {"
+      "  }"
+      "}"};
+  for (const char* source : context_bindings1) {
+    TestProgram(source, [=](const i::ParseInfo& info, i::DeclarationScope* s) {
+      i::Scope* fn = s->inner_scope();
+      CHECK(fn->is_function_scope());
+
+      i::Scope* loop_block = fn->inner_scope();
+      CHECK(loop_block->is_block_scope());
+
+      const i::AstRawString* var_name =
+          info.ast_value_factory()->GetOneByteString("loop_var");
+      i::Variable* loop_var = loop_block->LookupLocal(var_name);
+      CHECK_NOT_NULL(loop_var);
+      CHECK(loop_var->IsContextSlot());
+      CHECK_EQ(loop_block->ContextLocalCount(), 1);
+
+      i::Variable* loop_var2 = loop_block->inner_scope()->LookupLocal(var_name);
+      CHECK_NE(loop_var, loop_var2);
+      CHECK(loop_var2->IsStackLocal());
+      CHECK_EQ(loop_block->inner_scope()->ContextLocalCount(), 0);
+    });
+  }
+
+  // Check `let` loop variable is not a stack local, and is duplicated in the
+  // loop body to ensure capturing can work correctly.
+  // In this version of the test, the inner loop block's duplicate `loop_var`
+  // binding is captured, and must be context allocated.
+  const char* context_bindings2[] = {
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < 10; ++loop_var) {"
+      "    eval('0');"
+      "  }"
+      "}",
+
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < eval('10'); ++loop_var) {"
+      "  }"
+      "}",
+
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < 10; eval('++loop_var')) {"
+      "  }"
+      "}",
+  };
+
+  for (const char* source : context_bindings2) {
+    TestProgram(source, [=](const i::ParseInfo& info, i::DeclarationScope* s) {
+      i::Scope* fn = s->inner_scope();
+      CHECK(fn->is_function_scope());
+
+      i::Scope* loop_block = fn->inner_scope();
+      CHECK(loop_block->is_block_scope());
+
+      const i::AstRawString* var_name =
+          info.ast_value_factory()->GetOneByteString("loop_var");
+      i::Variable* loop_var = loop_block->LookupLocal(var_name);
+      CHECK_NOT_NULL(loop_var);
+      CHECK(loop_var->IsContextSlot());
+      CHECK_EQ(loop_block->ContextLocalCount(), 1);
+
+      i::Variable* loop_var2 = loop_block->inner_scope()->LookupLocal(var_name);
+      CHECK_NE(loop_var, loop_var2);
+      CHECK(loop_var2->IsContextSlot());
+      CHECK_EQ(loop_block->inner_scope()->ContextLocalCount(), 1);
+    });
+  }
+
+  // Similar to the above, but the first block scope's variables are not
+  // captured due to the closure occurring in a nested scope.
+  const char* context_bindings3[] = {
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < 10; ++loop_var) {"
+      "    (() => loop_var)();"
+      "  }"
+      "}",
+
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < (() => (loop_var, 10))();"
+      "       ++loop_var) {"
+      "  }"
+      "}",
+
+      "function loop() {"
+      "  for (let loop_var = 0; loop_var < 10; (() => ++loop_var)()) {"
+      "  }"
+      "}",
+  };
+
+  for (const char* source : context_bindings3) {
+    TestProgram(source, [=](const i::ParseInfo& info, i::DeclarationScope* s) {
+      i::Scope* fn = s->inner_scope();
+      CHECK(fn->is_function_scope());
+
+      i::Scope* loop_block = fn->inner_scope();
+      CHECK(loop_block->is_block_scope());
+
+      const i::AstRawString* var_name =
+          info.ast_value_factory()->GetOneByteString("loop_var");
+      i::Variable* loop_var = loop_block->LookupLocal(var_name);
+      CHECK_NOT_NULL(loop_var);
+      CHECK(loop_var->IsStackLocal());
+      CHECK_EQ(loop_block->ContextLocalCount(), 0);
+
+      i::Variable* loop_var2 = loop_block->inner_scope()->LookupLocal(var_name);
+      CHECK_NE(loop_var, loop_var2);
+      CHECK(loop_var2->IsContextSlot());
+      CHECK_EQ(loop_block->inner_scope()->ContextLocalCount(), 1);
+    });
+  }
+}

test/debugger/debug/debug-scopes.js

@@ -1144,7 +1144,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeChainPositions(
-      [{start: 52, end: 111}, {start: 22, end: 145}, {}, {}], exec_state);
+      [{start: 42, end: 111}, {start: 22, end: 145}, {}, {}], exec_state);
 }
 eval(code3);
 EndTest();
@@ -1165,7 +1165,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeChainPositions([{start: 66, end: 147},
-                            {start: 52, end: 147},
+                            {start: 42, end: 147},
                             {start: 22, end: 181},
                             {}, {}], exec_state);
 }

test/debugger/debug/harmony/modules-debug-scopes1.js

@@ -751,7 +751,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Module,
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
-  CheckScopeChainPositions([{start: 52, end: 111}, {start: 22, end: 145}],
+  CheckScopeChainPositions([{start: 42, end: 111}, {start: 22, end: 145}],
                            exec_state);
 }
 eval(code3);
@@ -774,7 +774,7 @@ listener_delegate = function(exec_state) {
                    debug.ScopeType.Script,
                    debug.ScopeType.Global], exec_state);
   CheckScopeChainPositions([{start: 66, end: 147},
-                            {start: 52, end: 147},
+                            {start: 42, end: 147},
                             {start: 22, end: 181}], exec_state);
 }
 eval(code4);