[include-cleaner] Add a signal to down-rank exporting headers

[kadircet]

Summary

Currently exporter can have same relevance signals as the origin header
when name match signals don't trigger.
This patch introduces a tie braker signal to boost origin headers in
such cases, this is deliberately introduced with lower significance than
public-ness to make sure we still prefer a public-exporter instead of a
private-origin header.

[hokein]

Thanks!

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

Hints isPublicHeader(const FileEntry *FE, const PragmaIncludes &PI) {
   return Hints::PublicHeader;
 }
 
 llvm::SmallVector<Hinted<Header>>
 hintedHeadersForStdHeaders(llvm::ArrayRef<tooling::stdlib::Header> Headers,
                            const SourceManager &SM, const PragmaIncludes *PI) {
   llvm::SmallVector<Hinted<Header>> Results;
   for (const auto &H : Headers) {
-    Results.emplace_back(H, Hints::PublicHeader);
+    Results.emplace_back(H, Hints::PublicHeader | Hints::OriginHeader);
     if (!PI)
       continue;
     for (const auto *Export : PI->getExporters(H, SM.getFileManager()))
       Results.emplace_back(Header(Export), isPublicHeader(Export, *PI));
   }
   // StandardLibrary returns headers in preference order, so only mark the
   // first.
   if (!Results.empty())
llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
   llvm::SmallVector<Hinted<Header>> Results;
   switch (Loc.kind()) {
   case SymbolLocation::Physical: {
     FileID FID = SM.getFileID(SM.getExpansionLoc(Loc.physical()));
     const FileEntry *FE = SM.getFileEntryForID(FID);
     if (!FE)
       return {};
     if (!PI)
-      return {{FE, Hints::PublicHeader}};
+      return {{FE, Hints::PublicHeader | Hints::OriginHeader}};
+    bool IsOrigin = true;
     while (FE) {
-      Hints CurrentHints = isPublicHeader(FE, *PI);
-      Results.emplace_back(FE, CurrentHints);
+      Results.emplace_back(FE,
+                           isPublicHeader(FE, *PI) |
+                               (IsOrigin ? Hints::OriginHeader : Hints::None));
       // FIXME: compute transitive exporter headers.
       for (const auto *Export : PI->getExporters(FE, SM.getFileManager()))
         Results.emplace_back(Export, isPublicHeader(Export, *PI));
 
       if (auto Verbatim = PI->getPublic(FE); !Verbatim.empty()) {
         Results.emplace_back(Verbatim,
                              Hints::PublicHeader | Hints::PreferredHeader);
         break;
       }
       if (PI->isSelfContained(FE) || FID == SM.getMainFileID())
         break;
 
       // Walkup the include stack for non self-contained headers.
       FID = SM.getDecomposedIncludedLoc(FID).first;
       FE = SM.getFileEntryForID(FID);
+      IsOrigin = false;
     }
     return Results;
   }
   case SymbolLocation::Standard: {
     return hintedHeadersForStdHeaders(Loc.standard().headers(), SM, PI);
   }
   }
   llvm_unreachable("unhandled SymbolLocation kind!");

clang-tools-extra/include-cleaner/lib/TypesInternal.h

 /// to merge hints. These hints are merged by taking the union of all the
 /// properties along all the paths. We choose the boolean sense accordingly,
 /// e.g. "Public" rather than "Private", because a header is good if it provides
 /// any public definition, even if it also provides private ones.
 ///
 /// Hints are sorted in ascending order of relevance.
 enum class Hints : uint8_t {
   None = 0x00,
+  /// Symbol is directly originating from this header, rather than being
+  /// exported or included transitively.
+  OriginHeader = 1 << 0,
   /// Provides a generally-usable definition for the symbol. (a function decl,
   /// or class definition and not a forward declaration of a template).
-  CompleteSymbol = 1 << 0,
+  CompleteSymbol = 1 << 1,
   /// Symbol is provided by a public file. Only absent in the cases where file
   /// is explicitly marked as such, non self-contained or IWYU private
   /// pragmas.
-  PublicHeader = 1 << 1,
+  PublicHeader = 1 << 2,
   /// Header providing the symbol is explicitly marked as preferred, with an
   /// IWYU private pragma that points at this provider or header and symbol has
   /// ~the same name.
-  PreferredHeader = 1 << 2,
+  PreferredHeader = 1 << 3,
   LLVM_MARK_AS_BITMASK_ENUM(PreferredHeader),
 };
 LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
 /// A wrapper to augment values with hints.
 template <typename T> struct Hinted : public T {
   Hints Hint;
   Hinted(T &&Wrapped, Hints H) : T(std::move(Wrapped)), Hint(H) {}
 
   /// Since hints are sorted by relevance, use it directly.
   bool operator<(const Hinted<T> &Other) const {
     return static_cast<int>(Hint) < static_cast<int>(Other.Hint);
   }
+
+  friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+                                       const Hinted<T> &H) {
+    return OS << static_cast<int>(H.Hint) << " - " << static_cast<T>(H);
+  }
 };
 
 } // namespace clang::include_cleaner
 
 #endif

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

if (RefLoc.isValid()) {
                                 spelledAt(MainFID, Main.point("spell"), &SM)))
           << T.Main.str();
     } else {
       EXPECT_THAT(Main.points(), testing::IsEmpty());
     }
   }
 }
 
+struct Tag {
+  friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Tag &T) {
+    return OS << "Anon Tag";
+  }
+};
 TEST(Hints, Ordering) {
-  struct Tag {};
   auto Hinted = [](Hints Hints) {
     return clang::include_cleaner::Hinted<Tag>({}, Hints);
   };
   EXPECT_LT(Hinted(Hints::None), Hinted(Hints::CompleteSymbol));
   EXPECT_LT(Hinted(Hints::CompleteSymbol), Hinted(Hints::PublicHeader));
   EXPECT_LT(Hinted(Hints::PublicHeader), Hinted(Hints::PreferredHeader));
   EXPECT_LT(Hinted(Hints::CompleteSymbol | Hints::PublicHeader),
             Hinted(Hints::PreferredHeader));

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

 //===--- FindHeadersTest.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AnalysisInternal.h"
 #include "TypesInternal.h"
+#include "clang-include-cleaner/Analysis.h"
 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Testing/TestAST.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <cassert>
 #include <memory>
 
 namespace clang::include_cleaner {
 namespace {
 using testing::ElementsAre;
 using testing::UnorderedElementsAre;
 
 std::string guard(llvm::StringRef Code) {
TEST_F(FindHeadersTest, PublicHeaderHint) {
   )cpp");
   Inputs.ExtraFiles["private.inc"] = "";
   buildAST();
   // Non self-contained files and headers marked with IWYU private pragma
   // shouldn't have PublicHeader hint.
   EXPECT_THAT(
       findHeaders("private.inc"),
       UnorderedElementsAre(
-          HintedHeader(physicalHeader("private.inc"), Hints::None),
+          HintedHeader(physicalHeader("private.inc"), Hints::OriginHeader),
           HintedHeader(physicalHeader("public.h"), Hints::PublicHeader)));
   EXPECT_THAT(findHeaders("private.h"),
-              UnorderedElementsAre(
-                  HintedHeader(physicalHeader("private.h"), Hints::None)));
+              UnorderedElementsAre(HintedHeader(physicalHeader("private.h"),
+                                                Hints::OriginHeader)));
 }
 
 TEST_F(FindHeadersTest, PreferredHeaderHint) {
   Inputs.Code = R"cpp(
     #include "private.h"
   )cpp";
   Inputs.ExtraFiles["private.h"] = guard(R"cpp(
     // IWYU pragma: private, include "public.h"
   )cpp");
   buildAST();
   // Headers explicitly marked should've preferred signal.
-  EXPECT_THAT(findHeaders("private.h"),
-              UnorderedElementsAre(
-                  HintedHeader(physicalHeader("private.h"), Hints::None),
-                  HintedHeader(Header("\"public.h\""),
-                               Hints::PreferredHeader | Hints::PublicHeader)));
+  EXPECT_THAT(
+      findHeaders("private.h"),
+          HintedHeader(physicalHeader("private.h"), Hints::OriginHeader),
 }
 
 class HeadersForSymbolTest : public FindHeadersTest {
 protected:
   llvm::SmallVector<Header> headersFor(llvm::StringRef Name) {
     struct Visitor : public RecursiveASTVisitor<Visitor> {
       const NamedDecl *Out = nullptr;
       llvm::StringRef Name;
Inputs.ExtraFiles["bar.h"] = guard(R"cpp(
     void foo();
   )cpp");
   buildAST();
   EXPECT_THAT(headersForFoo(),
               ElementsAre(physicalHeader("bar.h"), physicalHeader("fox.h")));
 }
 
 TEST_F(HeadersForSymbolTest, Ranking) {
-  // Sorting is done over (canonical, public, complete) triplet.
+  // Sorting is done over (canonical, public, complete, origin)-tuple.
   Inputs.Code = R"cpp(
     #include "private.h"
     #include "public.h"
     #include "public_complete.h"
+    #include "exporter.h"
   )cpp";
   Inputs.ExtraFiles["public.h"] = guard(R"cpp(
     struct foo;
   )cpp");
   Inputs.ExtraFiles["private.h"] = guard(R"cpp(
     // IWYU pragma: private, include "canonical.h"
     struct foo;
   )cpp");
+  Inputs.ExtraFiles["exporter.h"] = guard(R"cpp(
+  #include "private.h" // IWYU pragma: export
+  )cpp");
   Inputs.ExtraFiles["public_complete.h"] = guard("struct foo {};");
   buildAST();
   EXPECT_THAT(headersForFoo(), ElementsAre(Header("\"canonical.h\""),
                                            physicalHeader("public_complete.h"),
                                            physicalHeader("public.h"),
+                                           physicalHeader("exporter.h"),
                                            physicalHeader("private.h")));
 }
 
 TEST_F(HeadersForSymbolTest, PreferPublicOverComplete) {
   Inputs.Code = R"cpp(
     #include "complete_private.h"
     #include "public.h"
   )cpp";
TEST_F(HeadersForSymbolTest, PreferExporterOfPrivate) {
   Inputs.ExtraFiles["exporter.h"] = guard(R"cpp(
     #include "private.h" // IWYU pragma: export
   )cpp");
   buildAST();
   EXPECT_THAT(headersForFoo(), ElementsAre(physicalHeader("exporter.h"),
                                            physicalHeader("private.h")));
 }
 
+TEST_F(HeadersForSymbolTest, ExporterIsDownRanked) {
+  Inputs.Code = R"cpp(
+    #include "exporter.h"
+    #include "zoo.h"
+  )cpp";
+  // Deliberately named as zoo to make sure it doesn't get name-match boost and
+  // also gets lexicographically bigger order than "exporter".
+  Inputs.ExtraFiles["zoo.h"] = guard(R"cpp(
+    struct foo {};
+  )cpp");
+  Inputs.ExtraFiles["exporter.h"] = guard(R"cpp(
+    #include "zoo.h" // IWYU pragma: export
+  )cpp");
+  buildAST();
+  EXPECT_THAT(headersForFoo(), ElementsAre(physicalHeader("zoo.h"),
+                                           physicalHeader("exporter.h")));
+}
+
 TEST_F(HeadersForSymbolTest, PreferPublicOverNameMatchOnPrivate) {
   Inputs.Code = R"cpp(
     #include "foo.h"
   )cpp";
   Inputs.ExtraFiles["foo.h"] = guard(R"cpp(
     // IWYU pragma: private, include "public.h"
     struct foo {};
   )cpp");
TEST_F(HeadersForSymbolTest, StandardHeaders) {
   buildAST();
   EXPECT_THAT(
       headersFor("assert"),
       // Respect the ordering from the stdlib mapping.
       UnorderedElementsAre(tooling::stdlib::Header::named("<cassert>"),
                            tooling::stdlib::Header::named("<assert.h>")));
 }
 
-
 } // namespace
 } // namespace clang::include_cleaner