diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index a71813a103df3..b73971c68301d 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -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 fileOptions = + OptionsProvider->getOptions(File); + + // If there was an error parsing the options, just use the default options. + // 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; } diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp index 1c480d107ae9c..9a8f0fd7370ac 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp @@ -265,16 +265,19 @@ const char ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] = "command-line option '-config'"; -ClangTidyOptions +llvm::ErrorOr ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) { ClangTidyOptions Result; unsigned Priority = 0; - for (auto &Source : getRawOptions(FileName)) + llvm::ErrorOr> Options = getRawOptions(FileName); + if (!Options) + return Options.getError(); + for (auto &Source : *Options) Result.mergeWith(Source.first, ++Priority); return Result; } -std::vector +llvm::ErrorOr> DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) { std::vector Result; Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary); @@ -290,10 +293,12 @@ ConfigOptionsProvider::ConfigOptionsProvider( std::move(OverrideOptions), std::move(FS)), ConfigOptions(std::move(ConfigOptions)) {} -std::vector +llvm::ErrorOr> ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) { - std::vector RawOptions = + llvm::ErrorOr> RawOptions = DefaultOptionsProvider::getRawOptions(FileName); + if (!RawOptions) + return RawOptions; if (ConfigOptions.InheritParentConfig.value_or(false)) { LLVM_DEBUG(llvm::dbgs() << "Getting options for file " << FileName << "...\n"); @@ -301,13 +306,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) { llvm::ErrorOr> 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; } @@ -343,21 +348,21 @@ FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) { return NormalizedAbsolutePath; } -void FileOptionsBaseProvider::addRawFileOptions( +std::error_code FileOptionsBaseProvider::addRawFileOptions( llvm::StringRef AbsolutePath, std::vector &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 { + [this, &RootPath](StringRef CurrentPath) -> llvm::ErrorOr { const auto Iter = CachedOptions.Memorized.find(CurrentPath); if (Iter != CachedOptions.Memorized.end()) return CachedOptions.Storage[Iter->second]; - std::optional OptionsSource = tryReadConfigFile(CurrentPath); + llvm::ErrorOr 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"); @@ -371,16 +376,21 @@ void FileOptionsBaseProvider::addRawFileOptions( }; for (StringRef CurrentPath = RootPath; !CurrentPath.empty(); CurrentPath = llvm::sys::path::parent_path(CurrentPath)) { - if (std::optional Result = - MemorizedConfigFile(CurrentPath)) { - CurOptions.emplace_back(Result.value()); + llvm::ErrorOr 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( @@ -402,7 +412,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 +llvm::ErrorOr> FileOptionsProvider::getRawOptions(StringRef FileName) { LLVM_DEBUG(llvm::dbgs() << "Getting options for file " << FileName << "...\n"); @@ -410,19 +420,23 @@ FileOptionsProvider::getRawOptions(StringRef FileName) { llvm::ErrorOr> AbsoluteFilePath = getNormalizedAbsolutePath(FileName); if (!AbsoluteFilePath) - return {}; + return std::vector{}; - std::vector RawOptions = + llvm::ErrorOr> 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 +llvm::ErrorOr FileOptionsBaseProvider::tryReadConfigFile(StringRef Directory) { assert(!Directory.empty()); @@ -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) { @@ -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. 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 + virtual llvm::ErrorOr> 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 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 getRawOptions(llvm::StringRef FileName) override; + llvm::ErrorOr> + 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 (llvm::MemoryBufferRef)>>; + using ConfigFileHandler = + std::pair( + 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 &CurOptions); + std::error_code addRawFileOptions(llvm::StringRef AbsolutePath, + std::vector &CurOptions); llvm::ErrorOr> getNormalizedAbsolutePath(llvm::StringRef AbsolutePath); /// Try to read configuration files from \p Directory using registered /// \c ConfigHandlers. - std::optional tryReadConfigFile(llvm::StringRef Directory); + llvm::ErrorOr tryReadConfigFile(llvm::StringRef Directory); struct OptionsCache { llvm::StringMap Memorized; @@ -262,7 +265,8 @@ class ConfigOptionsProvider : public FileOptionsBaseProvider { ClangTidyGlobalOptions GlobalOptions, ClangTidyOptions DefaultOptions, ClangTidyOptions ConfigOptions, ClangTidyOptions OverrideOptions, llvm::IntrusiveRefCntPtr FS = nullptr); - std::vector getRawOptions(llvm::StringRef FileName) override; + llvm::ErrorOr> + getRawOptions(llvm::StringRef FileName) override; private: ClangTidyOptions ConfigOptions; @@ -315,7 +319,8 @@ class FileOptionsProvider : public FileOptionsBaseProvider { ClangTidyOptions OverrideOptions, ConfigFileHandlers ConfigHandlers); - std::vector getRawOptions(llvm::StringRef FileName) override; + llvm::ErrorOr> + 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 7c3aee29c146d..570990389dfd1 100644 --- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp +++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp @@ -624,17 +624,37 @@ int clangTidyMain(int argc, const char **argv) { } SmallString<256> FilePath = makeAbsolute(FileName); - ClangTidyOptions EffectiveOptions = OptionsProvider->getOptions(FilePath); + llvm::ErrorOr 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 Options = + OptionsProvider->getOptions(*iter); + if (!Options) + return 1; + } + } std::vector EnabledChecks = - getCheckNames(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers); + getCheckNames(*EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers); if (ExplainConfig) { // FIXME: Show other ClangTidyOptions' fields, like ExtraArg. - std::vector + llvm::ErrorOr< + std::vector> 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"; @@ -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 RawOptions = - OptionsProvider->getRawOptions(FileName); + llvm::ErrorOr> + 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/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index d6f2d2b37624e..13b230a877f66 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -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 ^^^^^^^^^^ diff --git a/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp b/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp index 5aa3730ac5ccf..652815493fade 100644 --- a/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/OptionsProviderTest.cpp @@ -1,4 +1,4 @@ -//===---- ObjCModuleTest.cpp - clang-tidy ---------------------------------===// +//===---- OptionsProviderTest.cpp - clang-tidy ----------------------------===// // // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // See https://llvm.org/LICENSE.txt for license information. @@ -46,20 +46,67 @@ TEST(ClangTidyOptionsProvider, InMemoryFileSystems) { FileOptionsProvider FileOpt({}, {}, {}, FileSystem); - ClangTidyOptions File1Options = + llvm::ErrorOr File1Options = FileOpt.getOptions("ProjectRoot/SubDir1/File.cpp"); - ClangTidyOptions File2Options = + llvm::ErrorOr File2Options = FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/File.cpp"); - ClangTidyOptions File3Options = + llvm::ErrorOr 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); + ASSERT_TRUE(File1Options->Checks.has_value()); + EXPECT_EQ(*File1Options->Checks, "-*,clang-diagnostic-*,readability-*"); + ASSERT_TRUE(File2Options); + 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); +} + +TEST(ClangTidyOptionsProvider, InvalidConfigurationFiles) { + llvm::IntrusiveRefCntPtr FileSystem( + new llvm::vfs::InMemoryFileSystem); + + StringRef BaseClangTidy = R"( + Checks: -*,clang-diagnostic-* + Unexpected + )"; + StringRef Sub1ClangTidy = R"( + Checks: readability-* + InheritParentConfig: true + )"; + StringRef Sub2ClangTidy = R"( + Checks: bugprone-*,misc-*,clang-diagnostic-* + InheritParentConfig: false + )"; + FileSystem->addFile("ProjectRoot/.clang-tidy", 0, + llvm::MemoryBuffer::getMemBuffer(BaseClangTidy)); + FileSystem->addFile("ProjectRoot/File.cpp", 0, + llvm::MemoryBuffer::getMemBuffer("")); + FileSystem->addFile("ProjectRoot/SubDir1/.clang-tidy", 0, + llvm::MemoryBuffer::getMemBuffer(Sub1ClangTidy)); + FileSystem->addFile("ProjectRoot/SubDir1/File.cpp", 0, + llvm::MemoryBuffer::getMemBuffer("")); + FileSystem->addFile("ProjectRoot/SubDir1/SubDir2/.clang-tidy", 0, + llvm::MemoryBuffer::getMemBuffer(Sub2ClangTidy)); + FileSystem->addFile("ProjectRoot/SubDir1/SubDir2/File.cpp", 0, + llvm::MemoryBuffer::getMemBuffer("")); + + FileOptionsProvider FileOpt({}, {}, {}, FileSystem); + + llvm::ErrorOr File1Options = + FileOpt.getOptions("ProjectRoot/File.cpp"); + llvm::ErrorOr File2Options = + FileOpt.getOptions("ProjectRoot/SubDir1/File.cpp"); + llvm::ErrorOr File3Options = + FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/File.cpp"); + + ASSERT_FALSE(File1Options); + ASSERT_FALSE(File2Options); + ASSERT_TRUE(File3Options); + ASSERT_TRUE(File3Options->Checks.has_value()); + EXPECT_EQ(*File3Options->Checks, "bugprone-*,misc-*,clang-diagnostic-*"); } } // namespace test