Combine DiagnosticConverter into DiagnosticEmitter (#4878)

At present, we typically define a DiagnosticConverter, then store an instance of it and a DiagnosticEmitter that wraps it. This is relatively minor in general, but I've been trying to create more self-contained DiagnosticEmitter classes (which hold their own DiagnosticConverter, similar to NullDiagnosticEmitter), and there it just gets in the way. Since we don't reuse DiagnosticConverter instances, this combines the definition into DiagnosticEmitter. Mainly this means we don't have a separate object in play, and less to carry around. The most impact is probably to SemIRDiagnosticConverter, which was also the most complex. Now `SemIRLocDiagnosticEmitter`, this gets some different construction flow. Note in the PR I've split the file rename to its own commit, to try to help delta views. However, the most substantial parts of the refactoring are split into #4876, which this depends upon.
carbon-language · Feb 6, 2025 · e79d3be · e79d3be
1 parent a16a17d
commit e79d3be
Show file tree

Hide file tree

Showing 31 changed files with 264 additions and 316 deletions.
diff --git a/toolchain/check/BUILD b/toolchain/check/BUILD
@@ -139,7 +139,7 @@ cc_library(
         ":dump",
         ":impl",
         ":pointer_dereference",
-        ":sem_ir_diagnostic_converter",
+        ":sem_ir_loc_diagnostic_emitter",
         "//common:check",
         "//common:error",
         "//common:map",
@@ -263,9 +263,9 @@ cc_library(
 )
 
 cc_library(
-    name = "sem_ir_diagnostic_converter",
-    srcs = ["sem_ir_diagnostic_converter.cpp"],
-    hdrs = ["sem_ir_diagnostic_converter.h"],
+    name = "sem_ir_loc_diagnostic_emitter",
+    srcs = ["sem_ir_loc_diagnostic_emitter.cpp"],
+    hdrs = ["sem_ir_loc_diagnostic_emitter.h"],
     deps = [
         ":context",
         "//common:raw_string_ostream",

diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp
@@ -9,7 +9,7 @@
 #include "toolchain/check/check_unit.h"
 #include "toolchain/check/context.h"
 #include "toolchain/check/diagnostic_helpers.h"
-#include "toolchain/check/sem_ir_diagnostic_converter.h"
+#include "toolchain/check/sem_ir_loc_diagnostic_emitter.h"
 #include "toolchain/diagnostics/diagnostic.h"
 #include "toolchain/diagnostics/format_providers.h"
 #include "toolchain/lex/token_kind.h"
@@ -320,9 +320,12 @@ auto CheckParseTrees(llvm::MutableArrayRef<Unit> units, bool prelude_import,
   // UnitAndImports is big due to its SmallVectors, so we default to 0 on the
   // stack.
   llvm::SmallVector<UnitAndImports, 0> unit_infos;
+  llvm::SmallVector<Parse::GetTreeAndSubtreesFn> tree_and_subtrees_getters;
   unit_infos.reserve(units.size());
+  tree_and_subtrees_getters.reserve(units.size());
   for (auto [i, unit] : llvm::enumerate(units)) {
     unit_infos.emplace_back(SemIR::CheckIRId(i), unit);
+    tree_and_subtrees_getters.push_back(unit.tree_and_subtrees_getter);
   }
 
   Map<ImportKey, UnitAndImports*> api_map =
@@ -372,7 +375,7 @@ auto CheckParseTrees(llvm::MutableArrayRef<Unit> units, bool prelude_import,
   for (int check_index = 0;
        check_index < static_cast<int>(ready_to_check.size()); ++check_index) {
     auto* unit_info = ready_to_check[check_index];
-    CheckUnit(unit_info, units.size(), fs, vlog_stream).Run();
+    CheckUnit(unit_info, tree_and_subtrees_getters, fs, vlog_stream).Run();
     for (auto* incoming_import : unit_info->incoming_imports) {
       --incoming_import->imports_remaining;
       if (incoming_import->imports_remaining == 0) {
@@ -419,7 +422,7 @@ auto CheckParseTrees(llvm::MutableArrayRef<Unit> units, bool prelude_import,
     // incomplete imports.
     for (auto& unit_info : unit_infos) {
       if (unit_info.imports_remaining > 0) {
-        CheckUnit(&unit_info, units.size(), fs, vlog_stream).Run();
+        CheckUnit(&unit_info, tree_and_subtrees_getters, fs, vlog_stream).Run();
       }
     }
   }

diff --git a/toolchain/check/check.h b/toolchain/check/check.h
@@ -8,7 +8,7 @@
 #include "common/ostream.h"
 #include "toolchain/base/shared_value_stores.h"
 #include "toolchain/base/timings.h"
-#include "toolchain/check/sem_ir_diagnostic_converter.h"
+#include "toolchain/check/sem_ir_loc_diagnostic_emitter.h"
 #include "toolchain/diagnostics/diagnostic_emitter.h"
 #include "toolchain/parse/tree_and_subtrees.h"
 #include "toolchain/sem_ir/file.h"
@@ -23,13 +23,10 @@ struct Unit {
   Timings* timings;
 
   // Returns a lazily constructed TreeAndSubtrees.
-  Parse::GetTreeAndSubtreesFn get_parse_tree_and_subtrees;
+  Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter;
 
   // The unit's SemIR, provided as empty and filled in by CheckParseTrees.
   SemIR::File* sem_ir;
-
-  // Diagnostic converters.
-  SemIRDiagnosticConverter* sem_ir_converter;
 };
 
 // Checks a group of parse trees. This will use imports to decide the order of

diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp
@@ -34,19 +34,21 @@ static auto GetImportedIRCount(UnitAndImports* unit_and_imports) -> int {
   return count;
 }
 
-CheckUnit::CheckUnit(UnitAndImports* unit_and_imports, int total_ir_count,
-                     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
-                     llvm::raw_ostream* vlog_stream)
+CheckUnit::CheckUnit(
+    UnitAndImports* unit_and_imports,
+    llvm::ArrayRef<Parse::GetTreeAndSubtreesFn> tree_and_subtrees_getters,
+    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
+    llvm::raw_ostream* vlog_stream)
     : unit_and_imports_(unit_and_imports),
-      total_ir_count_(total_ir_count),
+      total_ir_count_(tree_and_subtrees_getters.size()),
       fs_(std::move(fs)),
       vlog_stream_(vlog_stream),
-      emitter_(*unit_and_imports_->unit->sem_ir_converter,
-               unit_and_imports_->err_tracker),
-      context_(&emitter_, unit_and_imports_->unit->get_parse_tree_and_subtrees,
+      emitter_(&unit_and_imports_->err_tracker, tree_and_subtrees_getters,
+               unit_and_imports_->unit->sem_ir),
+      context_(&emitter_, unit_and_imports_->unit->tree_and_subtrees_getter,
                unit_and_imports_->unit->sem_ir,
-               GetImportedIRCount(unit_and_imports), total_ir_count,
-               vlog_stream) {}
+               GetImportedIRCount(unit_and_imports),
+               tree_and_subtrees_getters.size(), vlog_stream) {}
 
 auto CheckUnit::Run() -> void {
   Timings::ScopedTiming timing(unit_and_imports_->unit->timings, "check");
@@ -363,7 +365,7 @@ auto CheckUnit::ProcessNodeIds() -> bool {
 
   // On crash, report which token we were handling.
   PrettyStackTraceFunction node_dumper([&](llvm::raw_ostream& output) {
-    const auto& tree = unit_and_imports_->unit->get_parse_tree_and_subtrees();
+    const auto& tree = unit_and_imports_->unit->tree_and_subtrees_getter();
     auto converted = tree.NodeToDiagnosticLoc(node_id, /*token_only=*/false);
     converted.loc.FormatLocation(output);
     output << "checking " << context_.parse_tree().node_kind(node_id) << "\n";
@@ -374,8 +376,7 @@ auto CheckUnit::ProcessNodeIds() -> bool {
   while (auto maybe_node_id = traversal.Next()) {
     node_id = *maybe_node_id;
 
-    unit_and_imports_->unit->sem_ir_converter->AdvanceToken(
-        context_.parse_tree().node_token(node_id));
+    emitter_.AdvanceToken(context_.parse_tree().node_token(node_id));
 
     if (context_.parse_tree().node_has_error(node_id)) {
       context_.TODO(node_id, "handle invalid parse trees in `check`");

diff --git a/toolchain/check/check_unit.h b/toolchain/check/check_unit.h
@@ -9,6 +9,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "toolchain/check/check.h"
 #include "toolchain/check/context.h"
+#include "toolchain/check/sem_ir_loc_diagnostic_emitter.h"
 #include "toolchain/sem_ir/ids.h"
 
 namespace Carbon::Check {
@@ -43,33 +44,33 @@ struct PackageImports {
   llvm::SmallVector<Import> imports;
 };
 
-// Converts a `NodeId` to a diagnostic location for `UnitAndImports`.
-class UnitAndImportsDiagnosticConverter
-    : public DiagnosticConverter<Parse::NodeId> {
- public:
-  explicit UnitAndImportsDiagnosticConverter(
-      Parse::GetTreeAndSubtreesFn get_parse_tree_and_subtrees)
-      : get_parse_tree_and_subtrees_(get_parse_tree_and_subtrees) {}
-
-  auto ConvertLoc(Parse::NodeId node_id, ContextFnT /*context_fn*/) const
-      -> ConvertedDiagnosticLoc override {
-    return get_parse_tree_and_subtrees_().NodeToDiagnosticLoc(
-        node_id, /*token_only=*/false);
-  }
-
- private:
-  Parse::GetTreeAndSubtreesFn get_parse_tree_and_subtrees_;
-};
-
 // Contains information accumulated while checking a `Unit` (primarily import
 // information), in addition to the `Unit` itself.
 struct UnitAndImports {
+  // Converts a `NodeId` to a diagnostic location for `UnitAndImports`.
+  class Emitter : public DiagnosticEmitter<Parse::NodeId> {
+   public:
+    explicit Emitter(DiagnosticConsumer* consumer,
+                     Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter)
+        : DiagnosticEmitter(consumer),
+          tree_and_subtrees_getter_(tree_and_subtrees_getter) {}
+
+   protected:
+    auto ConvertLoc(Parse::NodeId node_id, ContextFnT /*context_fn*/) const
+        -> ConvertedDiagnosticLoc override {
+      return tree_and_subtrees_getter_().NodeToDiagnosticLoc(
+          node_id, /*token_only=*/false);
+    }
+
+   private:
+    Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter_;
+  };
+
   explicit UnitAndImports(SemIR::CheckIRId check_ir_id, Unit& unit)
       : check_ir_id(check_ir_id),
         unit(&unit),
         err_tracker(*unit.consumer),
-        converter(unit.get_parse_tree_and_subtrees),
-        emitter(converter, err_tracker) {}
+        emitter(&err_tracker, unit.tree_and_subtrees_getter) {}
 
   auto parse_tree() -> const Parse::Tree& { return unit->sem_ir->parse_tree(); }
   auto source() -> const SourceBuffer& {
@@ -81,8 +82,7 @@ struct UnitAndImports {
 
   // Emitter information.
   ErrorTrackingDiagnosticConsumer err_tracker;
-  UnitAndImportsDiagnosticConverter converter;
-  DiagnosticEmitter<Parse::NodeId> emitter;
+  Emitter emitter;
 
   // List of the outgoing imports. If a package includes unavailable library
   // imports, it has an entry with has_load_error set. Invalid imports (for
@@ -119,9 +119,11 @@ struct UnitAndImports {
 // logic in check.cpp.
 class CheckUnit {
  public:
-  explicit CheckUnit(UnitAndImports* unit_and_imports, int total_ir_count,
-                     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
-                     llvm::raw_ostream* vlog_stream);
+  explicit CheckUnit(
+      UnitAndImports* unit_and_imports,
+      llvm::ArrayRef<Parse::GetTreeAndSubtreesFn> tree_and_subtrees_getters,
+      llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
+      llvm::raw_ostream* vlog_stream);
 
   // Produces and checks the IR for the provided unit.
   auto Run() -> void;
@@ -171,7 +173,7 @@ class CheckUnit {
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs_;
   llvm::raw_ostream* vlog_stream_;
 
-  Context::DiagnosticEmitter emitter_;
+  SemIRLocDiagnosticEmitter emitter_;
   Context context_;
 };
 

diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp
@@ -39,11 +39,11 @@
 namespace Carbon::Check {
 
 Context::Context(DiagnosticEmitter* emitter,
-                 Parse::GetTreeAndSubtreesFn get_parse_tree_and_subtrees,
+                 Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter,
                  SemIR::File* sem_ir, int imported_ir_count, int total_ir_count,
                  llvm::raw_ostream* vlog_stream)
     : emitter_(emitter),
-      get_parse_tree_and_subtrees_(get_parse_tree_and_subtrees),
+      tree_and_subtrees_getter_(tree_and_subtrees_getter),
       sem_ir_(sem_ir),
       vlog_stream_(vlog_stream),
       node_stack_(sem_ir->parse_tree(), vlog_stream),

diff --git a/toolchain/check/context.h b/toolchain/check/context.h
@@ -73,7 +73,7 @@ class Context {
 
   // Stores references for work.
   explicit Context(DiagnosticEmitter* emitter,
-                   Parse::GetTreeAndSubtreesFn get_parse_tree_and_subtrees,
+                   Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter,
                    SemIR::File* sem_ir, int imported_ir_count,
                    int total_ir_count, llvm::raw_ostream* vlog_stream);
 
@@ -540,7 +540,7 @@ class Context {
   auto emitter() -> DiagnosticEmitter& { return *emitter_; }
 
   auto parse_tree_and_subtrees() -> const Parse::TreeAndSubtrees& {
-    return get_parse_tree_and_subtrees_();
+    return tree_and_subtrees_getter_();
   }
 
   auto sem_ir() -> SemIR::File& { return *sem_ir_; }
@@ -747,7 +747,7 @@ class Context {
   DiagnosticEmitter* emitter_;
 
   // Returns a lazily constructed TreeAndSubtrees.
-  Parse::GetTreeAndSubtreesFn get_parse_tree_and_subtrees_;
+  Parse::GetTreeAndSubtreesFn tree_and_subtrees_getter_;
 
   // The SemIR::File being added to.
   SemIR::File* sem_ir_;

diff --git a/toolchain/check/diagnostic_helpers.h b/toolchain/check/diagnostic_helpers.h
@@ -33,7 +33,7 @@ class SemIRLoc {
 
  private:
   // Only allow member access for diagnostics.
-  friend class SemIRDiagnosticConverter;
+  friend class SemIRLocDiagnosticEmitter;
 
   union {
     SemIR::InstId inst_id_;

diff --git a/...ain/check/sem_ir_diagnostic_converter.cpp → ...n/check/sem_ir_loc_diagnostic_emitter.cpp b/...ain/check/sem_ir_diagnostic_converter.cpp → ...n/check/sem_ir_loc_diagnostic_emitter.cpp
@@ -2,16 +2,16 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include "toolchain/check/sem_ir_diagnostic_converter.h"
+#include "toolchain/check/sem_ir_loc_diagnostic_emitter.h"
 
 #include "common/raw_string_ostream.h"
 #include "toolchain/sem_ir/absolute_node_id.h"
 #include "toolchain/sem_ir/stringify_type.h"
 
 namespace Carbon::Check {
 
-auto SemIRDiagnosticConverter::ConvertLoc(SemIRLoc loc,
-                                          ContextFnT context_fn) const
+auto SemIRLocDiagnosticEmitter::ConvertLoc(SemIRLoc loc,
+                                           ContextFnT context_fn) const
     -> ConvertedDiagnosticLoc {
   auto converted = ConvertLocImpl(loc, context_fn);
 
@@ -33,8 +33,8 @@ auto SemIRDiagnosticConverter::ConvertLoc(SemIRLoc loc,
   return converted;
 }
 
-auto SemIRDiagnosticConverter::ConvertLocImpl(SemIRLoc loc,
-                                              ContextFnT context_fn) const
+auto SemIRLocDiagnosticEmitter::ConvertLocImpl(SemIRLoc loc,
+                                               ContextFnT context_fn) const
     -> ConvertedDiagnosticLoc {
   llvm::SmallVector<SemIR::AbsoluteNodeId> absolute_node_ids =
       loc.is_inst_id_ ? SemIR::GetAbsoluteNodeId(sem_ir_, loc.inst_id_)
@@ -57,16 +57,16 @@ auto SemIRDiagnosticConverter::ConvertLocImpl(SemIRLoc loc,
   return ConvertLocInFile(final_node_id, loc.token_only_, context_fn);
 }
 
-auto SemIRDiagnosticConverter::ConvertLocInFile(
+auto SemIRLocDiagnosticEmitter::ConvertLocInFile(
     SemIR::AbsoluteNodeId absolute_node_id, bool token_only,
     ContextFnT /*context_fn*/) const -> ConvertedDiagnosticLoc {
   const auto& tree_and_subtrees =
-      imported_trees_and_subtrees_[absolute_node_id.check_ir_id.index]();
+      tree_and_subtrees_getters_[absolute_node_id.check_ir_id.index]();
   return tree_and_subtrees.NodeToDiagnosticLoc(absolute_node_id.node_id,
                                                token_only);
 }
 
-auto SemIRDiagnosticConverter::ConvertArg(llvm::Any arg) const -> llvm::Any {
+auto SemIRLocDiagnosticEmitter::ConvertArg(llvm::Any arg) const -> llvm::Any {
   if (auto* library_name_id = llvm::any_cast<SemIR::LibraryNameId>(&arg)) {
     std::string library_name;
     if (*library_name_id == SemIR::LibraryNameId::Default) {
@@ -118,7 +118,7 @@ auto SemIRDiagnosticConverter::ConvertArg(llvm::Any arg) const -> llvm::Any {
     return llvm::APSInt(typed_int->value,
                         !sem_ir_->types().IsSignedInt(typed_int->type));
   }
-  return DiagnosticConverter<SemIRLoc>::ConvertArg(arg);
+  return DiagnosticEmitter<SemIRLoc>::ConvertArg(arg);
 }
 
 }  // namespace Carbon::Check