Preparse inner functions (new try)

This is an overly pessimistic approach where PreParser only keeps
track of unresolved variables, but doesn't declare anything. This
will result in context-allocating variables in the outer function
unnecessarily, if the variable names clash with variable names
used by the inner function (even if the variables are not the
same). However, we have been unable to prove that this approach
wouldn't be good enough for the practical purposes.

Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
Keep the context-allocation decision stable when compiling fully eagerly.

Tests which exercise this functionality:
mjsunit/fixed-context-shapes-when-recompiling.js

Design document (chromium):

https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiBAdRnTiSYhazseNHFg/edit?usp=sharing

BUG=

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

src/ast/scopes.cc

@@ -1073,12 +1073,14 @@ void DeclarationScope::AllocateVariables(ParseInfo* info, AnalyzeMode mode) {
   }
 }
 
-bool Scope::AllowsLazyParsing() const {
-  // If we are inside a block scope, we must parse eagerly to find out how
-  // to allocate variables on the block scope. At this point, declarations may
-  // not have yet been parsed.
+bool Scope::AllowsLazyParsingWithoutUnresolvedVariables() const {
+  // If we are inside a block scope, we must find unresolved variables in the
+  // inner scopes to find out how to allocate variables on the block scope. At
+  // this point, declarations may not have yet been parsed.
   for (const Scope* s = this; s != nullptr; s = s->outer_scope_) {
     if (s->is_block_scope()) return false;
+    // TODO(marja): Refactor parsing modes: also add s->is_function_scope()
+    // here.
   }
   return true;
 }
@@ -1178,7 +1180,7 @@ Scope* Scope::GetOuterScopeWithContext() {
 
 Handle<StringSet> DeclarationScope::CollectNonLocals(
     ParseInfo* info, Handle<StringSet> non_locals) {
-  VariableProxy* free_variables = FetchFreeVariables(this, info);
+  VariableProxy* free_variables = FetchFreeVariables(this, true, info);
   for (VariableProxy* proxy = free_variables; proxy != nullptr;
        proxy = proxy->next_unresolved()) {
     non_locals = StringSet::Add(non_locals, proxy->name());
@@ -1191,8 +1193,9 @@ void DeclarationScope::AnalyzePartially(DeclarationScope* migrate_to,
   // Try to resolve unresolved variables for this Scope and migrate those which
   // cannot be resolved inside. It doesn't make sense to try to resolve them in
   // the outer Scopes here, because they are incomplete.
-  for (VariableProxy* proxy = FetchFreeVariables(this); proxy != nullptr;
-       proxy = proxy->next_unresolved()) {
+  for (VariableProxy* proxy =
+           FetchFreeVariables(this, !FLAG_lazy_inner_functions);
+       proxy != nullptr; proxy = proxy->next_unresolved()) {
     DCHECK(!proxy->is_resolved());
     VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
     migrate_to->AddUnresolved(copy);
@@ -1515,6 +1518,29 @@ void Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy) {
   DCHECK(!proxy->is_resolved());
   Variable* var = LookupRecursive(proxy, nullptr);
   ResolveTo(info, proxy, var);
+
+  if (FLAG_lazy_inner_functions) {
+    if (info != nullptr && info->is_native()) return;
+    // Pessimistically force context allocation for all variables to which inner
+    // scope variables could potentially resolve to.
+    Scope* scope = GetClosureScope()->outer_scope_;
+    while (scope != nullptr && scope->scope_info_.is_null()) {
+      var = scope->LookupLocal(proxy->raw_name());
+      if (var != nullptr) {
+        // Since we don't lazy parse inner arrow functions, inner functions
+        // cannot refer to the outer "this".
+        if (!var->is_dynamic() && !var->is_this() &&
+            !var->has_forced_context_allocation()) {
+          var->ForceContextAllocation();
+          var->set_is_used();
+          // We don't know what the (potentially lazy parsed) inner function
+          // does with the variable; pessimistically assume that it's assigned.
+          var->set_maybe_assigned();
+        }
+      }
+      scope = scope->outer_scope_;
+    }
+  }
 }
 
 void Scope::ResolveTo(ParseInfo* info, VariableProxy* proxy, Variable* var) {
@@ -1560,13 +1586,16 @@ void Scope::ResolveVariablesRecursively(ParseInfo* info) {
 }
 
 VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
-                                         ParseInfo* info,
+                                         bool try_to_resolve, ParseInfo* info,
                                          VariableProxy* stack) {
   for (VariableProxy *proxy = unresolved_, *next = nullptr; proxy != nullptr;
        proxy = next) {
     next = proxy->next_unresolved();
     DCHECK(!proxy->is_resolved());
-    Variable* var = LookupRecursive(proxy, max_outer_scope->outer_scope());
+    Variable* var = nullptr;
+    if (try_to_resolve) {
+      var = LookupRecursive(proxy, max_outer_scope->outer_scope());
+    }
     if (var == nullptr) {
       proxy->set_next_unresolved(stack);
       stack = proxy;
@@ -1579,7 +1608,8 @@ VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
   unresolved_ = nullptr;
 
   for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    stack = scope->FetchFreeVariables(max_outer_scope, info, stack);
+    stack =
+        scope->FetchFreeVariables(max_outer_scope, try_to_resolve, info, stack);
   }
 
   return stack;

src/ast/scopes.h

@@ -342,8 +342,9 @@ class Scope: public ZoneObject {
   int StackLocalCount() const;
   int ContextLocalCount() const;
 
-  // Determine if we can parse a function literal in this scope lazily.
-  bool AllowsLazyParsing() const;
+  // Determine if we can parse a function literal in this scope lazily without
+  // caring about the unresolved variables within.
+  bool AllowsLazyParsingWithoutUnresolvedVariables() const;
 
   // The number of contexts between this and scope; zero if this == scope.
   int ContextChainLength(Scope* scope) const;
@@ -538,6 +539,7 @@ class Scope: public ZoneObject {
   // list along the way, so full resolution cannot be done afterwards.
   // If a ParseInfo* is passed, non-free variables will be resolved.
   VariableProxy* FetchFreeVariables(DeclarationScope* max_outer_scope,
+                                    bool try_to_resolve = true,
                                     ParseInfo* info = nullptr,
                                     VariableProxy* stack = nullptr);
 

src/flag-definitions.h

@@ -837,6 +837,7 @@ DEFINE_BOOL(trace_maps, false, "trace map creation")
 // parser.cc
 DEFINE_BOOL(allow_natives_syntax, false, "allow natives syntax")
 DEFINE_BOOL(trace_parse, false, "trace parsing and preparsing")
+DEFINE_BOOL(lazy_inner_functions, true, "enable lazy parsing inner functions")
 
 // simulator-arm.cc, simulator-arm64.cc and simulator-mips.cc
 DEFINE_BOOL(trace_sim, false, "Trace simulator execution")

src/parsing/parser-base.h

@@ -3915,13 +3915,17 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
       // Multiple statement body
       Consume(Token::LBRACE);
       DCHECK_EQ(scope(), formal_parameters.scope);
-      bool is_lazily_parsed = (mode() == PARSE_LAZILY &&
-                               formal_parameters.scope->AllowsLazyParsing());
+      bool is_lazily_parsed =
+          (mode() == PARSE_LAZILY &&
+           formal_parameters.scope
+               ->AllowsLazyParsingWithoutUnresolvedVariables());
+      // TODO(marja): consider lazy-parsing inner arrow functions too. is_this
+      // handling in Scope::ResolveVariable needs to change.
       if (is_lazily_parsed) {
         Scanner::BookmarkScope bookmark(scanner());
         bookmark.Set();
         LazyParsingResult result = impl()->SkipLazyFunctionBody(
-            &materialized_literal_count, &expected_property_count, true,
+            &materialized_literal_count, &expected_property_count, false, true,
             CHECK_OK);
 
         if (formal_parameters.materialized_literals_count > 0) {

src/parsing/parser.h

@@ -484,10 +484,11 @@ class Parser : public ParserBase<Parser> {
   // in order to force the function to be eagerly parsed, after all.
   LazyParsingResult SkipLazyFunctionBody(int* materialized_literal_count,
                                          int* expected_property_count,
-                                         bool may_abort, bool* ok);
+                                         bool is_inner_function, bool may_abort,
+                                         bool* ok);
 
   PreParser::PreParseResult ParseLazyFunctionBodyWithPreParser(
-      SingletonLogger* logger, bool may_abort);
+      SingletonLogger* logger, bool is_inner_function, bool may_abort);
 
   Block* BuildParameterInitializationBlock(
       const ParserFormalParameters& parameters, bool* ok);
@@ -1063,6 +1064,9 @@ class Parser : public ParserBase<Parser> {
   }
 
   // Parser's private field members.
+  friend class DiscardableZoneScope;  // Uses reusable_preparser_.
+  // FIXME(marja): Make reusable_preparser_ always use its own temp Zone (call
+  // DeleteAll after each function), so this won't be needed.
 
   Scanner scanner_;
   PreParser* reusable_preparser_;

src/parsing/preparser.cc

@@ -38,8 +38,10 @@ namespace internal {
 #define CHECK_OK CHECK_OK_VALUE(Statement::Default())
 #define CHECK_OK_VOID CHECK_OK_VALUE(this->Void())
 
-PreParserIdentifier PreParser::GetSymbol() const {
-  switch (scanner()->current_token()) {
+namespace {
+
+PreParserIdentifier GetSymbolHelper(Scanner* scanner) {
+  switch (scanner->current_token()) {
     case Token::ENUM:
       return PreParserIdentifier::Enum();
     case Token::AWAIT:
@@ -55,34 +57,46 @@ PreParserIdentifier PreParser::GetSymbol() const {
     case Token::ASYNC:
       return PreParserIdentifier::Async();
     default:
-      if (scanner()->UnescapedLiteralMatches("eval", 4))
+      if (scanner->UnescapedLiteralMatches("eval", 4))
         return PreParserIdentifier::Eval();
-      if (scanner()->UnescapedLiteralMatches("arguments", 9))
+      if (scanner->UnescapedLiteralMatches("arguments", 9))
         return PreParserIdentifier::Arguments();
-      if (scanner()->UnescapedLiteralMatches("undefined", 9))
+      if (scanner->UnescapedLiteralMatches("undefined", 9))
         return PreParserIdentifier::Undefined();
-      if (scanner()->LiteralMatches("prototype", 9))
+      if (scanner->LiteralMatches("prototype", 9))
         return PreParserIdentifier::Prototype();
-      if (scanner()->LiteralMatches("constructor", 11))
+      if (scanner->LiteralMatches("constructor", 11))
         return PreParserIdentifier::Constructor();
       return PreParserIdentifier::Default();
   }
 }
 
+}  // unnamed namespace
+
+PreParserIdentifier PreParser::GetSymbol() const {
+  PreParserIdentifier symbol = GetSymbolHelper(scanner());
+  if (track_unresolved_variables_) {
+    const AstRawString* result = scanner()->CurrentSymbol(ast_value_factory());
+    DCHECK_NOT_NULL(result);
+    symbol.string_ = result;
+  }
+  return symbol;
+}
+
 PreParser::PreParseResult PreParser::PreParseLazyFunction(
-    LanguageMode language_mode, FunctionKind kind, bool has_simple_parameters,
-    bool parsing_module, ParserRecorder* log, bool may_abort, int* use_counts) {
+    FunctionKind kind, DeclarationScope* function_scope, bool parsing_module,
+    ParserRecorder* log, bool is_inner_function, bool may_abort,
+    int* use_counts) {
   parsing_module_ = parsing_module;
   log_ = log;
   use_counts_ = use_counts;
-  // Lazy functions always have trivial outer scopes (no with/catch scopes).
+  DCHECK(!track_unresolved_variables_);
+  track_unresolved_variables_ = is_inner_function;
+
+  // The caller passes the function_scope which is not yet inserted into the
+  // scope_state_. All scopes above the function_scope are ignored by the
+  // PreParser.
   DCHECK_NULL(scope_state_);
-  DeclarationScope* top_scope = NewScriptScope();
-  FunctionState top_state(&function_state_, &scope_state_, top_scope,
-                          kNormalFunction);
-  scope()->SetLanguageMode(language_mode);
-  DeclarationScope* function_scope = NewFunctionScope(kind);
-  if (!has_simple_parameters) function_scope->SetHasNonSimpleParameters();
   FunctionState function_state(&function_state_, &scope_state_, function_scope,
                                kind);
   DCHECK_EQ(Token::LBRACE, scanner()->current_token());
@@ -90,6 +104,7 @@ PreParser::PreParseResult PreParser::PreParseLazyFunction(
   int start_position = peek_position();
   LazyParsingResult result = ParseLazyFunctionLiteralBody(may_abort, &ok);
   use_counts_ = nullptr;
+  track_unresolved_variables_ = false;
   if (result == kLazyParsingAborted) {
     return kPreParseAbort;
   } else if (stack_overflow()) {
@@ -340,6 +355,21 @@ void PreParser::ParseAsyncArrowSingleExpressionBody(PreParserStatementList body,
   body->Add(PreParserStatement::ExpressionStatement(return_value), zone());
 }
 
+PreParserExpression PreParser::ExpressionFromIdentifier(
+    PreParserIdentifier name, int start_position, int end_position,
+    InferName infer) {
+  if (track_unresolved_variables_) {
+    AstNodeFactory factory(ast_value_factory());
+    // Setting the Zone is necessary because zone_ might be the temp Zone, and
+    // AstValueFactory doesn't know about it.
+    factory.set_zone(zone());
+    DCHECK_NOT_NULL(name.string_);
+    scope()->NewUnresolved(&factory, name.string_, start_position, end_position,
+                           NORMAL_VARIABLE);
+  }
+  return PreParserExpression::FromIdentifier(name);
+}
+
 #undef CHECK_OK
 #undef CHECK_OK_CUSTOM
 

src/parsing/preparser.h

@@ -112,10 +112,12 @@ class PreParserIdentifier {
     kAsyncIdentifier
   };
 
-  explicit PreParserIdentifier(Type type) : type_(type) {}
+  explicit PreParserIdentifier(Type type) : type_(type), string_(nullptr) {}
   Type type_;
-
+  // Only non-nullptr when PreParser.track_unresolved_variables_ is true.
+  const AstRawString* string_;
   friend class PreParserExpression;
+  friend class PreParser;
 };
 
 
@@ -774,7 +776,8 @@ class PreParser : public ParserBase<PreParser> {
             ParserRecorder* log, uintptr_t stack_limit)
       : ParserBase<PreParser>(zone, scanner, stack_limit, NULL,
                               ast_value_factory, log),
-        use_counts_(nullptr) {}
+        use_counts_(nullptr),
+        track_unresolved_variables_(false) {}
 
   // Pre-parse the program from the character stream; returns true on
   // success (even if parsing failed, the pre-parse data successfully
@@ -820,10 +823,10 @@ class PreParser : public ParserBase<PreParser> {
   // keyword and parameters, and have consumed the initial '{'.
   // At return, unless an error occurred, the scanner is positioned before the
   // the final '}'.
-  PreParseResult PreParseLazyFunction(LanguageMode language_mode,
-                                      FunctionKind kind,
-                                      bool has_simple_parameters,
+  PreParseResult PreParseLazyFunction(FunctionKind kind,
+                                      DeclarationScope* function_scope,
                                       bool parsing_module, ParserRecorder* log,
+                                      bool track_unresolved_variables,
                                       bool may_abort, int* use_counts);
 
  private:
@@ -850,9 +853,9 @@ class PreParser : public ParserBase<PreParser> {
       const PreParserFormalParameters& parameters, FunctionKind kind,
       FunctionLiteral::FunctionType function_type, bool* ok);
 
-  V8_INLINE LazyParsingResult
-  SkipLazyFunctionBody(int* materialized_literal_count,
-                       int* expected_property_count, bool may_abort, bool* ok) {
+  V8_INLINE LazyParsingResult SkipLazyFunctionBody(
+      int* materialized_literal_count, int* expected_property_count,
+      bool track_unresolved_variables, bool may_abort, bool* ok) {
     UNREACHABLE();
     return kLazyParsingComplete;
   }
@@ -1304,11 +1307,9 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserExpression::Default();
   }
 
-  V8_INLINE PreParserExpression ExpressionFromIdentifier(
+  PreParserExpression ExpressionFromIdentifier(
       PreParserIdentifier name, int start_position, int end_position,
-      InferName infer = InferName::kYes) {
-    return PreParserExpression::FromIdentifier(name);
-  }
+      InferName infer = InferName::kYes);
 
   V8_INLINE PreParserExpression ExpressionFromString(int pos) {
     if (scanner()->UnescapedLiteralMatches("use strict", 10)) {
@@ -1417,6 +1418,7 @@ class PreParser : public ParserBase<PreParser> {
   // Preparser's private field members.
 
   int* use_counts_;
+  bool track_unresolved_variables_;
 };
 
 PreParserExpression PreParser::SpreadCall(PreParserExpression function,

test/cctest/test-parsing.cc

@@ -38,6 +38,7 @@
 #include "src/ast/ast.h"
 #include "src/compiler.h"
 #include "src/execution.h"
+#include "src/flags.h"
 #include "src/isolate.h"
 #include "src/objects.h"
 #include "src/parsing/parse-info.h"
@@ -3416,7 +3417,14 @@ TEST(InnerAssignment) {
         bool expected = outers[i].assigned || inners[j].assigned;
         CHECK(var != NULL);
         CHECK(var->is_used() || !expected);
-        CHECK((var->maybe_assigned() == i::kMaybeAssigned) == expected);
+        bool is_maybe_assigned = var->maybe_assigned() == i::kMaybeAssigned;
+        if (i::FLAG_lazy_inner_functions) {
+          // If we parse inner functions lazily, allow being pessimistic about
+          // maybe_assigned.
+          CHECK(is_maybe_assigned || (is_maybe_assigned == expected));
+        } else {
+          CHECK(is_maybe_assigned == expected);
+        }
       }
     }
   }

test/mjsunit/debug-function-scopes.js

@@ -87,7 +87,6 @@ var f3 = (function F1(invisible_parameter) {
     var invisible2 = 2;
     return (function F3() {
       var visible2 = 20;
-      var invisible2 = 3;
       return (function () {return visible1 + visible2 + visible1a;});
     })();
   })();

test/mjsunit/debug-scopes.js

@@ -157,20 +157,20 @@ function CheckScopeChainNames(names, exec_state) {
 }
 
 
-// Check that the content of the scope is as expected. For functions just check
-// that there is a function.
-function CheckScopeContent(content, number, exec_state) {
+// Check that the scope contains at least minimum_content. For functions just
+// check that there is a function.
+function CheckScopeContent(minimum_content, number, exec_state) {
   var scope = exec_state.frame().scope(number);
-  var count = 0;
-  for (var p in content) {
+  var minimum_count = 0;
+  for (var p in minimum_content) {
     var property_mirror = scope.scopeObject().property(p);
     assertFalse(property_mirror.isUndefined(), 'property ' + p + ' not found in scope');
-    if (typeof(content[p]) === 'function') {
+    if (typeof(minimum_content[p]) === 'function') {
       assertTrue(property_mirror.value().isFunction());
     } else {
-      assertEquals(content[p], property_mirror.value().value(), 'property ' + p + ' has unexpected value');
+      assertEquals(minimum_content[p], property_mirror.value().value(), 'property ' + p + ' has unexpected value');
     }
-    count++;
+    minimum_count++;
   }
 
   // 'arguments' and might be exposed in the local and closure scope. Just
@@ -186,14 +186,14 @@ function CheckScopeContent(content, number, exec_state) {
   // Temporary variables introduced by the parser have not been materialized.
   assertTrue(scope.scopeObject().property('').isUndefined());
 
-  if (count != scope_size) {
+  if (scope_size < minimum_count) {
     print('Names found in scope:');
     var names = scope.scopeObject().propertyNames();
     for (var i = 0; i < names.length; i++) {
       print(names[i]);
     }
   }
-  assertEquals(count, scope_size);
+  assertTrue(scope_size >= minimum_count);
 
   // Get the debug command processor.
   var dcp = exec_state.debugCommandProcessor("unspecified_running_state");

test/mjsunit/lazy-inner-functions.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: --min-preparse-length 1
+
+(function TestLazyInnerFunctionCallsEval() {
+  var i = (function eager_outer() {
+    var a = 41; // Should be context-allocated
+    function lazy_inner() {
+      return eval("a");
+    }
+    return lazy_inner;
+  })();
+  assertEquals(41, i());
+})();