[clang-tidy][include-cleaner] Don't warn for the same symbol twice

[abrachet]

Summary

Only one diagnostic per symbol is necessary, after that the diagnostics
can become overbearing.

[VitaNuo]

Hi Alex,

Thank you for the patch.
We are aware that issuing one diagnostic per symbol reference might be overwhelming. However, we have decided to choose it over other options after considering the use cases that we'd like to cover.
This check is intended to be used in review tooling, where the user usually only sees their own change/diff. Issuing only one diagnostic for the first symbol reference would render this check mostly useless in code review, since the first symbol reference is not in the scope of the user's change most of the time.

Moreover, it's desirable to have similar behavior in include-cleaner checks across multiple clang tools. The current behaviour corresponds to the behaviour of built-in include-cleaner in clangd for similar reasons, i.e., users are most prone to act on a finding if it shows up in the piece of code they're currently editing.

The currently ongoing cleanup efforts should help the make the findings less overwhelming as well.

[PiotrZSL]

Check emits warnings per symbol/usage, but fixes header includes, won't it make duplicate includes ?

Problem that I see is that multiple warnings for same symbol will generate lot of noise.
And when dealing with diff why developer should be forced to fix legacy issues just because he/she used same symbol, after all issue is actually peer symbol, whatever how many times is used.
Stil...

I cannot make "decision" here.
Both patch author, and check author got a point.

There is only one option, could this be made configurable ?

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

return llvm::any_of(IgnoreHeadersRegex, [&H](const llvm::Regex &R) {
     llvm_unreachable("Unknown Header kind.");
   });
 }
 
 void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
   const SourceManager *SM = Result.SourceManager;
   const FileEntry *MainFile = SM->getFileEntryForID(SM->getMainFileID());
   llvm::DenseSet<const include_cleaner::Include *> Used;
-  std::vector<MissingIncludeInfo> Missing;
+  using SymRef = include_cleaner::SymbolReference;
+  auto Cmp = [](const SymRef &Left, const SymRef &Right) {
+    return Left.Target.name() < Right.Target.name();
+  };
+  std::map<SymRef, include_cleaner::Header, decltype(Cmp)> Missing{Cmp};
   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);
   }
   // FIXME: Find a way to have less code duplication between include-cleaner
walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
                     RecordedPreprocessor.Includes.match(H)) {
                  Used.insert(I);
                  Satisfied = true;
                }
              }
              if (!Satisfied && !Providers.empty() &&
                  Ref.RT == include_cleaner::RefType::Explicit &&
                  !shouldIgnore(Providers.front()))
-               Missing.push_back({Ref, Providers.front()});
+               Missing.try_emplace(Ref, Providers.front());
            });
 
   std::vector<const include_cleaner::Include *> Unused;
   for (const include_cleaner::Include &I :
        RecordedPreprocessor.Includes.all()) {
     if (Used.contains(&I) || !I.Resolved)
       continue;
     if (RecordedPI.shouldKeep(I.Line))
diag(Inc->HashLocation, "included header %0 is not used directly")
                                      llvm::sys::path::Style::posix)
         << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
                SM->translateLineCol(SM->getMainFileID(), Inc->Line, 1),
                SM->translateLineCol(SM->getMainFileID(), Inc->Line + 1, 1)));
   }
 
   tooling::HeaderIncludes HeaderIncludes(getCurrentMainFile(), Code,
                                          FileStyle->IncludeStyle);
-  for (const auto &Inc : Missing) {
+  for (const auto &[SymRef, Header] : Missing) {
     std::string Spelling =
-        include_cleaner::spellHeader({Inc.Missing, *HS, MainFile});
+        include_cleaner::spellHeader({Header, *HS, MainFile});
     bool Angled = llvm::StringRef{Spelling}.starts_with("<");
     // We might suggest insertion of an existing include in edge cases, e.g.,
     // include is present in a PP-disabled region, or spelling of the header
     // turns out to be the same as one of the unresolved includes in the
     // main file.
     if (auto Replacement =
             HeaderIncludes.insert(llvm::StringRef{Spelling}.trim("\"<>"),
                                   Angled, tooling::IncludeDirective::Include))
-      diag(SM->getSpellingLoc(Inc.SymRef.RefLocation),
+      diag(SM->getSpellingLoc(SymRef.RefLocation),
            "no header providing \"%0\" is directly included")
-          << Inc.SymRef.Target.name()
+          << SymRef.Target.name()
           << FixItHint::CreateInsertion(
                  SM->getComposedLoc(SM->getMainFileID(),
                                     Replacement->getOffset()),
                  Replacement->getReplacementText());
   }
 }
 
 } // namespace clang::tidy::misc

clang-tools-extra/test/clang-tidy/checkers/misc/include-cleaner.cpp

 // CHECK-FIXES: {{^}}
 int BarResult = bar();
 int BazResult = baz();
 // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: no header providing "baz" is directly included [misc-include-cleaner]
 std::string HelloString;
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: no header providing "std::string" is directly included [misc-include-cleaner]
 int FooBarResult = foobar();
 // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: no header providing "foobar" is directly included [misc-include-cleaner]
+int FooBarResult2 = foobar();
+// CHECK-MESSAGES-NOT: :[[@LINE-1]]: