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

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 311948.
njames93 marked 2 inline comments as done.
njames93 added a comment.

Address comments


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/TidyProvider.cpp
  clang-tools-extra/clangd/TidyProvider.h
  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("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) {
Index: clang-tools-extra/clangd/TidyProvider.h
===================================================================
--- clang-tools-extra/clangd/TidyProvider.h
+++ 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 @@
 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
 
Index: clang-tools-extra/clangd/TidyProvider.cpp
===================================================================
--- clang-tools-extra/clangd/TidyProvider.cpp
+++ 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 @@
     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
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ 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 @@
 
   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 += '-';

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits