PreParser: track variable declarations and parameters

This makes the context allocation less pessimistic in the following cases:

function outer() {
  var a; // Won't be context allocated
  function inner1() { var a; a; }
  function inner2(a) { a; }
  function inner3([a]) { a; }
  function inner4({ a: b}) { a; }
}

BUG=v8:5501

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

src/ast/ast-traversal-visitor.h

@@ -288,7 +288,7 @@ void AstTraversalVisitor<Subclass>::VisitFunctionLiteral(
   DeclarationScope* scope = expr->scope();
   RECURSE_EXPRESSION(VisitDeclarations(scope->declarations()));
   // A lazily parsed function literal won't have a body.
-  if (expr->scope()->is_lazily_parsed()) return;
+  if (expr->scope()->was_lazily_parsed()) return;
   RECURSE_EXPRESSION(VisitStatements(expr->body()));
 }
 

src/ast/scopes.h

@@ -34,6 +34,10 @@ class VariableMap: public ZoneHashMap {
                     MaybeAssignedFlag maybe_assigned_flag = kNotAssigned,
                     bool* added = nullptr);
 
+  // Records that "name" exists but doesn't create a Variable. Useful for
+  // preparsing.
+  void DeclareName(Zone* zone, const AstRawString* name);
+
   Variable* Lookup(const AstRawString* name);
   void Remove(Variable* var);
   void Add(Zone* zone, Variable* var);
@@ -157,6 +161,8 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
                             bool* sloppy_mode_block_scope_function_redefinition,
                             bool* ok);
 
+  void DeclareVariableName(const AstRawString* name, VariableMode mode);
+
   // Declarations list.
   ThreadedList<Declaration>* declarations() { return &decls_; }
 
@@ -539,7 +545,6 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(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);
 
@@ -576,6 +581,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   void SetDefaults();
 
   friend class DeclarationScope;
+  friend class ScopeTestHelper;
 };
 
 class DeclarationScope : public Scope {
@@ -612,7 +618,15 @@ class DeclarationScope : public Scope {
                                         IsClassConstructor(function_kind())));
   }
 
-  bool is_lazily_parsed() const { return is_lazily_parsed_; }
+  bool was_lazily_parsed() const { return was_lazily_parsed_; }
+
+#ifdef DEBUG
+  void set_is_being_lazily_parsed(bool is_being_lazily_parsed) {
+    is_being_lazily_parsed_ = is_being_lazily_parsed;
+  }
+  bool is_being_lazily_parsed() const { return is_being_lazily_parsed_; }
+#endif
+
   bool ShouldEagerCompile() const;
   void set_should_eager_compile();
 
@@ -815,7 +829,11 @@ class DeclarationScope : public Scope {
   // This scope uses "super" property ('super.foo').
   bool scope_uses_super_property_ : 1;
   bool should_eager_compile_ : 1;
-  bool is_lazily_parsed_ : 1;
+  // Set to true after we have finished lazy parsing the scope.
+  bool was_lazily_parsed_ : 1;
+#if DEBUG
+  bool is_being_lazily_parsed_ : 1;
+#endif
 
   // Parameter list in source order.
   ZoneList<Variable*> params_;

src/parsing/preparser.cc

@@ -91,6 +91,9 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   use_counts_ = use_counts;
   DCHECK(!track_unresolved_variables_);
   track_unresolved_variables_ = is_inner_function;
+#ifdef DEBUG
+  function_scope->set_is_being_lazily_parsed(true);
+#endif
 
   // In the preparser, we use the function literal ids to count how many
   // FunctionLiterals were encountered. The PreParser doesn't actually persist
@@ -133,6 +136,7 @@ PreParser::PreParseResult PreParser::PreParseFunction(
       &formals, has_duplicate_parameters, may_abort, ok);
   use_counts_ = nullptr;
   track_unresolved_variables_ = false;
+
   if (result == kLazyParsingAborted) {
     return kPreParseAbort;
   } else if (stack_overflow()) {
@@ -296,7 +300,10 @@ void PreParser::DeclareAndInitializeVariables(
        var + initializer -> RemoveUnresolved followed by NewUnresolved
        let / const + initializer -> RemoveUnresolved
     */
-
+    Scope* scope = declaration_descriptor->hoist_scope;
+    if (scope == nullptr) {
+      scope = this->scope();
+    }
     if (declaration->initializer.IsEmpty() ||
         (declaration_descriptor->mode == VariableMode::LET ||
          declaration_descriptor->mode == VariableMode::CONST)) {
@@ -304,6 +311,9 @@ void PreParser::DeclareAndInitializeVariables(
         declaration_descriptor->scope->RemoveUnresolved(identifier);
       }
     }
+    for (auto identifier : *(declaration->pattern.identifiers_)) {
+      scope->DeclareVariableName(identifier, declaration_descriptor->mode);
+    }
   }
 }
 

src/parsing/preparser.h

@@ -149,9 +149,11 @@ class PreParserExpression {
     return PreParserExpression(TypeField::encode(kExpression));
   }
 
-  static PreParserExpression Assignment() {
+  static PreParserExpression Assignment(
+      ZoneList<const AstRawString*>* identifiers = nullptr) {
     return PreParserExpression(TypeField::encode(kExpression) |
-                               ExpressionTypeField::encode(kAssignment));
+                                   ExpressionTypeField::encode(kAssignment),
+                               identifiers);
   }
 
   static PreParserExpression ObjectLiteral(
@@ -604,7 +606,8 @@ class PreParserFactory {
                                     PreParserExpression left,
                                     PreParserExpression right,
                                     int pos) {
-    return PreParserExpression::Assignment();
+    // For tracking identifiers for parameters with a default value.
+    return PreParserExpression::Assignment(left.identifiers_);
   }
   PreParserExpression NewYield(PreParserExpression generator_object,
                                PreParserExpression expression, int pos,
@@ -746,10 +749,17 @@ class PreParserFactory {
 
 
 struct PreParserFormalParameters : FormalParametersBase {
+  struct Parameter : public ZoneObject {
+    explicit Parameter(PreParserExpression pattern) : pattern(pattern) {}
+    Parameter** next() { return &next_parameter; }
+    Parameter* const* next() const { return &next_parameter; }
+    PreParserExpression pattern;
+    Parameter* next_parameter = nullptr;
+  };
   explicit PreParserFormalParameters(DeclarationScope* scope)
       : FormalParametersBase(scope) {}
 
-  void* params = nullptr;  // Dummy
+  ThreadedList<Parameter> params;
 };
 
 
@@ -1440,14 +1450,30 @@ class PreParser : public ParserBase<PreParser> {
                                     PreParserExpression initializer,
                                     int initializer_end_position,
                                     bool is_rest) {
+    if (track_unresolved_variables_) {
+      DCHECK(FLAG_lazy_inner_functions);
+      parameters->params.Add(new (zone())
+                                 PreParserFormalParameters::Parameter(pattern));
+    }
     parameters->UpdateArityAndFunctionLength(!initializer.IsEmpty(), is_rest);
   }
 
-  V8_INLINE void DeclareFormalParameters(DeclarationScope* scope,
-                                         void* parameters) {
+  V8_INLINE void DeclareFormalParameters(
+      DeclarationScope* scope,
+      const ThreadedList<PreParserFormalParameters::Parameter>& parameters) {
     if (!classifier()->is_simple_parameter_list()) {
       scope->SetHasNonSimpleParameters();
     }
+    if (track_unresolved_variables_) {
+      DCHECK(FLAG_lazy_inner_functions);
+      for (auto parameter : parameters) {
+        if (parameter->pattern.identifiers_ != nullptr) {
+          for (auto i : *parameter->pattern.identifiers_) {
+            scope->DeclareVariableName(i, VAR);
+          }
+        }
+      }
+    }
   }
 
   V8_INLINE void DeclareArrowFunctionFormalParameters(
@@ -1456,6 +1482,8 @@ class PreParser : public ParserBase<PreParser> {
       bool* ok) {
     // TODO(wingo): Detect duplicated identifiers in paramlists.  Detect
     // parameter lists that are too long.
+    // FIXME(marja): Add code to declare arrow function parameters to allocate
+    // less pessimistically.
   }
 
   V8_INLINE void ReindexLiterals(const PreParserFormalParameters& parameters) {}

test/cctest/test-parsing.cc

@@ -8403,3 +8403,143 @@ TEST(ArgumentsRedeclaration) {
     RunParserSyncTest(context_data, data, kSuccess);
   }
 }
+
+namespace v8 {
+namespace internal {
+
+class ScopeTestHelper {
+ public:
+  static bool MustAllocateInContext(Variable* var) {
+    return var->scope()->MustAllocateInContext(var);
+  }
+};
+}  // namespace internal
+}  // namespace v8
+
+// Test that lazily parsed inner functions don't result in overly pessimistic
+// context allocations.
+TEST(NoPessimisticContextAllocation) {
+  i::FLAG_lazy_inner_functions = true;
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::Factory* factory = isolate->factory();
+  i::HandleScope scope(isolate);
+  LocalContext env;
+
+  const char* prefix = "(function outer() { var my_var; ";
+  const char* suffix = " })();";
+  int prefix_len = Utf8LengthHelper(prefix);
+  int suffix_len = Utf8LengthHelper(suffix);
+
+  struct {
+    const char* source;
+    bool ctxt_allocate;
+  } inners[] = {
+      // Context allocating because we need to:
+      {"function inner() { my_var; }", true},
+      {"function inner() { eval(\"foo\"); }", true},
+      {"function inner() { function inner2() { my_var; } }", true},
+      {"function inner() { function inner2() { eval(\"foo\"); } }", true},
+      {"function inner() { var {my_var : a} = {my_var}; }", true},
+      {"function inner() { let {my_var : a} = {my_var}; }", true},
+      {"function inner() { const {my_var : a} = {my_var}; }", true},
+      // No pessimistic context allocation:
+      {"function inner() { var my_var; my_var; }", false},
+      {"function inner() { var my_var; }", false},
+      {"function inner() { let my_var; my_var; }", false},
+      {"function inner() { let my_var; }", false},
+      {"function inner() { const my_var = 0; my_var; }", false},
+      {"function inner() { const my_var = 0; }", false},
+      {"function inner() { var [a, my_var] = [1, 2]; my_var; }", false},
+      {"function inner() { let [a, my_var] = [1, 2]; my_var; }", false},
+      {"function inner() { const [a, my_var] = [1, 2]; my_var; }", false},
+      {"function inner() { var {a: my_var} = {a: 3}; my_var; }", false},
+      {"function inner() { let {a: my_var} = {a: 3}; my_var; }", false},
+      {"function inner() { const {a: my_var} = {a: 3}; my_var; }", false},
+      {"function inner() { var {my_var} = {my_var: 3}; my_var; }", false},
+      {"function inner() { let {my_var} = {my_var: 3}; my_var; }", false},
+      {"function inner() { const {my_var} = {my_var: 3}; my_var; }", false},
+      {"function inner(my_var) { my_var; }", false},
+      {"function inner(my_var) { }", false},
+      {"function inner(...my_var) { my_var; }", false},
+      {"function inner(...my_var) { }", false},
+      {"function inner([a, my_var, b]) { my_var; }", false},
+      {"function inner([a, my_var, b]) { }", false},
+      {"function inner({x: my_var}) { my_var; }", false},
+      {"function inner({x: my_var}) { }", false},
+      {"function inner({my_var}) { my_var; }", false},
+      {"function inner({my_var}) { }", false},
+      {"function inner() { function inner2(my_var) { my_var; } }", false},
+      {"function inner() { function inner2(my_var) { } }", false},
+      {"function inner() { function inner2(...my_var) { my_var; } }", false},
+      {"function inner() { function inner2(...my_var) { } }", false},
+      {"function inner() { function inner2([a, my_var, b]) { my_var; } }",
+       false},
+      {"function inner() { function inner2([a, my_var, b]) { } }", false},
+      {"function inner() { function inner2({x: my_var}) { my_var; } }", false},
+      {"function inner() { function inner2({x: my_var}) { } }", false},
+      {"function inner() { function inner2({my_var}) { my_var; } }", false},
+      {"function inner() { function inner2({my_var}) { } }", false},
+      {"my_var =>  my_var; ", false},
+      {"my_var => { }", false},
+      {"(...my_var) =>  my_var;", false},
+      {"(...my_var) => { }", false},
+      {"([a, my_var, b]) => my_var;", false},
+      {"([a, my_var, b]) => { }", false},
+      {"({x: my_var}) => my_var;", false},
+      {"({x: my_var}) => { }", false},
+      {"({my_var}) => my_var;", false},
+      {"({my_var}) => { }", false},
+      {"function inner() { try { } catch (my_var) { } }", false},
+      {"function inner() { class my_var {}; }", false},
+      // In the following cases we still context allocate pessimistically:
+      {"function inner() { function my_var() {} my_var; }", true},
+      {"function inner() { if (true) { function my_var() {} }  my_var; }",
+       true},
+      {"function inner() { try { } catch (my_var) { my_var; } }", true},
+      {"function inner() { for (my_var of {}) { my_var; } }", true},
+      {"function inner() { for (my_var of {}) { } }", true},
+      {"function inner() { for (my_var in []) { my_var; } }", true},
+      {"function inner() { for (my_var in []) { } }", true},
+      {"function inner() { my_var =>  my_var; }", true},
+      {"function inner() { my_var => { }}", true},
+      {"function inner() { (...my_var) =>  my_var;}", true},
+      {"function inner() { (...my_var) => { }}", true},
+      {"function inner() { ([a, my_var, b]) => my_var;}", true},
+      {"function inner() { ([a, my_var, b]) => { }}", true},
+      {"function inner() { ({x: my_var}) => my_var;}", true},
+      {"function inner() { ({x: my_var}) => { }}", true},
+      {"function inner() { ({my_var}) => my_var;}", true},
+      {"function inner() { ({my_var}) => { }}", true},
+      {"function inner() { class my_var {}; my_var }", true},
+  };
+
+  for (unsigned i = 0; i < arraysize(inners); ++i) {
+    const char* inner = inners[i].source;
+    int inner_len = Utf8LengthHelper(inner);
+    int len = prefix_len + inner_len + suffix_len;
+    i::ScopedVector<char> program(len + 1);
+    i::SNPrintF(program, "%s%s%s", prefix, inner, suffix);
+    i::Handle<i::String> source =
+        factory->InternalizeUtf8String(program.start());
+    source->PrintOn(stdout);
+    printf("\n");
+
+    i::Handle<i::Script> script = factory->NewScript(source);
+    i::Zone zone(isolate->allocator(), ZONE_NAME);
+    i::ParseInfo info(&zone, script);
+
+    CHECK(i::parsing::ParseProgram(&info));
+    CHECK(i::Compiler::Analyze(&info));
+    CHECK(info.literal() != NULL);
+
+    i::Scope* scope = info.literal()->scope()->inner_scope();
+    DCHECK_NOT_NULL(scope);
+    DCHECK_NULL(scope->sibling());
+    DCHECK(scope->is_function_scope());
+    const i::AstRawString* var_name =
+        info.ast_value_factory()->GetOneByteString("my_var");
+    i::Variable* var = scope->Lookup(var_name);
+    CHECK_EQ(inners[i].ctxt_allocate,
+             i::ScopeTestHelper::MustAllocateInContext(var));
+  }
+}

test/mjsunit/lazy-inner-functions.js

@@ -2,11 +2,26 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --lazy-inner-functions
+
 (function TestLazyInnerFunctionCallsEval() {
   var i = (function eager_outer() {
-    var a = 41; // Should be context-allocated
+    var outer_var = 41; // Should be context-allocated
+    function lazy_inner() {
+      return eval("outer_var");
+    }
+    return lazy_inner;
+  })();
+  assertEquals(41, i());
+})();
+
+(function TestLazyInnerFunctionDestructuring() {
+  var i = (function eager_outer() {
+    var outer_var = 41; // Should be context-allocated
     function lazy_inner() {
-      return eval("a");
+      // This introduces b and refers to outer_var.
+      var {outer_var : b} = {outer_var};
+      return b;
     }
     return lazy_inner;
   })();