Emit diagnostics produced by Clang after the ASTUnit is constructed. (#5876)

Previously we would drop these diagnostics; now periodically flush them
to Carbon's diagnostics emitter. We flush them at the end of checking,
and also immediately before changing the set of diagnostic annotation
scopes, so that Clang diagnostics get properly annotated.

This exposes some double diagnostics being produced in situations where
Clang's name lookup logic would produce a diagnostic and we also
produced one. For access control issues, use the Carbon diagnostic, in
order to properly handle protected access. For ambiguity issues, use the
Clang diagnostic that produces helpful notes.

Also fix rendering of note diagnostics produced by Clang, by attaching
them to the prior error / warning diagnostic.

---------

Co-authored-by: Boaz Brickner <[email protected]>
Co-authored-by: Jon Ross-Perkins <[email protected]>

Author: 3 people

toolchain/check/import_cpp.cpp

@@ -105,13 +105,29 @@ static auto AddImportIRInst(Context& context,
 namespace {
 
 // Used to convert Clang diagnostics to Carbon diagnostics.
+//
+// Handling of Clang notes is a little subtle: as far as Clang is concerned,
+// notes are separate diagnostics, not connected to the error or warning that
+// precedes them. But in Carbon's diagnostics system, notes are part of the
+// enclosing diagnostic. To handle this, we buffer Clang diagnostics until we
+// reach a point where we know we're not in the middle of a diagnostic, and then
+// emit a diagnostic along with all of its notes. This is triggered when adding
+// or removing a Carbon context note, which could otherwise get attached to the
+// 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.
   explicit CarbonClangDiagnosticConsumer(
       Context* context, std::shared_ptr<clang::CompilerInvocation> invocation)
-      : context_(context), invocation_(std::move(invocation)) {}
+      : context_(context), invocation_(std::move(invocation)) {
+    context->emitter().AddFlushFn([this] { EmitDiagnostics(); });
+  }
+
+  ~CarbonClangDiagnosticConsumer() override {
+    CARBON_CHECK(diagnostic_infos_.empty(),
+                 "Missing flush before destroying diagnostic consumer");
+  }
 
   // Generates a Carbon warning for each Clang warning and a Carbon error for
   // each Clang error or fatal.
@@ -136,6 +152,8 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
       return;
     }
 
+    // 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(),
@@ -154,7 +172,11 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
   // Outputs Carbon diagnostics based on the collected Clang diagnostics. Must
   // be called after the AST is set in the context.
   auto EmitDiagnostics() -> void {
-    for (const ClangDiagnosticInfo& info : diagnostic_infos_) {
+    CARBON_CHECK(context_->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:
@@ -172,16 +194,27 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer {
           CARBON_DIAGNOSTIC(CppInteropParseWarning, Warning, "{0}",
                             std::string);
           CARBON_DIAGNOSTIC(CppInteropParseError, Error, "{0}", std::string);
-          context_->emitter().Emit(
+          auto builder = context_->emitter().Build(
               SemIR::LocId(info.import_ir_inst_id),
               info.level == clang::DiagnosticsEngine::Warning
                   ? CppInteropParseWarning
                   : CppInteropParseError,
               info.message);
+          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.Emit();
           break;
         }
       }
     }
+    diagnostic_infos_.clear();
   }
 
  private:
@@ -223,12 +256,11 @@ static auto GenerateAst(Context& context,
                         std::shared_ptr<clang::CompilerInvocation> invocation)
     -> std::pair<std::unique_ptr<clang::ASTUnit>, bool> {
   // Build a diagnostics engine.
-  auto diagnostics_consumer =
-      std::make_unique<CarbonClangDiagnosticConsumer>(&context, invocation);
   llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diags(
       clang::CompilerInstance::createDiagnostics(
-          *fs, invocation->getDiagnosticOpts(), diagnostics_consumer.get(),
-          /*ShouldOwnClient=*/false));
+          *fs, invocation->getDiagnosticOpts(),
+          new CarbonClangDiagnosticConsumer(&context, invocation),
+          /*ShouldOwnClient=*/true));
 
   // Extract the input from the frontend invocation and make sure it makes
   // sense.
@@ -246,6 +278,8 @@ static auto GenerateAst(Context& context,
   invocation->getPreprocessorOpts().addRemappedFile(file_name,
                                                     includes_buffer.get());
 
+  clang::DiagnosticErrorTrap trap(*diags);
+
   // Create the AST unit.
   auto ast = clang::ASTUnit::LoadFromCompilerInvocation(
       invocation, std::make_shared<clang::PCHContainerOperations>(), nullptr,
@@ -260,15 +294,9 @@ static auto GenerateAst(Context& context,
   context.sem_ir().set_cpp_ast(ast.get());
 
   // Emit any diagnostics we queued up while building the AST.
-  diagnostics_consumer->EmitDiagnostics();
-  bool any_errors = diagnostics_consumer->getNumErrors() > 0;
+  context.emitter().Flush();
 
-  // Transfer ownership of the consumer to the AST unit, in case more
-  // diagnostics are produced by AST queries.
-  ast->getDiagnostics().setClient(diagnostics_consumer.release(),
-                                  /*ShouldOwnClient=*/true);
-
-  return {std::move(ast), !ast || any_errors};
+  return {std::move(ast), !ast || trap.hasErrorOccurred()};
 }
 
 // Adds a namespace for the `Cpp` import and returns its `NameScopeId`.
@@ -1436,12 +1464,18 @@ auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id,
     return SemIR::ScopeLookupResult::MakeNotFound();
   }
 
+  // Access checks are performed separately by the Carbon name lookup logic.
+  lookup->suppressAccessDiagnostics();
+
   if (!lookup->isSingleResult()) {
-    context.TODO(loc_id,
-                 llvm::formatv("Unsupported: Lookup succeeded but couldn't "
-                               "find a single result; LookupResultKind: {0}",
-                               static_cast<int>(lookup->getResultKind()))
-                     .str());
+    // Clang will diagnose ambiguous lookup results for us.
+    if (!lookup->isAmbiguous()) {
+      context.TODO(loc_id,
+                   llvm::formatv("Unsupported: Lookup succeeded but couldn't "
+                                 "find a single result; LookupResultKind: {0}",
+                                 static_cast<int>(lookup->getResultKind()))
+                       .str());
+    }
     context.name_scopes().AddRequiredName(scope_id, name_id,
                                           SemIR::ErrorInst::InstId);
     return SemIR::ScopeLookupResult::MakeError();

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

@@ -403,6 +403,40 @@ fn F() {
   Cpp.foo();
 }
 
+// ============================================================================
+// Diagnostic with notes
+// ============================================================================
+
+// --- with_notes.h
+
+void foobar(int);
+
+inline void call_foobar() {
+  foobar(1, 2);
+}
+
+// --- fail_with_notes.carbon
+
+library "[[@TEST_NAME]]";
+
+// CHECK:STDERR: fail_with_notes.carbon:[[@LINE+12]]:1: in import [InImport]
+// CHECK:STDERR: ./with_notes.h:5: error: In file included from fail_with_notes.carbon:[[@LINE+11]]:
+// CHECK:STDERR: ./with_notes.h:5:3: error: no matching function for call to 'foobar'
+// CHECK:STDERR:     5 |   foobar(1, 2);
+// CHECK:STDERR:       |   ^~~~~~
+// CHECK:STDERR:  [CppInteropParseError]
+// CHECK:STDERR: fail_with_notes.carbon:[[@LINE+6]]:1: in import [InImport]
+// CHECK:STDERR: ./with_notes.h:2: note: ./with_notes.h:2:6: note: candidate function not viable: requires 1 argument, but 2 were provided
+// CHECK:STDERR:     2 | void foobar(int);
+// CHECK:STDERR:       |      ^      ~~~
+// CHECK:STDERR:  [CppInteropParseNote]
+// CHECK:STDERR:
+import Cpp library "with_notes.h";
+
+fn F() {
+  Cpp.call_foobar();
+}
+
 // CHECK:STDOUT: --- import_cpp_file_with_one_warning.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {

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

@@ -121,12 +121,22 @@ inline namespace { namespace N {} }
 
 library "[[@TEST_NAME]]";
 
+// CHECK:STDERR: fail_import_inline_ambiguity.carbon:[[@LINE+13]]:1: in import [InImport]
+// CHECK:STDERR: error: error: reference to 'N' is ambiguous
+// CHECK:STDERR:  [CppInteropParseError]
+// CHECK:STDERR: fail_import_inline_ambiguity.carbon:[[@LINE+10]]:1: in import [InImport]
+// CHECK:STDERR: ./inline_ambiguity.h:2: note: ./inline_ambiguity.h:2:11: note: candidate found by name lookup is 'N'
+// CHECK:STDERR:     2 | namespace N { void foo(); }
+// CHECK:STDERR:       |           ^
+// CHECK:STDERR:  [CppInteropParseNote]
+// CHECK:STDERR: fail_import_inline_ambiguity.carbon:[[@LINE+5]]:1: in import [InImport]
+// CHECK:STDERR: ./inline_ambiguity.h:3: note: ./inline_ambiguity.h:3:30: note: candidate found by name lookup is '(anonymous namespace)::N'
+// CHECK:STDERR:     3 | inline namespace { namespace N {} }
+// CHECK:STDERR:       |                              ^
+// CHECK:STDERR:  [CppInteropParseNote]
 import Cpp library "inline_ambiguity.h";
 
 fn MyF() {
-  // CHECK:STDERR: fail_import_inline_ambiguity.carbon:[[@LINE+7]]:3: error: semantics TODO: `Unsupported: Lookup succeeded but couldn't find a single result; LookupResultKind: 5` [SemanticsTodo]
-  // CHECK:STDERR:   Cpp.N.foo();
-  // CHECK:STDERR:   ^~~~~
   // CHECK:STDERR: fail_import_inline_ambiguity.carbon:[[@LINE+4]]:3: note: in `Cpp` name lookup for `N` [InCppNameLookup]
   // CHECK:STDERR:   Cpp.N.foo();
   // CHECK:STDERR:   ^~~~~

toolchain/diagnostics/diagnostic_emitter.h

@@ -137,7 +137,7 @@ class Emitter {
   // `consumer` is required to outlive the diagnostic emitter.
   explicit Emitter(Consumer* consumer) : consumer_(consumer) {}
 
-  virtual ~Emitter() = default;
+  virtual ~Emitter() { Flush(); }
 
   // Emits an error.
   //
@@ -161,6 +161,32 @@ class Emitter {
   // be silently ignored.
   auto BuildSuppressed() -> Builder { return Builder(); }
 
+  // Adds a flush function to flush pending diagnostics that might be enqueued
+  // and not yet emitted. The flush function will be called whenever `Flush` is
+  // called.
+  //
+  // No mechanism is provided to unregister a flush function, so the function
+  // must ensure that it remains callable until the emitter is destroyed.
+  //
+  // This is used to register a handler to flush diagnostics from Clang.
+  auto AddFlushFn(std::function<auto()->void> flush_fn) -> void {
+    flush_fns_.push_back(std::move(flush_fn));
+  }
+
+  // Flush all pending diagnostics that are queued externally, such as Clang
+  // diagnostics. This should not be called when the external source might be in
+  // the middle of producing a diagnostic, such as between Clang producing an
+  // error and producing the attached notes.
+  //
+  // This is called automatically before any diagnostic annotator is added or
+  // removed, to flush any pending diagnostics with suitable notes attached, and
+  // when the emitter is destroyed.
+  auto Flush() -> void {
+    for (auto& flush_fn : flush_fns_) {
+      flush_fn();
+    }
+  }
+
  protected:
   // Callback type used to report context messages from ConvertLoc.
   // Note that the first parameter type is Loc rather than
@@ -189,6 +215,7 @@ class Emitter {
   friend class NoLocEmitter;
 
   Consumer* consumer_;
+  llvm::SmallVector<std::function<auto()->void>, 1> flush_fns_;
   llvm::SmallVector<llvm::function_ref<auto(Builder& builder)->void>>
       annotate_fns_;
 };
@@ -235,9 +262,13 @@ class AnnotationScope {
  public:
   AnnotationScope(Emitter<LocT>* emitter, AnnotateFn annotate)
       : emitter_(emitter), annotate_(std::move(annotate)) {
+    emitter_->Flush();
     emitter_->annotate_fns_.push_back(annotate_);
   }
-  ~AnnotationScope() { emitter_->annotate_fns_.pop_back(); }
+  ~AnnotationScope() {
+    emitter_->Flush();
+    emitter_->annotate_fns_.pop_back();
+  }
 
  private:
   Emitter<LocT>* emitter_;

toolchain/diagnostics/diagnostic_emitter_test.cpp

@@ -80,5 +80,39 @@ TEST_F(EmitterTest, EmitNote) {
   emitter_.Build(1, TestDiagnostic).Note(2, TestDiagnosticNote).Emit();
 }
 
+TEST_F(EmitterTest, Flush) {
+  bool flushed = false;
+  auto flush_fn = [&]() { flushed = true; };
+
+  {
+    FakeEmitter emitter(&consumer_);
+    emitter.AddFlushFn(flush_fn);
+
+    // Registering the function does not flush.
+    EXPECT_FALSE(flushed);
+
+    // Explicit calls to `Flush` should flush.
+    emitter.Flush();
+    EXPECT_TRUE(flushed);
+    flushed = false;
+
+    {
+      Diagnostics::AnnotationScope annot(&emitter, [](auto&) {});
+
+      // Registering an annotation scope should flush.
+      EXPECT_TRUE(flushed);
+      flushed = false;
+    }
+
+    // Unregistering an annotation scope should flush.
+    EXPECT_TRUE(flushed);
+    flushed = false;
+  }
+
+  // Destroying the emitter should flush.
+  EXPECT_TRUE(flushed);
+  flushed = false;
+}
+
 }  // namespace
 }  // namespace Carbon::Testing

toolchain/diagnostics/diagnostic_kind.def

@@ -171,6 +171,7 @@ CARBON_DIAGNOSTIC_KIND(CppInteropDriverError)
 CARBON_DIAGNOSTIC_KIND(CppInteropDriverWarning)
 CARBON_DIAGNOSTIC_KIND(CppInteropParseError)
 CARBON_DIAGNOSTIC_KIND(CppInteropParseWarning)
+CARBON_DIAGNOSTIC_KIND(CppInteropParseNote)
 CARBON_DIAGNOSTIC_KIND(IncorrectExtension)
 CARBON_DIAGNOSTIC_KIND(IncorrectExtensionImplNote)
 CARBON_DIAGNOSTIC_KIND(DuplicateLibraryApi)