[clang][deps] Prefer DiagnosticConsumer over llvm::{Error,Expected}
#172389
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) ChangesThis PR moves the Full diff: https://github.com/llvm/llvm-project/pull/172389.diff 7 Files Affected:
diff --git a/clang/include/clang/DependencyScanning/DependencyScanningWorker.h b/clang/include/clang/DependencyScanning/DependencyScanningWorker.h
index 489fba4ed3f6b..3c5e7660a8c7b 100644
--- a/clang/include/clang/DependencyScanning/DependencyScanningWorker.h
+++ b/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
diff --git a/clang/include/clang/Tooling/DependencyScanningTool.h b/clang/include/clang/Tooling/DependencyScanningTool.h
index e796ed648db35..c231da621e164 100644
--- a/clang/include/clang/Tooling/DependencyScanningTool.h
+++ b/clang/include/clang/Tooling/DependencyScanningTool.h
@@ -47,8 +47,9 @@ class DependencyScanningTool {
///
/// \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);
+ std::optional<std::string>
+ getDependencyFile(StringRef CWD, ArrayRef<std::string> CommandLine,
+ DiagnosticConsumer &DiagConsumer);
/// Collect the module dependency in P1689 format for C++20 named modules.
///
@@ -61,17 +62,19 @@ class DependencyScanningTool {
///
/// \returns A \c StringError with the diagnostic output if clang errors
/// occurred, P1689 dependency format rules otherwise.
- llvm::Expected<P1689Rule>
+ 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
@@ -91,9 +94,10 @@ class DependencyScanningTool {
///
/// \returns a \c StringError with the diagnostic output if clang errors
/// occurred, \c TranslationUnitDeps otherwise.
- llvm::Expected<dependencies::TranslationUnitDeps>
+ 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);
diff --git a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/DependencyScanning/DependencyScanningWorker.cpp
index 99fe082634524..ea2a6d67948c3 100644
--- a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp
+++ b/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,
diff --git a/clang/lib/Tooling/DependencyScanningTool.cpp b/clang/lib/Tooling/DependencyScanningTool.cpp
index 3687db03628f4..b9ca21da359d3 100644
--- a/clang/lib/Tooling/DependencyScanningTool.cpp
+++ b/clang/lib/Tooling/DependencyScanningTool.cpp
@@ -74,23 +74,23 @@ class MakeDependencyPrinterConsumer : public DependencyConsumer {
};
} // anonymous namespace
-llvm::Expected<std::string>
-DependencyScanningTool::getDependencyFile(ArrayRef<std::string> CommandLine,
- StringRef CWD) {
- MakeDependencyPrinterConsumer Consumer;
+std::optional<std::string>
+DependencyScanningTool::getDependencyFile(StringRef CWD,
+ ArrayRef<std::string> CommandLine,
+ 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,10 +132,9 @@ 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())
@@ -143,19 +142,19 @@ llvm::Expected<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile(
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();
}
diff --git a/clang/test/ClangScanDeps/error.cpp b/clang/test/ClangScanDeps/error.cpp
index 593dbf35edca5..03126d8812613 100644
--- a/clang/test/ClangScanDeps/error.cpp
+++ b/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
diff --git a/clang/tools/clang-scan-deps/ClangScanDeps.cpp b/clang/tools/clang-scan-deps/ClangScanDeps.cpp
index 9acd0aca737ba..7a6e56214f680 100644
--- a/clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ b/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(CWD, Input->CommandLine, 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;
}
}
diff --git a/clang/unittests/Tooling/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScannerTest.cpp
index da47252b37097..4efcb32020a26 100644
--- a/clang/unittests/Tooling/DependencyScannerTest.cpp
+++ b/clang/unittests/Tooling/DependencyScannerTest.cpp
@@ -235,12 +235,11 @@ TEST(DependencyScanner, ScanDepsWithFS) {
ScanningOutputFormat::Make);
DependencyScanningTool ScanTool(Service, VFS);
- std::string DepFile;
- ASSERT_THAT_ERROR(
- ScanTool.getDependencyFile(CommandLine, CWD).moveInto(DepFile),
- llvm::Succeeded());
- using llvm::sys::path::convert_to_slash;
- EXPECT_EQ(convert_to_slash(DepFile),
+ IgnoringDiagConsumer DiagConsumer;
+ std::optional<std::string> DepFile =
+ ScanTool.getDependencyFile(CWD, CommandLine, DiagConsumer);
+ ASSERT_TRUE(DepFile.has_value());
+ EXPECT_EQ(llvm::sys::path::convert_to_slash(*DepFile),
"test.cpp.o: /root/test.cpp /root/header.h\n");
}
@@ -296,10 +295,10 @@ TEST(DependencyScanner, ScanDepsWithModuleLookup) {
// This will fail with "fatal error: module 'Foo' not found" but it doesn't
// matter, the point of the test is to check that files are not read
// unnecessarily.
- std::string DepFile;
- ASSERT_THAT_ERROR(
- ScanTool.getDependencyFile(CommandLine, CWD).moveInto(DepFile),
- llvm::Failed());
+ IgnoringDiagConsumer DiagConsumer;
+ std::optional<std::string> DepFile =
+ ScanTool.getDependencyFile(CWD, CommandLine, DiagConsumer);
+ ASSERT_FALSE(DepFile.has_value());
EXPECT_TRUE(!llvm::is_contained(InterceptFS->StatPaths, OtherPath));
EXPECT_EQ(InterceptFS->ReadFiles, std::vector<std::string>{"test.m"});
|
🪟 Windows x64 Test Results
All executed tests passed, but another part of the build failed. Click on a failure below to see the details. [code=1] tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ScanningProjectModules.cpp.objIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🐧 Linux x64 Test Results
All executed tests passed, but another part of the build failed. Click on a failure below to see the details. tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ScanningProjectModules.cpp.oIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
naveen-seth
reviewed
Dec 16, 2025
Contributor
There was a problem hiding this comment.
Overall, I think this is good!
In the open PR implementing dependency scanning in the modules driver, we also report all warnings and remarks, but wanted the diagnostics produced during the dependency scan to match the output style of diagnostics emitted directly through the DiagnosticsEngine (colors and other formatting), rather than using TextDiagnosticPrinter.
With this PR, I believe we can do the same for clang-scan-deps to further improve the diagnostics output.
See: PR #152770, StandaloneDiagCollector, PR #152770, Emitting the diagnostics
(Since #169599, the StandaloneDiagnostic type and related utilities have also been extracted to clang/include/clang/Frontend/StandaloneDiagnostic.h.)
Doing this might be out of scope for this PR, but I just to share the idea just in case / for future consideration!
Comment on lines
144
to
145
| llvm::Expected<dependencies::TranslationUnitDeps> | ||
| computeDependenciesByNameWithContextOrError( |
Contributor
There was a problem hiding this comment.
Is making the same change to the by-name scanning API also in scope of this PR / planned as a follow up?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the
DependencyScanning{Worker,Tool}APIs from usingllvm::{Error,Expected}to usingDiagnosticConsumer. This makes it possible to learn about non-error diagnostics (warnings, remarks) emitted by the scanner. I found a need for this while exploring the possibility of caching dependency scans and emitting remarks to report cache hits/misses.