[clang][include cleaner] Fix MainHeader insertion issue - #212852
Open
dmaclach wants to merge 7 commits into
Open
[clang][include cleaner] Fix MainHeader insertion issue#212852dmaclach wants to merge 7 commits into
dmaclach wants to merge 7 commits into
Conversation
Contributor
Author
|
@llvm/pr-subscribers-clang-format @llvm/pr-subscribers-clang-tools-extra Author: dmaclach (dmaclach) ChangesInstead of relying on magic UINT_MAX replacements for clang-format to resolve, this change uses tooling::HeaderIncludes to calculate precise offsets and replacement text for adding and removing headers. Full diff: https://github.com/llvm/llvm-project/pull/212852.diff 1 Files Affected:
diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
index e48a380211af0..a05c6145e6ca9 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -20,8 +20,10 @@
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Inclusions/HeaderIncludes.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/STLFunctionalExtras.h"
@@ -31,7 +33,6 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include <cassert>
-#include <climits>
#include <string>
namespace clang::include_cleaner {
@@ -168,12 +169,30 @@ std::string fixIncludes(const AnalysisResults &Results,
const format::FormatStyle &Style) {
assert(Style.isCpp() && "Only C++ style supports include insertions!");
tooling::Replacements R;
- // Encode insertions/deletions in the magic way clang-format understands.
- for (const Include *I : Results.Unused)
- cantFail(R.add(tooling::Replacement(FileName, UINT_MAX, 1, I->quote())));
- for (auto &[Spelled, _] : Results.Missing)
- cantFail(R.add(
- tooling::Replacement(FileName, UINT_MAX, 0, "#include " + Spelled)));
+ tooling::HeaderIncludes HeaderIncludes(FileName, Code, Style.IncludeStyle);
+
+ for (const Include *I : Results.Unused) {
+ auto Deletion = HeaderIncludes.remove(I->Spelled, I->Angled);
+ for (const auto &Del : Deletion) {
+ cantFail(R.add(Del));
+ }
+ }
+
+ llvm::DenseMap<unsigned, std::string> InsertionsByOffset;
+ for (auto &[Spelled, _] : Results.Missing) {
+ auto Insertion = HeaderIncludes.insert(StringRef{Spelled}.trim("\"<>"),
+ Spelled.starts_with('<'),
+ tooling::IncludeDirective::Include);
+ if (Insertion) {
+ InsertionsByOffset[Insertion->getOffset()] +=
+ Insertion->getReplacementText();
+ }
+ }
+
+ for (const auto &Entry : InsertionsByOffset) {
+ cantFail(
+ R.add(tooling::Replacement(FileName, Entry.first, 0, Entry.second)));
+ }
// "cleanup" actually turns the UINT_MAX replacements into concrete edits.
auto Positioned = cantFail(format::cleanupAroundReplacements(Code, R, Style));
return cantFail(tooling::applyAllReplacements(Code, Positioned));
|
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) AddressSanitizer-x86_64-linuxAddressSanitizer-x86_64-linux.TestCases/invalid-pointer-pairs-vector-extract.cppAddressSanitizer-x86_64-linux-dynamicAddressSanitizer-x86_64-linux-dynamic.TestCases/invalid-pointer-pairs-vector-extract.cppIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
dmaclach
force-pushed
the
headerIncludes
branch
2 times, most recently
from
August 3, 2026 19:40
8dd3892 to
d49a325
Compare
Member
Thanks, unfortunately I also no longer remember the reason why we went with clang-format's special handling in the first place. I am worried that these might be behaving differently on certain edge cases. What do we gain in particular by migrating from one way of doing this to another (apart from new way being less magic)? |
Contributor
Author
kadircet
requested changes
Aug 17, 2026
Comment on lines
+192
to
+204
| auto Insertion = HeaderIncludes.insert( | ||
| llvm::StringRef{Spelled}.trim("\"<>"), isAngled(Spelled), | ||
| tooling::IncludeDirective::Include); | ||
| if (Insertion) { | ||
| auto &Info = InsertionsByOffset[Insertion->getOffset()]; | ||
| Info.Text += Insertion->getReplacementText(); | ||
| if (Insertion->getLength() > 0) { | ||
| // We can concatenate pure insertions (length 0), but at most one | ||
| // true replacement (length > 0) to avoid overwriting the length. | ||
| assert(Info.Length == 0 && "Multiple replacements at same offset?"); | ||
| Info.Length = Insertion->getLength(); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
i am slightly worried about all the new logic here, especially around grouping/sorting of includes etc.
can you add some more unittests for exercising those? e.g. have "b.h" and then "a.h" as missing, also have a "foo.h" include on a file named "foo.cc", preferably some more with insertions of "a.h" and <foo> etc
Contributor
Author
There was a problem hiding this comment.
Added more tests as requested. Turns out that one of the new tests would've failed in the old code.
Moved the batch insertion logic into HeaderIncludes for locality and potential future reuse by others.
dmaclach
force-pushed
the
headerIncludes
branch
from
August 17, 2026 19:59
d49a325 to
7f3fdbf
Compare
kadircet
requested changes
Aug 18, 2026
|
|
||
| HeaderToInsert(StringRef RawOrSpelledHeader, | ||
| IncludeDirective Directive = IncludeDirective::Include, | ||
| std::optional<bool> IsAngled = std::nullopt); |
Member
There was a problem hiding this comment.
enum Spelling { AUTO, ANGLED, QUOTES } instead of tri-state (or just make it explicit IsAngled)?
| /// returns std::nullopt. | ||
| std::optional<tooling::Replacement> insert(llvm::StringRef Header, | ||
| bool IsAngled, | ||
| std::optional<tooling::Replacement> insert(StringRef Header, bool IsAngled, |
Member
There was a problem hiding this comment.
can we keep llvm:: here and elsewhere ?
| this->IsAngled = IsAngled.value_or(false); | ||
| } else { | ||
| Header = RawOrSpelledHeader.str(); | ||
| this->IsAngled = IsAngled.value_or(false); |
Member
There was a problem hiding this comment.
can we document this default somewhere?
moreover the rest of the logic is using IsAngled as a fallback, even if it's set explicitly, which feels unexpected.
Contributor
Author
There was a problem hiding this comment.
With the enum it is pretty self documenting. Added documentation as well though.
| if (Headers.empty()) | ||
| return Result; | ||
|
|
||
| std::vector<HeaderToInsert> SortedHeaders = Headers.vec(); |
Member
There was a problem hiding this comment.
i am uneasy about re-implementing all of this logic, can we try to utilize bits from clang/include/clang/Format/Format.h or figure out why format::cleanupAroundReplacements call in include-cleaner/lib/Analysis.cpp is not getting those fixed ?
Contributor
Author
There was a problem hiding this comment.
So Format.cpp depends on HeaderIncludes. I have updated Format.cpp to use my new bulk insertion function. This overall improves Format.cpp in that previously it mishandled Main Headers when multiple headers were being added one by one. Now when doing bulk insertions we pass them to the single insertion function in the correct order to make the bulk insertion respect MainHeaders, quoted includes and angle brackets appropriately.
…and deletions Instead of relying on magic UINT_MAX replacements for clang-format to resolve, this change uses tooling::HeaderIncludes to calculate precise offsets and replacement text for adding and removing headers.
…d locality. Add more tests as requested.
- Restored `llvm::` prefixes - Added enum - Updated documentation - Made Format.cpp use the new bulk insertion function from HeaderIncludes. - Restored Analyze.cpp back to original calling through Format.cpp
dmaclach
force-pushed
the
headerIncludes
branch
from
August 25, 2026 21:53
7f3fdbf to
7375bd5
Compare
- Reset Analysis back to original version. - Move tests to single change block.
dmaclach
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When doing multiple header insertions (like from include cleaner) there was an issue with MainHeaders inserted in the wrong location. HeaderIncludes now supports a bulk insertion where it sorts the insertions appropriately before inserting them to make sure that the MainHeader ends up in the correct location.
Format.cpp has been updated to use the bulk insertion method correctly.
Tests added to verify.