[IncludeCleaner][clangd] Mark umbrella headers as users of private

Private headers inside umbrella files shouldn't be marked as unused.

Differential Revision: https://reviews.llvm.org/D146406

clang-tools-extra/clangd/IncludeCleaner.cpp

@@ -93,8 +93,6 @@ bool isFilteredByConfig(const Config &Cfg, llvm::StringRef HeaderPath) {
 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 @@ static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
   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(

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

@@ -30,6 +30,7 @@
 #include "gtest/gtest.h"
 #include <cstddef>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace clang {
@@ -328,6 +329,26 @@ TEST(IncludeCleaner, NoDiagsForObjC) {
   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

clang-tools-extra/include-cleaner/lib/Analysis.cpp

@@ -90,9 +90,25 @@ AnalysisResults analyze(llvm::ArrayRef<Decl *> ASTRoots,
            });
 
   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);

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

@@ -24,6 +24,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cstddef>
+#include <vector>
 
 namespace clang::include_cleaner {
 namespace {
@@ -212,17 +213,34 @@ int x = a + c;
     return std::make_unique<Hook>(PP, PI);
   };
 
-  TestAST AST(Inputs);
-  auto Decls = AST.context().getTranslationUnitDecl()->decls();
-  auto Results =
-      analyze(std::vector<Decl *>{Decls.begin(), Decls.end()},
-              PP.MacroReferences, PP.Includes, &PI, AST.sourceManager(),
-              AST.preprocessor().getHeaderSearchInfo());
+  {
+    TestAST AST(Inputs);
+    auto Decls = AST.context().getTranslationUnitDecl()->decls();
+    auto Results =
+        analyze(std::vector<Decl *>{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));
+  }
 
-  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);
+    EXPECT_FALSE(PP.Includes.all().empty());
+    auto Results = analyze({}, {}, PP.Includes, &PI, AST.sourceManager(),
+                           AST.preprocessor().getHeaderSearchInfo());
+    EXPECT_THAT(Results.Unused, testing::IsEmpty());
+  }
 }
 
 TEST(FixIncludes, Basic) {

clang/include/clang/Testing/TestAST.h

@@ -49,6 +49,9 @@ struct TestInputs {
   /// Keys are plain filenames ("foo.h"), values are file content.
   llvm::StringMap<std::string> 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().

clang/lib/Testing/TestAST.cpp

@@ -16,6 +16,7 @@
 #include "llvm/Support/VirtualFileSystem.h"
 
 #include "gtest/gtest.h"
+#include <string>
 
 namespace clang {
 namespace {
@@ -91,7 +92,9 @@ TestAST::TestAST(const TestInputs &In) {
     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<CompilerInvocation>());
   if (!CompilerInvocation::CreateFromArgs(Clang->getInvocation(), Argv,