[parser] Move Compiler::Analyze into Parser

Move rewriting, scope analysis, and internalization, to be unconditional
operations done after parsing rather than a separate compile phase. This
removes some of the complexity about rememberering when to call
Compiler::Analyze, and makes these paths a bit more uniform.

Also, forbid allocating any more AST strings after AstValueFactory
internalization, by nulling out the Zone. Add an InternalizePartial
method which doesn't null out the zone for those cases where we do want
to be able to allocate after internalizing (e.g. internalization before
scope analysis).

Change-Id: Id444246d8362a1d169baf664fc37657d9576fd96
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2182458Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67608}

src/ast/ast-value-factory.cc

@@ -332,20 +332,22 @@ const AstRawString* AstValueFactory::CloneFromOtherFactory(
 }
 
 AstConsString* AstValueFactory::NewConsString() {
-  return new (zone_) AstConsString;
+  return new (zone()) AstConsString;
 }
 
 AstConsString* AstValueFactory::NewConsString(const AstRawString* str) {
-  return NewConsString()->AddString(zone_, str);
+  return NewConsString()->AddString(zone(), str);
 }
 
 AstConsString* AstValueFactory::NewConsString(const AstRawString* str1,
                                               const AstRawString* str2) {
-  return NewConsString()->AddString(zone_, str1)->AddString(zone_, str2);
+  return NewConsString()->AddString(zone(), str1)->AddString(zone(), str2);
 }
 
 template <typename LocalIsolate>
 void AstValueFactory::Internalize(LocalIsolate* isolate) {
+  if (!zone_) return;
+
   // Strings need to be internalized before values, because values refer to
   // strings.
   for (AstRawString* current = strings_; current != nullptr;) {
@@ -355,6 +357,7 @@ void AstValueFactory::Internalize(LocalIsolate* isolate) {
   }
 
   ResetStrings();
+  zone_ = nullptr;
 }
 template EXPORT_TEMPLATE_DEFINE(
     V8_EXPORT_PRIVATE) void AstValueFactory::Internalize<Isolate>(Isolate*
@@ -373,9 +376,9 @@ AstRawString* AstValueFactory::GetString(uint32_t hash_field, bool is_one_byte,
   if (entry->value == nullptr) {
     // Copy literal contents for later comparison.
     int length = literal_bytes.length();
-    byte* new_literal_bytes = zone_->NewArray<byte>(length);
+    byte* new_literal_bytes = zone()->NewArray<byte>(length);
     memcpy(new_literal_bytes, literal_bytes.begin(), length);
-    AstRawString* new_string = new (zone_) AstRawString(
+    AstRawString* new_string = new (zone()) AstRawString(
         is_one_byte, Vector<const byte>(new_literal_bytes, length), hash_field);
     CHECK_NOT_NULL(new_string);
     AddString(new_string);

src/ast/ast-value-factory.h

@@ -288,6 +288,7 @@ class AstValueFactory {
         empty_cons_string_(nullptr),
         zone_(zone),
         hash_seed_(hash_seed) {
+    DCHECK_NOT_NULL(zone_);
     DCHECK_EQ(hash_seed, string_constants->hash_seed());
     std::fill(one_character_strings_,
               one_character_strings_ + arraysize(one_character_strings_),
@@ -295,7 +296,10 @@ class AstValueFactory {
     empty_cons_string_ = NewConsString();
   }
 
-  Zone* zone() const { return zone_; }
+  Zone* zone() const {
+    DCHECK_NOT_NULL(zone_);
+    return zone_;
+  }
 
   const AstRawString* GetOneByteString(Vector<const uint8_t> literal) {
     return GetOneByteStringInternal(literal);
@@ -317,6 +321,9 @@ class AstValueFactory {
   V8_EXPORT_PRIVATE AstConsString* NewConsString(const AstRawString* str1,
                                                  const AstRawString* str2);
 
+  // Internalize all the strings in the factory, and prevent any more from being
+  // allocated. Multiple calls to Internalize are allowed, for simplicity, where
+  // subsequent calls are a no-op.
   template <typename LocalIsolate>
   void Internalize(LocalIsolate* isolate);
 

src/ast/ast.h

@@ -2243,7 +2243,7 @@ class FunctionLiteral final : public Expression {
  private:
   friend class AstNodeFactory;
 
-  FunctionLiteral(Zone* zone, const AstRawString* name,
+  FunctionLiteral(Zone* zone, const AstConsString* name,
                   AstValueFactory* ast_value_factory, DeclarationScope* scope,
                   const ScopedPtrList<Statement>& body,
                   int expected_property_count, int parameter_count,
@@ -2259,7 +2259,7 @@ class FunctionLiteral final : public Expression {
         function_token_position_(kNoSourcePosition),
         suspend_count_(0),
         function_literal_id_(function_literal_id),
-        raw_name_(name ? ast_value_factory->NewConsString(name) : nullptr),
+        raw_name_(name),
         scope_(scope),
         body_(0, nullptr),
         raw_inferred_name_(ast_value_factory->empty_cons_string()),
@@ -3110,7 +3110,8 @@ class AstNodeFactory final {
       bool has_braces, int function_literal_id,
       ProducedPreparseData* produced_preparse_data = nullptr) {
     return new (zone_) FunctionLiteral(
-        zone_, name, ast_value_factory_, scope, body, expected_property_count,
+        zone_, name ? ast_value_factory_->NewConsString(name) : nullptr,
+        ast_value_factory_, scope, body, expected_property_count,
         parameter_count, function_length, function_syntax_kind,
         has_duplicate_parameters, eager_compile_hint, position, has_braces,
         function_literal_id, produced_preparse_data);
@@ -3123,8 +3124,8 @@ class AstNodeFactory final {
       DeclarationScope* scope, const ScopedPtrList<Statement>& body,
       int expected_property_count, int parameter_count) {
     return new (zone_) FunctionLiteral(
-        zone_, ast_value_factory_->empty_string(), ast_value_factory_, scope,
-        body, expected_property_count, parameter_count, parameter_count,
+        zone_, ast_value_factory_->empty_cons_string(), ast_value_factory_,
+        scope, body, expected_property_count, parameter_count, parameter_count,
         FunctionSyntaxKind::kAnonymousExpression,
         FunctionLiteral::kNoDuplicateParameters,
         FunctionLiteral::kShouldLazyCompile, 0, /* has_braces */ false,

src/ast/scopes.cc

@@ -598,7 +598,7 @@ bool DeclarationScope::Analyze(ParseInfo* info) {
     DCHECK_EQ(scope->scope_type_, ScopeType::FUNCTION_SCOPE);
     allow_deref.emplace();
     info->consumed_preparse_data()->RestoreScopeAllocationData(
-        scope, info->ast_value_factory());
+        scope, info->ast_value_factory(), info->zone());
   }
 
   if (!scope->AllocateVariables(info)) return false;

src/codegen/compiler.cc

@@ -47,7 +47,6 @@
 #include "src/parsing/parser.h"
 #include "src/parsing/parsing.h"
 #include "src/parsing/pending-compilation-error-handler.h"
-#include "src/parsing/rewriter.h"
 #include "src/parsing/scanner-character-streams.h"
 #include "src/snapshot/code-serializer.h"
 #include "src/utils/ostreams.h"
@@ -624,11 +623,9 @@ std::unique_ptr<UnoptimizedCompilationJob> GenerateUnoptimizedCode(
   DisallowHeapAccess no_heap_access;
   DCHECK(inner_function_jobs->empty());
 
-  std::unique_ptr<UnoptimizedCompilationJob> job;
-  if (Compiler::Analyze(parse_info)) {
-    job = ExecuteUnoptimizedCompileJobs(parse_info, parse_info->literal(),
-                                        allocator, inner_function_jobs);
-  }
+  std::unique_ptr<UnoptimizedCompilationJob> job =
+      ExecuteUnoptimizedCompileJobs(parse_info, parse_info->literal(),
+                                    allocator, inner_function_jobs);
 
   // Character stream shouldn't be used again.
   parse_info->ResetCharacterStream();
@@ -640,9 +637,6 @@ MaybeHandle<SharedFunctionInfo> GenerateUnoptimizedCodeForToplevel(
     Isolate* isolate, Handle<Script> script, ParseInfo* parse_info,
     AccountingAllocator* allocator, IsCompiledScope* is_compiled_scope) {
   EnsureSharedFunctionInfosArrayOnScript(script, parse_info, isolate);
-  parse_info->ast_value_factory()->Internalize(isolate);
-
-  if (!Compiler::Analyze(parse_info)) return MaybeHandle<SharedFunctionInfo>();
   DeclarationScope::AllocateScopeInfos(parse_info, isolate);
 
   // Prepare and execute compilation of the outer-most function.
@@ -1399,25 +1393,6 @@ void BackgroundCompileTask::Run() {
 // ----------------------------------------------------------------------------
 // Implementation of Compiler
 
-bool Compiler::Analyze(ParseInfo* parse_info) {
-  DCHECK_NOT_NULL(parse_info->literal());
-  RuntimeCallTimerScope runtimeTimer(parse_info->runtime_call_stats(),
-                                     RuntimeCallCounterId::kCompileAnalyse,
-                                     RuntimeCallStats::kThreadSpecific);
-  if (!Rewriter::Rewrite(parse_info)) return false;
-  if (!DeclarationScope::Analyze(parse_info)) return false;
-  return true;
-}
-
-bool Compiler::ParseAndAnalyze(ParseInfo* parse_info,
-                               Handle<SharedFunctionInfo> shared_info,
-                               Isolate* isolate) {
-  if (!parsing::ParseAny(parse_info, shared_info, isolate)) {
-    return false;
-  }
-  return Compiler::Analyze(parse_info);
-}
-
 // static
 bool Compiler::CollectSourcePositions(Isolate* isolate,
                                       Handle<SharedFunctionInfo> shared_info) {
@@ -1485,14 +1460,6 @@ bool Compiler::CollectSourcePositions(Isolate* isolate,
   // wasting time fully parsing them when they won't ever be used.
   std::unique_ptr<UnoptimizedCompilationJob> job;
   {
-    if (!Compiler::Analyze(&parse_info)) {
-      // Recompiling failed probably as a result of stack exhaustion.
-      bytecode->SetSourcePositionsFailedToCollect();
-      return FailWithPendingException(
-          isolate, handle(Script::cast(shared_info->script()), isolate),
-          &parse_info, Compiler::ClearExceptionFlag::CLEAR_EXCEPTION);
-    }
-
     job = interpreter::Interpreter::NewSourcePositionCollectionJob(
         &parse_info, parse_info.literal(), bytecode, isolate->allocator());
 
@@ -1590,9 +1557,6 @@ bool Compiler::Compile(Handle<SharedFunctionInfo> shared_info,
         &parse_info, flag);
   }
 
-  // Internalize ast values onto the heap.
-  parse_info.ast_value_factory()->Internalize(isolate);
-
   // Finalize compilation of the unoptimized bytecode or asm-js data.
   if (!FinalizeUnoptimizedCode(&parse_info, isolate, shared_info,
                                outer_function_job.get(),

src/codegen/compiler.h

@@ -94,13 +94,6 @@ class V8_EXPORT_PRIVATE Compiler : public AllStatic {
   // offer this chance, optimized closure instantiation will not call this.
   static void PostInstantiation(Handle<JSFunction> function);
 
-  // Parser::Parse, then Compiler::Analyze.
-  static bool ParseAndAnalyze(ParseInfo* parse_info,
-                              Handle<SharedFunctionInfo> shared_info,
-                              Isolate* isolate);
-  // Rewrite and analyze scopes.
-  static bool Analyze(ParseInfo* parse_info);
-
   // ===========================================================================
   // The following family of methods instantiates new functions for scripts or
   // function literals. The decision whether those functions will be compiled,

src/debug/debug-scopes.cc

@@ -272,8 +272,7 @@ void ScopeIterator::TryParseAndRetrieveScopes(ReparseStrategy strategy) {
                                   isolate_)
           : parsing::ParseFunction(info_.get(), shared_info, isolate_);
 
-  if (parse_result && Rewriter::Rewrite(info_.get())) {
-    info_->ast_value_factory()->Internalize(isolate_);
+  if (parse_result) {
     DeclarationScope* literal_scope = info_->literal()->scope();
 
     ScopeChainRetriever scope_chain_retriever(literal_scope, function_,
@@ -287,7 +286,6 @@ void ScopeIterator::TryParseAndRetrieveScopes(ReparseStrategy strategy) {
                          ? scope_chain_retriever.ClosureScope()
                          : literal_scope;
 
-    CHECK(DeclarationScope::Analyze(info_.get()));
     if (ignore_nested_scopes) {
       current_scope_ = closure_scope_;
       start_scope_ = current_scope_;

src/debug/liveedit.cc

@@ -758,10 +758,6 @@ bool ParseScript(Isolate* isolate, Handle<Script> script, ParseInfo* parse_info,
                   .ToHandle(&shared);
   } else {
     success = parsing::ParseProgram(parse_info, script, isolate);
-    if (success) {
-      success = Compiler::Analyze(parse_info);
-      parse_info->ast_value_factory()->Internalize(isolate);
-    }
   }
   if (!success) {
     isolate->OptionalRescheduleException(false);

src/parsing/parser.cc

@@ -508,9 +508,9 @@ void MaybeProcessSourceRanges(ParseInfo* parse_info, Expression* root,
 
 }  // namespace
 
-FunctionLiteral* Parser::ParseProgram(
-    Isolate* isolate, Handle<Script> script, ParseInfo* info,
-    MaybeHandle<ScopeInfo> maybe_outer_scope_info) {
+void Parser::ParseProgram(Isolate* isolate, Handle<Script> script,
+                          ParseInfo* info,
+                          MaybeHandle<ScopeInfo> maybe_outer_scope_info) {
   // TODO(bmeurer): We temporarily need to pass allow_nesting = true here,
   // see comment for HistogramTimerScope class.
   DCHECK_EQ(script->id(), flags().script_id());
@@ -539,6 +539,7 @@ FunctionLiteral* Parser::ParseProgram(
   FunctionLiteral* result = DoParseProgram(isolate, info);
   MaybeResetCharacterStream(info, result);
   MaybeProcessSourceRanges(info, result, stack_limit_);
+  PostProcessParseResult(isolate, info, result);
 
   HandleSourceURLComments(isolate, script);
 
@@ -555,7 +556,6 @@ FunctionLiteral* Parser::ParseProgram(
     LOG(isolate,
         FunctionEvent(event_name, flags().script_id(), ms, start, end, "", 0));
   }
-  return result;
 }
 
 FunctionLiteral* Parser::DoParseProgram(Isolate* isolate, ParseInfo* info) {
@@ -684,6 +684,33 @@ FunctionLiteral* Parser::DoParseProgram(Isolate* isolate, ParseInfo* info) {
   return result;
 }
 
+void Parser::PostProcessParseResult(Isolate* isolate, ParseInfo* info,
+                                    FunctionLiteral* literal) {
+  if (literal == nullptr) return;
+
+  info->set_literal(literal);
+  info->set_language_mode(literal->language_mode());
+  if (info->flags().is_eval()) {
+    info->set_allow_eval_cache(allow_eval_cache());
+  }
+
+  // We cannot internalize on a background thread; a foreground task will take
+  // care of calling AstValueFactory::Internalize just before compilation.
+  DCHECK_EQ(isolate != nullptr, parsing_on_main_thread_);
+  if (isolate) info->ast_value_factory()->Internalize(isolate);
+
+  {
+    RuntimeCallTimerScope runtimeTimer(info->runtime_call_stats(),
+                                       RuntimeCallCounterId::kCompileAnalyse,
+                                       RuntimeCallStats::kThreadSpecific);
+    if (!Rewriter::Rewrite(info) || !DeclarationScope::Analyze(info)) {
+      // Null out the literal to indicate that something failed.
+      info->set_literal(nullptr);
+      return;
+    }
+  }
+}
+
 ZonePtrList<const AstRawString>* Parser::PrepareWrappedArguments(
     Isolate* isolate, ParseInfo* info, Zone* zone) {
   DCHECK(parsing_on_main_thread_);
@@ -783,8 +810,8 @@ Expression* Parser::WrapREPLResult(Expression* value) {
                                      false);
 }
 
-FunctionLiteral* Parser::ParseFunction(Isolate* isolate, ParseInfo* info,
-                                       Handle<SharedFunctionInfo> shared_info) {
+void Parser::ParseFunction(Isolate* isolate, ParseInfo* info,
+                           Handle<SharedFunctionInfo> shared_info) {
   // It's OK to use the Isolate & counters here, since this function is only
   // called in the main thread.
   DCHECK(parsing_on_main_thread_);
@@ -835,11 +862,12 @@ FunctionLiteral* Parser::ParseFunction(Isolate* isolate, ParseInfo* info,
     Handle<String> inferred_name(shared_info->inferred_name(), isolate);
     result->set_inferred_name(inferred_name);
   }
+  PostProcessParseResult(isolate, info, result);
 
   if (V8_UNLIKELY(FLAG_log_function_events) && result != nullptr) {
     double ms = timer.Elapsed().InMillisecondsF();
-    // We need to make sure that the debug-name is available.
-    ast_value_factory()->Internalize(isolate);
+    // We should already be internalized by now, so the debug name will be
+    // available.
     DeclarationScope* function_scope = result->scope();
     std::unique_ptr<char[]> function_name = result->GetDebugName();
     LOG(isolate,
@@ -848,7 +876,6 @@ FunctionLiteral* Parser::ParseFunction(Isolate* isolate, ParseInfo* info,
                       function_scope->end_position(), function_name.get(),
                       strlen(function_name.get())));
   }
-  return result;
 }
 
 FunctionLiteral* Parser::DoParseFunction(Isolate* isolate, ParseInfo* info,
@@ -3117,11 +3144,7 @@ void Parser::ParseOnBackground(ParseInfo* info, int start_position,
   }
   MaybeResetCharacterStream(info, result);
   MaybeProcessSourceRanges(info, result, stack_limit_);
-
-  info->set_literal(result);
-
-  // We cannot internalize on a background thread; a foreground task will take
-  // care of calling AstValueFactory::Internalize just before compilation.
+  PostProcessParseResult(/* isolate = */ nullptr, info, result);
 }
 
 Parser::TemplateLiteralState Parser::OpenTemplateLiteral(int pos) {

src/parsing/parser.h

@@ -135,6 +135,7 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
 
   static bool IsPreParser() { return false; }
 
+  // Sets the literal on |info| if parsing succeeded.
   void ParseOnBackground(ParseInfo* info, int start_position, int end_position,
                          int function_literal_id);
 
@@ -209,13 +210,17 @@ class V8_EXPORT_PRIVATE Parser : public NON_EXPORTED_BASE(ParserBase<Parser>) {
 
   void PrepareGeneratorVariables();
 
-  // Returns nullptr if parsing failed.
-  FunctionLiteral* ParseProgram(Isolate* isolate, Handle<Script> script,
-                                ParseInfo* info,
-                                MaybeHandle<ScopeInfo> maybe_outer_scope_info);
+  // Sets the literal on |info| if parsing succeeded.
+  void ParseProgram(Isolate* isolate, Handle<Script> script, ParseInfo* info,
+                    MaybeHandle<ScopeInfo> maybe_outer_scope_info);
+
+  // Sets the literal on |info| if parsing succeeded.
+  void ParseFunction(Isolate* isolate, ParseInfo* info,
+                     Handle<SharedFunctionInfo> shared_info);
+
+  void PostProcessParseResult(Isolate* isolate, ParseInfo* info,
+                              FunctionLiteral* literal);
 
-  FunctionLiteral* ParseFunction(Isolate* isolate, ParseInfo* info,
-                                 Handle<SharedFunctionInfo> shared_info);
   FunctionLiteral* DoParseFunction(Isolate* isolate, ParseInfo* info,
                                    int start_position, int end_position,
                                    int function_literal_id,

src/parsing/parsing.cc

@@ -12,6 +12,7 @@
 #include "src/objects/objects-inl.h"
 #include "src/parsing/parse-info.h"
 #include "src/parsing/parser.h"
+#include "src/parsing/rewriter.h"
 #include "src/parsing/scanner-character-streams.h"
 #include "src/zone/zone-list-inl.h"  // crbug.com/v8/8816
 
@@ -19,6 +20,22 @@ namespace v8 {
 namespace internal {
 namespace parsing {
 
+namespace {
+
+void MaybeReportErrorsAndStatistics(ParseInfo* info, Handle<Script> script,
+                                    Isolate* isolate, Parser* parser,
+                                    ReportErrorsAndStatisticsMode mode) {
+  if (mode == ReportErrorsAndStatisticsMode::kYes) {
+    if (info->literal() == nullptr) {
+      info->pending_error_handler()->ReportErrors(isolate, script,
+                                                  info->ast_value_factory());
+    }
+    parser->UpdateStatistics(isolate, script);
+  }
+}
+
+}  // namespace
+
 bool ParseProgram(ParseInfo* info, Handle<Script> script,
                   MaybeHandle<ScopeInfo> maybe_outer_scope_info,
                   Isolate* isolate, ReportErrorsAndStatisticsMode mode) {
@@ -36,27 +53,11 @@ bool ParseProgram(ParseInfo* info, Handle<Script> script,
 
   Parser parser(info);
 
-  FunctionLiteral* result = nullptr;
   // Ok to use Isolate here; this function is only called in the main thread.
   DCHECK(parser.parsing_on_main_thread_);
-
-  result = parser.ParseProgram(isolate, script, info, maybe_outer_scope_info);
-  info->set_literal(result);
-  if (result) {
-    info->set_language_mode(info->literal()->language_mode());
-    if (info->flags().is_eval()) {
-      info->set_allow_eval_cache(parser.allow_eval_cache());
-    }
-  }
-
-  if (mode == ReportErrorsAndStatisticsMode::kYes) {
-    if (result == nullptr) {
-      info->pending_error_handler()->ReportErrors(isolate, script,
-                                                  info->ast_value_factory());
-    }
-    parser.UpdateStatistics(isolate, script);
-  }
-  return (result != nullptr);
+  parser.ParseProgram(isolate, script, info, maybe_outer_scope_info);
+  MaybeReportErrorsAndStatistics(info, script, isolate, &parser, mode);
+  return info->literal() != nullptr;
 }
 
 bool ParseProgram(ParseInfo* info, Handle<Script> script, Isolate* isolate,
@@ -70,6 +71,8 @@ bool ParseFunction(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,
   DCHECK(!shared_info.is_null());
   DCHECK_NULL(info->literal());
 
+  VMState<PARSER> state(isolate);
+
   // Create a character stream for the parser.
   Handle<Script> script(Script::cast(shared_info->script()), isolate);
   Handle<String> source(String::cast(script->source()), isolate);
@@ -79,31 +82,13 @@ bool ParseFunction(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,
                          shared_info->EndPosition()));
   info->set_character_stream(std::move(stream));
 
-  VMState<PARSER> state(isolate);
-
   Parser parser(info);
 
-  FunctionLiteral* result = nullptr;
   // Ok to use Isolate here; this function is only called in the main thread.
   DCHECK(parser.parsing_on_main_thread_);
-
-  result = parser.ParseFunction(isolate, info, shared_info);
-  info->set_literal(result);
-  if (result) {
-    info->ast_value_factory()->Internalize(isolate);
-    if (info->flags().is_eval()) {
-      info->set_allow_eval_cache(parser.allow_eval_cache());
-    }
-  }
-
-  if (mode == ReportErrorsAndStatisticsMode::kYes) {
-    if (result == nullptr) {
-      info->pending_error_handler()->ReportErrors(isolate, script,
-                                                  info->ast_value_factory());
-    }
-    parser.UpdateStatistics(isolate, script);
-  }
-  return (result != nullptr);
+  parser.ParseFunction(isolate, info, shared_info);
+  MaybeReportErrorsAndStatistics(info, script, isolate, &parser, mode);
+  return info->literal() != nullptr;
 }
 
 bool ParseAny(ParseInfo* info, Handle<SharedFunctionInfo> shared_info,

src/parsing/preparse-data-impl.h

@@ -158,17 +158,20 @@ class BaseConsumedPreparseData : public ConsumedPreparseData {
       LanguageMode* language_mode) final;
 
   void RestoreScopeAllocationData(DeclarationScope* scope,
-                                  AstValueFactory* ast_value_factory) final;
+                                  AstValueFactory* ast_value_factory,
+                                  Zone* zone) final;
 
 #ifdef DEBUG
   bool VerifyDataStart();
 #endif
 
  private:
-  void RestoreDataForScope(Scope* scope, AstValueFactory* ast_value_factory);
+  void RestoreDataForScope(Scope* scope, AstValueFactory* ast_value_factory,
+                           Zone* zone);
   void RestoreDataForVariable(Variable* var);
   void RestoreDataForInnerScopes(Scope* scope,
-                                 AstValueFactory* ast_value_factory);
+                                 AstValueFactory* ast_value_factory,
+                                 Zone* zone);
 
   std::unique_ptr<ByteData> scope_data_;
   // When consuming the data, these indexes point to the data we're going to

src/parsing/preparse-data.cc

@@ -613,7 +613,7 @@ BaseConsumedPreparseData<Data>::GetDataForSkippableFunction(
 
 template <class Data>
 void BaseConsumedPreparseData<Data>::RestoreScopeAllocationData(
-    DeclarationScope* scope, AstValueFactory* ast_value_factory) {
+    DeclarationScope* scope, AstValueFactory* ast_value_factory, Zone* zone) {
   DCHECK_EQ(scope->scope_type(), ScopeType::FUNCTION_SCOPE);
   typename ByteData::ReadingScope reading_scope(this);
 
@@ -628,7 +628,7 @@ void BaseConsumedPreparseData<Data>::RestoreScopeAllocationData(
   DCHECK_EQ(end_position_from_data, scope->end_position());
 #endif
 
-  RestoreDataForScope(scope, ast_value_factory);
+  RestoreDataForScope(scope, ast_value_factory, zone);
 
   // Check that we consumed all scope data.
   DCHECK_EQ(scope_data_->RemainingBytes(), 0);
@@ -636,7 +636,7 @@ void BaseConsumedPreparseData<Data>::RestoreScopeAllocationData(
 
 template <typename Data>
 void BaseConsumedPreparseData<Data>::RestoreDataForScope(
-    Scope* scope, AstValueFactory* ast_value_factory) {
+    Scope* scope, AstValueFactory* ast_value_factory, Zone* zone) {
   if (scope->is_declaration_scope() &&
       scope->AsDeclarationScope()->is_skipped_function()) {
     return;
@@ -670,7 +670,7 @@ void BaseConsumedPreparseData<Data>::RestoreDataForScope(
     if (scope->AsClassScope()->is_anonymous_class()) {
       var = scope->AsClassScope()->DeclareClassVariable(
           ast_value_factory, nullptr, kNoSourcePosition);
-      AstNodeFactory factory(ast_value_factory, ast_value_factory->zone());
+      AstNodeFactory factory(ast_value_factory, zone);
       Declaration* declaration =
           factory.NewVariableDeclaration(kNoSourcePosition);
       scope->declarations()->Add(declaration);
@@ -692,7 +692,7 @@ void BaseConsumedPreparseData<Data>::RestoreDataForScope(
     if (IsSerializableVariableMode(var->mode())) RestoreDataForVariable(var);
   }
 
-  RestoreDataForInnerScopes(scope, ast_value_factory);
+  RestoreDataForInnerScopes(scope, ast_value_factory, zone);
 }
 
 template <typename Data>
@@ -732,10 +732,10 @@ void BaseConsumedPreparseData<Data>::RestoreDataForVariable(Variable* var) {
 
 template <typename Data>
 void BaseConsumedPreparseData<Data>::RestoreDataForInnerScopes(
-    Scope* scope, AstValueFactory* ast_value_factory) {
+    Scope* scope, AstValueFactory* ast_value_factory, Zone* zone) {
   for (Scope* inner = scope->inner_scope(); inner != nullptr;
        inner = inner->sibling()) {
-    RestoreDataForScope(inner, ast_value_factory);
+    RestoreDataForScope(inner, ast_value_factory, zone);
   }
 }
 

src/parsing/preparse-data.h

@@ -297,8 +297,9 @@ class ConsumedPreparseData {
 
   // Restores the information needed for allocating the Scope's (and its
   // subscopes') variables.
-  virtual void RestoreScopeAllocationData(
-      DeclarationScope* scope, AstValueFactory* ast_value_factory) = 0;
+  virtual void RestoreScopeAllocationData(DeclarationScope* scope,
+                                          AstValueFactory* ast_value_factory,
+                                          Zone* zone) = 0;
 
  protected:
   ConsumedPreparseData() = default;

src/parsing/rewriter.cc

@@ -17,12 +17,12 @@ namespace internal {
 class Processor final : public AstVisitor<Processor> {
  public:
   Processor(uintptr_t stack_limit, DeclarationScope* closure_scope,
-            Variable* result, AstValueFactory* ast_value_factory)
+            Variable* result, AstValueFactory* ast_value_factory, Zone* zone)
       : result_(result),
         replacement_(nullptr),
-        zone_(ast_value_factory->zone()),
+        zone_(zone),
         closure_scope_(closure_scope),
-        factory_(ast_value_factory, ast_value_factory->zone()),
+        factory_(ast_value_factory, zone),
         result_assigned_(false),
         is_set_(false),
         breakable_(false) {
@@ -31,10 +31,10 @@ class Processor final : public AstVisitor<Processor> {
   }
 
   Processor(Parser* parser, DeclarationScope* closure_scope, Variable* result,
-            AstValueFactory* ast_value_factory)
+            AstValueFactory* ast_value_factory, Zone* zone)
       : result_(result),
         replacement_(nullptr),
-        zone_(ast_value_factory->zone()),
+        zone_(zone),
         closure_scope_(closure_scope),
         factory_(ast_value_factory, zone_),
         result_assigned_(false),
@@ -392,7 +392,7 @@ base::Optional<VariableProxy*> Rewriter::RewriteBody(
     Variable* result = scope->AsDeclarationScope()->NewTemporary(
         info->ast_value_factory()->dot_result_string());
     Processor processor(info->stack_limit(), scope->AsDeclarationScope(),
-                        result, info->ast_value_factory());
+                        result, info->ast_value_factory(), info->zone());
     processor.Process(body);
 
     DCHECK_IMPLIES(scope->is_module_scope(), processor.result_assigned());

test/cctest/interpreter/test-interpreter.cc

@@ -522,6 +522,7 @@ TEST(InterpreterStringAdd) {
       {ast_factory.GetOneByteString(""), LiteralForTest(2.5),
        factory->NewStringFromStaticChars("2.5"), BinaryOperationFeedback::kAny},
   };
+  ast_factory.Internalize(isolate);
 
   for (size_t i = 0; i < arraysize(test_cases); i++) {
     FeedbackVectorSpec feedback_spec(zone);
@@ -534,7 +535,6 @@ TEST(InterpreterStringAdd) {
     builder.LoadLiteral(test_cases[i].lhs).StoreAccumulatorInRegister(reg);
     LoadLiteralForTest(&builder, test_cases[i].rhs);
     builder.BinaryOperation(Token::Value::ADD, reg, GetIndex(slot)).Return();
-    ast_factory.Internalize(isolate);
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray(isolate);
 
     InterpreterTester tester(isolate, bytecode_array, metadata);
@@ -744,6 +744,7 @@ TEST(InterpreterBinaryOpTypeFeedback) {
       {Token::Value::MOD, LiteralForTest(3),
        LiteralForTest(ast_factory.GetOneByteString("-2")),
        Handle<Smi>(Smi::FromInt(1), isolate), BinaryOperationFeedback::kAny}};
+  ast_factory.Internalize(isolate);
 
   for (const BinaryOpExpectation& test_case : kTestCases) {
     i::FeedbackVectorSpec feedback_spec(zone);
@@ -760,7 +761,6 @@ TEST(InterpreterBinaryOpTypeFeedback) {
     LoadLiteralForTest(&builder, test_case.arg2);
     builder.BinaryOperation(test_case.op, reg, GetIndex(slot0)).Return();
 
-    ast_factory.Internalize(isolate);
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray(isolate);
 
     InterpreterTester tester(isolate, bytecode_array, metadata);
@@ -849,6 +849,7 @@ TEST(InterpreterBinaryOpSmiTypeFeedback) {
        Handle<Smi>(Smi::zero(), isolate), BinaryOperationFeedback::kNumber},
       {Token::Value::SAR, LiteralForTest(ast_factory.GetOneByteString("2")), 1,
        Handle<Smi>(Smi::FromInt(1), isolate), BinaryOperationFeedback::kAny}};
+  ast_factory.Internalize(isolate);
 
   for (const BinaryOpExpectation& test_case : kTestCases) {
     i::FeedbackVectorSpec feedback_spec(zone);
@@ -866,7 +867,6 @@ TEST(InterpreterBinaryOpSmiTypeFeedback) {
         .BinaryOperation(test_case.op, reg, GetIndex(slot0))
         .Return();
 
-    ast_factory.Internalize(isolate);
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray(isolate);
 
     InterpreterTester tester(isolate, bytecode_array, metadata);
@@ -1358,8 +1358,6 @@ TEST(InterpreterCall) {
   Isolate* isolate = handles.main_isolate();
   Zone* zone = handles.main_zone();
   Factory* factory = isolate->factory();
-  AstValueFactory ast_factory(zone, isolate->ast_string_constants(),
-                              HashSeed(isolate));
 
   FeedbackVectorSpec feedback_spec(zone);
   FeedbackSlot slot = feedback_spec.AddLoadICSlot();
@@ -1371,10 +1369,12 @@ TEST(InterpreterCall) {
   int call_slot_index = -1;
   call_slot_index = GetIndex(call_slot);
 
-  const AstRawString* name = ast_factory.GetOneByteString("func");
-
   // Check with no args.
   {
+    AstValueFactory ast_factory(zone, isolate->ast_string_constants(),
+                                HashSeed(isolate));
+    const AstRawString* name = ast_factory.GetOneByteString("func");
+
     BytecodeArrayBuilder builder(zone, 1, 1, &feedback_spec);
     Register reg = builder.register_allocator()->NewRegister();
     RegisterList args = builder.register_allocator()->NewRegisterList(1);
@@ -1399,6 +1399,10 @@ TEST(InterpreterCall) {
 
   // Check that receiver is passed properly.
   {
+    AstValueFactory ast_factory(zone, isolate->ast_string_constants(),
+                                HashSeed(isolate));
+    const AstRawString* name = ast_factory.GetOneByteString("func");
+
     BytecodeArrayBuilder builder(zone, 1, 1, &feedback_spec);
     Register reg = builder.register_allocator()->NewRegister();
     RegisterList args = builder.register_allocator()->NewRegisterList(1);
@@ -1424,6 +1428,10 @@ TEST(InterpreterCall) {
 
   // Check with two parameters (+ receiver).
   {
+    AstValueFactory ast_factory(zone, isolate->ast_string_constants(),
+                                HashSeed(isolate));
+    const AstRawString* name = ast_factory.GetOneByteString("func");
+
     BytecodeArrayBuilder builder(zone, 1, 4, &feedback_spec);
     Register reg = builder.register_allocator()->NewRegister();
     RegisterList args = builder.register_allocator()->NewRegisterList(3);
@@ -1457,6 +1465,10 @@ TEST(InterpreterCall) {
 
   // Check with 10 parameters (+ receiver).
   {
+    AstValueFactory ast_factory(zone, isolate->ast_string_constants(),
+                                HashSeed(isolate));
+    const AstRawString* name = ast_factory.GetOneByteString("func");
+
     BytecodeArrayBuilder builder(zone, 1, 12, &feedback_spec);
     Register reg = builder.register_allocator()->NewRegister();
     RegisterList args = builder.register_allocator()->NewRegisterList(11);
@@ -2286,14 +2298,15 @@ TEST(InterpreterTestIn) {
   Isolate* isolate = handles.main_isolate();
   Zone* zone = handles.main_zone();
   Factory* factory = isolate->factory();
-  AstValueFactory ast_factory(zone, isolate->ast_string_constants(),
-                              HashSeed(isolate));
   // Allocate an array
   Handle<i::JSArray> array =
       factory->NewJSArray(0, i::ElementsKind::PACKED_SMI_ELEMENTS);
   // Check for these properties on the array object
   const char* properties[] = {"length", "fuzzle", "x", "0"};
   for (size_t i = 0; i < arraysize(properties); i++) {
+    AstValueFactory ast_factory(zone, isolate->ast_string_constants(),
+                                HashSeed(isolate));
+
     bool expected_value = (i == 0);
     FeedbackVectorSpec feedback_spec(zone);
     BytecodeArrayBuilder builder(zone, 1, 1, &feedback_spec);
@@ -2363,6 +2376,7 @@ TEST(InterpreterUnaryNotNonBoolean) {
                      false),
       std::make_pair(LiteralForTest(ast_factory.GetOneByteString("")), true),
   };
+  ast_factory.Internalize(isolate);
 
   for (size_t i = 0; i < arraysize(object_type_tuples); i++) {
     BytecodeArrayBuilder builder(zone, 1, 0);
@@ -2370,7 +2384,6 @@ TEST(InterpreterUnaryNotNonBoolean) {
     Register r0(0);
     LoadLiteralForTest(&builder, object_type_tuples[i].first);
     builder.LogicalNot(ToBooleanMode::kConvertToBoolean).Return();
-    ast_factory.Internalize(isolate);
     Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray(isolate);
     InterpreterTester tester(isolate, bytecode_array);
     auto callable = tester.GetCallable<>();

test/cctest/parsing/test-preparser.cc

@@ -724,9 +724,6 @@ TEST(PreParserScopeAnalysis) {
       CHECK(i::ScopeTestHelper::HasSkippedFunctionInside(
           scope_with_skipped_functions));
 
-      // Do scope allocation (based on the preparsed scope data).
-      CHECK(i::DeclarationScope::Analyze(&using_scope_data));
-
       // Parse the lazy function again eagerly to produce baseline data.
       i::UnoptimizedCompileState not_using_scope_state(isolate);
       i::ParseInfo not_using_scope_data(isolate, flags, &not_using_scope_state);
@@ -739,9 +736,6 @@ TEST(PreParserScopeAnalysis) {
       CHECK(!i::ScopeTestHelper::HasSkippedFunctionInside(
           scope_without_skipped_functions));
 
-      // Do normal scope allocation.
-      CHECK(i::DeclarationScope::Analyze(&not_using_scope_data));
-
       // Verify that scope allocation gave the same results when parsing w/ the
       // scope data (and skipping functions), and when parsing without.
       i::ScopeTestHelper::CompareScopes(

test/cctest/test-parsing.cc

@@ -1082,9 +1082,6 @@ TEST(ScopeUsesArgumentsSuperThis) {
       flags.set_allow_lazy_parsing(false);
       i::ParseInfo info(isolate, flags, &compile_state);
       CHECK(i::parsing::ParseProgram(&info, script, isolate));
-      CHECK(i::Rewriter::Rewrite(&info));
-      info.ast_value_factory()->Internalize(isolate);
-      CHECK(i::DeclarationScope::Analyze(&info));
       i::DeclarationScope::AllocateScopeInfos(&info, isolate);
       CHECK_NOT_NULL(info.literal());
 
@@ -1513,10 +1510,20 @@ TEST(DiscardFunctionBody) {
     i::parsing::ParseProgram(&info, script, isolate);
     function = info.literal();
     CHECK_NOT_NULL(function);
-    CHECK_EQ(1, function->body()->length());
-    i::FunctionLiteral* inner =
-        function->body()->first()->AsExpressionStatement()->expression()->
-        AsCall()->expression()->AsFunctionLiteral();
+    // The rewriter will rewrite this to
+    //     .result = (function f(){...})();
+    //     return .result;
+    // so extract the function from there.
+    CHECK_EQ(2, function->body()->length());
+    i::FunctionLiteral* inner = function->body()
+                                    ->first()
+                                    ->AsExpressionStatement()
+                                    ->expression()
+                                    ->AsAssignment()
+                                    ->value()
+                                    ->AsCall()
+                                    ->expression()
+                                    ->AsFunctionLiteral();
     i::Scope* inner_scope = inner->scope();
     i::FunctionLiteral* fun = nullptr;
     if (!inner_scope->declarations()->is_empty()) {
@@ -3526,7 +3533,6 @@ TEST(InnerAssignment) {
           info = std::make_unique<i::ParseInfo>(isolate, flags, &compile_state);
           CHECK(i::parsing::ParseProgram(info.get(), script, isolate));
         }
-        CHECK(i::Compiler::Analyze(info.get()));
         CHECK_NOT_NULL(info->literal());
 
         i::Scope* scope = info->literal()->scope();
@@ -3636,7 +3642,6 @@ TEST(MaybeAssignedParameters) {
       flags.set_allow_lazy_parsing(allow_lazy);
       info = std::make_unique<i::ParseInfo>(isolate, flags, &state);
       CHECK(i::parsing::ParseFunction(info.get(), shared, isolate));
-      CHECK(i::Compiler::Analyze(info.get()));
       CHECK_NOT_NULL(info->literal());
 
       i::Scope* scope = info->literal()->scope();
@@ -3678,7 +3683,6 @@ static void TestMaybeAssigned(Input input, const char* variable, bool module,
       std::make_unique<i::ParseInfo>(isolate, flags, &state);
 
   CHECK(i::parsing::ParseProgram(info.get(), script, isolate));
-  CHECK(i::Compiler::Analyze(info.get()));
 
   CHECK_NOT_NULL(info->literal());
   i::Scope* scope = info->literal()->scope();
@@ -7837,7 +7841,6 @@ TEST(ModuleParsingInternals) {
   flags.set_is_module(true);
   i::ParseInfo info(isolate, flags, &compile_state);
   CHECK(i::parsing::ParseProgram(&info, script, isolate));
-  CHECK(i::Compiler::Analyze(&info));
   i::FunctionLiteral* func = info.literal();
   i::ModuleScope* module_scope = func->scope()->AsModuleScope();
   i::Scope* outer_scope = module_scope->outer_scope();
@@ -10867,7 +10870,6 @@ TEST(NoPessimisticContextAllocation) {
       i::ParseInfo info(isolate, flags, &compile_state);
 
       CHECK(i::parsing::ParseProgram(&info, script, isolate));
-      CHECK(i::Compiler::Analyze(&info));
       CHECK_NOT_NULL(info.literal());
 
       i::Scope* scope = info.literal()->scope()->inner_scope();
@@ -11430,8 +11432,6 @@ TEST(LexicalLoopVariable) {
     flags.set_allow_lazy_parsing(false);
     i::ParseInfo info(isolate, flags, &compile_state);
     CHECK(i::parsing::ParseProgram(&info, script, isolate));
-    CHECK(i::Rewriter::Rewrite(&info));
-    CHECK(i::DeclarationScope::Analyze(&info));
     i::DeclarationScope::AllocateScopeInfos(&info, isolate);
     CHECK_NOT_NULL(info.literal());
 

test/unittests/heap/off-thread-factory-unittest.cc

@@ -84,8 +84,6 @@ class OffThreadFactoryTest : public TestWithIsolateAndZone {
       Parser parser(parse_info());
       parser.InitializeEmptyScopeChain(parse_info());
       parser.ParseOnBackground(parse_info(), 0, 0, kFunctionLiteralIdTopLevel);
-
-      CHECK(DeclarationScope::Analyze(parse_info()));
     }
 
     parse_info()->ast_value_factory()->Internalize(off_thread_isolate());
@@ -310,10 +308,13 @@ TEST_F(OffThreadFactoryTest, LazyFunction) {
 
 TEST_F(OffThreadFactoryTest, EagerFunction) {
   FunctionLiteral* program = ParseProgram("(function eager() {})");
+  // Rewritten to `.result = (function eager() {}); return .result`
   FunctionLiteral* eager = program->body()
                                ->at(0)
                                ->AsExpressionStatement()
                                ->expression()
+                               ->AsAssignment()
+                               ->value()
                                ->AsFunctionLiteral();
 
   OffThreadTransferHandle<SharedFunctionInfo> shared;