Skip to content

Commit b8faa7b

Browse files
galeneliasgalenelias
committed
[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.
1 parent a76936f commit b8faa7b

File tree

5 files changed

+91
-63
lines changed

5 files changed

+91
-63
lines changed

‎clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ ClangTidyOptions ClangTidyContext::getOptionsForFile(StringRef File) const {
266266
// Merge options on top of getDefaults() as a safeguard against options with
267267
// unset values.
268268
return ClangTidyOptions::getDefaults().merge(
269-
OptionsProvider->getOptions(File), 0);
269+
*OptionsProvider->getOptions(File), 0);
270270
}
271271

272272
void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }

‎clang-tools-extra/clang-tidy/ClangTidyOptions.cpp‎

Lines changed: 40 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -267,16 +267,19 @@ const char
267267
ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
268268
"command-line option '-config'";
269269

270-
ClangTidyOptions
270+
llvm::ErrorOr<ClangTidyOptions>
271271
ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
272272
ClangTidyOptions Result;
273273
unsigned Priority = 0;
274-
for (auto &Source : getRawOptions(FileName))
274+
llvm::ErrorOr<std::vector<OptionsSource>> Options = getRawOptions(FileName);
275+
if (!Options)
276+
return Options.getError();
277+
for (auto &Source : *Options)
275278
Result.mergeWith(Source.first, ++Priority);
276279
return Result;
277280
}
278281

279-
std::vector<OptionsSource>
282+
llvm::ErrorOr<std::vector<OptionsSource>>
280283
DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
281284
std::vector<OptionsSource> Result;
282285
Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,24 +295,26 @@ ConfigOptionsProvider::ConfigOptionsProvider(
292295
std::move(OverrideOptions), std::move(FS)),
293296
ConfigOptions(std::move(ConfigOptions)) {}
294297

295-
std::vector<OptionsSource>
298+
llvm::ErrorOr<std::vector<OptionsSource>>
296299
ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
297-
std::vector<OptionsSource> RawOptions =
300+
llvm::ErrorOr<std::vector<OptionsSource>> RawOptions =
298301
DefaultOptionsProvider::getRawOptions(FileName);
302+
if (!RawOptions)
303+
return RawOptions;
299304
if (ConfigOptions.InheritParentConfig.value_or(false)) {
300305
LLVM_DEBUG(llvm::dbgs()
301306
<< "Getting options for file " << FileName << "...\n");
302307

303308
llvm::ErrorOr<llvm::SmallString<128>> AbsoluteFilePath =
304309
getNormalizedAbsolutePath(FileName);
305310
if (AbsoluteFilePath) {
306-
addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
311+
addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
307312
}
308313
}
309-
RawOptions.emplace_back(ConfigOptions,
310-
OptionsSourceTypeConfigCommandLineOption);
311-
RawOptions.emplace_back(OverrideOptions,
312-
OptionsSourceTypeCheckCommandLineOption);
314+
RawOptions->emplace_back(ConfigOptions,
315+
OptionsSourceTypeConfigCommandLineOption);
316+
RawOptions->emplace_back(OverrideOptions,
317+
OptionsSourceTypeCheckCommandLineOption);
313318
return RawOptions;
314319
}
315320

@@ -345,21 +350,21 @@ FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) {
345350
return NormalizedAbsolutePath;
346351
}
347352

348-
void FileOptionsBaseProvider::addRawFileOptions(
353+
std::error_code FileOptionsBaseProvider::addRawFileOptions(
349354
llvm::StringRef AbsolutePath, std::vector<OptionsSource> &CurOptions) {
350355
auto CurSize = CurOptions.size();
351356
// Look for a suitable configuration file in all parent directories of the
352357
// file. Start with the immediate parent directory and move up.
353358
StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
354359
auto MemorizedConfigFile =
355-
[this, &RootPath](StringRef CurrentPath) -> std::optional<OptionsSource> {
360+
[this, &RootPath](StringRef CurrentPath) -> llvm::ErrorOr<OptionsSource> {
356361
const auto Iter = CachedOptions.Memorized.find(CurrentPath);
357362
if (Iter != CachedOptions.Memorized.end())
358363
return CachedOptions.Storage[Iter->second];
359-
std::optional<OptionsSource> OptionsSource = tryReadConfigFile(CurrentPath);
364+
llvm::ErrorOr<OptionsSource> OptionsSource = tryReadConfigFile(CurrentPath);
360365
if (OptionsSource) {
361366
const size_t Index = CachedOptions.Storage.size();
362-
CachedOptions.Storage.emplace_back(OptionsSource.value());
367+
CachedOptions.Storage.emplace_back(*OptionsSource);
363368
while (RootPath != CurrentPath) {
364369
LLVM_DEBUG(llvm::dbgs()
365370
<< "Caching configuration for path " << RootPath << ".\n");
@@ -373,16 +378,21 @@ void FileOptionsBaseProvider::addRawFileOptions(
373378
};
374379
for (StringRef CurrentPath = RootPath; !CurrentPath.empty();
375380
CurrentPath = llvm::sys::path::parent_path(CurrentPath)) {
376-
if (std::optional<OptionsSource> Result =
377-
MemorizedConfigFile(CurrentPath)) {
378-
CurOptions.emplace_back(Result.value());
381+
llvm::ErrorOr<OptionsSource> Result = MemorizedConfigFile(CurrentPath);
382+
383+
if (Result) {
384+
CurOptions.emplace_back(*Result);
379385
if (!Result->first.InheritParentConfig.value_or(false))
380386
break;
387+
} else if (Result.getError() != llvm::errc::no_such_file_or_directory) {
388+
return Result.getError();
381389
}
382390
}
383391
// Reverse order of file configs because closer configs should have higher
384392
// priority.
385393
std::reverse(CurOptions.begin() + CurSize, CurOptions.end());
394+
395+
return {};
386396
}
387397

388398
FileOptionsProvider::FileOptionsProvider(
@@ -404,27 +414,31 @@ FileOptionsProvider::FileOptionsProvider(
404414
// FIXME: This method has some common logic with clang::format::getStyle().
405415
// Consider pulling out common bits to a findParentFileWithName function or
406416
// similar.
407-
std::vector<OptionsSource>
417+
llvm::ErrorOr<std::vector<OptionsSource>>
408418
FileOptionsProvider::getRawOptions(StringRef FileName) {
409419
LLVM_DEBUG(llvm::dbgs() << "Getting options for file " << FileName
410420
<< "...\n");
411421

412422
llvm::ErrorOr<llvm::SmallString<128>> AbsoluteFilePath =
413423
getNormalizedAbsolutePath(FileName);
414424
if (!AbsoluteFilePath)
415-
return {};
425+
return std::vector<OptionsSource>{};
416426

417-
std::vector<OptionsSource> RawOptions =
427+
llvm::ErrorOr<std::vector<OptionsSource>> RawOptions =
418428
DefaultOptionsProvider::getRawOptions(AbsoluteFilePath->str());
419-
addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
429+
std::error_code Err = addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
430+
431+
if (Err)
432+
return Err;
433+
420434
OptionsSource CommandLineOptions(OverrideOptions,
421435
OptionsSourceTypeCheckCommandLineOption);
422436

423-
RawOptions.push_back(CommandLineOptions);
437+
RawOptions->push_back(CommandLineOptions);
424438
return RawOptions;
425439
}
426440

427-
std::optional<OptionsSource>
441+
llvm::ErrorOr<OptionsSource>
428442
FileOptionsBaseProvider::tryReadConfigFile(StringRef Directory) {
429443
assert(!Directory.empty());
430444

@@ -433,7 +447,7 @@ FileOptionsBaseProvider::tryReadConfigFile(StringRef Directory) {
433447
if (!DirectoryStatus || !DirectoryStatus->isDirectory()) {
434448
llvm::errs() << "Error reading configuration from " << Directory
435449
<< ": directory doesn't exist.\n";
436-
return std::nullopt;
450+
return make_error_code(llvm::errc::not_a_directory);
437451
}
438452

439453
for (const ConfigFileHandler &ConfigHandler : ConfigHandlers) {
@@ -464,11 +478,11 @@ FileOptionsBaseProvider::tryReadConfigFile(StringRef Directory) {
464478
if (ParsedOptions.getError())
465479
llvm::errs() << "Error parsing " << ConfigFile << ": "
466480
<< ParsedOptions.getError().message() << "\n";
467-
continue;
481+
return ParsedOptions.getError();
468482
}
469483
return OptionsSource(*ParsedOptions, std::string(ConfigFile));
470484
}
471-
return std::nullopt;
485+
return make_error_code(llvm::errc::no_such_file_or_directory);
472486
}
473487

474488
/// Parses -line-filter option and stores it to the \c Options.

‎clang-tools-extra/clang-tidy/ClangTidyOptions.h‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -174,12 +174,12 @@ class ClangTidyOptionsProvider {
174174

175175
/// Returns an ordered vector of OptionsSources, in order of increasing
176176
/// priority.
177-
virtual std::vector<OptionsSource>
177+
virtual llvm::ErrorOr<std::vector<OptionsSource>>
178178
getRawOptions(llvm::StringRef FileName) = 0;
179179

180180
/// Returns options applying to a specific translation unit with the
181181
/// specified \p FileName.
182-
ClangTidyOptions getOptions(llvm::StringRef FileName);
182+
llvm::ErrorOr<ClangTidyOptions> getOptions(llvm::StringRef FileName);
183183
};
184184

185185
/// Implementation of the \c ClangTidyOptionsProvider interface, which
@@ -193,7 +193,8 @@ class DefaultOptionsProvider : public ClangTidyOptionsProvider {
193193
const ClangTidyGlobalOptions &getGlobalOptions() override {
194194
return GlobalOptions;
195195
}
196-
std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
196+
llvm::ErrorOr<std::vector<OptionsSource>>
197+
getRawOptions(llvm::StringRef FileName) override;
197198

198199
private:
199200
ClangTidyGlobalOptions GlobalOptions;
@@ -204,7 +205,9 @@ class FileOptionsBaseProvider : public DefaultOptionsProvider {
204205
protected:
205206
// A pair of configuration file base name and a function parsing
206207
// configuration from text in the corresponding format.
207-
using ConfigFileHandler = std::pair<std::string, std::function<llvm::ErrorOr<ClangTidyOptions> (llvm::MemoryBufferRef)>>;
208+
using ConfigFileHandler =
209+
std::pair<std::string, std::function<llvm::ErrorOr<ClangTidyOptions>(
210+
llvm::MemoryBufferRef)>>;
208211

209212
/// Configuration file handlers listed in the order of priority.
210213
///
@@ -235,15 +238,15 @@ class FileOptionsBaseProvider : public DefaultOptionsProvider {
235238
ClangTidyOptions OverrideOptions,
236239
ConfigFileHandlers ConfigHandlers);
237240

238-
void addRawFileOptions(llvm::StringRef AbsolutePath,
239-
std::vector<OptionsSource> &CurOptions);
241+
std::error_code addRawFileOptions(llvm::StringRef AbsolutePath,
242+
std::vector<OptionsSource> &CurOptions);
240243

241244
llvm::ErrorOr<llvm::SmallString<128>>
242245
getNormalizedAbsolutePath(llvm::StringRef AbsolutePath);
243246

244247
/// Try to read configuration files from \p Directory using registered
245248
/// \c ConfigHandlers.
246-
std::optional<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
249+
llvm::ErrorOr<OptionsSource> tryReadConfigFile(llvm::StringRef Directory);
247250

248251
struct OptionsCache {
249252
llvm::StringMap<size_t> Memorized;
@@ -262,7 +265,8 @@ class ConfigOptionsProvider : public FileOptionsBaseProvider {
262265
ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions,
263266
ClangTidyOptions ConfigOptions, ClangTidyOptions OverrideOptions,
264267
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS = nullptr);
265-
std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
268+
llvm::ErrorOr<std::vector<OptionsSource>>
269+
getRawOptions(llvm::StringRef FileName) override;
266270

267271
private:
268272
ClangTidyOptions ConfigOptions;
@@ -315,7 +319,8 @@ class FileOptionsProvider : public FileOptionsBaseProvider {
315319
ClangTidyOptions OverrideOptions,
316320
ConfigFileHandlers ConfigHandlers);
317321

318-
std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override;
322+
llvm::ErrorOr<std::vector<OptionsSource>>
323+
getRawOptions(llvm::StringRef FileName) override;
319324
};
320325

321326
/// Parses LineFilter from JSON and stores it to the \p Options.

‎clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp‎

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -337,8 +337,7 @@ Allow empty enabled checks. This suppresses
337337
the "no checks enabled" error when disabling
338338
all of the checks.
339339
)"),
340-
cl::init(false),
341-
cl::cat(ClangTidyCategory));
340+
cl::init(false), cl::cat(ClangTidyCategory));
342341

343342
namespace clang::tidy {
344343

@@ -370,8 +369,8 @@ static void printStats(const ClangTidyStats &Stats) {
370369
}
371370
}
372371

373-
static std::unique_ptr<ClangTidyOptionsProvider> createOptionsProvider(
374-
llvm::IntrusiveRefCntPtr<vfs::FileSystem> FS) {
372+
static std::unique_ptr<ClangTidyOptionsProvider>
373+
createOptionsProvider(llvm::IntrusiveRefCntPtr<vfs::FileSystem> FS) {
375374
ClangTidyGlobalOptions GlobalOptions;
376375
if (std::error_code Err = parseLineFilter(LineFilter, GlobalOptions)) {
377376
llvm::errs() << "Invalid LineFilter: " << Err.message() << "\n\nUsage:\n";
@@ -624,21 +623,29 @@ int clangTidyMain(int argc, const char **argv) {
624623
}
625624

626625
SmallString<256> FilePath = makeAbsolute(FileName);
627-
ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath);
626+
llvm::ErrorOr<ClangTidyOptions> EffectiveOptions =
627+
OptionsProvider->getOptions(FilePath);
628+
629+
if (!EffectiveOptions)
630+
return 1;
628631

629632
std::vector<std::string> EnabledChecks =
630-
getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
633+
getCheckNames(*EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
631634

632635
if (ExplainConfig) {
633636
// FIXME: Show other ClangTidyOptions' fields, like ExtraArg.
634-
std::vector<clang::tidy::ClangTidyOptionsProvider::OptionsSource>
637+
llvm::ErrorOr<
638+
std::vector<clang::tidy::ClangTidyOptionsProvider::OptionsSource>>
635639
RawOptions = OptionsProvider->getRawOptions(FilePath);
636-
for (const std::string &Check : EnabledChecks) {
637-
for (const auto &[Opts, Source] : llvm::reverse(RawOptions)) {
638-
if (Opts.Checks && GlobList(*Opts.Checks).contains(Check)) {
639-
llvm::outs() << "'" << Check << "' is enabled in the " << Source
640-
<< ".\n";
641-
break;
640+
641+
if (RawOptions) {
642+
for (const std::string &Check : EnabledChecks) {
643+
for (const auto &[Opts, Source] : llvm::reverse(*RawOptions)) {
644+
if (Opts.Checks && GlobList(*Opts.Checks).contains(Check)) {
645+
llvm::outs() << "'" << Check << "' is enabled in the " << Source
646+
<< ".\n";
647+
break;
648+
}
642649
}
643650
}
644651
}
@@ -658,21 +665,23 @@ int clangTidyMain(int argc, const char **argv) {
658665
}
659666

660667
if (DumpConfig) {
661-
EffectiveOptions.CheckOptions =
662-
getCheckOptions(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
668+
EffectiveOptions->CheckOptions =
669+
getCheckOptions(*EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
663670
llvm::outs() << configurationAsText(ClangTidyOptions::getDefaults().merge(
664-
EffectiveOptions, 0))
671+
*EffectiveOptions, 0))
665672
<< "\n";
666673
return 0;
667674
}
668675

669676
if (VerifyConfig) {
670-
std::vector<ClangTidyOptionsProvider::OptionsSource> RawOptions =
671-
OptionsProvider->getRawOptions(FileName);
677+
llvm::ErrorOr<std::vector<ClangTidyOptionsProvider::OptionsSource>>
678+
RawOptions = OptionsProvider->getRawOptions(FileName);
679+
if (!RawOptions)
680+
return 1;
672681
ChecksAndOptions Valid =
673682
getAllChecksAndOptions(AllowEnablingAnalyzerAlphaCheckers);
674683
bool AnyInvalid = false;
675-
for (const auto &[Opts, Source] : RawOptions) {
684+
for (const auto &[Opts, Source] : *RawOptions) {
676685
if (Opts.Checks)
677686
AnyInvalid |= verifyChecks(Valid.Checks, *Opts.Checks, Source);
678687
if (Opts.HeaderFileExtensions && Opts.ImplementationFileExtensions)

‎clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,20 +46,20 @@ TEST(ClangTidyOptionsProvider, InMemoryFileSystems) {
4646

4747
FileOptionsProvider FileOpt({}, {}, {}, FileSystem);
4848

49-
ClangTidyOptions File1Options =
49+
llvm::ErrorOr<ClangTidyOptions> File1Options =
5050
FileOpt.getOptions("ProjectRoot/SubDir1/File.cpp");
51-
ClangTidyOptions File2Options =
51+
llvm::ErrorOr<ClangTidyOptions> File2Options =
5252
FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/File.cpp");
53-
ClangTidyOptions File3Options =
53+
llvm::ErrorOr<ClangTidyOptions> File3Options =
5454
FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/SubDir3/File.cpp");
5555

56-
ASSERT_TRUE(File1Options.Checks.has_value());
57-
EXPECT_EQ(*File1Options.Checks, "-*,clang-diagnostic-*,readability-*");
58-
ASSERT_TRUE(File2Options.Checks.has_value());
59-
EXPECT_EQ(*File2Options.Checks, "bugprone-*,misc-*,clang-diagnostic-*");
56+
ASSERT_TRUE(File1Options->Checks.has_value());
57+
EXPECT_EQ(*File1Options->Checks, "-*,clang-diagnostic-*,readability-*");
58+
ASSERT_TRUE(File2Options->Checks.has_value());
59+
EXPECT_EQ(*File2Options->Checks, "bugprone-*,misc-*,clang-diagnostic-*");
6060

6161
// 2 and 3 should use the same config so these should also be the same.
62-
EXPECT_EQ(File2Options.Checks, File3Options.Checks);
62+
EXPECT_EQ(File2Options->Checks, File3Options->Checks);
6363
}
6464

6565
} // namespace test

0 commit comments

Comments
 (0)