Skip to content

[clang-tidy] Return error code on config parse error #136167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
galenelias wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[clang-tidy] Return error code on config parse error #136167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b8faa7b
[clang-tidy] Return error code on config parse error
galenelias Apr 16, 2025
6263a3b
Add Release Notes
galenelias May 1, 2025
db40a30
Add some Unit Test coverage
galenelias May 1, 2025
e8d6c28
Revert unrelated clang-format fixes
galenelias May 1, 2025
c05ed3d
Return error code in ExplainConfig path
galenelias May 1, 2025
ef6baf7
Validate configs for all inputs files up front
galenelias May 1, 2025
980dd47
Address review comments
galenelias May 27, 2025
e8eac1e
Fix formatting of test header comment
galenelias May 27, 2025
f5923c8
Merge remote-tracking branch 'upstream/main' into user/gelias/issue77…
galenelias May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,16 @@ const ClangTidyOptions &ClangTidyContext::getOptions() const {
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);
ClangTidyOptions defaultOptions = ClangTidyOptions::getDefaults();
llvm::ErrorOr<ClangTidyOptions> fileOptions =
OptionsProvider->getOptions(File);

// If there was an error parsing the options, just use the default options.
Copy link
Contributor

@carlosgalvezp carlosgalvezp May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we fail hard/propagate the error further here? Returning the default options hides the problem for the user.

Copy link
Contributor Author

@galenelias galenelias May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but it seemed quite difficult to propagate an error from this function and would drastically increase the size and invasiveness of this change, as every call of this function would need to propagate the error, and so on and so forth recursively until dozens+ of functions have been changed.

Given that in the primary clang-tidy usage scenario that these configs have already been pre-validated, and that this fallback behavior is basically equivalent to the previous behavior of silently continuing on with the default config, this seemed like a reasonable compromise.

I can pursue propagating the error, but when I initially investigated it, it seems quite complicated given the current places this method is called and how prepared the callers were for propagating errors.

// Ideally, the options for each file should be validated before this point.
if (!fileOptions)
return defaultOptions;

return defaultOptions.merge(*fileOptions, 0);
}

void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
Expand Down
66 changes: 40 additions & 26 deletions clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,16 +265,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);
Expand All @@ -290,24 +293,26 @@ 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");

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;
}

Expand Down Expand Up @@ -343,21 +348,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");
Expand All @@ -371,16 +376,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(
Expand All @@ -402,27 +412,31 @@ 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");

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());

Expand All @@ -431,7 +445,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) {
Expand Down Expand Up @@ -462,11 +476,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.
Expand Down
23 changes: 14 additions & 9 deletions clang-tools-extra/clang-tidy/ClangTidyOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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.
///
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
42 changes: 32 additions & 10 deletions clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,17 +624,37 @@ 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;

// Validate the configuration files associated with all input files so we can
// return an error up front.
if (PathList.size() > 1) {
for (auto iter = PathList.begin() + 1; iter != PathList.end(); ++iter) {
llvm::ErrorOr<ClangTidyOptions> Options =
OptionsProvider->getOptions(*iter);
if (!Options)
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);

if (!RawOptions)
return 1;

for (const std::string &Check : EnabledChecks) {
for (const auto &[Opts, Source] : llvm::reverse(RawOptions)) {
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";
Expand All @@ -658,21 +678,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)
Expand Down
3 changes: 3 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ Improvements to clang-tidy
- Fixed bug in :program:`run_clang_tidy.py` where the program would not
correctly display the checks enabled by the top-level `.clang-tidy` file.

- Changed :program:`clang-tidy` to exit with an error code if it encounters an
invalid configuration file.

New checks
^^^^^^^^^^

Expand Down
Loading
Loading