Improve mapping of Clang diagnostics into Carbon diagnostics (#5894)

Instead of taking the complete text of the Clang diagnostic and using it
as the message portion of a Carbon diagnostic, generate the individual
pieces separately and pass them into the Carbon diagnostic
infrastructure.

* Clang's context lines are generated by running a custom "diagnostic
renderer" and tracking which lines it wants to print as context for a
given source location. When mapping from a C++ source location back to a
Carbon location, the Carbon `Loc` structure is now fully populated,
including filling in the context line and the column number.
* Clang's snippet is generated by running a custom diagnostic renderer
that is a cut-down version of the full text diagnostic renderer that
only prints a snippet. This is then attached to the Carbon diagnostic
manually as an override for the snippet we'd usually create.

We no longer repeat the file location twice on each diagnostic, and no
longer produce a bogus "in import" line for all locations coming from
clang that point arbitrarily to the first C++ import in the Carbon file.
The `[diagnostic kind]` marker is now displayed at the end of the
diagnostic message, not on a line of its own after the snippet.

Author: zygoloid

toolchain/check/diagnostic_emitter.cpp

@@ -11,6 +11,7 @@
 #include "common/raw_string_ostream.h"
 #include "toolchain/check/diagnostic_helpers.h"
 #include "toolchain/sem_ir/absolute_node_id.h"
+#include "toolchain/sem_ir/diagnostic_loc_converter.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/stringify.h"
 
@@ -23,7 +24,28 @@ auto DiagnosticEmitter::ConvertLoc(LocIdForDiagnostics loc_id,
       loc_id.loc_id(), loc_id.is_token_only());
   for (const auto& import : imports) {
     CARBON_DIAGNOSTIC(InImport, LocationInfo, "in import");
-    context_fn(import.loc, InImport);
+    CARBON_DIAGNOSTIC(InCppInclude, LocationInfo, "in file included here");
+    CARBON_DIAGNOSTIC(InCppModule, LocationInfo, "in module imported here");
+    CARBON_DIAGNOSTIC(InCppMacroExpansion, LocationInfo,
+                      "in expansion of macro defined here");
+    switch (import.kind) {
+      case Carbon::SemIR::DiagnosticLocConverter::ImportLoc::Import:
+        // TODO: Include the library name in the note.
+        context_fn(import.loc, InImport);
+        break;
+      case Carbon::SemIR::DiagnosticLocConverter::ImportLoc::CppInclude:
+        // TODO: Include the file name in the note.
+        context_fn(import.loc, InCppInclude);
+        break;
+      case Carbon::SemIR::DiagnosticLocConverter::ImportLoc::CppModuleImport:
+        // TODO: Include the module name in the note.
+        context_fn(import.loc, InCppModule);
+        break;
+      case Carbon::SemIR::DiagnosticLocConverter::ImportLoc::CppMacroExpansion:
+        // TODO: Include the macro name in the note.
+        context_fn(import.loc, InCppMacroExpansion);
+        break;
+    }
   }
 
   // Use the token when possible, but -1 is the default value.

toolchain/check/import_cpp.cpp

@@ -141,32 +141,26 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
     llvm::SmallString<256> message;
     info.FormatDiagnostic(message);
 
+    // Render a code snippet including any highlighted ranges and fixit hints.
+    // TODO: Also include the #include stack and macro expansion stack in the
+    // diagnostic output in some way.
+    RawStringOstream snippet_stream;
     if (!info.hasSourceManager()) {
-      // If we don't have a source manager, we haven't actually started
-      // compiling yet, and this is an error from the driver or early in the
-      // frontend. Pass it on directly.
+      // If we don't have a source manager, this is an error from early in the
+      // frontend. Don't produce a snippet.
       CARBON_CHECK(info.getLocation().isInvalid());
-      diagnostic_infos_.push_back({.level = diag_level,
-                                   .import_ir_inst_id = clang_import_ir_inst_id,
-                                   .message = message.str().str()});
-      return;
+    } else {
+      CodeContextRenderer(snippet_stream, invocation_->getLangOpts(),
+                          invocation_->getDiagnosticOpts())
+          .emitDiagnostic(
+              clang::FullSourceLoc(info.getLocation(), info.getSourceManager()),
+              diag_level, message, info.getRanges(), info.getFixItHints());
     }
 
-    // TODO: This includes the full clang diagnostic, including the source
-    // location, resulting in the location appearing twice in the output.
-    RawStringOstream diagnostics_stream;
-    clang::TextDiagnostic text_diagnostic(diagnostics_stream,
-                                          invocation_->getLangOpts(),
-                                          invocation_->getDiagnosticOpts());
-    text_diagnostic.emitDiagnostic(
-        clang::FullSourceLoc(info.getLocation(), info.getSourceManager()),
-        diag_level, message, info.getRanges(), info.getFixItHints());
-
-    std::string diagnostics_str = diagnostics_stream.TakeStr();
-
     diagnostic_infos_.push_back({.level = diag_level,
                                  .import_ir_inst_id = clang_import_ir_inst_id,
-                                 .message = diagnostics_str});
+                                 .message = message.str().str(),
+                                 .snippet = snippet_stream.TakeStr()});
   }
 
   // Outputs Carbon diagnostics based on the collected Clang diagnostics. Must
@@ -200,15 +194,22 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
                   ? 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);
+            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;
         }
@@ -218,11 +219,36 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
   }
 
  private:
-  // The type-checking context in which we're running Clang.
-  Context* context_;
-
-  // The compiler invocation that is producing the diagnostics.
-  std::shared_ptr<clang::CompilerInvocation> invocation_;
+  // A diagnostics renderer based on clang's TextDiagnostic that captures just
+  // the code context (the snippet).
+  class CodeContextRenderer : public clang::TextDiagnostic {
+   public:
+    using TextDiagnostic::TextDiagnostic;
+
+    void emitDiagnosticMessage(
+        clang::FullSourceLoc /*loc*/, clang::PresumedLoc /*ploc*/,
+        clang::DiagnosticsEngine::Level /*level*/, llvm::StringRef /*message*/,
+        llvm::ArrayRef<clang::CharSourceRange> /*ranges*/,
+        clang::DiagOrStoredDiag /*info*/) override {}
+    void emitDiagnosticLoc(
+        clang::FullSourceLoc /*loc*/, clang::PresumedLoc /*ploc*/,
+        clang::DiagnosticsEngine::Level /*level*/,
+        llvm::ArrayRef<clang::CharSourceRange> /*ranges*/) override {}
+
+    // emitCodeContext is inherited from clang::TextDiagnostic.
+
+    void emitIncludeLocation(clang::FullSourceLoc /*loc*/,
+                             clang::PresumedLoc /*ploc*/) override {}
+    void emitImportLocation(clang::FullSourceLoc /*loc*/,
+                            clang::PresumedLoc /*ploc*/,
+                            llvm::StringRef /*module_name*/) override {}
+    void emitBuildingModuleLocation(clang::FullSourceLoc /*loc*/,
+                                    clang::PresumedLoc /*ploc*/,
+                                    llvm::StringRef /*module_name*/) override {}
+
+    // beginDiagnostic and endDiagnostic are inherited from
+    // clang::TextDiagnostic in case it wants to do any setup / teardown work.
+  };
 
   // Information on a Clang diagnostic that can be converted to a Carbon
   // diagnostic.
@@ -236,25 +262,56 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
 
     // The Clang diagnostic textual message.
     std::string message;
+
+    // The code snippet produced by clang.
+    std::string snippet;
   };
 
+  // The type-checking context in which we're running Clang.
+  Context* context_;
+
+  // The compiler invocation that is producing the diagnostics.
+  std::shared_ptr<clang::CompilerInvocation> invocation_;
+
   // Collects the information for all Clang diagnostics to be converted to
   // Carbon diagnostics after the context has been initialized with the Clang
   // AST.
   llvm::SmallVector<ClangDiagnosticInfo> diagnostic_infos_;
 };
 
+// A wrapper around a clang::CompilerInvocation that allows us to make a shallow
+// copy of most of the invocation and only make a deep copy of the parts that we
+// want to change.
+//
+// clang::CowCompilerInvocation almost allows this, but doesn't derive from
+// CompilerInvocation or support shallow copies from a CompilerInvocation, so is
+// not useful to us as we can't build an ASTUnit from it.
+class ShallowCopyCompilerInvocation : public clang::CompilerInvocation {
+ public:
+  explicit ShallowCopyCompilerInvocation(
+      const clang::CompilerInvocation& invocation) {
+    shallow_copy_assign(invocation);
+
+    // The preprocessor options are modified to hold a replacement includes
+    // buffer, so make our own version of those options.
+    PPOpts = std::make_shared<clang::PreprocessorOptions>(*PPOpts);
+  }
+};
+
 }  // namespace
 
 // Returns an AST for the C++ imports and a bool that represents whether
 // compilation errors where encountered or the generated AST is null due to an
 // error. Sets the AST in the context's `sem_ir`.
 // TODO: Consider to always have a (non-null) AST.
-static auto GenerateAst(Context& context,
-                        llvm::ArrayRef<Parse::Tree::PackagingNames> imports,
-                        llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
-                        std::shared_ptr<clang::CompilerInvocation> invocation)
+static auto GenerateAst(
+    Context& context, llvm::ArrayRef<Parse::Tree::PackagingNames> imports,
+    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
+    std::shared_ptr<clang::CompilerInvocation> base_invocation)
     -> std::pair<std::unique_ptr<clang::ASTUnit>, bool> {
+  auto invocation =
+      std::make_shared<ShallowCopyCompilerInvocation>(*base_invocation);
+
   // Build a diagnostics engine.
   llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diags(
       clang::CompilerInstance::createDiagnostics(
@@ -274,9 +331,10 @@ static auto GenerateAst(Context& context,
   // TODO: Modify the frontend options to specify this memory buffer as input
   // instead of remapping the file.
   std::string includes = GenerateCppIncludesHeaderCode(context, imports);
-  auto includes_buffer = llvm::MemoryBuffer::getMemBuffer(includes, file_name);
+  auto includes_buffer =
+      llvm::MemoryBuffer::getMemBufferCopy(includes, file_name);
   invocation->getPreprocessorOpts().addRemappedFile(file_name,
-                                                    includes_buffer.get());
+                                                    includes_buffer.release());
 
   clang::DiagnosticErrorTrap trap(*diags);
 
@@ -285,9 +343,6 @@ static auto GenerateAst(Context& context,
       invocation, std::make_shared<clang::PCHContainerOperations>(), nullptr,
       diags, new clang::FileManager(invocation->getFileSystemOpts(), fs));
 
-  // Remove remapped file before its underlying storage is destroyed.
-  invocation->getPreprocessorOpts().clearRemappedFiles();
-
   // Attach the AST to SemIR. This needs to be done before we can emit any
   // diagnostics, so their locations can be properly interpreted by our
   // diagnostics machinery.
@@ -377,6 +432,9 @@ static auto ClangLookup(Context& context, SemIR::NameScopeId scope_id,
   CARBON_CHECK(ast);
   clang::Sema& sema = ast->getSema();
 
+  // TODO: Map the LocId of the lookup to a clang SourceLocation and provide it
+  // here so that clang's diagnostics can point into the carbon code that uses
+  // the name.
   clang::LookupResult lookup(
       sema,
       clang::DeclarationNameInfo(

toolchain/check/testdata/interop/cpp/bad_import.carbon

@@ -34,10 +34,8 @@ import Cpp library "";
 
 library "[[@TEST_NAME]]";
 
-// CHECK:STDERR: fail_import_cpp_library_file_with_quotes.carbon:[[@LINE+6]]:1: in import [InImport]
-// CHECK:STDERR: fail_import_cpp_library_file_with_quotes.carbon:[[@LINE+5]]: error: fail_import_cpp_library_file_with_quotes.carbon:[[@LINE+5]]:10: fatal error: '\"foo.h\"' file not found
-// CHECK:STDERR:    10 | #include "\"foo.h\""
+// CHECK:STDERR: fail_import_cpp_library_file_with_quotes.carbon:[[@LINE+4]]:10: error: '\"foo.h\"' file not found [CppInteropParseError]
+// CHECK:STDERR:     8 | #include "\"foo.h\""
 // CHECK:STDERR:       |          ^~~~~~~~~~~
-// CHECK:STDERR:  [CppInteropParseError]
 // CHECK:STDERR:
 import Cpp library "\"foo.h\"";

toolchain/check/testdata/interop/cpp/class/access.carbon

@@ -50,11 +50,13 @@ library "[[@TEST_NAME]]";
 import Cpp library "non_function_member_protected.h";
 
 fn F(c: Cpp.C) {
-  // CHECK:STDERR: fail_import_non_function_member_protected.carbon:[[@LINE+6]]:16: error: cannot access protected member `x` of type `Cpp.C` [ClassInvalidMemberAccess]
+  // CHECK:STDERR: fail_import_non_function_member_protected.carbon:[[@LINE+8]]:16: error: cannot access protected member `x` of type `Cpp.C` [ClassInvalidMemberAccess]
   // CHECK:STDERR:   let x: i32 = c.x;
   // CHECK:STDERR:                ^~~
-  // CHECK:STDERR: fail_import_non_function_member_protected.carbon:[[@LINE-6]]:1: in import [InImport]
-  // CHECK:STDERR: ./non_function_member_protected.h:2: note: declared here [ClassMemberDeclaration]
+  // CHECK:STDERR: fail_import_non_function_member_protected.carbon:[[@LINE-6]]:10: in file included here [InCppInclude]
+  // CHECK:STDERR: ./non_function_member_protected.h:2:7: note: declared here [ClassMemberDeclaration]
+  // CHECK:STDERR: class C {
+  // CHECK:STDERR:       ^
   // CHECK:STDERR:
   let x: i32 = c.x;
 }
@@ -76,11 +78,13 @@ library "[[@TEST_NAME]]";
 import Cpp library "non_function_member_private.h";
 
 fn F(c: Cpp.C) {
-  // CHECK:STDERR: fail_import_non_function_member_private.carbon:[[@LINE+6]]:16: error: cannot access private member `x` of type `Cpp.C` [ClassInvalidMemberAccess]
+  // CHECK:STDERR: fail_import_non_function_member_private.carbon:[[@LINE+8]]:16: error: cannot access private member `x` of type `Cpp.C` [ClassInvalidMemberAccess]
   // CHECK:STDERR:   let x: i32 = c.x;
   // CHECK:STDERR:                ^~~
-  // CHECK:STDERR: fail_import_non_function_member_private.carbon:[[@LINE-6]]:1: in import [InImport]
-  // CHECK:STDERR: ./non_function_member_private.h:2: note: declared here [ClassMemberDeclaration]
+  // CHECK:STDERR: fail_import_non_function_member_private.carbon:[[@LINE-6]]:10: in file included here [InCppInclude]
+  // CHECK:STDERR: ./non_function_member_private.h:2:7: note: declared here [ClassMemberDeclaration]
+  // CHECK:STDERR: class C {
+  // CHECK:STDERR:       ^
   // CHECK:STDERR:
   let x: i32 = c.x;
 }

toolchain/check/testdata/interop/cpp/class/class.carbon

@@ -35,11 +35,13 @@ library "[[@TEST_NAME]]";
 import Cpp library "declaration.h";
 
 fn MyF() {
-  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE+6]]:12: error: binding pattern has incomplete type `Bar` in name binding declaration [IncompleteTypeInBindingDecl]
+  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE+8]]:12: error: binding pattern has incomplete type `Bar` in name binding declaration [IncompleteTypeInBindingDecl]
   // CHECK:STDERR:   var bar: Cpp.Bar;
   // CHECK:STDERR:            ^~~~~~~
-  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE-6]]:1: in import [InImport]
-  // CHECK:STDERR: ./declaration.h:2: note: class was forward declared here [ClassForwardDeclaredHere]
+  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE-6]]:10: in file included here [InCppInclude]
+  // CHECK:STDERR: ./declaration.h:2:7: note: class was forward declared here [ClassForwardDeclaredHere]
+  // CHECK:STDERR: class Bar;
+  // CHECK:STDERR:       ^
   // CHECK:STDERR:
   var bar: Cpp.Bar;
 }

toolchain/check/testdata/interop/cpp/class/struct.carbon

@@ -35,11 +35,13 @@ library "[[@TEST_NAME]]";
 import Cpp library "declaration.h";
 
 fn MyF() {
-  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE+6]]:12: error: binding pattern has incomplete type `Bar` in name binding declaration [IncompleteTypeInBindingDecl]
+  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE+8]]:12: error: binding pattern has incomplete type `Bar` in name binding declaration [IncompleteTypeInBindingDecl]
   // CHECK:STDERR:   var bar: Cpp.Bar;
   // CHECK:STDERR:            ^~~~~~~
-  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE-6]]:1: in import [InImport]
-  // CHECK:STDERR: ./declaration.h:2: note: class was forward declared here [ClassForwardDeclaredHere]
+  // CHECK:STDERR: fail_use_declaration_as_definition.carbon:[[@LINE-6]]:10: in file included here [InCppInclude]
+  // CHECK:STDERR: ./declaration.h:2:8: note: class was forward declared here [ClassForwardDeclaredHere]
+  // CHECK:STDERR: struct Bar;
+  // CHECK:STDERR:        ^
   // CHECK:STDERR:
   var bar: Cpp.Bar;
 }