Don't use Check::Context when emitting C++ diagnostics. (#5910)

Diagnostics can get flushed after the context is destroyed. Should fix
an issue found by msan.

Author: zygoloid

toolchain/check/import_cpp.cpp

@@ -93,13 +93,12 @@ static auto AddIdentifierName(Context& context, llvm::StringRef name)
 
 // Adds the given source location and an `ImportIRInst` referring to it in
 // `ImportIRId::Cpp`.
-static auto AddImportIRInst(Context& context,
+static auto AddImportIRInst(SemIR::File& file,
                             clang::SourceLocation clang_source_loc)
     -> SemIR::ImportIRInstId {
   SemIR::ClangSourceLocId clang_source_loc_id =
-      context.sem_ir().clang_source_locs().Add(clang_source_loc);
-  return context.import_ir_insts().Add(
-      SemIR::ImportIRInst(clang_source_loc_id));
+      file.clang_source_locs().Add(clang_source_loc);
+  return file.import_ir_insts().Add(SemIR::ImportIRInst(clang_source_loc_id));
 }
 
 namespace {
@@ -116,15 +115,22 @@ namespace {
 // wrong C++ diagnostics, and at the end of the Carbon program.
 class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
  public:
-  // Creates an instance with the location that triggers calling Clang.
-  // `context` must not be null.
+  // Creates an instance with the location that triggers calling Clang. The
+  // `context` is not stored here, and the diagnostics consumer is expected to
+  // outlive it.
   explicit CarbonClangDiagnosticConsumer(
-      Context* context, std::shared_ptr<clang::CompilerInvocation> invocation)
-      : context_(context), invocation_(std::move(invocation)) {
-    context->emitter().AddFlushFn([this] { EmitDiagnostics(); });
+      Context& context, std::shared_ptr<clang::CompilerInvocation> invocation)
+      : sem_ir_(&context.sem_ir()),
+        emitter_(&context.emitter()),
+        invocation_(std::move(invocation)) {
+    emitter_->AddFlushFn([this] { EmitDiagnostics(); });
   }
 
   ~CarbonClangDiagnosticConsumer() override {
+    // Do not inspect `emitter_` here; it's typically destroyed before the
+    // consumer is.
+    // TODO: If Clang produces diagnostics after check finishes, they'll get
+    // added to the list of pending diagnostics and never emitted.
     CARBON_CHECK(diagnostic_infos_.empty(),
                  "Missing flush before destroying diagnostic consumer");
   }
@@ -136,7 +142,7 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
     DiagnosticConsumer::HandleDiagnostic(diag_level, info);
 
     SemIR::ImportIRInstId clang_import_ir_inst_id =
-        AddImportIRInst(*context_, info.getLocation());
+        AddImportIRInst(*sem_ir_, info.getLocation());
 
     llvm::SmallString<256> message;
     info.FormatDiagnostic(message);
@@ -163,57 +169,57 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
                                  .snippet = snippet_stream.TakeStr()});
   }
 
+  // Returns the diagnostic to use for a given Clang diagnostic level.
+  static auto GetDiagnostic(clang::DiagnosticsEngine::Level level)
+      -> const Diagnostics::DiagnosticBase<std::string>& {
+    switch (level) {
+      case clang::DiagnosticsEngine::Ignored: {
+        CARBON_FATAL("Emitting an ignored diagnostic");
+        break;
+      }
+      case clang::DiagnosticsEngine::Note: {
+        CARBON_DIAGNOSTIC(CppInteropParseNote, Note, "{0}", std::string);
+        return CppInteropParseNote;
+      }
+      case clang::DiagnosticsEngine::Remark:
+      case clang::DiagnosticsEngine::Warning: {
+        // TODO: Add a distinct Remark level to Carbon diagnostics, and stop
+        // mapping remarks to warnings.
+        CARBON_DIAGNOSTIC(CppInteropParseWarning, Warning, "{0}", std::string);
+        return CppInteropParseWarning;
+      }
+      case clang::DiagnosticsEngine::Error:
+      case clang::DiagnosticsEngine::Fatal: {
+        CARBON_DIAGNOSTIC(CppInteropParseError, Error, "{0}", std::string);
+        return CppInteropParseError;
+      }
+    }
+  }
+
   // Outputs Carbon diagnostics based on the collected Clang diagnostics. Must
   // be called after the AST is set in the context.
   auto EmitDiagnostics() -> void {
-    CARBON_CHECK(context_->sem_ir().cpp_ast(),
+    CARBON_CHECK(sem_ir_->cpp_ast(),
                  "Attempted to emit diagnostics before the AST Unit is loaded");
 
     for (size_t i = 0; i != diagnostic_infos_.size(); ++i) {
       const ClangDiagnosticInfo& info = diagnostic_infos_[i];
-      switch (info.level) {
-        case clang::DiagnosticsEngine::Ignored:
-        case clang::DiagnosticsEngine::Note:
-        case clang::DiagnosticsEngine::Remark: {
-          context_->TODO(
-              SemIR::LocId(info.import_ir_inst_id),
-              llvm::formatv(
-                  "Unsupported: C++ diagnostic level for diagnostic\n{0}",
-                  info.message));
-          break;
-        }
-        case clang::DiagnosticsEngine::Warning:
-        case clang::DiagnosticsEngine::Error:
-        case clang::DiagnosticsEngine::Fatal: {
-          CARBON_DIAGNOSTIC(CppInteropParseWarning, Warning, "{0}",
-                            std::string);
-          CARBON_DIAGNOSTIC(CppInteropParseError, Error, "{0}", std::string);
-          auto builder = context_->emitter().Build(
-              SemIR::LocId(info.import_ir_inst_id),
-              info.level == clang::DiagnosticsEngine::Warning
-                  ? CppInteropParseWarning
-                  : CppInteropParseError,
-              info.message);
-          builder.OverrideSnippet(info.snippet);
-          for (;
-               i + 1 < diagnostic_infos_.size() &&
-               diagnostic_infos_[i + 1].level == clang::DiagnosticsEngine::Note;
-               ++i) {
-            const ClangDiagnosticInfo& note_info = diagnostic_infos_[i + 1];
-            CARBON_DIAGNOSTIC(CppInteropParseNote, Note, "{0}", std::string);
-            builder
-                .Note(SemIR::LocId(note_info.import_ir_inst_id),
-                      CppInteropParseNote, note_info.message)
-                .OverrideSnippet(note_info.snippet);
-          }
-          // TODO: This will apply all current Carbon annotation functions. We
-          // should instead track how Clang's context notes and Carbon's
-          // annotation functions are interleaved, and interleave the notes in
-          // the same order.
-          builder.Emit();
-          break;
-        }
+      auto builder = emitter_->Build(SemIR::LocId(info.import_ir_inst_id),
+                                     GetDiagnostic(info.level), info.message);
+      builder.OverrideSnippet(info.snippet);
+      for (; i + 1 < diagnostic_infos_.size() &&
+             diagnostic_infos_[i + 1].level == clang::DiagnosticsEngine::Note;
+           ++i) {
+        const ClangDiagnosticInfo& note_info = diagnostic_infos_[i + 1];
+        builder
+            .Note(SemIR::LocId(note_info.import_ir_inst_id),
+                  GetDiagnostic(note_info.level), note_info.message)
+            .OverrideSnippet(note_info.snippet);
       }
+      // TODO: This will apply all current Carbon annotation functions. We
+      // should instead track how Clang's context notes and Carbon's annotation
+      // functions are interleaved, and interleave the notes in the same order.
+      builder.Emit();
     }
     diagnostic_infos_.clear();
   }
@@ -267,8 +273,11 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
     std::string snippet;
   };
 
-  // The type-checking context in which we're running Clang.
-  Context* context_;
+  // The Carbon file that this C++ compilation is attached to.
+  SemIR::File* sem_ir_;
+
+  // The diagnostic emitter that we're emitting diagnostics into.
+  DiagnosticEmitterBase* emitter_;
 
   // The compiler invocation that is producing the diagnostics.
   std::shared_ptr<clang::CompilerInvocation> invocation_;
@@ -316,7 +325,7 @@ static auto GenerateAst(
   llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diags(
       clang::CompilerInstance::createDiagnostics(
           *fs, invocation->getDiagnosticOpts(),
-          new CarbonClangDiagnosticConsumer(&context, invocation),
+          new CarbonClangDiagnosticConsumer(context, invocation),
           /*ShouldOwnClient=*/true));
 
   // Extract the input from the frontend invocation and make sure it makes
@@ -587,7 +596,8 @@ static auto BuildClassDecl(Context& context,
 static auto ImportCXXRecordDecl(Context& context,
                                 clang::CXXRecordDecl* clang_decl)
     -> SemIR::InstId {
-  auto import_ir_inst_id = AddImportIRInst(context, clang_decl->getLocation());
+  auto import_ir_inst_id =
+      AddImportIRInst(context.sem_ir(), clang_decl->getLocation());
 
   auto [class_id, class_inst_id] = BuildClassDecl(
       context, import_ir_inst_id, GetParentNameScopeId(context, clang_decl),