[clang-tools-extra] dfac97d - [clangd] Validate clang-tidy Checks in clangd config.

[Nathan James via cfe-commits]

Author: Nathan James
Date: 2020-12-15T21:10:57Z
New Revision: dfac97d557690dd4b1f2fab798680234b238d11f

URL: https://github.com/llvm/llvm-project/commit/dfac97d557690dd4b1f2fab798680234b238d11f
DIFF: https://github.com/llvm/llvm-project/commit/dfac97d557690dd4b1f2fab798680234b238d11f.diff

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

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.

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D92874

Added: 
    

Modified: 
    clang-tools-extra/clangd/ConfigCompile.cpp
    clang-tools-extra/clangd/TidyProvider.cpp
    clang-tools-extra/clangd/TidyProvider.h
    clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index b1189e286826..2040ea4649fe 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -28,6 +28,7 @@
 #include "ConfigFragment.h"
 #include "ConfigProvider.h"
 #include "Features.inc"
+#include "TidyProvider.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "llvm/ADT/None.h"
@@ -349,13 +350,19 @@ struct FragmentCompiler {
 
   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 (!Str.contains('*') && !isRegisteredTidyCheck(Str)) {
+      diag(Warning,
+           llvm::formatv("clang-tidy check '{0}' was not found", Str).str(),
+           Arg.Range);
+      return;
+    }
     CurSpec += ',';
     if (!IsPositive)
       CurSpec += '-';

diff  --git a/clang-tools-extra/clangd/TidyProvider.cpp b/clang-tools-extra/clangd/TidyProvider.cpp
index 730a402b5df1..c6ee09ae16d2 100644
--- a/clang-tools-extra/clangd/TidyProvider.cpp
+++ b/clang-tools-extra/clangd/TidyProvider.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "TidyProvider.h"
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "Config.h"
 #include "support/FileCache.h"
 #include "support/Logger.h"
@@ -14,6 +15,8 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -266,5 +269,25 @@ tidy::ClangTidyOptions getTidyOptionsForFile(TidyProviderRef Provider,
     Provider(Opts, Filename);
   return Opts;
 }
+
+bool isRegisteredTidyCheck(llvm::StringRef Check) {
+  assert(!Check.empty());
+  assert(!Check.contains('*') && !Check.contains(',') &&
+         "isRegisteredCheck doesn't support globs");
+  assert(Check.ltrim().front() != '-');
+
+  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(Check);
+}
 } // namespace clangd
 } // namespace clang

diff  --git a/clang-tools-extra/clangd/TidyProvider.h b/clang-tools-extra/clangd/TidyProvider.h
index f3f679c3f0d5..b295a9b05eff 100644
--- a/clang-tools-extra/clangd/TidyProvider.h
+++ b/clang-tools-extra/clangd/TidyProvider.h
@@ -13,6 +13,7 @@
 #include "support/ThreadsafeFS.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
 
 namespace clang {
 namespace clangd {
@@ -56,6 +57,10 @@ TidyProviderRef provideClangdConfig();
 tidy::ClangTidyOptions getTidyOptionsForFile(TidyProviderRef Provider,
                                              llvm::StringRef Filename);
 
+/// Returns if \p Check is a registered clang-tidy check
+/// \pre \p must not be empty, must not contain '*' or ',' or start with '-'.
+bool isRegisteredTidyCheck(llvm::StringRef Check);
+
 } // namespace clangd
 } // namespace clang
 

diff  --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index a2423094b17a..578f69821e4d 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -200,6 +200,24 @@ TEST_F(ConfigCompileTests, Tidy) {
   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("clang-tidy check 'unknown-check' was not found"),
+                DiagKind(llvm::SourceMgr::DK_Warning)),
+          AllOf(
+              DiagMessage("clang-tidy check 'llvm-includeorder' was not found"),
+              DiagKind(llvm::SourceMgr::DK_Warning))));
 }
 
 TEST_F(ConfigCompileTests, ExternalBlockWarnOnMultipleSource) {