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

[kadircet]

Summary

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

[hokein]

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

100

The beahvior (no showing the diagnostic) seems reasonable to me as we can infer that the included header is supposed to be exported by the main file. Just explore this a bit more.

The sad bit is that we start diverging from the classical IWYU tool (I have check it, for this case, it gives an unused-include unless you add an export pragma).

I think the main cause is that we're missing the // IWYU pragma: export, we should still want to recommend users to add the pragma export, right?

My only concern is that without the export pragma, whether the include is used can't be reason about by just "reading" the main-file content by human, e.g. for the case below, there is no usage of the private.h (previously the trailing // IWYU pragma: export comment is considered a usage), we have to open the private.h and find the private mapping to verify the usage.

// public.h

#include "private.h"

It seems better to provide an adding //IWYU pragma: export FIXIT.

[kadircet]

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

100

The sad bit is that we start diverging from the classical IWYU tool (I have check it, for this case, it gives an unused-include unless you add an export pragma).

we're not providing the same behavior with IWYU tool, it's nice to be compatible with it and this change won't break that compatibility (i.e. if IWYU tool drops this include, we're not suggesting to insert or while IWYU tool is suggesting to insert we're not suggesting to remove). so i don't think this argument applies here.

I think the main cause is that we're missing the // IWYU pragma: export, we should still want to recommend users to add the pragma export, right?

i don't see the reason why. i think we're actually losing value by enforcing people to annotate headers in both places. if the library provider already put a pragma in the included header, marking the umbrella header as the private header, what's the extra value we're getting by making them also annotate the public header with a bunch of export pragmas? i feel like this goes against the nature of letting tooling automate things as much as possible.

My only concern is that without the export pragma, whether the include is used can't be reason about by just "reading" the main-file content by human, e.g. for the case below, there is no usage of the private.h (previously the trailing // IWYU pragma: export comment is considered a usage), we have to open the private.h and find the private mapping to verify the usage.

this definitely makes sense, and having an explicit IWYU export/keep pragma on these includes are definitely nice but I believe in the common case, because private-public matching is 1:1, the reason why a private header is included inside a public header is quite obvious hence absence of these pragmas won't really cause confusion, to the contrary forcing people to introduce them will actually create frustration.

WDYT?

[hokein]

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

100

this change won't break that compatibility (i.e. if IWYU tool drops this include, we're not suggesting to insert or while IWYU tool is suggesting to insert we're not suggesting to remove). so i don't think this argument applies here.

Agree on this point. This is a safe change. But now clangd is diverging from the clang-include-cleaner tool.

i feel like this goes against the nature of letting tooling automate things as much as possible

I think the proposed change is good.

From the tooling automation, for example, we're doing a large cleanup (remove unused includes), we should not remove this include.

For interactive tools (like clangd), I actually think in addition to that, clangd should tell the user that they are missing an IWYU export pragma here, and suggest a FIXIT (but that's probably another thread).

but I believe in the common case, because private-public matching is 1:1, the reason why a private header is included inside a public header is quite obvious hence absence of these pragmas won't really cause confusion

I'm not sure this is true, I think private=>public is 1:1, but public=>private is not always 1:1 (e.g. public header use begin_export/end_export to export a bunch of headers),
for these public headers, missing export pragmas is hard to justify the purpose.

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

231

nit: fix the format, or just use a string.

232

nit: add a trailing comment // line 2, to make the L243 clearer.

[kadircet]

thanks a lot for the review!

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

100

But now clangd is diverging from the clang-include-cleaner tool.

And I was trying to make the point that we're already not trying to avoid that, deliberately. As our use definitions & objectives around automation are drastically different.

For interactive tools (like clangd), I actually think in addition to that, clangd should tell the user that they are missing an IWYU export pragma here, and suggest a FIXIT (but that's probably another thread).

I think there's harm rather than value in enabling people to introduce annotations, especially when they're not needed. Happy to discuss this more.

I think private=>public is 1:1, but public=>private is not always 1:1

There's no matching like public "header" => private "header". The mapping goes from "an include in a header" to "a private header", which is still 1:1 and I believe even in that case it's obvious (regular case for an umbrella header)

[kadircet]

• address comments

clang-tools-extra/clangd/IncludeCleaner.cpp

if (Filter(NormalizedPath))
       return true;
   }
   return false;
 }
 
 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.
   if (Inc.Written.front() == '<') {
     if (AnalyzeStdlib && tooling::stdlib::Header::named(Inc.Written))
       return true;
     return false;
   }
   assert(Inc.HeaderID);
   auto HID = static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID);
   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(
           &FE->getFileEntry())) {
     dlog("{0} doesn't have header guard and will not be considered unused",
          FE->getName());
     return false;
   }

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

 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cstddef>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::AllOf;
 using ::testing::ElementsAre;
TEST(IncludeCleaner, NoDiagsForObjC) {
   Config Cfg;
 
   Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict;
   Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict;
   WithContextValue Ctx(Config::Key, std::move(Cfg));
   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

walkUsed(ASTRoots, MacroRefs, PI, SM,
                }
              }
              if (!Satisfied && !Providers.empty() &&
                  Ref.RT == RefType::Explicit)
                Missing.insert(spellHeader(Providers.front(), HS, MainFile));
            });
 
   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;
+      }
+    }
+  }
   for (llvm::StringRef S : Missing.keys())
     Results.Missing.push_back(S.str());
   llvm::sort(Results.Missing);
   return Results;
 }
 
 std::string fixIncludes(const AnalysisResults &Results, llvm::StringRef Code,
                         const format::FormatStyle &Style) {

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

public:
       }
 
       RecordedPP &PP;
       PragmaIncludes Π
     };
     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());
 
     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) {
   llvm::StringRef Code = R"cpp(
 #include "a.h"
 #include "b.h"
 #include <c.h>
 )cpp";
 
   Includes Inc;

clang/include/clang/Testing/TestAST.h

struct TestInputs {
 
   /// Extra argv to pass to clang -cc1.
   std::vector<std::string> ExtraArgs = {};
 
   /// Extra virtual files that are available to be #included.
   /// 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().
   bool ErrorOK = false;
 
   /// The action used to parse the code.
   /// By default, a SyntaxOnlyAction is used.
   std::function<std::unique_ptr<FrontendAction>()> MakeAction;

clang/lib/Testing/TestAST.cpp

 #include "clang/Basic/LangOptions.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/TextDiagnostic.h"
 #include "clang/Testing/CommandLineArgs.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/VirtualFileSystem.h"
 
 #include "gtest/gtest.h"
+#include <string>
 
 namespace clang {
 namespace {
 
 // Captures diagnostics into a vector, optionally reporting errors to gtest.
 class StoreDiagnostics : public DiagnosticConsumer {
   std::vector<StoredDiagnostic> &Out;
   bool ReportErrors;
TestAST::TestAST(const TestInputs &In) {
 
   // Parse cc1 argv, (typically [-std=c++20 input.cc]) into CompilerInvocation.
   std::vector<const char *> Argv;
   std::vector<std::string> LangArgs = getCC1ArgsForTesting(In.Language);
   for (const auto &S : LangArgs)
     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,
                                           Clang->getDiagnostics(), "clang")) {
     ADD_FAILURE() << "Failed to create invocation";
     return;
   }
   assert(!Clang->getInvocation().getFrontendOpts().DisableFree);