From e7b68572fa242d34c0b0db12b950e95631ada351 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Mon, 24 Feb 2025 18:07:43 -0800 Subject: [PATCH] Consolidate post-check logic (#5003) Right now, some post-run logic is does in `Run()` (`CheckRequiredDefinitions();` and `context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error());`) whereas other parts are done by `Finalize`. Noting the goal to move things off `Context`, this consolidates into a new `FinishRun`. Note #4962 is adding another bit of post-run that can be consolidated in; this seems likely to keep growing slowly. Note this also creates more parity with mutation source, like the `context_.scope_stack().Pop();` matches the push done by `CheckUnit::ImportCurrentPackage` and `context_.inst_block_stack().Pop()` was pushed in `CheckUnit::Run()`. Also makes `exports()` more consistent with other Context APIs. Makes `VerifyOnFinish` `const` so that it can't accidentally mutate state, and is instead only validating that the Context is in its expected configuration at completion. --- toolchain/check/check_unit.cpp | 39 +++++++++++++--------- toolchain/check/check_unit.h | 12 ++++--- toolchain/check/context.cpp | 20 ++++------- toolchain/check/context.h | 9 ++--- toolchain/check/decl_name_stack.cpp | 2 +- toolchain/check/handle_export.cpp | 2 +- toolchain/check/param_and_arg_refs_stack.h | 2 +- toolchain/check/scope_stack.cpp | 2 +- toolchain/check/scope_stack.h | 2 +- 9 files changed, 46 insertions(+), 44 deletions(-) diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index b7ff88304a00..5854a5091ed8 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -76,22 +76,7 @@ auto CheckUnit::Run() -> void { return; } - CheckRequiredDefinitions(); - - CheckRequiredDeclarations(); - - context_.Finalize(); - - context_.VerifyOnFinish(); - - context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error()); - -#ifndef NDEBUG - if (auto verify = context_.sem_ir().Verify(); !verify.ok()) { - CARBON_FATAL("{0}Built invalid semantics IR: {1}\n", context_.sem_ir(), - verify.error()); - } -#endif + FinishRun(); } auto CheckUnit::InitPackageScopeAndImports() -> void { @@ -505,4 +490,26 @@ auto CheckUnit::CheckRequiredDefinitions() -> void { } } +auto CheckUnit::FinishRun() -> void { + CheckRequiredDeclarations(); + CheckRequiredDefinitions(); + + // Pop information for the file-level scope. + context_.sem_ir().set_top_inst_block_id(context_.inst_block_stack().Pop()); + context_.scope_stack().Pop(); + + // Finalizes the list of exports on the IR. + context_.inst_blocks().Set(SemIR::InstBlockId::Exports, context_.exports()); + // Finalizes the ImportRef inst block. + context_.inst_blocks().Set(SemIR::InstBlockId::ImportRefs, + context_.import_ref_ids()); + // Finalizes __global_init. + context_.global_init().Finalize(); + + context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error()); + + // Verify that Context cleanly finished. + context_.VerifyOnFinish(); +} + } // namespace Carbon::Check diff --git a/toolchain/check/check_unit.h b/toolchain/check/check_unit.h index 9c2060e12362..f5ae799655d0 100644 --- a/toolchain/check/check_unit.h +++ b/toolchain/check/check_unit.h @@ -153,16 +153,20 @@ class CheckUnit { // Imports all other Carbon packages (excluding the current package). auto ImportOtherPackages(SemIR::TypeId namespace_type_id) -> void; + // Checks that each required declaration is available. This applies for + // declarations that should exist in an owning library, for which an extern + // declaration exists that assigns ownership to the current API. + auto CheckRequiredDeclarations() -> void; + // Checks that each required definition is available. If the definition can be // generated by resolving a specific, does so, otherwise emits a diagnostic // for each declaration in context.definitions_required() that doesn't have a // definition. auto CheckRequiredDefinitions() -> void; - // Checks that each required declaration is available. This applies for - // declarations that should exist in an owning library, for which an extern - // declaration exists that assigns ownership to the current API. - auto CheckRequiredDeclarations() -> void; + // Does work after processing the parse tree, such as finishing the IR and + // checking for missing contents. + auto FinishRun() -> void; // Loops over all nodes in the tree. On some errors, this may return early, // for example if an unrecoverable state is encountered. diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index ffaaf026593d..26fe5c15bbb2 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -75,7 +75,7 @@ auto Context::TODO(SemIRLoc loc, std::string label) -> bool { return false; } -auto Context::VerifyOnFinish() -> void { +auto Context::VerifyOnFinish() const -> void { // Information in all the various context objects should be cleaned up as // various pieces of context go out of scope. At this point, nothing should // remain. @@ -89,19 +89,13 @@ auto Context::VerifyOnFinish() -> void { // decl_introducer_state_stack_. scope_stack_.VerifyOnFinish(); // TODO: Add verification for generic_region_stack_. -} -auto Context::Finalize() -> void { - // Pop information for the file-level scope. - sem_ir().set_top_inst_block_id(inst_block_stack().Pop()); - scope_stack().Pop(); - - // Finalizes the list of exports on the IR. - inst_blocks().Set(SemIR::InstBlockId::Exports, exports_); - // Finalizes the ImportRef inst block. - inst_blocks().Set(SemIR::InstBlockId::ImportRefs, import_ref_ids_); - // Finalizes __global_init. - global_init_.Finalize(); +#ifndef NDEBUG + if (auto verify = sem_ir_->Verify(); !verify.ok()) { + CARBON_FATAL("{0}Built invalid semantics IR: {1}\n", sem_ir_, + verify.error()); + } +#endif } auto Context::PrintForStackDump(llvm::raw_ostream& output) const -> void { diff --git a/toolchain/check/context.h b/toolchain/check/context.h index bd9a5b71ab96..58d8d11b9d9a 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -65,12 +65,7 @@ class Context { auto TODO(SemIRLoc loc, std::string label) -> bool; // Runs verification that the processing cleanly finished. - auto VerifyOnFinish() -> void; - - // Adds an exported name. - auto AddExport(SemIR::InstId inst_id) -> void { exports_.push_back(inst_id); } - - auto Finalize() -> void; + auto VerifyOnFinish() const -> void; // Prints information for a stack dump. auto PrintForStackDump(llvm::raw_ostream& output) const -> void; @@ -146,6 +141,8 @@ class Context { auto vtable_stack() -> InstBlockStack& { return vtable_stack_; } + auto exports() -> llvm::SmallVector& { return exports_; } + auto check_ir_map() -> llvm::MutableArrayRef { return check_ir_map_; } diff --git a/toolchain/check/decl_name_stack.cpp b/toolchain/check/decl_name_stack.cpp index 9045065f46a9..3ab9c52446a2 100644 --- a/toolchain/check/decl_name_stack.cpp +++ b/toolchain/check/decl_name_stack.cpp @@ -158,7 +158,7 @@ auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id, // scope. Otherwise, it's in some other entity, such as a class. if (access_kind == SemIR::AccessKind::Public && name_context.initial_scope_index == ScopeIndex::Package) { - context_->AddExport(target_id); + context_->exports().push_back(target_id); } name_scope.AddRequired({.name_id = name_context.name_id, diff --git a/toolchain/check/handle_export.cpp b/toolchain/check/handle_export.cpp index 1a11e3900361..4ce47f73b94e 100644 --- a/toolchain/check/handle_export.cpp +++ b/toolchain/check/handle_export.cpp @@ -75,7 +75,7 @@ auto HandleParseNode(Context& context, Parse::ExportDeclId node_id) -> bool { {.type_id = import_ref->type_id, .entity_name_id = import_ref->entity_name_id, .value_id = inst_id}); - context.AddExport(export_id); + context.exports().push_back(export_id); // Replace the ImportRef in name lookup, both for the above duplicate // diagnostic and so that cross-package imports can find it easily. diff --git a/toolchain/check/param_and_arg_refs_stack.h b/toolchain/check/param_and_arg_refs_stack.h index 41f631482af8..7cafa93b3ef5 100644 --- a/toolchain/check/param_and_arg_refs_stack.h +++ b/toolchain/check/param_and_arg_refs_stack.h @@ -69,7 +69,7 @@ class ParamAndArgRefsStack { } // Runs verification that the processing cleanly finished. - auto VerifyOnFinish() -> void { stack_.VerifyOnFinish(); } + auto VerifyOnFinish() const -> void { stack_.VerifyOnFinish(); } // Prints the stack for a stack dump. auto PrintForStackDump(int indent, llvm::raw_ostream& output) const -> void { diff --git a/toolchain/check/scope_stack.cpp b/toolchain/check/scope_stack.cpp index 5f31128d5869..ff6fa1621487 100644 --- a/toolchain/check/scope_stack.cpp +++ b/toolchain/check/scope_stack.cpp @@ -9,7 +9,7 @@ namespace Carbon::Check { -auto ScopeStack::VerifyOnFinish() -> void { +auto ScopeStack::VerifyOnFinish() const -> void { CARBON_CHECK(scope_stack_.empty(), "{0}", scope_stack_.size()); } diff --git a/toolchain/check/scope_stack.h b/toolchain/check/scope_stack.h index 2169578e6063..cae4d4b90260 100644 --- a/toolchain/check/scope_stack.h +++ b/toolchain/check/scope_stack.h @@ -155,7 +155,7 @@ class ScopeStack { auto Restore(SuspendedScope scope) -> void; // Runs verification that the processing cleanly finished. - auto VerifyOnFinish() -> void; + auto VerifyOnFinish() const -> void; auto return_scope_stack() -> llvm::SmallVector& { return return_scope_stack_;