From 1fce2d2d61151170864936c7f9f9abed21ae4b73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marja=20H=C3=B6ltt=C3=A4?= Date: Thu, 22 Jun 2017 15:09:01 +0200 Subject: [PATCH] [parser] Skipping inner funcs: Fix function name declarations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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ä Reviewed-by: Daniel Vogelheim Cr-Commit-Position: refs/heads/master@{#46133} --- src/ast/ast.h | 4 ++ src/ast/scopes.cc | 5 +++ src/ast/variables.cc | 8 ++++ src/ast/variables.h | 2 + src/parsing/parser-base.h | 7 ++-- src/parsing/parser.cc | 20 +++++----- src/parsing/parser.h | 4 +- src/parsing/preparsed-scope-data.cc | 12 ++++++ src/parsing/preparser.cc | 6 ++- src/parsing/preparser.h | 33 ++++++++++++---- test/cctest/parsing/test-preparser.cc | 16 ++++++++ test/cctest/scope-test-helper.h | 49 +++++++++++++++--------- test/mjsunit/skipping-inner-functions.js | 20 ++++++++++ 13 files changed, 146 insertions(+), 40 deletions(-) diff --git a/src/ast/ast.h b/src/ast/ast.h index 93ebaf4368..5326a57505 100644 --- a/src/ast/ast.h +++ b/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); } diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc index 4cc90f2790..630d2ebd28 100644 --- a/src/ast/scopes.cc +++ b/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. diff --git a/src/ast/variables.cc b/src/ast/variables.cc index c6611bd0d9..df839c5d8c 100644 --- a/src/ast/variables.cc +++ b/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 diff --git a/src/ast/variables.h b/src/ast/variables.h index c01db36274..786db2a07c 100644 --- a/src/ast/variables.h +++ b/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 diff --git a/src/parsing/parser-base.h b/src/parsing/parser-base.h index 251c04a71f..b963a5902c 100644 --- a/src/parsing/parser-base.h +++ b/src/parsing/parser-base.h @@ -4326,9 +4326,10 @@ ParserBase::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_, diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc index bcf19886d8..533287e4b8 100644 --- a/src/parsing/parser.cc +++ b/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; diff --git a/src/parsing/parser.h b/src/parsing/parser.h index 648e1f8b2c..f458a5544c 100644 --- a/src/parsing/parser.h +++ b/src/parsing/parser.h @@ -555,7 +555,9 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase) { // 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); diff --git a/src/parsing/preparsed-scope-data.cc b/src/parsing/preparsed-scope-data.cc index 6a677fe83a..dd3606ddf6 100644 --- a/src/parsing/preparsed-scope-data.cc +++ b/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) { diff --git a/src/parsing/preparser.cc b/src/parsing/preparser.cc index 4fb32611b1..c19d2dd728 100644 --- a/src/parsing/preparser.cc +++ b/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'. diff --git a/src/parsing/preparser.h b/src/parsing/preparser.h index 9c5fb43c7c..5cb93ff2e6 100644 --- a/src/parsing/preparser.h +++ b/src/parsing/preparser.h @@ -925,7 +925,9 @@ class PreParser : public ParserBase { // 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 { 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 { 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) { diff --git a/test/cctest/parsing/test-preparser.cc b/test/cctest/parsing/test-preparser.cc index 76d1b57118..b728cdfa9f 100644 --- a/test/cctest/parsing/test-preparser.cc +++ b/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;"}, diff --git a/test/cctest/scope-test-helper.h b/test/cctest/scope-test-helper.h index 61a5167854..615f0aa75e 100644 --- a/test/cctest/scope-test-helper.h +++ b/test/cctest/scope-test-helper.h @@ -32,30 +32,23 @@ 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) { - // Sanity check the variable name. If this fails, the variable order - // is not deterministic. - CHECK_EQ(scope_local->raw_name()->length(), - baseline_local->raw_name()->length()); - for (int i = 0; i < scope_local->raw_name()->length(); ++i) { - CHECK_EQ(scope_local->raw_name()->raw_data()[i], - baseline_local->raw_name()->raw_data()[i]); - } - - CHECK_EQ(scope_local->location(), baseline_local->location()); - if (precise_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()); - } + CompareVariables(*baseline_local, *scope_local, precise_maybe_assigned); } } @@ -67,6 +60,26 @@ class ScopeTestHelper { } } + 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(), + baseline_local->raw_name()->length()); + for (int i = 0; i < scope_local->raw_name()->length(); ++i) { + CHECK_EQ(scope_local->raw_name()->raw_data()[i], + baseline_local->raw_name()->raw_data()[i]); + } + + CHECK_EQ(scope_local->location(), baseline_local->location()); + if (precise_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()); + } + } + // Finds a scope given a start point and directions to it (which inner scope // to pick). static Scope* FindScope(Scope* scope, const std::vector& location) { diff --git a/test/mjsunit/skipping-inner-functions.js b/test/mjsunit/skipping-inner-functions.js index c95d33c9e9..784ea4d9d2 100644 --- a/test/mjsunit/skipping-inner-functions.js +++ b/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();