[clang-tidy] misc-include-cleaner: avoid duplicated fixes

[danix800]

Summary

Do deduplication when inserting missing headers when diags are not self contained.

Fixes https://github.com/llvm/llvm-project/issues/65285

[sammccall]

Thanks! @kadircet should make the call on these features (they look good to me), I have some implementation notes.

This is making two independent changes, one is a policy change that should be applied to the include-cleaner library, and one is a bugfix to the clang-tidy check. I think they should be separate patches.

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

169

If we want this policy, it should be provided at the include-cleaner library level so it applies to all clients, not just the clang-tidy check.

I think the clearest way is to encapsulate this in include_cleaner::Includes, so that match() never matches a Header requirement against a duplicative Include. This will naturally lead to the directives being marked as unused.

(In practice, the result will work in clangd, and will respect configuration of headers where analysis should be ignored, keep directives, etc)

227

This means that if you're looking at individual diagnostics rather than applying all fixes to a file, only the first diagnostic will have any fix available at all.

I believe the preferred solution is to do this conditionally based on areDiagsSelfContained(). clang_tidy::utils::IncludeInserter encapsulates this, but isn't used here.

I don't know whether we would want to use it here, or how carefully it's already been considered. (It definitely contains a lot of logic that is dubious to apply when the include to insert has already been precisely calculated, but also some things that would be helpful). Will defer to Kadir on that.

[danix800]

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

169

I'll leave the policy part to another revision (maybe by others).

227

Thanks for mentioning the clang_tidy::utils::IncludeInserter. I took a further look and found that IncludeInserter is
more suitable for this checker.

[danix800]

1. Use clang::tidy::utils::IncludeInserter to avoid repeated insertion;
2. Leave out the deduplication policy part.

[danix800]

Ping @kadircet~, could you please take a look at this revision?

[kadircet]

Thanks a lot for the patch @danix800 !

I initially was rather focused on behavior of this check in workflows that require seeing "self-contained-diags", but also I rather see the bulk-apply use cases as always requiring clang-format afterwards. Hence didn't really try to polish that use case a lot, but I believe changes in this patch improve those use cases reasonably. But users do still need to run clang-format afterwards, e.g. if we first generate a finding that inserts "b.h" and then "a.h", they'll be in wrong order. So this is only fixing some cases. If you'd like to work on a more complete approach, we can prepare all the edits in a single tooling::Replacements and run clang::format::cleanupAroundReplacements on top of it to generate proper edits, and emit FixIts with those merged replacements. Note that this still won't be enough in all cases, as there can be other checks that emit fixes that are touching includes :/

Regarding usage of IncludeInserter; we were deliberately not using IncludeInserter here, as it has slightly different semantics to HeaderIncludes, which is used by all the other applications of include-cleaner when producing edits. That way it's easier for us to reason about the behavior in various places (and also to fix them). But moreover, HeaderIncludes uses clang-format config to figure out include-styles and works with a bigger set of projects without requiring extra configurations (hence this patch will actually be a regression for those). Therefore can you keep using HeaderIncludes, while skipping generation of duplicate fixits when we're in non-self-contained-diags mode (assuming you don't want to generalize the approach as I explained above).

[danix800]

In D159263#4638167, @kadircet wrote:

Thanks a lot for the patch @danix800 !

I initially was rather focused on behavior of this check in workflows that require seeing "self-contained-diags", but also I rather see the bulk-apply use cases as always requiring clang-format afterwards. Hence didn't really try to polish that use case a lot, but I believe changes in this patch improve those use cases reasonably. But users do still need to run clang-format afterwards, e.g. if we first generate a finding that inserts "b.h" and then "a.h", they'll be in wrong order. So this is only fixing some cases. If you'd like to work on a more complete approach, we can prepare all the edits in a single tooling::Replacements and run clang::format::cleanupAroundReplacements on top of it to generate proper edits, and emit FixIts with those merged replacements. Note that this still won't be enough in all cases, as there can be other checks that emit fixes that are touching includes :/

Regarding usage of IncludeInserter; we were deliberately not using IncludeInserter here, as it has slightly different semantics to HeaderIncludes, which is used by all the other applications of include-cleaner when producing edits. That way it's easier for us to reason about the behavior in various places (and also to fix them). But moreover, HeaderIncludes uses clang-format config to figure out include-styles and works with a bigger set of projects without requiring extra configurations (hence this patch will actually be a regression for those). Therefore can you keep using HeaderIncludes, while skipping generation of duplicate fixits when we're in non-self-contained-diags mode (assuming you don't want to generalize the approach as I explained above).

Thanks for the background!

If we have further plans from upstream on these checker/clang-format related logic, I can wait and abandon this revision.

Or if such issue does need fixing then coud you take a look at the previous diff 1
which uses an internal set to prevent this issue?

[kadircet]

In D159263#4638199, @danix800 wrote:

If we have further plans from upstream on these checker/clang-format related logic, I can wait and abandon this revision.

Not at the moment, so please don't!

Or if such issue does need fixing then coud you take a look at the previous diff 1
which uses an internal set to prevent this issue?

Yes, I think that approach makes sense. But we should make it conditional on areDiagsSelfContained (basically de-duplicate only when diags are not self contained).

[danix800]

1. Revert to internal set (not using IncludeCleaner);
2. Only do deduplication when not areDiagsSelfContained().

[danix800]

Duplicated to https://github.com/llvm/llvm-project/pull/65431, abandon this one.

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

 #include "clang/Format/Format.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Inclusions/HeaderIncludes.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include <optional>
 #include <string>
 #include <vector>
 
 using namespace clang::ast_matchers;
walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
              }
              if (!Satisfied && !Providers.empty() &&
                  Ref.RT == include_cleaner::RefType::Explicit &&
                  !shouldIgnore(Providers.front()))
                Missing.push_back({Ref, Providers.front()});
            });
 
   std::vector<const include_cleaner::Include *> Unused;
+  llvm::StringSet<> AlreadyIncludedHeaders;
   for (const include_cleaner::Include &I :
        RecordedPreprocessor.Includes.all()) {
-    if (Used.contains(&I) || !I.Resolved)
+    if (!I.Resolved)
       continue;
+    if (Used.contains(&I)) {
+      if (!AlreadyIncludedHeaders.contains(I.Spelled))
+        AlreadyIncludedHeaders.insert(I.Spelled);
+      else
+        Unused.push_back(&I);
+      continue;
+    }
     if (RecordedPI.shouldKeep(*I.Resolved))
       continue;
     // Check if main file is the public interface for a private header. If so
     // we shouldn't diagnose it as unused.
     if (auto PHeader = RecordedPI.getPublic(*I.Resolved); !PHeader.empty()) {
       PHeader = PHeader.trim("<>\"");
       // Since most private -> public mappings happen in a verbatim way, we
       // check textually here. This might go wrong in presence of symlinks or
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);
+  llvm::StringSet<> AlreadyInserted;
   for (const auto &Inc : Missing) {
     std::string Spelling = include_cleaner::spellHeader(
         {Inc.Missing, PP->getHeaderSearchInfo(), 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))
+                                  Angled, tooling::IncludeDirective::Include)) {
+      DiagnosticBuilder DB =
           diag(SM->getSpellingLoc(Inc.SymRef.RefLocation),
                "no header providing \"%0\" is directly included")
-          << Inc.SymRef.Target.name()
-          << FixItHint::CreateInsertion(
-                 SM->getComposedLoc(SM->getMainFileID(),
-                                    Replacement->getOffset()),
-                 Replacement->getReplacementText());
+          << Inc.SymRef.Target.name();
+      if (!AlreadyInserted.contains(Inc.Missing.resolvedPath())) {
+        DB << FixItHint::CreateInsertion(
+            SM->getComposedLoc(SM->getMainFileID(), Replacement->getOffset()),
+        AlreadyInserted.insert(Inc.Missing.resolvedPath());
+      }
+    }
   }
 }
 
 } // namespace clang::tidy::misc

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/baz.h

 #pragma once
+#define BAZ 10
 int baz();

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

 // RUN: %check_clang_tidy %s misc-include-cleaner %t -- -- -I%S/Inputs -isystem%S/Inputs/system
 #include "bar.h"
+#include "bar.h"
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: included header bar.h is not used directly [misc-include-cleaner]
+// CHECK-FIXES: {{^}}
 // CHECK-FIXES: {{^}}#include "baz.h"{{$}}
 #include "foo.h"
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: included header foo.h is not used directly [misc-include-cleaner]
 // CHECK-FIXES: {{^}}
 // CHECK-FIXES: {{^}}#include <string>{{$}}
 #include <vector.h>
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: included header vector.h is not used directly [misc-include-cleaner]
 // CHECK-FIXES: {{^}}
 int BarResult = bar();
 int BazResult = baz();
 // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: no header providing "baz" is directly included [misc-include-cleaner]
+int BazResultAgain = BAZ; // Header should not be inserted more than once
+// CHECK-MESSAGES: :[[@LINE-1]]:22: 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]