[clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol

We received some user feedback around this being disruptful rather than
useful in certain workflows so add an option to control the output behaviour.

Differential Revision: https://reviews.llvm.org/D157390

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp

@@ -55,7 +55,9 @@ IncludeCleanerCheck::IncludeCleanerCheck(StringRef Name,
                                          ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IgnoreHeaders(utils::options::parseStringList(
-          Options.getLocalOrGlobal("IgnoreHeaders", ""))) {
+          Options.getLocalOrGlobal("IgnoreHeaders", ""))),
+      DeduplicateFindings(
+          Options.getLocalOrGlobal("DeduplicateFindings", true)) {
   for (const auto &Header : IgnoreHeaders) {
     if (!llvm::Regex{Header}.isValid())
       configurationDiag("Invalid ignore headers regex '%0'") << Header;
@@ -69,6 +71,7 @@ IncludeCleanerCheck::IncludeCleanerCheck(StringRef Name,
 void IncludeCleanerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreHeaders",
                 utils::options::serializeStringList(IgnoreHeaders));
+  Options.store(Opts, "DeduplicateFindings", DeduplicateFindings);
 }
 
 bool IncludeCleanerCheck::isLanguageVersionSupported(
@@ -114,12 +117,26 @@ void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
     // FIXME: Filter out implicit template specializations.
     MainFileDecls.push_back(D);
   }
+  llvm::DenseSet<include_cleaner::Symbol> SeenSymbols;
   // FIXME: Find a way to have less code duplication between include-cleaner
   // analysis implementation and the below code.
   walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
            *SM,
            [&](const include_cleaner::SymbolReference &Ref,
                llvm::ArrayRef<include_cleaner::Header> Providers) {
+             // Process each symbol once to reduce noise in the findings.
+             // Tidy checks are used in two different workflows:
+             // - Ones that show all the findings for a given file. For such
+             // workflows there is not much point in showing all the occurences,
+             // as one is enough to indicate the issue.
+             // - Ones that show only the findings on changed pieces. For such
+             // workflows it's useful to show findings on every reference of a
+             // symbol as otherwise tools might give incosistent results
+             // depending on the parts of the file being edited. But it should
+             // still help surface findings for "new violations" (i.e.
+             // dependency did not exist in the code at all before).
+             if (DeduplicateFindings && !SeenSymbols.insert(Ref.Target).second)
+               return;
              bool Satisfied = false;
              for (const include_cleaner::Header &H : Providers) {
                if (H.kind() == include_cleaner::Header::Physical &&

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h

@@ -44,6 +44,8 @@ private:
   include_cleaner::PragmaIncludes RecordedPI;
   HeaderSearch *HS;
   std::vector<StringRef> IgnoreHeaders;
+  // Whether emit only one finding per usage of a symbol.
+  const bool DeduplicateFindings;
   llvm::SmallVector<llvm::Regex> IgnoreHeadersRegex;
   bool shouldIgnore(const include_cleaner::Header &H);
 };

clang-tools-extra/docs/ReleaseNotes.rst

@@ -192,6 +192,9 @@ Changes in existing checks
   <clang-tidy/checks/readability/identifier-naming>` check to emit proper
   warnings when a type forward declaration precedes its definition.
 
+- Misc-include-cleaner check has option `DeduplicateFindings` to output one
+  finding per occurence of a symbol.
+
 Removed checks
 ^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst

@@ -33,3 +33,8 @@ Options
    files that match this regex as a suffix.  E.g., `foo/.*` disables
    insertion/removal for all headers under the directory `foo`. By default, no 
    headers will be ignored.
+
+.. option:: DeduplicateFindings
+
+   A boolean that controls whether the check should deduplicate findings for the
+   same symbol. Defaults to true.

clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp

@@ -15,6 +15,8 @@
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include "llvm/Testing/Annotations/Annotations.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <initializer_list>
 
@@ -108,6 +110,50 @@ int BazResult = baz();
                            )"}}));
 }
 
+TEST(IncludeCleanerCheckTest, DedupsMissingIncludes) {
+  llvm::Annotations Code(R"(
+#include "baz.h" // IWYU pragma: keep
+
+int BarResult1 = $diag1^bar();
+int BarResult2 = $diag2^bar();)");
+
+  {
+    std::vector<ClangTidyError> Errors;
+    runCheckOnCode<IncludeCleanerCheck>(Code.code(), &Errors, "file.cpp",
+                                        std::nullopt, ClangTidyOptions(),
+                                        {{"baz.h", R"(#pragma once
+                              #include "bar.h"
+                           )"},
+                                         {"bar.h", R"(#pragma once
+                              int bar();
+                           )"}});
+    ASSERT_THAT(Errors.size(), testing::Eq(1U));
+    EXPECT_EQ(Errors.front().Message.Message,
+              "no header providing \"bar\" is directly included");
+    EXPECT_EQ(Errors.front().Message.FileOffset, Code.point("diag1"));
+  }
+  {
+    std::vector<ClangTidyError> Errors;
+    ClangTidyOptions Opts;
+    Opts.CheckOptions.insert({"DeduplicateFindings", "false"});
+    runCheckOnCode<IncludeCleanerCheck>(Code.code(), &Errors, "file.cpp",
+                                        std::nullopt, Opts,
+                                        {{"baz.h", R"(#pragma once
+                              #include "bar.h"
+                           )"},
+                                         {"bar.h", R"(#pragma once
+                              int bar();
+                           )"}});
+    ASSERT_THAT(Errors.size(), testing::Eq(2U));
+    EXPECT_EQ(Errors.front().Message.Message,
+              "no header providing \"bar\" is directly included");
+    EXPECT_EQ(Errors.front().Message.FileOffset, Code.point("diag1"));
+    EXPECT_EQ(Errors.back().Message.Message,
+              "no header providing \"bar\" is directly included");
+    EXPECT_EQ(Errors.back().Message.FileOffset, Code.point("diag2"));
+  }
+}
+
 TEST(IncludeCleanerCheckTest, SuppressMissingIncludes) {
   const char *PreCode = R"(
 #include "bar.h"