[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 @@ class IncludeCleanerCheck : public ClangTidyCheck {
   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"