diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h --- a/clang-tools-extra/clangd/IncludeCleaner.h +++ b/clang-tools-extra/clangd/IncludeCleaner.h @@ -13,9 +13,6 @@ /// to provide useful warnings in most popular scenarios but not 1:1 exact /// feature compatibility. /// -/// FIXME(kirillbobyrev): Add support for IWYU pragmas. -/// FIXME(kirillbobyrev): Add support for standard library headers. -/// //===----------------------------------------------------------------------===// #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDECLEANER_H @@ -23,10 +20,12 @@ #include "Headers.h" #include "ParsedAST.h" +#include "index/CanonicalIncludes.h" #include "clang/Basic/SourceLocation.h" #include "clang/Tooling/Inclusions/StandardLibrary.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLFunctionalExtras.h" +#include "llvm/ADT/StringSet.h" #include namespace clang { @@ -58,6 +57,11 @@ struct ReferencedFiles { llvm::DenseSet User; llvm::DenseSet Stdlib; + /// Files responsible for the symbols referenced in the main file and defined + /// in private headers (private headers have IWYU pragma: private, include + /// "public.h"). We store spelling of the public header files here to avoid + /// dealing with full filenames and visibility. + llvm::StringSet<> PublicHeaders; }; /// Retrieves IDs of all files containing SourceLocations from \p Locs. @@ -68,9 +72,12 @@ /// preferred to non self-contained and private headers). ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs, const SourceManager &SM, - llvm::function_ref HeaderResponsible); + llvm::function_ref &, + llvm::StringSet<> &)> + AddFile); ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs, const IncludeStructure &Includes, + const CanonicalIncludes &CanonIncludes, const SourceManager &SM); /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs). @@ -83,7 +90,8 @@ /// In unclear cases, headers are not marked as unused. std::vector getUnused(ParsedAST &AST, - const llvm::DenseSet &ReferencedFiles); + const llvm::DenseSet &ReferencedFiles, + const llvm::StringSet<> &ReferencedPublicHeaders); std::vector computeUnusedIncludes(ParsedAST &AST); diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp --- a/clang-tools-extra/clangd/IncludeCleaner.cpp +++ b/clang-tools-extra/clangd/IncludeCleaner.cpp @@ -12,6 +12,7 @@ #include "ParsedAST.h" #include "Protocol.h" #include "SourceCode.h" +#include "index/CanonicalIncludes.h" #include "support/Logger.h" #include "support/Trace.h" #include "clang/AST/ASTContext.h" @@ -23,6 +24,7 @@ #include "clang/Lex/Preprocessor.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/STLFunctionalExtras.h" +#include "llvm/ADT/StringSet.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Path.h" @@ -305,7 +307,9 @@ ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs, const SourceManager &SM, - llvm::function_ref HeaderResponsible) { + llvm::function_ref &, + llvm::StringSet<> &)> + AddFile) { std::vector Sorted{Locs.User.begin(), Locs.User.end()}; llvm::sort(Sorted); // Group by FileID. ReferencedFilesBuilder Builder(SM); @@ -324,33 +328,52 @@ // non-self-contained FileIDs as used. Perform this on FileIDs rather than // HeaderIDs, as each inclusion of a non-self-contained file is distinct. llvm::DenseSet UserFiles; + llvm::StringSet<> PublicHeaders; for (FileID ID : Builder.Files) - UserFiles.insert(HeaderResponsible(ID)); + AddFile(ID, UserFiles, PublicHeaders); llvm::DenseSet StdlibFiles; for (const auto &Symbol : Locs.Stdlib) for (const auto &Header : Symbol.headers()) StdlibFiles.insert(Header); - return {std::move(UserFiles), std::move(StdlibFiles)}; + return {std::move(UserFiles), std::move(StdlibFiles), + std::move(PublicHeaders)}; } ReferencedFiles findReferencedFiles(const ReferencedLocations &Locs, const IncludeStructure &Includes, + const CanonicalIncludes &CanonIncludes, const SourceManager &SM) { - return findReferencedFiles(Locs, SM, [&SM, &Includes](FileID ID) { - return headerResponsible(ID, SM, Includes); - }); + return findReferencedFiles( + Locs, SM, + [&SM, &Includes, &CanonIncludes](FileID ID, + llvm::DenseSet &UserFiles, + llvm::StringSet<> &PublicHeaders) { + auto FID = headerResponsible(ID, SM, Includes); + const FileEntry *Entry = SM.getFileEntryForID(FID); + if (Entry) { + auto PublicHeader = CanonIncludes.mapHeader(Entry->getName()); + if (!PublicHeader.empty()) { + PublicHeaders.insert(PublicHeader); + return; + } + } + UserFiles.insert(FID); + }); } std::vector getUnused(ParsedAST &AST, - const llvm::DenseSet &ReferencedFiles) { + const llvm::DenseSet &ReferencedFiles, + const llvm::StringSet<> &ReferencedPublicHeaders) { trace::Span Tracer("IncludeCleaner::getUnused"); std::vector Unused; for (const Inclusion &MFI : AST.getIncludeStructure().MainFileIncludes) { if (!MFI.HeaderID) continue; + if (ReferencedPublicHeaders.contains(MFI.Written)) + continue; auto IncludeID = static_cast(*MFI.HeaderID); bool Used = ReferencedFiles.contains(IncludeID); if (!Used && !mayConsiderUnused(MFI, AST)) { @@ -400,11 +423,12 @@ const auto &SM = AST.getSourceManager(); auto Refs = findReferencedLocations(AST); - auto ReferencedFileIDs = findReferencedFiles(Refs, AST.getIncludeStructure(), - AST.getSourceManager()); + auto ReferencedFiles = + findReferencedFiles(Refs, AST.getIncludeStructure(), + AST.getCanonicalIncludes(), AST.getSourceManager()); auto ReferencedHeaders = - translateToHeaderIDs(ReferencedFileIDs, AST.getIncludeStructure(), SM); - return getUnused(AST, ReferencedHeaders); + translateToHeaderIDs(ReferencedFiles, AST.getIncludeStructure(), SM); + return getUnused(AST, ReferencedHeaders, ReferencedFiles.PublicHeaders); } std::vector issueUnusedIncludesDiagnostics(ParsedAST &AST, diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -1643,10 +1643,15 @@ ]] #include "used.h" +$fix_private[[ $diag_private[[#include "private.h"]] +]] + #include "public.h" + #include void foo() { used(); + foo(); } )cpp"); TestTU TU; @@ -1659,6 +1664,15 @@ #pragma once void used() {} )cpp"; + TU.AdditionalFiles["public.h"] = R"cpp( + #pragma once + #include "private.h" + )cpp"; + TU.AdditionalFiles["private.h"] = R"cpp( + // IWYU pragma: private, include "public.h" + #pragma once + void foo(); + )cpp"; TU.AdditionalFiles["system/system_header.h"] = ""; TU.ExtraArgs = {"-isystem" + testPath("system")}; // Off by default. @@ -1668,10 +1682,16 @@ WithContextValue WithCfg(Config::Key, std::move(Cfg)); EXPECT_THAT( *TU.build().getDiagnostics(), - UnorderedElementsAre(AllOf( - Diag(Test.range("diag"), "included header unused.h is not used"), - withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd), - withFix(Fix(Test.range("fix"), "", "remove #include directive"))))); + UnorderedElementsAre( + AllOf( + Diag(Test.range("diag"), "included header unused.h is not used"), + withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd), + withFix(Fix(Test.range("fix"), "", "remove #include directive"))), + AllOf(Diag(Test.range("diag_private"), + "included header private.h is not used"), + withTag(DiagnosticTag::Unnecessary), diagSource(Diag::Clangd), + withFix(Fix(Test.range("fix_private"), "", + "remove #include directive"))))); Cfg.Diagnostics.SuppressAll = true; WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg)); EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty()); diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp --- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp +++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp @@ -9,6 +9,7 @@ #include "Annotations.h" #include "IncludeCleaner.h" #include "SourceCode.h" +#include "TestFS.h" #include "TestTU.h" #include "llvm/ADT/ScopeExit.h" #include "llvm/Testing/Support/SupportHelpers.h" @@ -280,8 +281,9 @@ ReferencedLocations Locs = findReferencedLocations(AST); EXPECT_THAT(Locs.Stdlib, ElementsAreArray(WantSyms)); - ReferencedFiles Files = findReferencedFiles(Locs, AST.getIncludeStructure(), - AST.getSourceManager()); + ReferencedFiles Files = + findReferencedFiles(Locs, AST.getIncludeStructure(), + AST.getCanonicalIncludes(), AST.getSourceManager()); EXPECT_THAT(Files.Stdlib, ElementsAreArray(WantHeaders)); } } @@ -392,8 +394,8 @@ auto &SM = AST.getSourceManager(); auto &Includes = AST.getIncludeStructure(); - auto ReferencedFiles = - findReferencedFiles(findReferencedLocations(AST), Includes, SM); + auto ReferencedFiles = findReferencedFiles( + findReferencedLocations(AST), Includes, AST.getCanonicalIncludes(), SM); llvm::StringSet<> ReferencedFileNames; for (FileID FID : ReferencedFiles.User) ReferencedFileNames.insert( @@ -412,7 +414,8 @@ EXPECT_THAT(ReferencedHeaderNames, ElementsAre(testPath("macros.h"))); // Sanity check. - EXPECT_THAT(getUnused(AST, ReferencedHeaders), IsEmpty()); + EXPECT_THAT(getUnused(AST, ReferencedHeaders, ReferencedFiles.PublicHeaders), + IsEmpty()); } TEST(IncludeCleaner, DistinctUnguardedInclusions) { @@ -441,9 +444,9 @@ ParsedAST AST = TU.build(); - auto ReferencedFiles = - findReferencedFiles(findReferencedLocations(AST), - AST.getIncludeStructure(), AST.getSourceManager()); + auto ReferencedFiles = findReferencedFiles( + findReferencedLocations(AST), AST.getIncludeStructure(), + AST.getCanonicalIncludes(), AST.getSourceManager()); llvm::StringSet<> ReferencedFileNames; auto &SM = AST.getSourceManager(); for (FileID FID : ReferencedFiles.User) @@ -475,9 +478,9 @@ ParsedAST AST = TU.build(); - auto ReferencedFiles = - findReferencedFiles(findReferencedLocations(AST), - AST.getIncludeStructure(), AST.getSourceManager()); + auto ReferencedFiles = findReferencedFiles( + findReferencedLocations(AST), AST.getIncludeStructure(), + AST.getCanonicalIncludes(), AST.getSourceManager()); llvm::StringSet<> ReferencedFileNames; auto &SM = AST.getSourceManager(); for (FileID FID : ReferencedFiles.User) @@ -493,14 +496,26 @@ TestTU TU; TU.Code = R"cpp( #include "behind_keep.h" // IWYU pragma: keep + #include "public.h" + + void bar() { foo(); } )cpp"; TU.AdditionalFiles["behind_keep.h"] = guard(""); + TU.AdditionalFiles["public.h"] = guard("#include \"private.h\""); + TU.AdditionalFiles["private.h"] = guard(R"cpp( + // IWYU pragma: private, include "public.h" + void foo() {} + )cpp"); ParsedAST AST = TU.build(); - auto ReferencedFiles = - findReferencedFiles(findReferencedLocations(AST), - AST.getIncludeStructure(), AST.getSourceManager()); - EXPECT_TRUE(ReferencedFiles.User.empty()); + auto ReferencedFiles = findReferencedFiles( + findReferencedLocations(AST), AST.getIncludeStructure(), + AST.getCanonicalIncludes(), AST.getSourceManager()); + EXPECT_EQ(ReferencedFiles.PublicHeaders.size(), 1u); + EXPECT_EQ(ReferencedFiles.PublicHeaders.begin()->getKey(), "\"public.h\""); + EXPECT_EQ(ReferencedFiles.User.size(), 1u); + EXPECT_EQ(*ReferencedFiles.User.begin(), + AST.getSourceManager().getMainFileID()); EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); }