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

[kadircet]

Summary

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

[VitaNuo]

Thanks.

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

48

Nit: let's name this similar or same to the option, i.e., const bool DeduplicateFindings. It might be easier to digest.

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

150

Nit: check that these are actually different findings (i.e., stemming from different symbol refs).

[kadircet]

Address comments

[kadircet]

• Rebase

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

195

invalid release note should be something like:

Improved :doc:`misc-include-cleaner <clang-tidy/checks/misc/include-cleaner>` check by adding option `DeduplicateFindings` to output one finding per symbol occurence.

Also this list is sorted per check, so it should be before modernize-loop-convert.

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

40

true -> true

[kadircet]

hi @PiotrZSL, addressed those in 724b40a1379f7c116473a02a3cec4d341c475b46

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

public:
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
 
 private:
   include_cleaner::RecordedPP RecordedPreprocessor;
   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);
 };
 
 } // namespace clang::tidy::misc
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INCLUDECLEANER_H

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

struct MissingIncludeInfo {
   include_cleaner::Header Missing;
 };
 } // namespace
 
 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;
     std::string HeaderSuffix{Header.str()};
     if (!Header.ends_with("$"))
       HeaderSuffix += "$";
     IgnoreHeadersRegex.emplace_back(HeaderSuffix);
   }
 }
 
 void IncludeCleanerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreHeaders",
                 utils::options::serializeStringList(IgnoreHeaders));
+  Options.store(Opts, "DeduplicateFindings", DeduplicateFindings);
 }
 
 bool IncludeCleanerCheck::isLanguageVersionSupported(
     const LangOptions &LangOpts) const {
   return !LangOpts.ObjC;
 }
 
 void IncludeCleanerCheck::registerMatchers(MatchFinder *Finder) {
void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
   std::vector<MissingIncludeInfo> Missing;
   llvm::SmallVector<Decl *> MainFileDecls;
   for (Decl *D : Result.Nodes.getNodeAs<TranslationUnitDecl>("top")->decls()) {
     if (!SM->isWrittenInMainFile(SM->getExpansionLoc(D->getLocation())))
       continue;
     // 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 &&
                    H.physical() == MainFile)
                  Satisfied = true;
 
                for (const include_cleaner::Include *I :
                     RecordedPreprocessor.Includes.match(H)) {

clang-tools-extra/docs/ReleaseNotes.rst

 - Improved :doc:`performanc-noexcept-swap
   <clang-tidy/checks/performance/noexcept-swap>` check to enforce a stricter
   match with the swap function signature, eliminating false-positives.
 
 - Improved :doc:`readability-identifier-naming
   <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
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

 -------
 
 .. option:: IgnoreHeaders
 
    A semicolon-separated list of regexes to disable insertion/removal of header
    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

 #include "ClangTidyOptions.h"
 #include "ClangTidyTest.h"
 #include "misc/IncludeCleanerCheck.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #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>
 
 #include <optional>
 #include <vector>
 
 using namespace clang::tidy::misc;
 
EXPECT_EQ(PostCode,
                               #include "baz.h"
                               int bar();
                            )"},
                  {"baz.h", R"(#pragma once
                               int 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"
 
 int BarResult = bar();
 int BazResult = baz();
 int QuxResult = qux();
 )";