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 @@ -93,8 +93,6 @@ static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST, const Config &Cfg, const include_cleaner::PragmaIncludes *PI) { - if (PI && PI->shouldKeep(Inc.HashLine + 1)) - return false; // FIXME(kirillbobyrev): We currently do not support the umbrella headers. // System headers are likely to be standard library headers. // Until we have good support for umbrella headers, don't warn about them. @@ -108,6 +106,20 @@ auto FE = AST.getSourceManager().getFileManager().getFileRef( AST.getIncludeStructure().getRealPath(HID)); assert(FE); + if (PI) { + if (PI->shouldKeep(Inc.HashLine + 1)) + return false; + // Check if main file is the public interface for a private header. If so we + // shouldn't diagnose it as unused. + if(auto PHeader = PI->getPublic(*FE); !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 + // header mappings. But that's not different than rest of the places. + if(AST.tuPath().endswith(PHeader)) + return false; + } + } // Headers without include guards have side effects and are not // self-contained, skip them. if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded( 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 @@ -30,6 +30,7 @@ #include "gtest/gtest.h" #include #include +#include #include namespace clang { @@ -328,6 +329,26 @@ ParsedAST AST = TU.build(); EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); } + +TEST(IncludeCleaner, UmbrellaUsesPrivate) { + TestTU TU; + TU.Code = R"cpp( + #include "private.h" + )cpp"; + TU.AdditionalFiles["private.h"] = guard(R"cpp( + // IWYU pragma: private, include "public.h" + void foo() {} + )cpp"); + TU.Filename = "public.h"; + Config Cfg; + Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict; + WithContextValue Ctx(Config::Key, std::move(Cfg)); + ParsedAST AST = TU.build(); + EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); + IncludeCleanerFindings Findings = computeIncludeCleanerFindings(AST); + EXPECT_THAT(Findings.UnusedIncludes, 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 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -90,9 +90,25 @@ }); AnalysisResults Results; - for (const Include &I : Inc.all()) - if (!Used.contains(&I) && PI && !PI->shouldKeep(I.Line)) - Results.Unused.push_back(&I); + for (const Include &I : Inc.all()) { + if (Used.contains(&I)) + continue; + if (PI) { + if (PI->shouldKeep(I.Line)) + 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 = PI->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 + // header mappings. But that's not different than rest of the places. + if (MainFile->tryGetRealPathName().endswith(PHeader)) + continue; + } + } + Results.Unused.push_back(&I); + } for (llvm::StringRef S : Missing.keys()) Results.Missing.push_back(S.str()); llvm::sort(Results.Missing); diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp --- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -212,17 +212,38 @@ return std::make_unique(PP, PI); }; - TestAST AST(Inputs); - auto Decls = AST.context().getTranslationUnitDecl()->decls(); - auto Results = - analyze(std::vector{Decls.begin(), Decls.end()}, - PP.MacroReferences, PP.Includes, &PI, AST.sourceManager(), - AST.preprocessor().getHeaderSearchInfo()); - - const Include *B = PP.Includes.atLine(3); - ASSERT_EQ(B->Spelled, "b.h"); - EXPECT_THAT(Results.Missing, ElementsAre("\"c.h\"")); - EXPECT_THAT(Results.Unused, ElementsAre(B)); + { + TestAST AST(Inputs); + auto Decls = AST.context().getTranslationUnitDecl()->decls(); + auto Results = + analyze(std::vector{Decls.begin(), Decls.end()}, + PP.MacroReferences, PP.Includes, &PI, AST.sourceManager(), + AST.preprocessor().getHeaderSearchInfo()); + + const Include *B = PP.Includes.atLine(3); + ASSERT_EQ(B->Spelled, "b.h"); + EXPECT_THAT(Results.Missing, ElementsAre("\"c.h\"")); + EXPECT_THAT(Results.Unused, ElementsAre(B)); + } + + // Check that umbrella header uses private include. + { + Inputs.Code = R"cpp( +#include "private.h" +)cpp"; + Inputs.ExtraFiles["private.h"] = + guard("// IWYU pragma: private, include \"public.h\""); + Inputs.FileName = "public.h"; + PP.Includes = {}; + PI = {}; + TestAST AST(Inputs); + auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(), + AST.preprocessor().getHeaderSearchInfo()); + + const Include *Private = PP.Includes.atLine(2); + ASSERT_EQ(Private->Spelled, "private.h"); + EXPECT_THAT(Results.Unused, testing::IsEmpty()); + } } TEST(FixIncludes, Basic) { diff --git a/clang/include/clang/Testing/TestAST.h b/clang/include/clang/Testing/TestAST.h --- a/clang/include/clang/Testing/TestAST.h +++ b/clang/include/clang/Testing/TestAST.h @@ -49,6 +49,9 @@ /// Keys are plain filenames ("foo.h"), values are file content. llvm::StringMap ExtraFiles = {}; + /// Filename to use for translation unit. A default will be used when empty. + std::string FileName; + /// By default, error diagnostics during parsing are reported as gtest errors. /// To suppress this, set ErrorOK or include "error-ok" in a comment in Code. /// In either case, all diagnostics appear in TestAST::diagnostics(). diff --git a/clang/lib/Testing/TestAST.cpp b/clang/lib/Testing/TestAST.cpp --- a/clang/lib/Testing/TestAST.cpp +++ b/clang/lib/Testing/TestAST.cpp @@ -16,6 +16,7 @@ #include "llvm/Support/VirtualFileSystem.h" #include "gtest/gtest.h" +#include namespace clang { namespace { @@ -91,7 +92,9 @@ Argv.push_back(S.c_str()); for (const auto &S : In.ExtraArgs) Argv.push_back(S.c_str()); - std::string Filename = getFilenameForTesting(In.Language).str(); + std::string Filename = In.FileName; + if (Filename.empty()) + Filename = getFilenameForTesting(In.Language).str(); Argv.push_back(Filename.c_str()); Clang->setInvocation(std::make_unique()); if (!CompilerInvocation::CreateFromArgs(Clang->getInvocation(), Argv,