[clang-tidy] Return error code on config parse error #136167
Open
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
|
@llvm/pr-subscribers-clang-tidy Author: Galen Elias (galenelias) ChangesThis 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 Full diff: https://github.com/llvm/llvm-project/pull/136167.diff 5 Files Affected:
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
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Galen Elias (galenelias) ChangesThis 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 Full diff: https://github.com/llvm/llvm-project/pull/136167.diff 5 Files Affected:
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
|
|
This is great, thank you! I'll have a look in the coming days. |
| @@ -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); | |||
There was a problem hiding this comment.
How do we know if we can safely dereference without checking if we have an error?
There was a problem hiding this comment.
My analysis was that because clangTidyMain does the validation explicitly up front and returns an error if it can't parse the config, that once it runs through here, we are basically guaranteed that it will succeed, because the errors have been caught.
However, digging in a bit deeper, I think there are at least two holes in this understanding: clangTidyMain only pre-validates the config for the first file passed in, so passing in a second file in a different directory with an invalid configuration will crash here. The second is potentially the 'ClangTidyPlugin' code, which seems to be another entry point into the clang tidy code that bypasses clangTidyMain. I'm not sure what the ClangTidyPlugin is or how to validate its behavior in the face of an invalid config.
I'll need to do a bit more digging to figure out the right strategy here. Just bubbling up ErrorOr returns doesn't always seem great, but might unfortunately be required.
There was a problem hiding this comment.
Ok, so my current plan is:
- Loop through all input files in clangTidyMain to explicitly validate configs for each input file and be able to return an error code there if a failure is found.
- In this method (
ClangTidyContext::getOptionsForFile) just return the default config options if an error is found, which is similar to the behavior before this PR. It seems like trying to make getOptionsForFile return an ErrorOr object and handle that at each call site is very cumbersome and leads to a lot of non-obvious failure modes that need to be resolved.
|
|
||
| // 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); | ||
| } |
There was a problem hiding this comment.
Missing a unit test that demonstrates that clang-tidy exists early with exit code 1 when the config file could not be parsed/found
There was a problem hiding this comment.
I couldn't find any tests which exercise clangTidyMain (which makes sense, as that is not really the layer a unit test would operate on. The best place I could find to put a test was near the ClangTidyOptionsProvider.InMemoryFileSystems test, which at least tests the behavior with multiple configuration files, as well as some amount of the propagation logic.
I will go ahead and add a test there unless there is a more appropriate spot for verifying the high level "early exit code 1" behavior of the PR.
carlosgalvezp
requested changes
Apr 29, 2025
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.
And make ClangTidyContext::getOptionsForFile resilient to errors at that point.
galenelias
force-pushed
the
user/gelias/issue77341_clangTidyBubbleConfigParseError
branch
from
2e09fe1 to
ef6baf7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 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
clangTidyMainso 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.