Reland [clangd] More precisely enable clang warnings through ClangTidy options

This reverts commit 26c82f3d1d.

When tests enable 'Checks: *', we may get extra diagnostics.

clang-tools-extra/clangd/ParsedAST.cpp

@@ -233,12 +233,69 @@ private:
   std::vector<syntax::Token> MainFileTokens;
 };
 
+// Filter for clang diagnostics groups enabled by CTOptions.Checks.
+//
+// These are check names like clang-diagnostics-unused.
+// Note that unlike -Wunused, clang-diagnostics-unused does not imply
+// subcategories like clang-diagnostics-unused-function.
+//
+// This is used to determine which diagnostics can be enabled by ExtraArgs in
+// the clang-tidy configuration.
+class TidyDiagnosticGroups {
+  // Whether all diagnostic groups are enabled by default.
+  // True if we've seen clang-diagnostic-*.
+  bool Default = false;
+  // Set of diag::Group whose enablement != Default.
+  // If Default is false, this is foo where we've seen clang-diagnostic-foo.
+  llvm::DenseSet<unsigned> Exceptions;
+
+public:
+  TidyDiagnosticGroups(llvm::StringRef Checks) {
+    constexpr llvm::StringLiteral CDPrefix = "clang-diagnostic-";
+
+    llvm::StringRef Check;
+    while (!Checks.empty()) {
+      std::tie(Check, Checks) = Checks.split(',');
+      if (Check.empty())
+        continue;
+
+      bool Enable = !Check.consume_front("-");
+      bool Glob = Check.consume_back("*");
+      if (Glob) {
+        // Is this clang-diagnostic-*, or *, or so?
+        // (We ignore all other types of globs).
+        if (CDPrefix.startswith(Check)) {
+          Default = Enable;
+          Exceptions.clear();
+        }
+        continue;
+      }
+
+      // In "*,clang-diagnostic-foo", the latter is a no-op.
+      if (Default == Enable)
+        continue;
+      // The only non-glob entries we care about are clang-diagnostic-foo.
+      if (!Check.consume_front(CDPrefix))
+        continue;
+
+      if (auto Group = DiagnosticIDs::getGroupForWarningOption(Check))
+        Exceptions.insert(static_cast<unsigned>(*Group));
+    }
+  }
+
+  bool operator()(diag::Group GroupID) const {
+    return Exceptions.contains(static_cast<unsigned>(GroupID)) ? !Default
+                                                               : Default;
+  }
+};
+
 // Find -W<group> and -Wno-<group> options in ExtraArgs and apply them to Diags.
 //
 // This is used to handle ExtraArgs in clang-tidy configuration.
 // We don't use clang's standard handling of this as we want slightly different
 // behavior (e.g. we want to exclude these from -Wno-error).
 void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
+                         llvm::function_ref<bool(diag::Group)> EnabledGroups,
                          DiagnosticsEngine &Diags) {
   for (llvm::StringRef Group : ExtraArgs) {
     // Only handle args that are of the form -W[no-]<group>.
@@ -259,6 +316,9 @@ void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
       if (Enable) {
         if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
             DiagnosticsEngine::Warning) {
+          auto Group = DiagnosticIDs::getGroupForDiag(ID);
+          if (!Group || !EnabledGroups(*Group))
+            continue;
           Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
           if (Diags.getWarningsAsErrors())
             NeedsWerrorExclusion = true;
@@ -354,18 +414,25 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
     //   - ExtraArgs: ["-Wfoo"] causes clang to produce the warnings
     //   - Checks: "clang-diagnostic-foo" prevents clang-tidy filtering them out
     //
-    // We treat these as clang warnings, so the Checks part is not relevant.
-    // We must enable the warnings specified in ExtraArgs.
+    // In clang-tidy, diagnostics are emitted if they pass both checks.
+    // When groups contain subgroups, -Wparent includes the child, but
+    // clang-diagnostic-parent does not.
     //
-    // We *don't* want to change the compile command directly. this can have
+    // We *don't* want to change the compile command directly. This can have
     // too many unexpected effects: breaking the command, interactions with
     // -- and -Werror, etc. Besides, we've already parsed the command.
     // Instead we parse the -W<group> flags and handle them directly.
+    //
+    // Similarly, we don't want to use Checks to filter clang diagnostics after
+    // they are generated, as this spreads clang-tidy emulation everywhere.
+    // Instead, we just use these to filter which extra diagnostics we enable.
     auto &Diags = Clang->getDiagnostics();
+    TidyDiagnosticGroups TidyGroups(ClangTidyOpts.Checks ? *ClangTidyOpts.Checks
+                                                         : llvm::StringRef());
     if (ClangTidyOpts.ExtraArgsBefore)
-      applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, Diags);
+      applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, TidyGroups, Diags);
     if (ClangTidyOpts.ExtraArgs)
-      applyWarningOptions(*ClangTidyOpts.ExtraArgs, Diags);
+      applyWarningOptions(*ClangTidyOpts.ExtraArgs, TidyGroups, Diags);
   } else {
     // Skips some analysis.
     Clang->getDiagnosticOpts().IgnoreWarnings = true;

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

@@ -517,13 +517,16 @@ TEST(DiagnosticTest, ClangTidyWarningAsError) {
           diagSeverity(DiagnosticsEngine::Error)))));
 }
 
-TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs) {
-  return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions &Opts,
-                                            llvm::StringRef) {
+TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs,
+                          llvm::StringRef Checks) {
+  return [ExtraArgs = std::move(ExtraArgs), Checks = Checks.str()](
+             tidy::ClangTidyOptions &Opts, llvm::StringRef) {
     if (!Opts.ExtraArgs)
       Opts.ExtraArgs.emplace();
     for (llvm::StringRef Arg : ExtraArgs)
       Opts.ExtraArgs->emplace_back(Arg);
+    if (!Checks.empty())
+      Opts.Checks = Checks;
   };
 }
 
@@ -541,53 +544,78 @@ TEST(DiagnosticTest, ClangTidyEnablesClangWarning) {
   // Check the -Wunused warning isn't initially on.
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 
-  // We enable warnings based on clang-tidy extra args.
-  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  // We enable warnings based on clang-tidy extra args, if the matching
+  // clang-diagnostic- is there.
+  TU.ClangTidyProvider =
+      addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
   EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
 
-  // But we don't respect other args.
-  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"});
+  // clang-diagnostic-* is acceptable
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-*");
+  EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
+  // And plain * (may turn on other checks too).
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "*");
+  EXPECT_THAT(*TU.build().getDiagnostics(), Contains(UnusedFooWarning));
+  // And we can explicitly exclude a category too.
+  TU.ClangTidyProvider = addClangArgs(
+      {"-Wunused"}, "clang-diagnostic-*,-clang-diagnostic-unused-function");
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+
+  // Without the exact check specified, the warnings are not enabled.
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-unused");
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+
+  // We don't respect other args.
+  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"},
+                                      "clang-diagnostic-unused-function");
   EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning))
       << "Not unused function 'bar'!";
 
   // -Werror doesn't apply to warnings enabled by clang-tidy extra args.
   TU.ExtraArgs = {"-Werror"};
-  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  TU.ClangTidyProvider =
+      addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
   EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(diagSeverity(DiagnosticsEngine::Warning)));
 
   // But clang-tidy extra args won't *downgrade* errors to warnings either.
   TU.ExtraArgs = {"-Wunused", "-Werror"};
-  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  TU.ClangTidyProvider =
+      addClangArgs({"-Wunused"}, "clang-diagnostic-unused-function");
   EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(diagSeverity(DiagnosticsEngine::Error)));
 
   // FIXME: we're erroneously downgrading the whole group, this should be Error.
   TU.ExtraArgs = {"-Wunused-function", "-Werror"};
-  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  TU.ClangTidyProvider =
+      addClangArgs({"-Wunused"}, "clang-diagnostic-unused-label");
   EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(diagSeverity(DiagnosticsEngine::Warning)));
 
   // This looks silly, but it's the typical result if a warning is enabled by a
   // high-level .clang-tidy file and disabled by a low-level one.
   TU.ExtraArgs = {};
-  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"});
+  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"},
+                                      "clang-diagnostic-unused-function");
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 
   // Overriding only works in the proper order.
-  TU.ClangTidyProvider = addClangArgs({"-Wno-unused", "-Wunused"});
+  TU.ClangTidyProvider =
+      addClangArgs({"-Wunused"}, {"clang-diagnostic-unused-function"});
   EXPECT_THAT(*TU.build().getDiagnostics(), SizeIs(1));
 
   // More specific vs less-specific: match clang behavior
-  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"});
+  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"},
+                                      {"clang-diagnostic-unused-function"});
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
-  TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"});
+  TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"},
+                                      {"clang-diagnostic-unused-function"});
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 
-  // We do allow clang-tidy config to disable warnings from the compile command.
-  // It's unclear this is ideal, but it's hard to avoid.
+  // We do allow clang-tidy config to disable warnings from the compile
+  // command. It's unclear this is ideal, but it's hard to avoid.
   TU.ExtraArgs = {"-Wunused"};
-  TU.ClangTidyProvider = addClangArgs({"-Wno-unused"});
+  TU.ClangTidyProvider = addClangArgs({"-Wno-unused"}, {});
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }
 

clang/include/clang/Basic/DiagnosticIDs.h

@@ -231,6 +231,14 @@ public:
   /// "deprecated-declarations".
   static StringRef getWarningOptionForGroup(diag::Group);
 
+  /// Given a group ID, returns the flag that toggles the group.
+  /// For example, for "deprecated-declarations", returns
+  /// Group::DeprecatedDeclarations.
+  static llvm::Optional<diag::Group> getGroupForWarningOption(StringRef);
+
+  /// Return the lowest-level group that contains the specified diagnostic.
+  static llvm::Optional<diag::Group> getGroupForDiag(unsigned DiagID);
+
   /// Return the lowest-level warning option that enables the specified
   /// diagnostic.
   ///

clang/lib/Basic/DiagnosticIDs.cpp

@@ -628,13 +628,27 @@ StringRef DiagnosticIDs::getWarningOptionForGroup(diag::Group Group) {
   return OptionTable[static_cast<int>(Group)].getName();
 }
 
+llvm::Optional<diag::Group>
+DiagnosticIDs::getGroupForWarningOption(StringRef Name) {
+  const auto *Found = llvm::partition_point(
+      OptionTable, [=](const WarningOption &O) { return O.getName() < Name; });
+  if (Found == std::end(OptionTable) || Found->getName() != Name)
+    return llvm::None;
+  return static_cast<diag::Group>(Found - OptionTable);
+}
+
+llvm::Optional<diag::Group> DiagnosticIDs::getGroupForDiag(unsigned DiagID) {
+  if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
+    return static_cast<diag::Group>(Info->getOptionGroupIndex());
+  return llvm::None;
+}
+
 /// getWarningOptionForDiag - Return the lowest-level warning option that
 /// enables the specified diagnostic.  If there is no -Wfoo flag that controls
 /// the diagnostic, this returns null.
 StringRef DiagnosticIDs::getWarningOptionForDiag(unsigned DiagID) {
-  if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID))
-    return getWarningOptionForGroup(
-        static_cast<diag::Group>(Info->getOptionGroupIndex()));
+  if (auto G = getGroupForDiag(DiagID))
+    return getWarningOptionForGroup(*G);
   return StringRef();
 }
 
@@ -683,12 +697,10 @@ static bool getDiagnosticsInGroup(diag::Flavor Flavor,
 bool
 DiagnosticIDs::getDiagnosticsInGroup(diag::Flavor Flavor, StringRef Group,
                                      SmallVectorImpl<diag::kind> &Diags) const {
-  auto Found = llvm::partition_point(
-      OptionTable, [=](const WarningOption &O) { return O.getName() < Group; });
-  if (Found == std::end(OptionTable) || Found->getName() != Group)
-    return true; // Option not found.
-
-  return ::getDiagnosticsInGroup(Flavor, Found, Diags);
+  if (llvm::Optional<diag::Group> G = getGroupForWarningOption(Group))
+    return ::getDiagnosticsInGroup(
+        Flavor, &OptionTable[static_cast<unsigned>(*G)], Diags);
+  return true;
 }
 
 void DiagnosticIDs::getAllDiagnostics(diag::Flavor Flavor,