[clangd] Validate clang-tidy Checks in clangd config.

[njames93]

Summary

Add instrumentation in ConfigCompile to validate that items in ClangTidy:[Add|Remove] correspond to actual clang-tidy checks.
If they don't a warning will be presented to the user.

This is especially useful for catching typos in the glob items.

[njames93]

Made GlobList::specifies const.
Made check validation function static and only taking a StringRef. This will make it easier to refactor out at a later date if this functionality wants to be used elsewhere.

[sammccall]

Thanks for doing this!
I wonder a simpler version would reach the 80/20 point... if we were to validate only that entries *without wildcards* matched a check name, I think we'd catch most of the typos, and it'd just be a simple set lookup.
(bugprone-* is quadratically less likely to have a typo than a list of checks, because it's shorter and also large globs just don't appear as many times)

clang-tools-extra/clangd/ConfigCompile.cpp

362

if you kept the ClangTidyCheckFactories alive forever, you can just use the StringRefs from its keys, right?

[njames93]

Dont check for globs coverage instead use a fast set based approach that only matches tidy checks exactly.

[sammccall]

Thanks! Just some organization nits.

clang-tools-extra/clangd/ConfigCompile.cpp

26

This is a pretty weird place to depend on clang-tidy.
Can we move the "is this a clang-tidy check name" function to somewhere more clang-tidy related, like TidyProvider.h or even clang-tidy/ClangTidyModuleRegistry.h? ("isRegisteredCheck")

356

naming nits:

• prefer positive over negative names
• "glob" is confusing here as we don't actually handle globs
• outside of the clang-tidy namespace, "check" needs a "tidy" qualifier

What about isTidyCheckName, and move the * test into the caller? Then the semantics of the function are really clear without having to read the body or comment.

410

message could be clearer - "clang-tidy check '{0}' was not found"

[njames93]

clang-tools-extra/clangd/ConfigCompile.cpp

26

Moving to ClangTidyModuleRegistry.h will still require this include.
Moving the function to TidyProvider.h may have a case, but as TidyProvider doesn't use the function I'm not sure it belongs in there.

[njames93]

Address some comments

[sammccall]

clang-tools-extra/clangd/ConfigCompile.cpp

26

Yeah, the include is unfortunate but I think it's an improvement over having the implementation details in the code.

TidyProvider.h - we could rename the header to Tidy.h if you think it's important - I can live with the naming discrepancy but think we should group clang-tidy-config related stuff.

54

nit: if this is in the clangd code, it should be in the clangd namespace (isRegisteredTidyCheck or so).

59

these asserts seem to violate principle-of-least-surprise - why can't isRegisteredCheck("-* ") just return false?

[njames93]

clang-tools-extra/clangd/ConfigCompile.cpp

26

That sounds like a refactor that shouldn't live in this patch.

59

The reason for this pre condition was because I wanted to assert that the first non whitespace item was not a -. But there are better ways around that.

[njames93]

Address comments

clang-tools-extra/clang-tidy/ClangTidyModule.h

public:
   /// Create instances of checks that are enabled.
   std::vector<std::unique_ptr<ClangTidyCheck>>
   createChecks(ClangTidyContext *Context);
 
   typedef llvm::StringMap<CheckFactory> FactoryMap;
   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;
 };
 
 /// A clang-tidy module groups a number of \c ClangTidyChecks and gives
 /// them a prefixed name.
 class ClangTidyModule {

clang-tools-extra/clang-tidy/GlobList.h

public:
   /// An empty \p Globs string is interpreted as one glob that matches an empty
   /// string.
   GlobList(StringRef Globs);
 
   /// Returns \c true if the pattern matches \p S. The result is the last
   /// 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 {
     bool IsPositive;
     llvm::Regex Regex;
   };
   SmallVector<GlobListItem, 0> Items;
 };
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GLOBLIST_H

clang-tools-extra/clang-tidy/GlobList.cpp

bool GlobList::contains(StringRef S) {
   // Iterating the container backwards as the last match determins if S is in
   // the list.
   for (const GlobListItem &Item : llvm::reverse(Items)) {
     if (Item.Regex.match(S))
       return Item.IsPositive;
   }
   return false;
 }
+
+bool GlobList::specifies(StringRef S) {
+  return llvm::any_of(
+      Items, [&S](const GlobListItem &Item) { return Item.Regex.match(S); });
+}

clang-tools-extra/clangd/ConfigCompile.cpp

 // do as much work as possible in the first step. For example, regexes are
 // compiled in stage 1 and captured by the apply function. This is because:
 //
 //  - it's more efficient, as the work done in stage 1 must only be done once
 //  - problems can be reported in stage 1, in stage 2 we must silently recover
 //
 //===----------------------------------------------------------------------===//
 
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
+#include "../clang-tidy/GlobList.h"
 #include "CompileCommands.h"
 #include "Config.h"
 #include "ConfigFragment.h"
 #include "ConfigProvider.h"
 #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"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace config {
 namespace {
 
 // Returns an empty stringref if Path is not under FragmentDir. Returns Path
 // as-is when FragmentDir is empty.
 llvm::StringRef configRelative(llvm::StringRef Path,
                                llvm::StringRef FragmentDir) {
   if (FragmentDir.empty())
     return Path;
   if (!Path.consume_front(FragmentDir))
     return llvm::StringRef();
   return Path.empty() ? "." : Path;
 }
if (!F.FullyQualifiedNamespaces.empty()) {
                               const Params &, Config &C) {
         C.Style.FullyQualifiedNamespaces.insert(
             C.Style.FullyQualifiedNamespaces.begin(),
             FullyQualifiedNamespaces.begin(), FullyQualifiedNamespaces.end());
       });
     }
   }
 
+  // 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<std::string> &CheckGlob) {
+    static llvm::ArrayRef<StringRef> AllChecks = [] {
+      // In a simple world this would be a std::vector<std::string>. 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<StringRef> AllChecks(
+          Alloc.Allocate<StringRef>(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<char>(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<std::string> &Arg, bool IsPositive) {
     StringRef Str = *Arg;
     // Don't support negating here, its handled if the item is in the Add or
     // Remove list.
     if (Str.startswith("-") || Str.contains(',')) {
       diag(Error, "Invalid clang-tidy check name", Arg.Range);
       return;
     }
     CurSpec += ',';
     if (!IsPositive)
       CurSpec += '-';
     CurSpec += Str;
   }
 
   void compile(Fragment::ClangTidyBlock &&F) {
     std::string Checks;
-    for (auto &CheckGlob : F.Add)
+    for (const auto &CheckGlob : F.Add) {
+      if (validateCheckGlob(CheckGlob))
         appendTidyCheckSpec(Checks, CheckGlob, true);
+    }
 
-    for (auto &CheckGlob : F.Remove)
+    for (const auto &CheckGlob : F.Remove) {
+      if (validateCheckGlob(CheckGlob))
         appendTidyCheckSpec(Checks, CheckGlob, false);
+    }
 
     if (!Checks.empty())
       Out.Apply.push_back(
           [Checks = std::move(Checks)](const Params &, Config &C) {
             C.ClangTidy.Checks.append(
                 Checks, C.ClangTidy.Checks.empty() ? /*skip comma*/ 1 : 0,
                 std::string::npos);
           });

clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp

TEST_F(ConfigCompileTests, Tidy) {
   EXPECT_TRUE(compileAndApply());
   EXPECT_EQ(
       Conf.ClangTidy.Checks,
       "bugprone-use-after-move,llvm-*,-llvm-include-order,-readability-*");
   EXPECT_EQ(Conf.ClangTidy.CheckOptions.size(), 2U);
   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) {
   Fragment::IndexBlock::ExternalBlock External;
   External.File.emplace("");
   External.Server.emplace("");
   Frag.Index.External = std::move(External);
   compileAndApply();