[clangd] Parse Diagnostics block, and nest ClangTidy block under it.

[sammccall]

Summary

(ClangTidy configuration block hasn't been in any release, so we should be OK
to move it around like this)

[njames93]

LG, Obviously we need to get this in before the branch or get it cherrypicked. Also documentation will need updating on https://github.com/llvm/clangd-www. Do you want to sort that out?

[sammccall]

In D95362#2519976, @njames93 wrote:

LG, Obviously we need to get this in before the branch or get it cherrypicked. Also documentation will need updating on https://github.com/llvm/clangd-www. Do you want to sort that out?

Yeah, I plan to land this today and use the slightly-less-frantic time while the branch is stablizing to update the docs (config and other).
(And to catch up on my review backlog, sorry about that!)

clang-tools-extra/clangd/Config.h

struct {
     BackgroundPolicy Background = BackgroundPolicy::Build;
     llvm::Optional<ExternalIndexSpec> External;
   } Index;
 
   /// Controls warnings and errors when parsing code.
   struct {
     bool SuppressAll = false;
     llvm::StringSet<> Suppress;
+
+    /// Configures what clang-tidy checks to run and options to use with them.
+    struct {
+      // A comma-seperated list of globs specify which clang-tidy checks to run.
+      std::string Checks;
+      llvm::StringMap<std::string> CheckOptions;
+    } ClangTidy;
   } Diagnostics;
 
   /// Style of the codebase.
   struct {
     // Namespaces that should always be fully qualified, meaning no "using"
     // declarations, always spell out the whole name (with or without leading
     // ::). All nested namespaces are affected as well.
     std::vector<std::string> FullyQualifiedNamespaces;
   } Style;
-
-  /// Configures what clang-tidy checks to run and options to use with them.
-  struct {
-    // A comma-seperated list of globs to specify which clang-tidy checks to
-    // run.
-    std::string Checks;
-    llvm::StringMap<std::string> CheckOptions;
-  } ClangTidy;
 };
 
 } // namespace clangd
 } // namespace clang
 
 namespace llvm {
 template <> struct DenseMapInfo<clang::clangd::Config::ExternalIndexSpec> {
   using ExternalIndexSpec = clang::clangd::Config::ExternalIndexSpec;

clang-tools-extra/clangd/ConfigCompile.cpp

if (!F.Source.Directory.empty()) {
       FragmentDirectory = llvm::sys::path::convert_to_slash(F.Source.Directory);
       if (FragmentDirectory.back() != '/')
         FragmentDirectory += '/';
     }
     compile(std::move(F.If));
     compile(std::move(F.CompileFlags));
     compile(std::move(F.Index));
     compile(std::move(F.Diagnostics));
-    compile(std::move(F.ClangTidy));
   }
 
   void compile(Fragment::IfBlock &&F) {
     if (F.HasUnrecognizedCondition)
       Out.Conditions.push_back([&](const Params &) { return false; });
 
     auto PathMatch = std::make_unique<std::vector<llvm::Regex>>();
     for (auto &Entry : F.PathMatch) {
void compile(Fragment::DiagnosticsBlock &&F) {
     }
     if (!Normalized.empty())
       Out.Apply.push_back([Normalized](const Params &, Config &C) {
         if (C.Diagnostics.SuppressAll)
           return;
         for (llvm::StringRef N : Normalized)
           C.Diagnostics.Suppress.insert(N);
       });
+
+    compile(std::move(F.ClangTidy));
   }
 
   void compile(Fragment::StyleBlock &&F) {
     if (!F.FullyQualifiedNamespaces.empty()) {
       std::vector<std::string> FullyQualifiedNamespaces;
       for (auto &N : F.FullyQualifiedNamespaces) {
         // Normalize the data by dropping both leading and trailing ::
         StringRef Namespace(*N);
if (!Str.contains('*') && !isRegisteredTidyCheck(Str)) {
       return;
     }
     CurSpec += ',';
     if (!IsPositive)
       CurSpec += '-';
     CurSpec += Str;
   }
 
-  void compile(Fragment::ClangTidyBlock &&F) {
+  void compile(Fragment::DiagnosticsBlock::ClangTidyBlock &&F) {
     std::string Checks;
     for (auto &CheckGlob : F.Add)
       appendTidyCheckSpec(Checks, CheckGlob, true);
 
     for (auto &CheckGlob : F.Remove)
       appendTidyCheckSpec(Checks, CheckGlob, false);
 
     if (!Checks.empty())
       Out.Apply.push_back(
           [Checks = std::move(Checks)](const Params &, Config &C) {
-            C.ClangTidy.Checks.append(
-                Checks, C.ClangTidy.Checks.empty() ? /*skip comma*/ 1 : 0,
+            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.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.ClangTidy.CheckOptions.insert_or_assign(StringPair.first,
-                                                        StringPair.second);
+              C.Diagnostics.ClangTidy.CheckOptions.insert_or_assign(
+                  StringPair.first, StringPair.second);
           });
     }
   }
 
   constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error;
   constexpr static llvm::SourceMgr::DiagKind Warning =
       llvm::SourceMgr::DK_Warning;
   void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,

clang-tools-extra/clangd/ConfigFragment.h

struct DiagnosticsBlock {
     /// - clang internal diagnostic codes (e.g. err_unknown_type)
     /// - warning categories (e.g. unused-result)
     /// - clang-tidy check names (e.g. bugprone-narrowing-conversions)
     ///
     /// This is a simple filter. Diagnostics can be controlled in other ways
     /// (e.g. by disabling a clang-tidy check, or the -Wunused compile flag).
     /// This often has other advantages, such as skipping some analysis.
     std::vector<Located<std::string>> Suppress;
-  };
-  DiagnosticsBlock Diagnostics;
-
-  // Describes the style of the codebase, beyond formatting.
-  struct StyleBlock {
-    // Namespaces that should always be fully qualified, meaning no "using"
-    // declarations, always spell out the whole name (with or without leading
-    // ::). All nested namespaces are affected as well.
-    // Affects availability of the AddUsing tweak.
-    std::vector<Located<std::string>> FullyQualifiedNamespaces;
-  };
-  StyleBlock Style;
 
     /// Controls how clang-tidy will run over the code base.
     ///
     /// The settings are merged with any settings found in .clang-tidy
     /// configiration files with these ones taking precedence.
-  // FIXME: move this to Diagnostics.Tidy.
     struct ClangTidyBlock {
       std::vector<Located<std::string>> Add;
       /// List of checks to disable.
       /// Takes precedence over Add. To enable all llvm checks except include
       /// order:
       ///   Add: llvm-*
       ///   Remove: llvm-include-onder
       std::vector<Located<std::string>> Remove;
 
       /// A Key-Value pair list of options to pass to clang-tidy checks
-    /// These take precedence over options specified in clang-tidy configuration
-    /// files. Example:
+      /// These take precedence over options specified in clang-tidy
+      /// configuration files. Example:
       ///   CheckOptions:
       ///     readability-braces-around-statements.ShortStatementLines: 2
       std::vector<std::pair<Located<std::string>, Located<std::string>>>
           CheckOptions;
     };
     ClangTidyBlock ClangTidy;
   };
+  DiagnosticsBlock Diagnostics;
+
+  // Describes the style of the codebase, beyond formatting.
+  struct StyleBlock {
+    // Namespaces that should always be fully qualified, meaning no "using"
+    // declarations, always spell out the whole name (with or without leading
+    // ::). All nested namespaces are affected as well.
+    // Affects availability of the AddUsing tweak.
+    std::vector<Located<std::string>> FullyQualifiedNamespaces;
+  };
+  StyleBlock Style;
+};
 
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/ConfigYAML.cpp

public:
   // Tries to parse N into F, returning false if it failed and we couldn't
   // meaningfully recover (YAML syntax error, or hard semantic error).
   bool parse(Fragment &F, Node &N) {
     DictParser Dict("Config", this);
     Dict.handle("If", [&](Node &N) { parse(F.If, N); });
     Dict.handle("CompileFlags", [&](Node &N) { parse(F.CompileFlags, N); });
     Dict.handle("Index", [&](Node &N) { parse(F.Index, N); });
     Dict.handle("Style", [&](Node &N) { parse(F.Style, N); });
-    Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
+    Dict.handle("Diagnostics", [&](Node &N) { parse(F.Diagnostics, N); });
     Dict.parse(N);
     return !(N.failed() || HadError);
   }
 
 private:
   void parse(Fragment::IfBlock &F, Node &N) {
     DictParser Dict("If", this);
     Dict.unrecognized([&](Located<std::string>, Node &) {
void parse(Fragment::StyleBlock &F, Node &N) {
     DictParser Dict("Style", this);
     Dict.handle("FullyQualifiedNamespaces", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.FullyQualifiedNamespaces = std::move(*Values);
     });
     Dict.parse(N);
   }
 
-  void parse(Fragment::ClangTidyBlock &F, Node &N) {
+  void parse(Fragment::DiagnosticsBlock &F, Node &N) {
+    DictParser Dict("Diagnostics", this);
+    Dict.handle("Suppress", [&](Node &N) {
+      if (auto Values = scalarValues(N))
+        F.Suppress = std::move(*Values);
+    });
+    Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
+    Dict.parse(N);
+  }
+
+  void parse(Fragment::DiagnosticsBlock::ClangTidyBlock &F, Node &N) {
     DictParser Dict("ClangTidy", this);
     Dict.handle("Add", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Add = std::move(*Values);
     });
     Dict.handle("Remove", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Remove = std::move(*Values);

clang-tools-extra/clangd/TidyProvider.cpp

return [DisableList(std::move(DisableGlob))](tidy::ClangTidyOptions &Opts,
                                                llvm::StringRef) {
     if (Opts.Checks && !Opts.Checks->empty())
       Opts.Checks->append(DisableList);
   };
 }
 
 TidyProviderRef provideClangdConfig() {
   return [](tidy::ClangTidyOptions &Opts, llvm::StringRef) {
-    const auto &CurTidyConfig = Config::current().ClangTidy;
+    const auto &CurTidyConfig = Config::current().Diagnostics.ClangTidy;
     if (!CurTidyConfig.Checks.empty())
       mergeCheckList(Opts.Checks, CurTidyConfig.Checks);
 
     for (const auto &CheckOption : CurTidyConfig.CheckOptions)
       Opts.CheckOptions.insert_or_assign(CheckOption.getKey(),
                                          tidy::ClangTidyOptions::ClangTidyValue(
                                              CheckOption.getValue(), 10000U));
   };

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

TEST_F(ConfigCompileTests, DiagnosticSuppression) {
 
   Frag.Diagnostics.Suppress.emplace_back("*");
   EXPECT_TRUE(compileAndApply());
   EXPECT_TRUE(Conf.Diagnostics.SuppressAll);
   EXPECT_THAT(Conf.Diagnostics.Suppress, IsEmpty());
 }
 
 TEST_F(ConfigCompileTests, Tidy) {
-  Frag.ClangTidy.Add.emplace_back("bugprone-use-after-move");
-  Frag.ClangTidy.Add.emplace_back("llvm-*");
-  Frag.ClangTidy.Remove.emplace_back("llvm-include-order");
-  Frag.ClangTidy.Remove.emplace_back("readability-*");
-  Frag.ClangTidy.CheckOptions.emplace_back(
+  auto &Tidy = Frag.Diagnostics.ClangTidy;
+  Tidy.Add.emplace_back("bugprone-use-after-move");
+  Tidy.Add.emplace_back("llvm-*");
+  Tidy.Remove.emplace_back("llvm-include-order");
+  Tidy.Remove.emplace_back("readability-*");
+  Tidy.CheckOptions.emplace_back(
       std::make_pair(std::string("StrictMode"), std::string("true")));
-  Frag.ClangTidy.CheckOptions.emplace_back(std::make_pair(
+  Tidy.CheckOptions.emplace_back(std::make_pair(
       std::string("example-check.ExampleOption"), std::string("0")));
   EXPECT_TRUE(compileAndApply());
   EXPECT_EQ(
-      Conf.ClangTidy.Checks,
+      Conf.Diagnostics.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"),
+  EXPECT_EQ(Conf.Diagnostics.ClangTidy.CheckOptions.size(), 2U);
+  EXPECT_EQ(Conf.Diagnostics.ClangTidy.CheckOptions.lookup("StrictMode"),
+            "true");
+  EXPECT_EQ(Conf.Diagnostics.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");
+  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.ClangTidy.Checks, "-*");
+  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))));

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

 CompileFlags:
   Add: |
     b
     az
 ---
 Index:
   Background: Skip
 ---
-ClangTidy: 
-  CheckOptions: 
+Diagnostics:
+  ClangTidy:
+    CheckOptions:
       IgnoreMacros: true
       example-check.ExampleOption: 0
   )yaml";
   auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
   EXPECT_THAT(Diags.Files, ElementsAre("config.yaml"));
   ASSERT_EQ(Results.size(), 4u);
   EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition);
   EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc")));
   EXPECT_THAT(Results[0].CompileFlags.Add, ElementsAre(Val("foo"), Val("bar")));
 
   EXPECT_THAT(Results[1].CompileFlags.Add, ElementsAre(Val("b\naz\n")));
 
   ASSERT_TRUE(Results[2].Index.Background);
   EXPECT_EQ("Skip", *Results[2].Index.Background.getValue());
-  EXPECT_THAT(Results[3].ClangTidy.CheckOptions,
+  EXPECT_THAT(Results[3].Diagnostics.ClangTidy.CheckOptions,
               ElementsAre(PairVal("IgnoreMacros", "true"),
                           PairVal("example-check.ExampleOption", "0")));
 }
 
 TEST(ParseYAML, Locations) {
   CapturedDiags Diags;
   Annotations YAML(R"yaml(
 If: