[clangd] More precisely enable clang warnings through ClangTidy options

[sammccall]

Summary

clang-tidy's behavior is to add the -W flags, and then map all clang diagnostics
to "clang-diagnostic-foo" pseudo-check-names, then use Checks to filter those.

Previous to this patch, we were handling -W flags but not filtering the
diagnostics, assuming both sets of information encoded the same thing.

However this intersection is nontrivial when diagnostic group hierarchy is
involved. e.g. -Wunused + clang-diagnostic-unused-function should not enable
unused label warnings.

This patch more closely emulates clang-tidy's behavior, while not going to
the extreme of generating tidy check names for all clang diagnostics and
filtering them with regexes.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/ParsedAST.cpp

240

s/clang-diagnostics-unused function/clang-diagnostics-unused-function/

254

static constexpr

[kstoimenov]

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

558

This breaks sanitizer buildbot: https://lab.llvm.org/buildbot/#/builders/5/builds/22933.

Could you please fix or revert?

/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:558
Value of: *TU.build().getDiagnostics()
Expected: has 1 element that (Diag at 1:16-1:19 = [unused function 'foo']) and (diag name "-Wunused-function") and (diag source 1) and (diag severity 3)
  Actual: { [1:16-1:19] unused function 'foo', [1:16-1:19] declaration must be declared within the '__llvm_libc' namespace }, which has 2 elements
/b/sanitizer-x86_64-linux-fast/build/llvm-project/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:562
Value of: *TU.build().getDiagnostics()
Expected: is empty
  Actual: { [1:16-1:19] declaration must be declared within the '__llvm_libc' namespace }, whose size is 1

[thakis]

This broke check-clangd everywhere, e.g.:

https://lab.llvm.org/buildbot/#/builders/109/builds/37635
http://45.33.8.238/linux/75019/step_9.txt
http://45.33.8.238/macm1/34369/step_9.txt
http://45.33.8.238/win/57278/step_9.txt

Please run tests before committing, please watch bots after committing, etc.

[sammccall]

Thanks for revert, and apologies for missing the bots.

(I did of course run the tests locally, but in a weird config that didn't trigger the failure)

clang-tools-extra/clangd/ParsedAST.cpp

private:
 
   const std::vector<Inclusion> Includes;
   PPCallbacks *Delegate;
   const SourceManager &SM;
   Preprocessor &PP;
   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>.
     // Other flags are possible but rare and deliberately out of scope.
     llvm::SmallVector<diag::kind> Members;
     if (!Group.consume_front("-W") || Group.empty())
       continue;
     bool Enable = !Group.consume_front("no-");
     if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
             diag::Flavor::WarningOrError, Group, Members))
       continue;
 
     // Upgrade (or downgrade) the severity of each diagnostic in the group.
     // If -Werror is on, newly added warnings will be treated as errors.
     // We don't want this, so keep track of them to fix afterwards.
     bool NeedsWerrorExclusion = false;
     for (diag::kind ID : Members) {
       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;
         }
       } else {
         Diags.setSeverity(ID, diag::Severity::Ignored, SourceLocation());
       }
     }
dlog("ClangTidy configuration for file {0}: {1}", Filename,
          tidy::configurationAsText(ClangTidyOpts));
 
     // If clang-tidy is configured to emit clang warnings, we should too.
     //
     // Such clang-tidy configuration consists of two parts:
     //   - 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;
   }
 
   auto Action = std::make_unique<ClangdFrontendAction>();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {

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

EXPECT_THAT(
       *TU.build().getDiagnostics(),
       ifTidyChecks(UnorderedElementsAre(::testing::AllOf(
           Diag(Main.range(), "result of integer division used in a floating "
                              "point context; possible loss of precision"),
           diagSource(Diag::ClangTidy), diagName("bugprone-integer-division"),
           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;
   };
 }
 
 TEST(DiagnosticTest, ClangTidyEnablesClangWarning) {
   Annotations Main(R"cpp( // error-ok
     static void [[foo]]() {}
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   // This is always emitted as a clang warning, not a clang-tidy diagnostic.
   auto UnusedFooWarning =
       AllOf(Diag(Main.range(), "unused function 'foo'"),
             diagName("-Wunused-function"), diagSource(Diag::Clang),
             diagSeverity(DiagnosticsEngine::Warning));
 
   // 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));
+
+  // clang-diagnostic-* is acceptable
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "clang-diagnostic-*");
   EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
+  // And plain *
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"}, "*");
+  EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
+  // And we can explicitly exclude a category too.
+  TU.ClangTidyProvider =
+      addClangArgs({"-Wunused"}, "*,-clang-diagnostic-unused-function");
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 
-  // But we don't respect other args.
-  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"});
+  // 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());
 }
 
 TEST(DiagnosticTest, LongFixMessages) {
   // We limit the size of printed code.
   Annotations Source(R"cpp(
     int main() {
       // error-ok

clang/include/clang/Basic/DiagnosticIDs.h

public:
   ///
   static bool isBuiltinExtensionDiag(unsigned DiagID, bool &EnabledByDefault);
 
   /// Given a group ID, returns the flag that toggles the group.
   /// For example, for Group::DeprecatedDeclarations, returns
   /// "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.
   ///
   /// If there is no -Wfoo flag that controls the diagnostic, this returns null.
   static StringRef getWarningOptionForDiag(unsigned DiagID);
 
   /// Return the category number that a specified \p DiagID belongs to,
   /// or 0 if no category.

clang/lib/Basic/DiagnosticIDs.cpp

 #include "clang/Basic/DiagnosticGroups.inc"
 #undef DIAG_ENTRY
 };
 
 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();
 }
 
 std::vector<std::string> DiagnosticIDs::getDiagnosticFlags() {
   std::vector<std::string> Res;
   for (size_t I = 1; DiagGroupNames[I] != '\0';) {
     std::string Diag(DiagGroupNames + I + 1, DiagGroupNames[I]);
     I += DiagGroupNames[I] + 1;
NotFound &= getDiagnosticsInGroup(Flavor, &OptionTable[(short)*SubGroups],
                                       Diags);
 
   return NotFound;
 }
 
 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,
                                       std::vector<diag::kind> &Diags) {
   for (unsigned i = 0; i != StaticDiagInfoSize; ++i)
     if (StaticDiagInfo[i].getFlavor() == Flavor)
       Diags.push_back(StaticDiagInfo[i].DiagID);
 }