From 47cc57201a5f067da44b245b05d60a28b35fdf30 Mon Sep 17 00:00:00 2001
From: Philip Salzmann <philip.salzmann@uibk.ac.at>
Date: Wed, 7 Aug 2024 13:01:10 +0200
Subject: [PATCH] Improve error reporting during indexing

The previous behavior was to basically suppress all error diagnostics
generated by Clang during indexing, and only rely on the return value of
`ClangTool::run` to decide whether a file was parsed correctly or not.

For irrecoverable errors such as missing files this doesn't give one
much to work with. The new behavior is to parse all diagnostics and
check for missing files, and if there are some, print those immediately.

For other errors that may be encountered, we now keep count of how many
there are, and offer the option to print them using --verbose output.
---
 src/support/ParallelExecutor.cpp | 77 +++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 16 deletions(-)

diff --git a/src/support/ParallelExecutor.cpp b/src/support/ParallelExecutor.cpp
index 73b5716..d67ab49 100644
--- a/src/support/ParallelExecutor.cpp
+++ b/src/support/ParallelExecutor.cpp
@@ -4,18 +4,49 @@
 #include "support/ParallelExecutor.hpp"
 #include "spdlog/spdlog.h"
 
+#include "clang/Basic/Diagnostic.h"
 #include "llvm/Support/VirtualFileSystem.h"
 
-void hdoc::indexer::ParallelExecutor::execute(std::unique_ptr<clang::tooling::FrontendActionFactory> action) {
-  std::mutex mutex;
+#include <atomic>
+
+class MyDiagnosticConsumer : public clang::DiagnosticConsumer {
+public:
+  MyDiagnosticConsumer(std::string fileName) : fileName(fileName){};
+
+  void HandleDiagnostic(clang::DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic& Info) override {
+    // Note: We deliberately don't call the parent implementation, which counts the number of warnings and errors
+    // This is because those counts are used by the tool to determine whether a file was successfully processed
+    // clang::DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
+
+    if (DiagLevel == clang::DiagnosticsEngine::Level::Error) {
+      clang::SmallString<100> message;
+      Info.FormatDiagnostic(message);
+      // There's probably a better way of doing this...
+      if (message.find("no such file or directory") != std::string::npos) {
+        // The message already contains the file name; no need to repeat it here
+        spdlog::error("{}", message.c_str());
+      } else {
+        // Running with --verbose sets the log level to info , so we emit additional errors as info
+        // Message is very bare bones w/o any location information, but it's better than nothing
+        spdlog::info("Encountered error while processing {}: {}", fileName, message.c_str());
+      }
+      numErrors++;
+    } else if (DiagLevel == clang::DiagnosticsEngine::Level::Warning) {
+      numWarnings++;
+    }
+  }
+
+  // We have to keep count manually because we don't call the parent implementation
+  size_t numWarnings = 0;
+  size_t numErrors   = 0;
 
-  // Add a counter to track progress
-  uint32_t    i                = 0;
-  std::string totalNumFiles    = std::to_string(this->cmpdb.getAllFiles().size());
-  auto        incrementCounter = [&]() {
-    std::unique_lock<std::mutex> lock(mutex);
-    return ++i;
-  };
+private:
+  std::string fileName;
+};
+
+void hdoc::indexer::ParallelExecutor::execute(std::unique_ptr<clang::tooling::FrontendActionFactory> action) {
+  std::atomic_size_t i             = 0;
+  std::string        totalNumFiles = std::to_string(this->cmpdb.getAllFiles().size());
 
   std::vector<std::string> allFilesInCmpdb = this->cmpdb.getAllFiles();
 
@@ -24,10 +55,12 @@ void hdoc::indexer::ParallelExecutor::execute(std::unique_ptr<clang::tooling::Fr
     totalNumFiles = std::to_string(this->debugLimitNumIndexedFiles);
   }
 
+  std::atomic_size_t totalWarnings = 0;
+  std::atomic_size_t totalErrors   = 0;
   for (const std::string& file : allFilesInCmpdb) {
     this->pool.async(
         [&](const std::string path) {
-          spdlog::info("[{}/{}] processing {}", incrementCounter(), totalNumFiles, path);
+          spdlog::info("[{}/{}] processing {}", ++i, totalNumFiles, path);
 
           // Each thread gets an independent copy of a VFS to allow different concurrent working directories
           llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = llvm::vfs::createPhysicalFileSystem().release();
@@ -41,12 +74,13 @@ void hdoc::indexer::ParallelExecutor::execute(std::unique_ptr<clang::tooling::Fr
           Tool.appendArgumentsAdjuster(clang::tooling::getInsertArgumentAdjuster(
               this->includePaths, clang::tooling::ArgumentInsertPosition::END));
 
-          // Ignore all diagnostics that clang might throw. Clang often has weird diagnostic settings that don't
-          // match what's in compile_commands.json, resulting in spurious errors. Instead of trying to change clang's
-          // behavior, we'll ignore all diagnostics and assume that the user supplied a project that builds on their
-          // machine.
-          clang::IgnoringDiagConsumer ignore;
-          Tool.setDiagnosticConsumer(&ignore);
+          // We ignore most diagnostics by default, except for files that cannot be found.
+          // Additional error messages are printed when using --verbose output.
+          MyDiagnosticConsumer diagConsumer(path);
+          Tool.setDiagnosticConsumer(&diagConsumer);
+
+          // Disable error messages from the tool itself, as they don't add any value ("Error while processing <file>")
+          Tool.setPrintErrorMessage(false);
 
           // Run the tool and print an error message if something goes wrong
           if (Tool.run(action.get())) {
@@ -54,9 +88,20 @@ void hdoc::indexer::ParallelExecutor::execute(std::unique_ptr<clang::tooling::Fr
                 "Clang failed to parse source file: {}. Information from this file may be missing from hdoc's output",
                 path);
           }
+
+          totalWarnings += diagConsumer.numWarnings;
+          totalErrors += diagConsumer.numErrors;
         },
         file);
   }
   // Make sure all tasks have finished before resetting the working directory
   this->pool.wait();
+
+  if (totalErrors > 0) {
+    const auto logVerboseMsg =
+        spdlog::get_level() > spdlog::level::info ? " (run with --verbose for more details)" : "";
+    spdlog::error("Clang encountered {} errors and {} warnings{}", totalErrors, totalWarnings, logVerboseMsg);
+  } else if (totalWarnings > 0) {
+    spdlog::warn("Clang encountered {} warnings", totalWarnings);
+  }
 }