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

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.

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

diff --git a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
index 1eff19c..a1d9d3b 100644 (file)
--- a/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
+++ b/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
@@ -96,7 +96,7 @@ 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()))
@@ -186,10 +186,12 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
     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));
@@ -205,6 +207,7 @@ llvm::SmallVector<Hinted<Header>> findHeaders(const SymbolLocation &Loc,
       // Walkup the include stack for non self-contained headers.
       FID = SM.getDecomposedIncludedLoc(FID).first;
       FE = SM.getFileEntryForID(FID);
+      IsOrigin = false;
     }
     return Results;
   }

diff --git a/clang-tools-extra/include-cleaner/lib/TypesInternal.h b/clang-tools-extra/include-cleaner/lib/TypesInternal.h
index 09a3933..e9f8689 100644 (file)
--- a/clang-tools-extra/include-cleaner/lib/TypesInternal.h
+++ b/clang-tools-extra/include-cleaner/lib/TypesInternal.h
@@ -63,17 +63,20 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &);
 /// 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();
@@ -86,6 +89,11 @@ template <typename T> struct Hinted : public T {
   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

diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index 5b5f77b..b6521d5 100644 (file)
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -422,8 +422,12 @@ TEST(WalkUsed, FilterRefsNotSpelledInMainFile) {
   }
 }
 
+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);
   };

diff --git a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
index 964f4c6..9f9ac11 100644 (file)
--- a/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp
@@ -8,17 +8,21 @@
 
 #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 {
@@ -253,11 +257,11 @@ TEST_F(FindHeadersTest, PublicHeaderHint) {
   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) {
@@ -269,11 +273,12 @@ TEST_F(FindHeadersTest, PreferredHeaderHint) {
   )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"),
+      UnorderedElementsAre(
+          HintedHeader(physicalHeader("private.h"), Hints::OriginHeader),
+          HintedHeader(Header("\"public.h\""),
+                       Hints::PreferredHeader | Hints::PublicHeader)));
 }
 
 class HeadersForSymbolTest : public FindHeadersTest {
@@ -339,11 +344,12 @@ TEST_F(HeadersForSymbolTest, RankByName) {
 }
 
 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;
@@ -352,11 +358,15 @@ TEST_F(HeadersForSymbolTest, Ranking) {
     // 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")));
 }
 
@@ -424,6 +434,24 @@ TEST_F(HeadersForSymbolTest, PreferExporterOfPrivate) {
                                            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"
@@ -496,6 +524,5 @@ TEST_F(HeadersForSymbolTest, StandardHeaders) {
                            tooling::stdlib::Header::named("<assert.h>")));
 }
 
-
 } // namespace
 } // namespace clang::include_cleaner