diff --git a/clang-tools-extra/clang-tidy/ClangTidyModule.h b/clang-tools-extra/clang-tidy/ClangTidyModule.h --- a/clang-tools-extra/clang-tidy/ClangTidyModule.h +++ b/clang-tools-extra/clang-tidy/ClangTidyModule.h @@ -73,6 +73,7 @@ FactoryMap::const_iterator begin() const { return Factories.begin(); } FactoryMap::const_iterator end() const { return Factories.end(); } bool empty() const { return Factories.empty(); } + size_t size() const { return Factories.size(); } private: FactoryMap Factories; diff --git a/clang-tools-extra/clang-tidy/GlobList.h b/clang-tools-extra/clang-tidy/GlobList.h --- a/clang-tools-extra/clang-tidy/GlobList.h +++ b/clang-tools-extra/clang-tidy/GlobList.h @@ -35,6 +35,10 @@ /// matching glob's Positive flag. bool contains(StringRef S); + /// Returns \c true if the pattern contains an entry matching \p S, Doesn't + /// specify if the entry is positive or negative. + bool specifies(StringRef S); + private: struct GlobListItem { diff --git a/clang-tools-extra/clang-tidy/GlobList.cpp b/clang-tools-extra/clang-tidy/GlobList.cpp --- a/clang-tools-extra/clang-tidy/GlobList.cpp +++ b/clang-tools-extra/clang-tidy/GlobList.cpp @@ -61,3 +61,8 @@ } return false; } + +bool GlobList::specifies(StringRef S) { + return llvm::any_of( + Items, [&S](const GlobListItem &Item) { return Item.Regex.match(S); }); +} diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -23,6 +23,8 @@ // //===----------------------------------------------------------------------===// +#include "../clang-tidy/ClangTidyModuleRegistry.h" +#include "../clang-tidy/GlobList.h" #include "CompileCommands.h" #include "Config.h" #include "ConfigFragment.h" @@ -30,12 +32,14 @@ #include "Features.inc" #include "support/Logger.h" #include "support/Trace.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/None.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/StringSwitch.h" +#include "llvm/Support/Allocator.h" #include "llvm/Support/Error.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Format.h" @@ -347,6 +351,50 @@ } } + // Warn against any Add or Remove check items that doesn't specify a valid + // tidy check, most likely due to a typo. + bool validateCheckGlob(const Located &CheckGlob) { + static llvm::ArrayRef AllChecks = [] { + // In a simple world this would be a std::vector. However, + // using the bump allocator and an ArrayRef, we can drastically reduce + // number of allocations while simultaneously increasing cache locality. + static llvm::BumpPtrAllocator Alloc; + tidy::ClangTidyCheckFactories Factories; + for (tidy::ClangTidyModuleRegistry::entry E : + tidy::ClangTidyModuleRegistry::entries()) + E.instantiate()->addCheckFactories(Factories); + llvm::MutableArrayRef AllChecks( + Alloc.Allocate(Factories.size()), Factories.size()); + llvm::StringRef *Iter = AllChecks.begin(); + for (const auto &Factory : Factories) { + StringRef Key = Factory.getKey(); + assert(!Key.empty() && "All checks should have valid names"); + char *Ptr = Alloc.Allocate(Key.size()); + // Copy the Key into our newly allocated buffer, We don't need to worry + // about writing a null terminator. + memcpy(Ptr, Key.data(), Key.size()); + *Iter++ = StringRef(Ptr, Key.size()); + } + assert(Iter == AllChecks.end()); + return AllChecks; + }(); + tidy::GlobList List(*CheckGlob); + // Looping over the list of checks is not great in complexity, but given the + // intricacies of glob lists, a set based approach wouldn't really be + // feasible. + if (llvm::any_of(AllChecks, + [&List](const StringRef &S) { return List.specifies(S); })) + return true; + + diag(Warning, + llvm::formatv( + "Check glob '{0}' doesn't specify any known clang-tidy check", + *CheckGlob) + .str(), + CheckGlob.Range); + return false; + } + void appendTidyCheckSpec(std::string &CurSpec, const Located &Arg, bool IsPositive) { StringRef Str = *Arg; @@ -364,11 +412,15 @@ void compile(Fragment::ClangTidyBlock &&F) { std::string Checks; - for (auto &CheckGlob : F.Add) - appendTidyCheckSpec(Checks, CheckGlob, true); + for (const auto &CheckGlob : F.Add) { + if (validateCheckGlob(CheckGlob)) + appendTidyCheckSpec(Checks, CheckGlob, true); + } - for (auto &CheckGlob : F.Remove) - appendTidyCheckSpec(Checks, CheckGlob, false); + for (const auto &CheckGlob : F.Remove) { + if (validateCheckGlob(CheckGlob)) + appendTidyCheckSpec(Checks, CheckGlob, false); + } if (!Checks.empty()) Out.Apply.push_back( diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -200,6 +200,28 @@ EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("StrictMode"), "true"); EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("example-check.ExampleOption"), "0"); + EXPECT_THAT(Diags.Diagnostics, IsEmpty()); +} + +TEST_F(ConfigCompileTests, TidyBadChecks) { + Frag.ClangTidy.Add.emplace_back("unknown-check"); + Frag.ClangTidy.Add.emplace_back("unknown-module-*"); + Frag.ClangTidy.Remove.emplace_back("*"); + Frag.ClangTidy.Remove.emplace_back("llvm-includeorder"); + EXPECT_TRUE(compileAndApply()); + // Ensure bad checks are stripped from the glob. + EXPECT_EQ(Conf.ClangTidy.Checks, "-*"); + EXPECT_THAT( + Diags.Diagnostics, + ElementsAre(AllOf(DiagMessage("Check glob 'unknown-check' doesn't " + "specify any known clang-tidy check"), + DiagKind(llvm::SourceMgr::DK_Warning)), + AllOf(DiagMessage("Check glob 'unknown-module-*' doesn't " + "specify any known clang-tidy check"), + DiagKind(llvm::SourceMgr::DK_Warning)), + AllOf(DiagMessage("Check glob 'llvm-includeorder' doesn't " + "specify any known clang-tidy check"), + DiagKind(llvm::SourceMgr::DK_Warning)))); } TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {