[clangd] Validate clang-tidy CheckOptions in clangd config

[njames93]

Summary

Expand D92874 to also ensure all items in CheckOptions correspond to actual options in the clang-tidy checks.

[njames93]

Gonna wait for the infrastructure from D127446 to land first, can reuse most of that implementation.

[njames93]

Reuse implementation from D127446

[sammccall]

FWIW, I think validating the names makes sense but I don't think typo correction pays for itself here. @kadircet?

clang-tools-extra/clangd/TidyProvider.cpp

301

it seems strange that clang-tidy provides this API to query what checks are linked in, but it constructs an expensive object every time rather than just creating a static one once and returning a reference to it. (i.e. the memoization is on the caller side)
Should we fix that API instead?

[kadircet]

I also agree with the typo correction verdict. In theory there'll be two cases:

• typo correction helps, in which case it'll be obvious from the warning itself.
• typo correction doesn't help, because the option doesn't exist at all, we'll be just showing a random option. I don't think how this'll be helpful.

So I don't think this is providing much of a value compared to the extra logic.

clang-tools-extra/clangd/TidyProvider.cpp

301

+1

[njames93]

In D126859#3599340, @kadircet wrote:

I also agree with the typo correction verdict. In theory there'll be two cases:

• typo correction helps, in which case it'll be obvious from the warning itself.
• typo correction doesn't help, because the option doesn't exist at all, we'll be just showing a random option. I don't think how this'll be helpful.

So I don't think this is providing much of a value compared to the extra logic.

The correction only shows near misses, so it wouldn't show completely random options.
However it's only really there to prevent you having to look at the documentation for the check, so I'm happy to remove it.

clang-tools-extra/clangd/TidyProvider.cpp

301

The clang-tidy API is still a WIP. but it doesn't really make sense to memoize it on the tidy side as its s function that is only ever going to be called once in clang-tidy.

[njames93]

Remove typo correction support.

clang-tools-extra/clangd/ConfigCompile.cpp

if (!Checks.empty())
           [Checks = std::move(Checks)](const Params &, Config &C) {
             C.Diagnostics.ClangTidy.Checks.append(
                 Checks,
                 C.Diagnostics.ClangTidy.Checks.empty() ? /*skip comma*/ 1 : 0,
                 std::string::npos);
           });
     if (!F.CheckOptions.empty()) {
       std::vector<std::pair<std::string, std::string>> CheckOptions;
-      for (auto &Opt : F.CheckOptions)
+      CheckOptions.reserve(F.CheckOptions.size());
+      for (auto &Opt : F.CheckOptions) {
+        if (!isRecognisedTidyOption(*Opt.first)) {
+          diag(Warning,
+               llvm::formatv("unknown clang-tidy option '{0}'", *Opt.first)
+                   .str(),
+               Opt.first.Range);
+        }
         CheckOptions.emplace_back(std::move(*Opt.first),
                                   std::move(*Opt.second));
+      }
       Out.Apply.push_back(
           [CheckOptions = std::move(CheckOptions)](const Params &, Config &C) {
             for (auto &StringPair : CheckOptions)
               C.Diagnostics.ClangTidy.CheckOptions.insert_or_assign(
                   StringPair.first, StringPair.second);
           });
     }
   }

clang-tools-extra/clangd/TidyProvider.h

 
 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);
 
+bool isRecognisedTidyOption(llvm::StringRef CheckOption);
+
 } // 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/ClangTidy.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
 #include "Config.h"
 #include "support/FileCache.h"
 #include "support/Logger.h"
 #include "support/Path.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/ManagedStatic.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/SourceMgr.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;
 }
 
+namespace {
+struct Creator {
+  static void *call() {
+    return new tidy::NamesAndOptions(tidy::getAllChecksAndOptions(false));
+  }
+};
+} // namespace
+
+static llvm::ManagedStatic<tidy::NamesAndOptions, Creator> CheckInfo;
+
 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 CheckInfo->Names.contains(Check);
+}
 
-  return AllChecks.contains(Check);
+bool isRecognisedTidyOption(llvm::StringRef CheckOption) {
+  assert(!CheckOption.empty());
+
+  return CheckInfo->Options.contains(CheckOption);
 }
+
 } // namespace clangd
 } // namespace clang

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

EXPECT_EQ(Conf.Diagnostics.ClangTidy.CheckOptions.lookup("StrictMode"),
             "true");
   EXPECT_EQ(Conf.Diagnostics.ClangTidy.CheckOptions.lookup(
                 "example-check.ExampleOption"),
             "0");
 #if CLANGD_TIDY_CHECKS
   EXPECT_EQ(
       Conf.Diagnostics.ClangTidy.Checks,
       "bugprone-use-after-move,llvm-*,-llvm-include-order,-readability-*");
-  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(Diags.Diagnostics,
+              ElementsAre(diagMessage(
+                  "unknown clang-tidy option 'example-check.ExampleOption'")));
 #else // !CLANGD_TIDY_CHECKS
   EXPECT_EQ(Conf.Diagnostics.ClangTidy.Checks, "llvm-*,-readability-*");
   EXPECT_THAT(
       Diags.Diagnostics,
       ElementsAre(
           diagMessage(
               "clang-tidy check 'bugprone-use-after-move' was not found"),
-          diagMessage("clang-tidy check 'llvm-include-order' was not found")));
+          diagMessage("clang-tidy check 'llvm-include-order' was not found"),
+          diagMessage(
+              "unknown clang-tidy option 'example-check.ExampleOption'")));
 #endif
 }
 
 TEST_F(ConfigCompileTests, TidyBadChecks) {
   auto &Tidy = Frag.Diagnostics.ClangTidy;
   Tidy.Add.emplace_back("unknown-check");
   Tidy.Remove.emplace_back("*");
   Tidy.Remove.emplace_back("llvm-includeorder");
+
   EXPECT_TRUE(compileAndApply());
   // Ensure bad checks are stripped from the glob.
   EXPECT_EQ(Conf.Diagnostics.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, TidyBadOptions) {
+  auto &Tidy = Frag.Diagnostics.ClangTidy;
+  Tidy.CheckOptions.emplace_back(
+      std::make_pair(std::string("BadGlobal"), std::string("true")));
+  Tidy.CheckOptions.emplace_back(
+      std::make_pair(std::string("StrictModes"), std::string("true")));
+  Tidy.CheckOptions.emplace_back(std::make_pair(
+      std::string("readability-braces-around-statements.ShortStatementsLines"),
+      std::string("1")));
+  Tidy.CheckOptions.emplace_back(std::make_pair(
+      std::string("readability-braces-around-statements.ShortStatementLines"),
+      std::string("1")));
+  EXPECT_TRUE(compileAndApply());
+#if CLANGD_TIDY_CHECKS
+  EXPECT_THAT(
+      Diags.Diagnostics,
+      ElementsAre(
+          diagMessage("unknown clang-tidy option 'BadGlobal'"),
+          diagMessage("unknown clang-tidy option 'StrictModes'"),
+          diagMessage(
+              "unknown clang-tidy option "
+              "'readability-braces-around-statements.ShortStatementsLines'")));
+#else // !CLANGD_TIDY_CHECKS
+  EXPECT_THAT(
+      Diags.Diagnostics,
+      ElementsAre(
+          diagMessage("unknown clang-tidy option 'BadGlobal'"),
+          diagMessage("unknown clang-tidy option 'StrictModes';"),
+          diagMessage(
+              "unknown clang-tidy option "
+              "'readability-braces-around-statements.ShortStatementsLines'"),
+          diagMessage(
+              "unknown clang-tidy option "
+              "'readability-braces-around-statements.ShortStatementLines'")));
+#endif
+}
+
 TEST_F(ConfigCompileTests, ExternalServerNeedsTrusted) {
   Fragment::IndexBlock::ExternalBlock External;
   External.Server.emplace("xxx");
   Frag.Index.External = std::move(External);
   compileAndApply();
   EXPECT_THAT(
       Diags.Diagnostics,
       ElementsAre(diagMessage(