[parser] Skipping inner funcs: Fix function name declarations

let f = function g() { ... } declares "g" inside the function. This
CL makes the preparser declare it too, and saves + restores the scope data for
it.

BUG=v8:5516

Change-Id: Id4c64f446d30f5252038cfb0f0f473b85ba24a9b
Reviewed-on: https://chromium-review.googlesource.com/544816
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46133}

src/ast/ast.h

@@ -3212,6 +3212,10 @@ class AstNodeFactory final BASE_EMBEDDED {
     return new (zone_) VariableProxy(proxy);
   }
 
+  Variable* CopyVariable(Variable* variable) {
+    return new (zone_) Variable(variable);
+  }
+
   Property* NewProperty(Expression* obj, Expression* key, int pos) {
     return new (zone_) Property(obj, key, pos);
   }

src/ast/scopes.cc

@@ -1554,6 +1554,11 @@ void DeclarationScope::AnalyzePartially(
       arguments_ = nullptr;
     }
 
+    // Migrate function_ to the right Zone.
+    if (function_ != nullptr) {
+      function_ = ast_node_factory->CopyVariable(function_);
+    }
+
     if (need_preparsed_scope_data) {
       // Store the information needed for allocating the locals of this scope
       // and its inner scopes.

src/ast/variables.cc

@@ -34,6 +34,14 @@ Variable::Variable(Scope* scope, const AstRawString* name, VariableMode mode,
   DCHECK(!(mode == VAR && initialization_flag == kNeedsInitialization));
 }
 
+Variable::Variable(Variable* other)
+    : scope_(other->scope_),
+      name_(other->name_),
+      local_if_not_shadowed_(nullptr),
+      next_(nullptr),
+      index_(other->index_),
+      initializer_position_(other->initializer_position_),
+      bit_field_(other->bit_field_) {}
 
 bool Variable::IsGlobalObjectProperty() const {
   // Temporaries are never global, they must always be allocated in the

src/ast/variables.h

@@ -22,6 +22,8 @@ class Variable final : public ZoneObject {
            VariableKind kind, InitializationFlag initialization_flag,
            MaybeAssignedFlag maybe_assigned_flag = kNotAssigned);
 
+  explicit Variable(Variable* other);
+
   // The source code for an eval() call may refer to a variable that is
   // in an outer scope about which we don't know anything (it may not
   // be the script scope). scope() is NULL in that case. Currently the

src/parsing/parser-base.h

@@ -4326,9 +4326,10 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
         // parameters.
         int dummy_num_parameters = -1;
         DCHECK((kind & FunctionKind::kArrowFunction) != 0);
-        LazyParsingResult result =
-            impl()->SkipFunction(kind, formal_parameters.scope,
-                                 &dummy_num_parameters, false, false, CHECK_OK);
+        LazyParsingResult result = impl()->SkipFunction(
+            nullptr, kind, FunctionLiteral::kAnonymousExpression,
+            formal_parameters.scope, &dummy_num_parameters, false, false,
+            CHECK_OK);
         DCHECK_NE(result, kLazyParsingAborted);
         USE(result);
         formal_parameters.scope->ResetAfterPreparsing(ast_value_factory_,

src/parsing/parser.cc

@@ -2726,9 +2726,9 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
              should_use_parse_task);
       Scanner::BookmarkScope bookmark(scanner());
       bookmark.Set();
-      LazyParsingResult result =
-          SkipFunction(kind, scope, &num_parameters, is_lazy_inner_function,
-                       is_lazy_top_level_function, CHECK_OK);
+      LazyParsingResult result = SkipFunction(
+          function_name, kind, function_type, scope, &num_parameters,
+          is_lazy_inner_function, is_lazy_top_level_function, CHECK_OK);
 
       if (result == kLazyParsingAborted) {
         DCHECK(is_lazy_top_level_function);
@@ -2818,11 +2818,11 @@ FunctionLiteral* Parser::ParseFunctionLiteral(
   return function_literal;
 }
 
-Parser::LazyParsingResult Parser::SkipFunction(FunctionKind kind,
-                                               DeclarationScope* function_scope,
-                                               int* num_parameters,
-                                               bool is_inner_function,
-                                               bool may_abort, bool* ok) {
+Parser::LazyParsingResult Parser::SkipFunction(
+    const AstRawString* function_name, FunctionKind kind,
+    FunctionLiteral::FunctionType function_type,
+    DeclarationScope* function_scope, int* num_parameters,
+    bool is_inner_function, bool may_abort, bool* ok) {
   FunctionState function_state(&function_state_, &scope_, function_scope);
 
   DCHECK_NE(kNoSourcePosition, function_scope->start_position());
@@ -2890,8 +2890,8 @@ Parser::LazyParsingResult Parser::SkipFunction(FunctionKind kind,
   DCHECK(!is_inner_function || !may_abort);
 
   PreParser::PreParseResult result = reusable_preparser()->PreParseFunction(
-      kind, function_scope, parsing_module_, is_inner_function, may_abort,
-      use_counts_);
+      function_name, kind, function_type, function_scope, parsing_module_,
+      is_inner_function, may_abort, use_counts_);
 
   // Return immediately if pre-parser decided to abort parsing.
   if (result == PreParser::kPreParseAbort) return kLazyParsingAborted;

src/parsing/parser.h

@@ -555,7 +555,9 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
   // by parsing the function with PreParser. Consumes the ending }.
   // If may_abort == true, the (pre-)parser may decide to abort skipping
   // in order to force the function to be eagerly parsed, after all.
-  LazyParsingResult SkipFunction(FunctionKind kind,
+  LazyParsingResult SkipFunction(const AstRawString* function_name,
+                                 FunctionKind kind,
+                                 FunctionLiteral::FunctionType function_type,
                                  DeclarationScope* function_scope,
                                  int* num_parameters, bool is_inner_function,
                                  bool may_abort, bool* ok);

src/parsing/preparsed-scope-data.cc

@@ -81,6 +81,12 @@ void PreParsedScopeData::SaveData(Scope* scope) {
   size_t data_end_index = backing_store_.size();
   backing_store_.push_back(0);
 
+  if (scope->scope_type() == ScopeType::FUNCTION_SCOPE) {
+    Variable* function = scope->AsDeclarationScope()->function_var();
+    if (function != nullptr) {
+      SaveDataForVariable(function);
+    }
+  }
   if (!scope->is_hidden()) {
     for (Variable* var : *scope->locals()) {
       if (IsDeclaredVariableMode(var->mode())) {
@@ -166,6 +172,12 @@ void PreParsedScopeData::RestoreData(Scope* scope, uint32_t* index_ptr) const {
   uint32_t data_end_index = backing_store_[index++];
   USE(data_end_index);
 
+  if (scope->scope_type() == ScopeType::FUNCTION_SCOPE) {
+    Variable* function = scope->AsDeclarationScope()->function_var();
+    if (function != nullptr) {
+      RestoreDataForVariable(function, index_ptr);
+    }
+  }
   if (!scope->is_hidden()) {
     for (Variable* var : *scope->locals()) {
       if (var->mode() == VAR || var->mode() == LET || var->mode() == CONST) {

src/parsing/preparser.cc

@@ -131,7 +131,9 @@ PreParser::PreParseResult PreParser::PreParseProgram(bool is_module) {
 }
 
 PreParser::PreParseResult PreParser::PreParseFunction(
-    FunctionKind kind, DeclarationScope* function_scope, bool parsing_module,
+    const AstRawString* function_name, FunctionKind kind,
+    FunctionLiteral::FunctionType function_type,
+    DeclarationScope* function_scope, bool parsing_module,
     bool is_inner_function, bool may_abort, int* use_counts) {
   DCHECK_EQ(FUNCTION_SCOPE, function_scope->scope_type());
   parsing_module_ = parsing_module;
@@ -208,6 +210,8 @@ PreParser::PreParseResult PreParser::PreParseFunction(
   }
 
   if (!IsArrowFunction(kind) && track_unresolved_variables_) {
+    CreateFunctionNameAssignment(function_name, function_type, function_scope);
+
     // Declare arguments after parsing the function since lexical 'arguments'
     // masks the arguments object. Declare arguments before declaring the
     // function var since the arguments object masks 'function arguments'.

src/parsing/preparser.h

@@ -925,7 +925,9 @@ 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 PreParseFunction(FunctionKind kind,
+  PreParseResult PreParseFunction(const AstRawString* function_name,
+                                  FunctionKind kind,
+                                  FunctionLiteral::FunctionType function_type,
                                   DeclarationScope* function_scope,
                                   bool parsing_module,
                                   bool track_unresolved_variables,
@@ -947,11 +949,11 @@ class PreParser : public ParserBase<PreParser> {
   bool AllowsLazyParsingWithoutUnresolvedVariables() const { return false; }
   bool parse_lazily() const { return false; }
 
-  V8_INLINE LazyParsingResult SkipFunction(FunctionKind kind,
-                                           DeclarationScope* function_scope,
-                                           int* num_parameters,
-                                           bool is_inner_function,
-                                           bool may_abort, bool* ok) {
+  V8_INLINE LazyParsingResult
+  SkipFunction(const AstRawString* name, FunctionKind kind,
+               FunctionLiteral::FunctionType function_type,
+               DeclarationScope* function_scope, int* num_parameters,
+               bool is_inner_function, bool may_abort, bool* ok) {
     UNREACHABLE();
   }
   Expression ParseFunctionLiteral(
@@ -1080,11 +1082,28 @@ class PreParser : public ParserBase<PreParser> {
       int pos, FunctionKind kind, PreParserStatementList body, bool* ok) {
     ParseStatementList(body, Token::RBRACE, ok);
   }
+  V8_INLINE void CreateFunctionNameAssignment(
+      const AstRawString* function_name,
+      FunctionLiteral::FunctionType function_type,
+      DeclarationScope* function_scope) {
+    if (track_unresolved_variables_ &&
+        function_type == FunctionLiteral::kNamedExpression) {
+      if (function_scope->LookupLocal(function_name) == nullptr) {
+        DCHECK_EQ(function_scope, scope());
+        Variable* fvar = function_scope->DeclareFunctionVar(function_name);
+        fvar->set_is_used();
+      }
+    }
+  }
+
   V8_INLINE void CreateFunctionNameAssignment(
       PreParserIdentifier function_name, int pos,
       FunctionLiteral::FunctionType function_type,
       DeclarationScope* function_scope, PreParserStatementList result,
-      int index) {}
+      int index) {
+    CreateFunctionNameAssignment(function_name.string_, function_type,
+                                 function_scope);
+  }
 
   V8_INLINE PreParserExpression RewriteDoExpression(PreParserStatement body,
                                                     int pos, bool* ok) {

test/cctest/parsing/test-preparser.cc

@@ -29,6 +29,7 @@ enum SkipTests {
 TEST(PreParserScopeAnalysis) {
   i::FLAG_lazy_inner_functions = true;
   i::FLAG_experimental_preparser_scope_analysis = true;
+  i::FLAG_aggressive_lazy_inner_functions = true;
   i::Isolate* isolate = CcTest::i_isolate();
   i::Factory* factory = isolate->factory();
   i::HandleScope scope(isolate);
@@ -69,6 +70,12 @@ TEST(PreParserScopeAnalysis) {
        false,
        {0, 0}},
 
+      {"(function outer() { let test2 = function test(%s) { %s } })();",
+       false,
+       false,
+       false,
+       {0, 0}},
+
       // Test function deeper:
       {"(function outer() { function inner() { "
        "function test(%s) { %s } } })();",
@@ -77,6 +84,13 @@ TEST(PreParserScopeAnalysis) {
        false,
        {0, 0}},
 
+      {"(function outer() { function inner() { "
+       "let test2 = function test(%s) { %s } } })();",
+       false,
+       false,
+       false,
+       {0, 0}},
+
       // Arrow functions (they can never be at the laziness boundary):
       {"(function outer() { function inner() { (%s) => { %s } } })();",
        false,
@@ -163,6 +177,8 @@ TEST(PreParserScopeAnalysis) {
       {"var1 = 5;"},
       {"if (true) {}"},
       {"function f1() {}"},
+      {"test;"},
+      {"test2;"},
 
       // Var declarations and assignments.
       {"var var1;"},

test/cctest/scope-test-helper.h

@@ -32,12 +32,36 @@ class ScopeTestHelper {
       return;
     }
 
+    if (baseline->scope_type() == ScopeType::FUNCTION_SCOPE) {
+      Variable* function = baseline->AsDeclarationScope()->function_var();
+      if (function != nullptr) {
+        CompareVariables(function, scope->AsDeclarationScope()->function_var(),
+                         precise_maybe_assigned);
+      } else {
+        CHECK_NULL(scope->AsDeclarationScope()->function_var());
+      }
+    }
+
     for (auto baseline_local = baseline->locals()->begin(),
               scope_local = scope->locals()->begin();
          baseline_local != baseline->locals()->end();
          ++baseline_local, ++scope_local) {
       if (scope_local->mode() == VAR || scope_local->mode() == LET ||
           scope_local->mode() == CONST) {
+        CompareVariables(*baseline_local, *scope_local, precise_maybe_assigned);
+      }
+    }
+
+    for (Scope *baseline_inner = baseline->inner_scope(),
+               *scope_inner = scope->inner_scope();
+         scope_inner != nullptr; scope_inner = scope_inner->sibling(),
+               baseline_inner = baseline_inner->sibling()) {
+      CompareScopes(baseline_inner, scope_inner, precise_maybe_assigned);
+    }
+  }
+
+  static void CompareVariables(Variable* baseline_local, Variable* scope_local,
+                               bool precise_maybe_assigned) {
     // Sanity check the variable name. If this fails, the variable order
     // is not deterministic.
     CHECK_EQ(scope_local->raw_name()->length(),
@@ -49,21 +73,10 @@ class ScopeTestHelper {
 
     CHECK_EQ(scope_local->location(), baseline_local->location());
     if (precise_maybe_assigned) {
-          CHECK_EQ(scope_local->maybe_assigned(),
-                   baseline_local->maybe_assigned());
+      CHECK_EQ(scope_local->maybe_assigned(), baseline_local->maybe_assigned());
     } else {
       STATIC_ASSERT(kMaybeAssigned > kNotAssigned);
-          CHECK_GE(scope_local->maybe_assigned(),
-                   baseline_local->maybe_assigned());
-        }
-      }
-    }
-
-    for (Scope *baseline_inner = baseline->inner_scope(),
-               *scope_inner = scope->inner_scope();
-         scope_inner != nullptr; scope_inner = scope_inner->sibling(),
-               baseline_inner = baseline_inner->sibling()) {
-      CompareScopes(baseline_inner, scope_inner, precise_maybe_assigned);
+      CHECK_GE(scope_local->maybe_assigned(), baseline_local->maybe_assigned());
     }
   }
 

test/mjsunit/skipping-inner-functions.js

@@ -93,3 +93,23 @@ function lazy_top_level(ctxt_alloc_param) {
 
 lazy_top_level(10);
 assertEquals(34, result);
+
+// Tests for using a function name in an inner function.
+var TestUsingNamedExpressionName1 = function this_is_the_name() {
+  function inner() {
+    this_is_the_name;
+  }
+  inner();
+}
+TestUsingNamedExpressionName1();
+
+function TestUsingNamedExpressionName2() {
+  let f = function this_is_the_name() {
+    function inner() {
+      this_is_the_name;
+    }
+    inner();
+  }
+  f();
+}
+TestUsingNamedExpressionName2();