https://github.com/galenelias created
https://github.com/llvm/llvm-project/pull/136167
This addresses:
Issue #77341: clang-tidy fails to parse config files silently, even with
--verify-config.
Currently when clang-tidy attempts to parse a .clang-tidy config file with
invalid syntax, it emits and error and moves on, which can often result in
applying the default set of checks, which is undesirable.
This change bubbles up the error state from parsing the configuration files up
to `clangTidyMain` so we can do an early return with an error code. It's
unfortunately quite a bit of plumbing - but this seemed to be the idiomatic
approach for the codebase.
>From f54d980bafd76863ab63e68f0568dcfc3329fd9f Mon Sep 17 00:00:00 2001
From: Galen Elias <gel...@gmail.com>
Date: Wed, 16 Apr 2025 12:48:18 -0700
Subject: [PATCH] [clang-tidy] Return error code on config parse error
This addresses:
Issue 77341: clang-tidy fails to parse config files silently, even with
--verify-config.
Currently when clang-tidy attempts to parse a .clang-tidy config file
with invalid syntax, it emits and error and moves on, which can often
result in applying the default set of checks, which is undesirable.
This change bubbles up the error state from parsing the configuration
files up to `clangTidyMain` so we can do an early return with an error
code. It's unfortunately quite a bit of plumbing - but this seemed to be
the idiomatic approach for the clang-tidy codebase.
---
.../ClangTidyDiagnosticConsumer.cpp | 2 +-
.../clang-tidy/ClangTidyOptions.cpp | 66 +++++++++++--------
.../clang-tidy/ClangTidyOptions.h | 23 ++++---
.../clang-tidy/tool/ClangTidyMain.cpp | 47 +++++++------
.../clang-tidy/OptionsProviderTest.cpp | 16 ++---
5 files changed, 91 insertions(+), 63 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 731141a545a48..4691b1606986b 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -266,7 +266,7 @@ ClangTidyOptions
ClangTidyContext::getOptionsForFile(StringRef File) const {
// Merge options on top of getDefaults() as a safeguard against options with
// unset values.
return ClangTidyOptions::getDefaults().merge(
- OptionsProvider->getOptions(File), 0);
+ *OptionsProvider->getOptions(File), 0);
}
void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index dd1d86882f5d4..7ae23c137106e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -267,16 +267,19 @@ const char
ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
"command-line option '-config'";
-ClangTidyOptions
+llvm::ErrorOr<ClangTidyOptions>
ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
ClangTidyOptions Result;
unsigned Priority = 0;
- for (auto &Source : getRawOptions(FileName))
+ llvm::ErrorOr<std::vector<OptionsSource>> Options = getRawOptions(FileName);
+ if (!Options)
+ return Options.getError();
+ for (auto &Source : *Options)
Result.mergeWith(Source.first, ++Priority);
return Result;
}
-std::vector<OptionsSource>
+llvm::ErrorOr<std::vector<OptionsSource>>
DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
std::vector<OptionsSource> Result;
Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,10 +295,12 @@ ConfigOptionsProvider::ConfigOptionsProvider(
std::move(OverrideOptions), std::move(FS)),
ConfigOptions(std::move(ConfigOptions)) {}
-std::vector<OptionsSource>
+llvm::ErrorOr<std::vector<OptionsSource>>
ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
- std::vector<OptionsSource> RawOptions =
+ llvm::ErrorOr<std::vector<OptionsSource>> RawOptions =
DefaultOptionsProvider::getRawOptions(FileName);
+ if (!RawOptions)
+ return RawOptions;
if (ConfigOptions.InheritParentConfig.value_or(false)) {
LLVM_DEBUG(llvm::dbgs()
<< "Getting options for file " << FileName << "...\n");
@@ -303,13 +308,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef
FileName) {
llvm::ErrorOr<llvm::SmallString<128>> AbsoluteFilePath =
getNormalizedAbsolutePath(FileName);
if (AbsoluteFilePath) {
- addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+ addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
}
}
- RawOptions.emplace_back(ConfigOptions,
- OptionsSourceTypeConfigCommandLineOption);
- RawOptions.emplace_back(OverrideOptions,
- OptionsSourceTypeCheckCommandLineOption);
+ RawOptions->emplace_back(ConfigOptions,
+ OptionsSourceTypeConfigCommandLineOption);
+ RawOptions->emplace_back(OverrideOptions,
+ OptionsSourceTypeCheckCommandLineOption);
return RawOptions;
}
@@ -345,21 +350,21 @@
FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) {
return NormalizedAbsolutePath;
}
-void FileOptionsBaseProvider::addRawFileOptions(
+std::error_code FileOptionsBaseProvider::addRawFileOptions(
llvm::StringRef AbsolutePath, std::vector<OptionsSource> &CurOptions) {
auto CurSize = CurOptions.size();
// Look for a suitable configuration file in all parent directories of the
// file. Start with the immediate parent directory and move up.
StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
auto MemorizedConfigFile =
- [this, &RootPath](StringRef CurrentPath) -> std::optional<OptionsSource>
{
+ [this, &RootPath](StringRef CurrentPath) -> llvm::ErrorOr<OptionsSource>
{
const auto Iter = CachedOptions.Memorized.find(CurrentPath);
if (Iter != CachedOptions.Memorized.end())
return CachedOptions.Storage[Iter->second];
- std::optional<OptionsSource> OptionsSource =
tryReadConfigFile(CurrentPath);
+ llvm::ErrorOr<OptionsSource> OptionsSource =
tryReadConfigFile(CurrentPath);
if (OptionsSource) {
const size_t Index = CachedOptions.Storage.size();
- CachedOptions.Storage.emplace_back(OptionsSource.value());
+ CachedOptions.Storage.emplace_back(*OptionsSource);
while (RootPath != CurrentPath) {
LLVM_DEBUG(llvm::dbgs()
<< "Caching configuration for path " << RootPath << ".\n");
@@ -373,16 +378,21 @@ void FileOptionsBaseProvider::addRawFileOptions(
};
for (StringRef CurrentPath = RootPath; !CurrentPath.empty();
CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
- if (std::optional<OptionsSource> Result =
- MemorizedConfigFile(CurrentPath)) {
- CurOptions.emplace_back(Result.value());
+ llvm::ErrorOr<OptionsSource> Result = MemorizedConfigFile(CurrentPath);
+
+ if (Result) {
+ CurOptions.emplace_back(*Result);
if (!Result->first.InheritParentConfig.value_or(false))
break;
+ } else if (Result.getError() != llvm::errc::no_such_file_or_directory) {
+ return Result.getError();
}
}
// Reverse order of file configs because closer configs should have higher
// priority.
std::reverse(CurOptions.begin() + CurSize, CurOptions.end());
+
+ return {};
}
FileOptionsProvider::FileOptionsProvider(
@@ -404,7 +414,7 @@ FileOptionsProvider::FileOptionsProvider(
// FIXME: This method has some common logic with clang::format::getStyle().
// Consider pulling out common bits to a findParentFileWithName function or
// similar.
-std::vector<OptionsSource>
+llvm::ErrorOr<std::vector<OptionsSource>>
FileOptionsProvider::getRawOptions(StringRef FileName) {
LLVM_DEBUG(llvm::dbgs() << "Getting options for file " << FileName
<< "...\n");
@@ -412,19 +422,23 @@ FileOptionsProvider::getRawOptions(StringRef FileName) {
llvm::ErrorOr<llvm::SmallString<128>> AbsoluteFilePath =
getNormalizedAbsolutePath(FileName);
if (!AbsoluteFilePath)
- return {};
+ return std::vector<OptionsSource>{};
- std::vector<OptionsSource> RawOptions =
+ llvm::ErrorOr<std::vector<OptionsSource>> RawOptions =
DefaultOptionsProvider::getRawOptions(AbsoluteFilePath->str());
- addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+ std::error_code Err = addRawFileOptions(AbsoluteFilePath->str(),
*RawOptions);
+
+ if (Err)
+ return Err;
+
OptionsSource CommandLineOptions(OverrideOptions,
OptionsSourceTypeCheckCommandLineOption);
- RawOptions.push_back(CommandLineOptions);
+ RawOptions->push_back(CommandLineOptions);
return RawOptions;
}
-std::optional<OptionsSource>
+llvm::ErrorOr<OptionsSource>
FileOptionsBaseProvider::tryReadConfigFile(StringRef Directory) {
assert(!Directory.empty());
@@ -433,7 +447,7 @@ FileOptionsBaseProvider::tryReadConfigFile(StringRef
Directory) {
if (!DirectoryStatus || !DirectoryStatus->isDirectory()) {
llvm::errs() << "Error reading configuration from " << Directory
<< ": directory doesn't exist.\n";
- return std::nullopt;
+ return make_error_code(llvm::errc::not_a_directory);
}
for (const ConfigFileHandler &ConfigHandler : ConfigHandlers) {
@@ -464,11 +478,11 @@ FileOptionsBaseProvider::tryReadConfigFile(StringRef
Directory) {
if (ParsedOptions.getError())
llvm::errs() << "Error parsing " << ConfigFile << ": "
<< ParsedOptions.getError().message() << "\n";
- continue;
+ return ParsedOptions.getError();
}
return OptionsSource(*ParsedOptions, std::string(ConfigFile));
}
- return std::nullopt;
+ return make_error_code(llvm::errc::no_such_file_or_directory);
}
/// Parses -line-filter option and stores it to the \c Options.
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
index dd78c570d25d9..83162e7c78d5a 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -174,12 +174,12 @@ class ClangTidyOptionsProvider {
/// Returns an ordered vector of OptionsSources, in order of increasing
/// priority.
- virtual std::vector<OptionsSource>
+ virtual llvm::ErrorOr<std::vector<OptionsSource>>
getRawOptions(llvm::StringRef FileName) = 0;
/// Returns options applying to a specific translation unit with the
/// specified \p FileName.
- ClangTidyOptions getOptions(llvm::StringRef FileName);
+ llvm::ErrorOr<ClangTidyOptions> getOptions(llvm::StringRef FileName);
};
/// Implementation of the \c ClangTidyOptionsProvider interface, which
@@ -193,7 +193,8 @@ class DefaultOptionsProvider : public
ClangTidyOptionsProvider {
const ClangTidyGlobalOptions &getGlobalOptions() override {
return GlobalOptions;
}
- std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
+ llvm::ErrorOr<std::vector<OptionsSource>>
+ getRawOptions(llvm::StringRef FileName) override;
private:
ClangTidyGlobalOptions GlobalOptions;
@@ -204,7 +205,9 @@ class FileOptionsBaseProvider : public
DefaultOptionsProvider {
protected:
// A pair of configuration file base name and a function parsing
// configuration from text in the corresponding format.
- using ConfigFileHandler = std::pair<std::string,
std::function<llvm::ErrorOr<ClangTidyOptions> (llvm::MemoryBufferRef)>>;
+ using ConfigFileHandler =
+ std::pair<std::string, std::function<llvm::ErrorOr<ClangTidyOptions>(
+ llvm::MemoryBufferRef)>>;
/// Configuration file handlers listed in the order of priority.
///
@@ -235,15 +238,15 @@ class FileOptionsBaseProvider : public
DefaultOptionsProvider {
ClangTidyOptions OverrideOptions,
ConfigFileHandlers ConfigHandlers);
- void addRawFileOptions(llvm::StringRef AbsolutePath,
- std::vector<OptionsSource> &CurOptions);
+ std::error_code addRawFileOptions(llvm::StringRef AbsolutePath,
+ std::vector<OptionsSource> &CurOptions);
llvm::ErrorOr<llvm::SmallString<128>>
getNormalizedAbsolutePath(llvm::StringRef AbsolutePath);
/// Try to read configuration files from \p Directory using registered
/// \c ConfigHandlers.
- std::optional<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
+ llvm::ErrorOr<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
struct OptionsCache {
llvm::StringMap<size_t> Memorized;
@@ -262,7 +265,8 @@ class ConfigOptionsProvider : public
FileOptionsBaseProvider {
ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
ClangTidyOptions ConfigOptions, ClangTidyOptions OverrideOptions,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
- std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
+ llvm::ErrorOr<std::vector<OptionsSource>>
+ getRawOptions(llvm::StringRef FileName) override;
private:
ClangTidyOptions ConfigOptions;
@@ -315,7 +319,8 @@ class FileOptionsProvider : public FileOptionsBaseProvider {
ClangTidyOptions OverrideOptions,
ConfigFileHandlers ConfigHandlers);
- std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
+ llvm::ErrorOr<std::vector<OptionsSource>>
+ getRawOptions(llvm::StringRef FileName) override;
};
/// Parses LineFilter from JSON and stores it to the \p Options.
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index fa8887e4639b4..4d7b511b0d1ac 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -337,8 +337,7 @@ Allow empty enabled checks. This suppresses
the "no checks enabled" error when disabling
all of the checks.
)"),
- cl::init(false),
- cl::cat(ClangTidyCategory));
+ cl::init(false),
cl::cat(ClangTidyCategory));
namespace clang::tidy {
@@ -370,8 +369,8 @@ static void printStats(const ClangTidyStats &Stats) {
}
}
-static std::unique_ptr<ClangTidyOptionsProvider> createOptionsProvider(
- llvm::IntrusiveRefCntPtr<vfs::FileSystem> FS) {
+static std::unique_ptr<ClangTidyOptionsProvider>
+createOptionsProvider(llvm::IntrusiveRefCntPtr<vfs::FileSystem> FS) {
ClangTidyGlobalOptions GlobalOptions;
if (std::error_code Err = parseLineFilter(LineFilter, GlobalOptions)) {
llvm::errs() << "Invalid LineFilter: " << Err.message() << "\n\nUsage:\n";
@@ -624,21 +623,29 @@ int clangTidyMain(int argc, const char **argv) {
}
SmallString<256> FilePath = makeAbsolute(FileName);
- ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath);
+ llvm::ErrorOr<ClangTidyOptions> EffectiveOptions =
+ OptionsProvider->getOptions(FilePath);
+
+ if (!EffectiveOptions)
+ return 1;
std::vector<std::string> EnabledChecks =
- getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
+ getCheckNames(*EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
if (ExplainConfig) {
// FIXME: Show other ClangTidyOptions' fields, like ExtraArg.
- std::vector<clang::tidy::ClangTidyOptionsProvider::OptionsSource>
+ llvm::ErrorOr<
+ std::vector<clang::tidy::ClangTidyOptionsProvider::OptionsSource>>
RawOptions = OptionsProvider->getRawOptions(FilePath);
- for (const std::string &Check : EnabledChecks) {
- for (const auto &[Opts, Source] : llvm::reverse(RawOptions)) {
- if (Opts.Checks && GlobList(*Opts.Checks).contains(Check)) {
- llvm::outs() << "'" << Check << "' is enabled in the " << Source
- << ".\n";
- break;
+
+ if (RawOptions) {
+ for (const std::string &Check : EnabledChecks) {
+ for (const auto &[Opts, Source] : llvm::reverse(*RawOptions)) {
+ if (Opts.Checks && GlobList(*Opts.Checks).contains(Check)) {
+ llvm::outs() << "'" << Check << "' is enabled in the " << Source
+ << ".\n";
+ break;
+ }
}
}
}
@@ -658,21 +665,23 @@ int clangTidyMain(int argc, const char **argv) {
}
if (DumpConfig) {
- EffectiveOptions.CheckOptions =
- getCheckOptions(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
+ EffectiveOptions->CheckOptions =
+ getCheckOptions(*EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
llvm::outs() << configurationAsText(ClangTidyOptions::getDefaults().merge(
- EffectiveOptions, 0))
+ *EffectiveOptions, 0))
<< "\n";
return 0;
}
if (VerifyConfig) {
- std::vector<ClangTidyOptionsProvider::OptionsSource> RawOptions =
- OptionsProvider->getRawOptions(FileName);
+ llvm::ErrorOr<std::vector<ClangTidyOptionsProvider::OptionsSource>>
+ RawOptions = OptionsProvider->getRawOptions(FileName);
+ if (!RawOptions)
+ return 1;
ChecksAndOptions Valid =
getAllChecksAndOptions(AllowEnablingAnalyzerAlphaCheckers);
bool AnyInvalid = false;
- for (const auto &[Opts, Source] : RawOptions) {
+ for (const auto &[Opts, Source] : *RawOptions) {
if (Opts.Checks)
AnyInvalid |= verifyChecks(Valid.Checks, *Opts.Checks, Source);
if (Opts.HeaderFileExtensions && Opts.ImplementationFileExtensions)
diff --git a/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp
b/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp
index 5aa3730ac5ccf..427d4e42ff567 100644
--- a/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp
@@ -46,20 +46,20 @@ TEST(ClangTidyOptionsProvider, InMemoryFileSystems) {
FileOptionsProvider FileOpt({}, {}, {}, FileSystem);
- ClangTidyOptions File1Options =
+ llvm::ErrorOr<ClangTidyOptions> File1Options =
FileOpt.getOptions("ProjectRoot/SubDir1/File.cpp");
- ClangTidyOptions File2Options =
+ llvm::ErrorOr<ClangTidyOptions> File2Options =
FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/File.cpp");
- ClangTidyOptions File3Options =
+ llvm::ErrorOr<ClangTidyOptions> File3Options =
FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/SubDir3/File.cpp");
- ASSERT_TRUE(File1Options.Checks.has_value());
- EXPECT_EQ(*File1Options.Checks, "-*,clang-diagnostic-*,readability-*");
- ASSERT_TRUE(File2Options.Checks.has_value());
- EXPECT_EQ(*File2Options.Checks, "bugprone-*,misc-*,clang-diagnostic-*");
+ ASSERT_TRUE(File1Options->Checks.has_value());
+ EXPECT_EQ(*File1Options->Checks, "-*,clang-diagnostic-*,readability-*");
+ ASSERT_TRUE(File2Options->Checks.has_value());
+ EXPECT_EQ(*File2Options->Checks, "bugprone-*,misc-*,clang-diagnostic-*");
// 2 and 3 should use the same config so these should also be the same.
- EXPECT_EQ(File2Options.Checks, File3Options.Checks);
+ EXPECT_EQ(File2Options->Checks, File3Options->Checks);
}
} // namespace test
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
