[clang-tools-extra] [IncludeCleaner] Also check for spellings of physical headers (PR #99843)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: kadir çetinkaya (kadircet)

<details>
<summary>Changes</summary>

Some physical headers can have "conflicting" spellings, when same
filename exists under two different include search paths. e.g. <stdlib.h>
can be provided by both standard library and underlying libc headers. In
such scenarios if the usage is from the latter include-search path,
users can't spell it directly.

This patch ensures we also consider spellings of such includes, in
addition to their physical files to prevent conflicting suggestions.


---
Full diff: https://github.com/llvm/llvm-project/pull/99843.diff


4 Files Affected:

- (modified) clang-tools-extra/clangd/IncludeCleaner.cpp (+20) 
- (modified) clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp (+22) 
- (modified) clang-tools-extra/include-cleaner/lib/Analysis.cpp (+14-3) 
- (modified) clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp (+26) 


``````````diff
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index dc5b7ec95db5f..e34706172f0bf 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -401,6 +401,26 @@ computeIncludeCleanerFindings(ParsedAST &AST, bool AnalyzeAngledIncludes) {
             Ref.RT != include_cleaner::RefType::Explicit)
           return;
 
+        // Check if we have any headers with the same spelling, in edge cases
+        // like `#include_next "foo.h"`, the user can't ever include the
+        // physical foo.h, but can have a spelling that refers to it.
+        // We postpone this check because spelling a header for every usage is
+        // expensive.
+        std::string Spelling = include_cleaner::spellHeader(
+            {Providers.front(), AST.getPreprocessor().getHeaderSearchInfo(),
+             MainFile});
+        for (auto *Inc :
+             ConvertedIncludes.match(include_cleaner::Header{Spelling})) {
+          Satisfied = true;
+          auto HeaderID =
+              AST.getIncludeStructure().getID(&Inc->Resolved->getFileEntry());
+          assert(HeaderID.has_value() &&
+                 "ConvertedIncludes only contains resolved includes.");
+          Used.insert(*HeaderID);
+        }
+        if (Satisfied)
+          return;
+
         // We actually always want to map usages to their spellings, but
         // spelling locations can point into preamble section. Using these
         // offsets could lead into crashes in presence of stale preambles. Hence
diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 7027232460354..0ee748c1ed2d0 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -644,6 +644,28 @@ TEST(IncludeCleaner, ResourceDirIsIgnored) {
   EXPECT_THAT(Findings.MissingIncludes, IsEmpty());
 }
 
+TEST(IncludeCleaner, DifferentHeaderSameSpelling) {
+  // `foo` is declared in foo_inner/foo.h, but there's no way to spell it
+  // directly. Make sure we don't generate unusued/missing include findings in
+  // such cases.
+  auto TU = TestTU::withCode(R"cpp(
+    #include <foo.h>
+    void baz() {
+      foo();
+    }
+  )cpp");
+  TU.AdditionalFiles["foo/foo.h"] = guard("#include_next <foo.h>");
+  TU.AdditionalFiles["foo_inner/foo.h"] = guard(R"cpp(
+    void foo();
+  )cpp");
+  TU.ExtraArgs.push_back("-Ifoo");
+  TU.ExtraArgs.push_back("-Ifoo_inner");
+
+  auto AST = TU.build();
+  auto Findings = computeIncludeCleanerFindings(AST);
+  EXPECT_THAT(Findings.UnusedIncludes, IsEmpty());
+  EXPECT_THAT(Findings.MissingIncludes, 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
index f1cd72f877ca2..68fe79d6929f6 100644
--- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp
+++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -105,9 +105,20 @@ analyze(llvm::ArrayRef<Decl *> ASTRoots,
              }
              if (!Satisfied && !Providers.empty() &&
                  Ref.RT == RefType::Explicit &&
-                 !HeaderFilter(Providers.front().resolvedPath()))
-               Missing.insert(spellHeader(
-                   {Providers.front(), PP.getHeaderSearchInfo(), MainFile}));
+                 !HeaderFilter(Providers.front().resolvedPath())) {
+               // Check if we have any headers with the same spelling, in edge
+               // cases like `#include_next "foo.h"`, the user can't ever
+               // include the physical foo.h, but can have a spelling that
+               // refers to it.
+               auto Spelling = spellHeader(
+                   {Providers.front(), PP.getHeaderSearchInfo(), MainFile});
+               for (const Include *I : Inc.match(Header{Spelling})) {
+                 Used.insert(I);
+                 Satisfied = true;
+               }
+               if (!Satisfied)
+                 Missing.insert(std::move(Spelling));
+             }
            });
 
   AnalysisResults Results;
diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
index 6558b68087684..5696c380758f8 100644
--- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
@@ -12,6 +12,7 @@
 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclBase.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/SourceLocation.h"
@@ -296,6 +297,31 @@ TEST_F(AnalyzeTest, ResourceDirIsIgnored) {
   EXPECT_THAT(Results.Missing, testing::IsEmpty());
 }
 
+TEST_F(AnalyzeTest, DifferentHeaderSameSpelling) {
+  Inputs.ExtraArgs.push_back("-Ifoo");
+  Inputs.ExtraArgs.push_back("-Ifoo_inner");
+  // `foo` is declared in foo_inner/foo.h, but there's no way to spell it
+  // directly. Make sure we don't generate unusued/missing include findings in
+  // such cases.
+  Inputs.Code = R"cpp(
+    #include <foo.h>
+    void baz() {
+      foo();
+    }
+  )cpp";
+  Inputs.ExtraFiles["foo/foo.h"] = guard("#include_next <foo.h>");
+  Inputs.ExtraFiles["foo_inner/foo.h"] = guard(R"cpp(
+    void foo();
+  )cpp");
+  TestAST AST(Inputs);
+  std::vector<Decl *> DeclsInTU;
+  for (auto *D : AST.context().getTranslationUnitDecl()->decls())
+    DeclsInTU.push_back(D);
+  auto Results = analyze(DeclsInTU, {}, PP.Includes, &PI, AST.preprocessor());
+  EXPECT_THAT(Results.Unused, testing::IsEmpty());
+  EXPECT_THAT(Results.Missing, testing::IsEmpty());
+}
+
 TEST(FixIncludes, Basic) {
   llvm::StringRef Code = R"cpp(#include "d.h"
 #include "a.h"

``````````

</details>


https://github.com/llvm/llvm-project/pull/99843