[clang][deps] Prefer DiagnosticConsumer over llvm::{Error,Expected}

clang-tools-extra/clangd/ScanningProjectModules.cpp

@@ -110,12 +110,18 @@ ModuleDependencyScanner::scan(PathRef FilePath,
   llvm::sys::path::remove_filename(FilePathDir);
   DependencyScanningTool ScanningTool(Service, TFS.view(FilePathDir));
 
-  llvm::Expected<P1689Rule> ScanningResult =
-      ScanningTool.getP1689ModuleDependencyFile(Cmd, Cmd.Directory);
-
-  if (auto E = ScanningResult.takeError()) {
-    elog("Scanning modules dependencies for {0} failed: {1}", FilePath,
-         llvm::toString(std::move(E)));
+  std::string S;
+  llvm::raw_string_ostream OS(S);
+  DiagnosticOptions DiagOpts;
+  DiagOpts.ShowCarets = false;
+  TextDiagnosticPrinter DiagConsumer(OS, DiagOpts);
+
+  std::optional<P1689Rule> ScanningResult =
+      ScanningTool.getP1689ModuleDependencyFile(Cmd, Cmd.Directory,
+                                                DiagConsumer);
+
+  if (!ScanningResult) {
+    elog("Scanning modules dependencies for {0} failed: {1}", FilePath, S);
     return std::nullopt;
   }
 

clang/include/clang/DependencyScanning/DependencyScanningWorker.h

@@ -107,16 +107,6 @@ class DependencyScanningWorker {
       DiagnosticConsumer &DiagConsumer,
       std::optional<llvm::MemoryBufferRef> TUBuffer = std::nullopt);
 
-  /// Run the dependency scanning tool for a given clang driver command-line
-  /// for a specific translation unit via file system or memory buffer.
-  ///
-  /// \returns A \c StringError with the diagnostic output if clang errors
-  /// occurred, success otherwise.
-  llvm::Error computeDependencies(
-      StringRef WorkingDirectory, ArrayRef<std::string> CommandLine,
-      DependencyConsumer &Consumer, DependencyActionController &Controller,
-      std::optional<llvm::MemoryBufferRef> TUBuffer = std::nullopt);
-
   /// The three method below implements a new interface for by name
   /// dependency scanning. They together enable the dependency scanning worker
   /// to more effectively perform scanning for a sequence of modules

clang/include/clang/Tooling/DependencyScanningTool.h

@@ -45,10 +45,11 @@ class DependencyScanningTool {
   /// file format that is specified in the options (-MD is the default) and
   /// return it.
   ///
-  /// \returns A \c StringError with the diagnostic output if clang errors
-  /// occurred, dependency file contents otherwise.
-  llvm::Expected<std::string>
-  getDependencyFile(ArrayRef<std::string> CommandLine, StringRef CWD);
+  /// \returns std::nullopt if errors occurred (reported to the DiagConsumer),
+  /// dependency file contents otherwise.
+  std::optional<std::string>
+  getDependencyFile(ArrayRef<std::string> CommandLine, StringRef CWD,
+                    DiagnosticConsumer &DiagConsumer);
 
   /// Collect the module dependency in P1689 format for C++20 named modules.
   ///
@@ -59,19 +60,21 @@ class DependencyScanningTool {
   /// \param MakeformatOutputPath The output parameter for the path to
   /// \param MakeformatOutput.
   ///
-  /// \returns A \c StringError with the diagnostic output if clang errors
-  /// occurred, P1689 dependency format rules otherwise.
-  llvm::Expected<P1689Rule>
+  /// \returns std::nullopt if errors occurred (reported to the DiagConsumer),
+  /// P1689 dependency format rules otherwise.
+  std::optional<P1689Rule>
   getP1689ModuleDependencyFile(const CompileCommand &Command, StringRef CWD,
                                std::string &MakeformatOutput,
-                               std::string &MakeformatOutputPath);
-  llvm::Expected<P1689Rule>
-  getP1689ModuleDependencyFile(const CompileCommand &Command, StringRef CWD) {
+                               std::string &MakeformatOutputPath,
+                               DiagnosticConsumer &DiagConsumer);
+  std::optional<P1689Rule>
+  getP1689ModuleDependencyFile(const CompileCommand &Command, StringRef CWD,
+                               DiagnosticConsumer &DiagConsumer) {
     std::string MakeformatOutput;
     std::string MakeformatOutputPath;
 
     return getP1689ModuleDependencyFile(Command, CWD, MakeformatOutput,
-                                        MakeformatOutputPath);
+                                        MakeformatOutputPath, DiagConsumer);
   }
 
   /// Given a Clang driver command-line for a translation unit, gather the
@@ -89,11 +92,12 @@ class DependencyScanningTool {
   ///                 TUBuffer is nullopt, the input should be included in the
   ///                 Commandline already.
   ///
-  /// \returns a \c StringError with the diagnostic output if clang errors
-  /// occurred, \c TranslationUnitDeps otherwise.
-  llvm::Expected<dependencies::TranslationUnitDeps>
+  /// \returns std::nullopt if errors occurred (reported to the DiagConsumer),
+  /// translation unit dependencies otherwise.
+  std::optional<dependencies::TranslationUnitDeps>
   getTranslationUnitDependencies(
       ArrayRef<std::string> CommandLine, StringRef CWD,
+      DiagnosticConsumer &DiagConsumer,
       const llvm::DenseSet<dependencies::ModuleID> &AlreadySeen,
       dependencies::LookupModuleOutputCallback LookupModuleOutput,
       std::optional<llvm::MemoryBufferRef> TUBuffer = std::nullopt);

clang/lib/DependencyScanning/DependencyScanningWorker.cpp

@@ -40,21 +40,6 @@ DependencyScanningWorker::DependencyScanningWorker(
 DependencyScanningWorker::~DependencyScanningWorker() = default;
 DependencyActionController::~DependencyActionController() = default;
 
-llvm::Error DependencyScanningWorker::computeDependencies(
-    StringRef WorkingDirectory, ArrayRef<std::string> CommandLine,
-    DependencyConsumer &Consumer, DependencyActionController &Controller,
-    std::optional<llvm::MemoryBufferRef> TUBuffer) {
-  // Capture the emitted diagnostics and report them to the client
-  // in the case of a failure.
-  TextDiagnosticsPrinterWithOutput DiagPrinterWithOS(CommandLine);
-
-  if (computeDependencies(WorkingDirectory, CommandLine, Consumer, Controller,
-                          DiagPrinterWithOS.DiagPrinter, TUBuffer))
-    return llvm::Error::success();
-  return llvm::make_error<llvm::StringError>(
-      DiagPrinterWithOS.DiagnosticsOS.str(), llvm::inconvertibleErrorCode());
-}
-
 static bool forEachDriverJob(
     ArrayRef<std::string> ArgStrs, DiagnosticsEngine &Diags,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,

clang/lib/Tooling/DependencyScanningTool.cpp

@@ -74,23 +74,23 @@ class MakeDependencyPrinterConsumer : public DependencyConsumer {
 };
 } // anonymous namespace
 
-llvm::Expected<std::string>
+std::optional<std::string>
 DependencyScanningTool::getDependencyFile(ArrayRef<std::string> CommandLine,
-                                          StringRef CWD) {
-  MakeDependencyPrinterConsumer Consumer;
+                                          StringRef CWD,
+                                          DiagnosticConsumer &DiagConsumer) {
+  MakeDependencyPrinterConsumer DepConsumer;
   CallbackActionController Controller(nullptr);
-  auto Result =
-      Worker.computeDependencies(CWD, CommandLine, Consumer, Controller);
-  if (Result)
-    return std::move(Result);
+  if (!Worker.computeDependencies(CWD, CommandLine, DepConsumer, Controller,
+                                  DiagConsumer))
+    return std::nullopt;
   std::string Output;
-  Consumer.printDependencies(Output);
+  DepConsumer.printDependencies(Output);
   return Output;
 }
 
-llvm::Expected<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile(
+std::optional<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile(
     const CompileCommand &Command, StringRef CWD, std::string &MakeformatOutput,
-    std::string &MakeformatOutputPath) {
+    std::string &MakeformatOutputPath, DiagnosticConsumer &DiagConsumer) {
   class P1689ModuleDependencyPrinterConsumer
       : public MakeDependencyPrinterConsumer {
   public:
@@ -132,30 +132,29 @@ llvm::Expected<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile(
   P1689Rule Rule;
   P1689ModuleDependencyPrinterConsumer Consumer(Rule, Command);
   P1689ActionController Controller;
-  auto Result = Worker.computeDependencies(CWD, Command.CommandLine, Consumer,
-                                           Controller);
-  if (Result)
-    return std::move(Result);
+  if (!Worker.computeDependencies(CWD, Command.CommandLine, Consumer,
+                                  Controller, DiagConsumer))
+    return std::nullopt;
 
   MakeformatOutputPath = Consumer.getMakeFormatDependencyOutputPath();
   if (!MakeformatOutputPath.empty())
     Consumer.printDependencies(MakeformatOutput);
   return Rule;
 }
 
-llvm::Expected<TranslationUnitDeps>
+std::optional<TranslationUnitDeps>
 DependencyScanningTool::getTranslationUnitDependencies(
     ArrayRef<std::string> CommandLine, StringRef CWD,
+    DiagnosticConsumer &DiagConsumer,
     const llvm::DenseSet<ModuleID> &AlreadySeen,
     LookupModuleOutputCallback LookupModuleOutput,
     std::optional<llvm::MemoryBufferRef> TUBuffer) {
   FullDependencyConsumer Consumer(AlreadySeen);
   CallbackActionController Controller(LookupModuleOutput);
-  llvm::Error Result = Worker.computeDependencies(CWD, CommandLine, Consumer,
-                                                  Controller, TUBuffer);
 
-  if (Result)
-    return std::move(Result);
+  if (!Worker.computeDependencies(CWD, CommandLine, Consumer, Controller,
+                                  DiagConsumer, TUBuffer))
+    return std::nullopt;
   return Consumer.takeTranslationUnitDeps();
 }
 

clang/test/ClangScanDeps/error.cpp

@@ -7,7 +7,7 @@
 // RUN: not clang-scan-deps -- %clang -c %t/missing_tu.c 2>%t/missing_tu.errs
 // RUN: echo EOF >> %t/missing_tu.errs
 // RUN: cat %t/missing_tu.errs | sed 's:\\\\\?:/:g' | FileCheck %s --check-prefix=CHECK-MISSING-TU -DPREFIX=%/t
-// CHECK-MISSING-TU: Error while scanning dependencies for [[PREFIX]]/missing_tu.c
+// CHECK-MISSING-TU: Diagnostics while scanning dependencies for '[[PREFIX]]/missing_tu.c':
 // CHECK-MISSING-TU-NEXT: error: no such file or directory: '[[PREFIX]]/missing_tu.c'
 // CHECK-MISSING-TU-NEXT: error: no input files
 // CHECK-MISSING-TU-NEXT: error:
@@ -16,6 +16,6 @@
 // RUN: not clang-scan-deps -- %clang -c %t/missing_header.c 2>%t/missing_header.errs
 // RUN: echo EOF >> %t/missing_header.errs
 // RUN: cat %t/missing_header.errs | sed 's:\\\\\?:/:g' | FileCheck %s --check-prefix=CHECK-MISSING-HEADER -DPREFIX=%/t
-// CHECK-MISSING-HEADER: Error while scanning dependencies for [[PREFIX]]/missing_header.c
-// CHECK-MISSING-HEADER-NEXT: fatal error: 'missing.h' file not found
+// CHECK-MISSING-HEADER: Diagnostics while scanning dependencies for '[[PREFIX]]/missing_header.c':
+// CHECK-MISSING-HEADER-NEXT: [[PREFIX]]/missing_header.c:1:10: fatal error: 'missing.h' file not found
 // CHECK-MISSING-HEADER-NEXT: EOF

clang/tools/clang-scan-deps/ClangScanDeps.cpp

@@ -325,26 +325,16 @@ class ResourceDirectoryCache {
 
 } // end anonymous namespace
 
-/// Takes the result of a dependency scan and prints error / dependency files
-/// based on the result.
-///
-/// \returns True on error.
-static bool
-handleMakeDependencyToolResult(const std::string &Input,
-                               llvm::Expected<std::string> &MaybeFile,
-                               SharedStream &OS, SharedStream &Errs) {
-  if (!MaybeFile) {
-    llvm::handleAllErrors(
-        MaybeFile.takeError(), [&Input, &Errs](llvm::StringError &Err) {
-          Errs.applyLocked([&](raw_ostream &OS) {
-            OS << "Error while scanning dependencies for " << Input << ":\n";
-            OS << Err.getMessage();
-          });
-        });
-    return true;
-  }
-  OS.applyLocked([&](raw_ostream &OS) { OS << *MaybeFile; });
-  return false;
+/// Prints any diagnostics produced during a dependency scan.
+static void handleDiagnostics(StringRef Input, StringRef Diagnostics,
+                              SharedStream &Errs) {
+  if (Diagnostics.empty())
+    return;
+
+  Errs.applyLocked([&](raw_ostream &OS) {
+    OS << "Diagnostics while scanning dependencies for '" << Input << "':\n";
+    OS << Diagnostics;
+  });
 }
 
 template <typename Container>
@@ -627,23 +617,6 @@ class FullDeps {
   std::vector<InputDeps> Inputs;
 };
 
-static bool handleTranslationUnitResult(
-    StringRef Input, llvm::Expected<TranslationUnitDeps> &MaybeTUDeps,
-    FullDeps &FD, size_t InputIndex, SharedStream &OS, SharedStream &Errs) {
-  if (!MaybeTUDeps) {
-    llvm::handleAllErrors(
-        MaybeTUDeps.takeError(), [&Input, &Errs](llvm::StringError &Err) {
-          Errs.applyLocked([&](raw_ostream &OS) {
-            OS << "Error while scanning dependencies for " << Input << ":\n";
-            OS << Err.getMessage();
-          });
-        });
-    return true;
-  }
-  FD.mergeDeps(Input, std::move(*MaybeTUDeps), InputIndex);
-  return false;
-}
-
 static bool handleModuleResult(StringRef ModuleName,
                                llvm::Expected<TranslationUnitDeps> &MaybeTUDeps,
                                FullDeps &FD, size_t InputIndex,
@@ -741,24 +714,6 @@ class P1689Deps {
   std::vector<P1689Rule> Rules;
 };
 
-static bool
-handleP1689DependencyToolResult(const std::string &Input,
-                                llvm::Expected<P1689Rule> &MaybeRule,
-                                P1689Deps &PD, SharedStream &Errs) {
-  if (!MaybeRule) {
-    llvm::handleAllErrors(
-        MaybeRule.takeError(), [&Input, &Errs](llvm::StringError &Err) {
-          Errs.applyLocked([&](raw_ostream &OS) {
-            OS << "Error while scanning dependencies for " << Input << ":\n";
-            OS << Err.getMessage();
-          });
-        });
-    return true;
-  }
-  PD.addRules(*MaybeRule);
-  return false;
-}
-
 /// Construct a path for the explicitly built PCM.
 static std::string constructPCMPath(ModuleID MID, StringRef OutputDir) {
   SmallString<256> ExplicitPCMPath(OutputDir);
@@ -1036,6 +991,12 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
       std::string Filename = std::move(Input->Filename);
       std::string CWD = std::move(Input->Directory);
 
+      std::string S;
+      llvm::raw_string_ostream OS(S);
+      DiagnosticOptions DiagOpts;
+      DiagOpts.ShowCarets = false;
+      TextDiagnosticPrinter DiagConsumer(OS, DiagOpts);
+
       std::string OutputDir(ModuleFilesDir);
       if (OutputDir.empty())
         OutputDir = getModuleCachePath(Input->CommandLine);
@@ -1045,9 +1006,12 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
 
       // Run the tool on it.
       if (Format == ScanningOutputFormat::Make) {
-        auto MaybeFile = WorkerTool.getDependencyFile(Input->CommandLine, CWD);
-        if (handleMakeDependencyToolResult(Filename, MaybeFile, DependencyOS,
-                                           Errs))
+        auto MaybeFile =
+            WorkerTool.getDependencyFile(Input->CommandLine, CWD, DiagConsumer);
+        handleDiagnostics(Filename, S, Errs);
+        if (MaybeFile)
+          DependencyOS.applyLocked([&](raw_ostream &OS) { OS << *MaybeFile; });
+        else
           HadErrors = true;
       } else if (Format == ScanningOutputFormat::P1689) {
         // It is useful to generate the make-format dependency output during
@@ -1058,9 +1022,11 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
         std::string MakeformatOutput;
 
         auto MaybeRule = WorkerTool.getP1689ModuleDependencyFile(
-            *Input, CWD, MakeformatOutput, MakeformatOutputPath);
-
-        if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs))
+            *Input, CWD, MakeformatOutput, MakeformatOutputPath, DiagConsumer);
+        handleDiagnostics(Filename, S, Errs);
+        if (MaybeRule)
+          PD.addRules(*MaybeRule);
+        else
           HadErrors = true;
 
         if (!MakeformatOutputPath.empty() && !MakeformatOutput.empty() &&
@@ -1086,10 +1052,8 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
           }
 
           SharedStream MakeformatOS(OSIter->second);
-          llvm::Expected<std::string> MaybeOutput(MakeformatOutput);
-          if (handleMakeDependencyToolResult(Filename, MaybeOutput,
-                                             MakeformatOS, Errs))
-            HadErrors = true;
+          MakeformatOS.applyLocked(
+              [&](raw_ostream &OS) { OS << MakeformatOutput; });
         }
       } else if (ModuleNames) {
         StringRef ModuleNameRef(*ModuleNames);
@@ -1150,10 +1114,12 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
           Filename = TU->getBufferIdentifier();
         }
         auto MaybeTUDeps = WorkerTool.getTranslationUnitDependencies(
-            Input->CommandLine, CWD, AlreadySeenModules, LookupOutput,
-            TUBuffer);
-        if (handleTranslationUnitResult(Filename, MaybeTUDeps, *FD, LocalIndex,
-                                        DependencyOS, Errs))
+            Input->CommandLine, CWD, DiagConsumer, AlreadySeenModules,
+            LookupOutput, TUBuffer);
+        handleDiagnostics(Filename, S, Errs);
+        if (MaybeTUDeps)
+          FD->mergeDeps(Filename, *MaybeTUDeps, LocalIndex);
+        else
           HadErrors = true;
       }
     }