https://github.com/kadircet created
https://github.com/llvm/llvm-project/pull/99843
Some physical headers can have "conflicting" spellings, when same
filename exists under two different include search paths. e.g. <stdlib.h>
can be provided by both standard library and underlying libc headers. In
such scenarios if the usage is from the latter include-search path,
users can't spell it directly.
This patch ensures we also consider spellings of such includes, in
addition to their physical files to prevent conflicting suggestions.
From e60ab4acfbb1b177e30618b0d4ce6a795a067cbc Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya <kadir...@google.com>
Date: Mon, 22 Jul 2024 10:07:08 +0200
Subject: [PATCH] [IncludeCleaner] Also check for spellings of physical headers
Some physical headers can have "conflicting" spellings, when same
filename exists under two different include search paths. e.g. <stdlib.h>
can be provided by both standard library and underlying libc headers. In
such scenarios if the usage is from the latter include-search path,
users can't spell it directly.
This patch ensures we also consider spellings of such includes, in
addition to their physical files to prevent conflicting suggestions.
---
clang-tools-extra/clangd/IncludeCleaner.cpp | 20 ++++++++++++++
.../clangd/unittests/IncludeCleanerTests.cpp | 22 ++++++++++++++++
.../include-cleaner/lib/Analysis.cpp | 17 +++++++++---
.../unittests/AnalysisTest.cpp | 26 +++++++++++++++++++
4 files changed, 82 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp
b/clang-tools-extra/clangd/IncludeCleaner.cpp
index dc5b7ec95db5f..e34706172f0bf 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -401,6 +401,26 @@ computeIncludeCleanerFindings(ParsedAST &AST, bool
AnalyzeAngledIncludes) {
Ref.RT != include_cleaner::RefType::Explicit)
return;
+ // Check if we have any headers with the same spelling, in edge cases
+ // like `#include_next "foo.h"`, the user can't ever include the
+ // physical foo.h, but can have a spelling that refers to it.
+ // We postpone this check because spelling a header for every usage is
+ // expensive.
+ std::string Spelling = include_cleaner::spellHeader(
+ {Providers.front(), AST.getPreprocessor().getHeaderSearchInfo(),
+ MainFile});
+ for (auto *Inc :
+ ConvertedIncludes.match(include_cleaner::Header{Spelling})) {
+ Satisfied = true;
+ auto HeaderID =
+ AST.getIncludeStructure().getID(&Inc->Resolved->getFileEntry());
+ assert(HeaderID.has_value() &&
+ "ConvertedIncludes only contains resolved includes.");
+ Used.insert(*HeaderID);
+ }
+ if (Satisfied)
+ return;
+
// We actually always want to map usages to their spellings, but
// spelling locations can point into preamble section. Using these
// offsets could lead into crashes in presence of stale preambles.
Hence
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 7027232460354..0ee748c1ed2d0 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -644,6 +644,28 @@ TEST(IncludeCleaner, ResourceDirIsIgnored) {
EXPECT_THAT(Findings.MissingIncludes, IsEmpty());
}
+TEST(IncludeCleaner, DifferentHeaderSameSpelling) {
+ // `foo` is declared in foo_inner/foo.h, but there's no way to spell it
+ // directly. Make sure we don't generate unusued/missing include findings in
+ // such cases.
+ auto TU = TestTU::withCode(R"cpp(
+ #include <foo.h>
+ void baz() {
+ foo();
+ }
+ )cpp");
+ TU.AdditionalFiles["foo/foo.h"] = guard("#include_next <foo.h>");
+ TU.AdditionalFiles["foo_inner/foo.h"] = guard(R"cpp(
+ void foo();
+ )cpp");
+ TU.ExtraArgs.push_back("-Ifoo");
+ TU.ExtraArgs.push_back("-Ifoo_inner");
+
+ auto AST = TU.build();
+ auto Findings = computeIncludeCleanerFindings(AST);
+ EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
+ EXPECT_THAT(Findings.MissingIncludes, IsEmpty());
+}
} // namespace
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
index f1cd72f877ca2..68fe79d6929f6 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -105,9 +105,20 @@ analyze(llvm::ArrayRef<Decl *> ASTRoots,
}
if (!Satisfied && !Providers.empty() &&
Ref.RT == RefType::Explicit &&
- !HeaderFilter(Providers.front().resolvedPath()))
- Missing.insert(spellHeader(
- {Providers.front(), PP.getHeaderSearchInfo(), MainFile}));
+ !HeaderFilter(Providers.front().resolvedPath())) {
+ // Check if we have any headers with the same spelling, in edge
+ // cases like `#include_next "foo.h"`, the user can't ever
+ // include the physical foo.h, but can have a spelling that
+ // refers to it.
+ auto Spelling = spellHeader(
+ {Providers.front(), PP.getHeaderSearchInfo(), MainFile});
+ for (const Include *I : Inc.match(Header{Spelling})) {
+ Used.insert(I);
+ Satisfied = true;
+ }
+ if (!Satisfied)
+ Missing.insert(std::move(Spelling));
+ }
});
AnalysisResults Results;
diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index 6558b68087684..5696c380758f8 100644
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -12,6 +12,7 @@
#include "clang-include-cleaner/Record.h"
#include "clang-include-cleaner/Types.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclBase.h"
#include "clang/Basic/FileManager.h"
#include "clang/Basic/IdentifierTable.h"
#include "clang/Basic/SourceLocation.h"
@@ -296,6 +297,31 @@ TEST_F(AnalyzeTest, ResourceDirIsIgnored) {
EXPECT_THAT(Results.Missing, testing::IsEmpty());
}
+TEST_F(AnalyzeTest, DifferentHeaderSameSpelling) {
+ Inputs.ExtraArgs.push_back("-Ifoo");
+ Inputs.ExtraArgs.push_back("-Ifoo_inner");
+ // `foo` is declared in foo_inner/foo.h, but there's no way to spell it
+ // directly. Make sure we don't generate unusued/missing include findings in
+ // such cases.
+ Inputs.Code = R"cpp(
+ #include <foo.h>
+ void baz() {
+ foo();
+ }
+ )cpp";
+ Inputs.ExtraFiles["foo/foo.h"] = guard("#include_next <foo.h>");
+ Inputs.ExtraFiles["foo_inner/foo.h"] = guard(R"cpp(
+ void foo();
+ )cpp");
+ TestAST AST(Inputs);
+ std::vector<Decl *> DeclsInTU;
+ for (auto *D : AST.context().getTranslationUnitDecl()->decls())
+ DeclsInTU.push_back(D);
+ auto Results = analyze(DeclsInTU, {}, PP.Includes, &PI, AST.preprocessor());
+ EXPECT_THAT(Results.Unused, testing::IsEmpty());
+ EXPECT_THAT(Results.Missing, testing::IsEmpty());
+}
+
TEST(FixIncludes, Basic) {
llvm::StringRef Code = R"cpp(#include "d.h"
#include "a.h"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
