[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

359

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")

353

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.

363

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.

51

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

56

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.

56

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/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 "CompileCommands.h"
 #include "Config.h"
 #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"
 #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/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()) {
             C.Style.FullyQualifiedNamespaces.begin(),
             FullyQualifiedNamespaces.begin(), FullyQualifiedNamespaces.end());
       });
     }
   }
 
   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 += '-';
     CurSpec += Str;
   }
 
   void compile(Fragment::ClangTidyBlock &&F) {
     std::string Checks;

clang-tools-extra/clangd/TidyProvider.h

 //===--- TidyProvider.h - create options for running clang-tidy------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H
 
 #include "../clang-tidy/ClangTidyOptions.h"
 #include "support/ThreadsafeFS.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
 
 namespace clang {
 namespace clangd {
 
 /// A factory to modify a \ref tidy::ClangTidyOptions.
 using TidyProvider =
     llvm::unique_function<void(tidy::ClangTidyOptions &,
                                /*Filename=*/llvm::StringRef) const>;

 TidyProvider provideClangTidyFiles(ThreadsafeFS &);
 
 // Provider that uses clangd configuration files.
 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
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H

clang-tools-extra/clangd/TidyProvider.cpp

 //===--- TidyProvider.cpp - create options for running clang-tidy----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "TidyProvider.h"
+#include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "Config.h"
 #include "support/FileCache.h"
 #include "support/Logger.h"
 #include "support/ThreadsafeFS.h"
 #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"
 #include <memory>
 
 namespace clang {
 namespace clangd {
 namespace {

 tidy::ClangTidyOptions getTidyOptionsForFile(TidyProviderRef Provider,
                                              llvm::StringRef Filename) {
   tidy::ClangTidyOptions Opts = tidy::ClangTidyOptions::getDefaults();
   Opts.Checks->clear();
   if (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

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.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) {
   Fragment::IndexBlock::ExternalBlock External;
   External.File.emplace("");
   External.Server.emplace("");
   Frag.Index.External = std::move(External);
   compileAndApply();