diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp index c5cb18978b649..f47609b19badf 100644 --- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp +++ b/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 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 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 && diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h index d5f75f2b1c7fa..9641c7fd9dfcf 100644 --- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h +++ b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h @@ -44,6 +44,8 @@ class IncludeCleanerCheck : public ClangTidyCheck { include_cleaner::PragmaIncludes RecordedPI; HeaderSearch *HS; std::vector IgnoreHeaders; + // Whether emit only one finding per usage of a symbol. + const bool DeduplicateFindings; llvm::SmallVector IgnoreHeadersRegex; bool shouldIgnore(const include_cleaner::Header &H); }; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 2af86407069a6..815237656a403 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -192,6 +192,9 @@ Changes in existing checks ` 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 ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst index 3246fea78cd42..d56bb4526ff01 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst +++ b/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. diff --git a/clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp b/clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp index db048f10a0c15..fe3e38958f898 100644 --- a/clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp +++ b/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 @@ -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 Errors; + runCheckOnCode(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 Errors; + ClangTidyOptions Opts; + Opts.CheckOptions.insert({"DeduplicateFindings", "false"}); + runCheckOnCode(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"