Author: kadir çetinkaya Date: 2024-07-22T14:58:07+02:00 New Revision: 2f5dc596b5719e5ed7f6978dafbce994f425a033
URL: https://github.com/llvm/llvm-project/commit/2f5dc596b5719e5ed7f6978dafbce994f425a033 DIFF: https://github.com/llvm/llvm-project/commit/2f5dc596b5719e5ed7f6978dafbce994f425a033.diff LOG: [IncludeCleaner] Also check for spellings of physical headers (#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. Added: Modified: clang-tools-extra/clangd/IncludeCleaner.cpp clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp clang-tools-extra/include-cleaner/lib/Analysis.cpp clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp Removed: ################################################################################ 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
