[PATCH] D92874: [clangd] Validate clang-tidy Checks in clangd config.

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 311094.
njames93 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92874/new/

https://reviews.llvm.org/D92874

Files:
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp


Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -200,6 +200,24 @@
   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.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 'llvm-includeorder' doesn't "
+                                    "specify any known clang-tidy check"),
+                        DiagKind(llvm::SourceMgr::DK_Warning))));
 }
 
 TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -23,6 +23,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "CompileCommands.h"
 #include "Config.h"
 #include "ConfigFragment.h"
@@ -35,7 +36,9 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.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,15 +350,45 @@
     }
   }
 
+  // Warn against any Add or Remove check items that doesn't specify a valid
+  // tidy check, most likely due to a typo.
+  static bool isInvalidCheckGlob(StringRef CheckGlob) {
+    // Any wildcards just assume they aren't invalid
+    if (CheckGlob.contains('*'))
+      return false;
+
+    static const llvm::StringSet<llvm::BumpPtrAllocator> AllChecks = [] {
+      llvm::StringSet<llvm::BumpPtrAllocator> Result;
+      tidy::ClangTidyCheckFactories Factories;
+      for (tidy::ClangTidyModuleRegistry::entry E :
+           tidy::ClangTidyModuleRegistry::entries())
+        E.instantiate()->addCheckFactories(Factories);
+      for (const auto &Factory : Factories)
+        Result.insert(Factory.getKey());
+      return Result;
+    }();
+
+    return !AllChecks.contains(CheckGlob);
+  }
+
   void appendTidyCheckSpec(std::string &CurSpec,
                            const Located<std::string> &Arg, bool IsPositive) {
-    StringRef Str = *Arg;
+    StringRef Str = StringRef(*Arg).trim();
     // 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;
     }
+    if (isInvalidCheckGlob(Str)) {
+      diag(Warning,
+           llvm::formatv(
+               "Check glob '{0}' doesn't specify any known clang-tidy check",
+               Str)
+               .str(),
+           Arg.Range);
+      return;
+    }
     CurSpec += ',';
     if (!IsPositive)
       CurSpec += '-';


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92874.311094.patch
Type: text/x-patch
Size: 3762 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201211/20f934ff/attachment.bin>