[PATCH] D149165: [clangd] Deduplicate missing-include findings

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: VitaNuo.
Herald added a subscriber: arphaman.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149165

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp


Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -403,15 +403,34 @@
   ParsedAST AST = TU.build();
 
   auto Findings = computeIncludeCleanerFindings(AST).MissingIncludes;
-  // FIXME: Deduplicate references resulting from expansion of the same macro in
-  // multiple places.
-  EXPECT_THAT(Findings, testing::SizeIs(2));
+  EXPECT_THAT(Findings, testing::SizeIs(1));
   auto RefRange = Findings.front().SymRefRange;
   auto &SM = AST.getSourceManager();
   EXPECT_EQ(RefRange.file(), SM.getMainFileID());
   // FIXME: Point at the spelling location, rather than the include.
   EXPECT_EQ(halfOpenToRange(SM, RefRange.toCharRange(SM)), MainFile.range());
-  EXPECT_EQ(RefRange, Findings[1].SymRefRange);
+}
+
+TEST(IncludeCleaner, MissingIncludesAreUnique) {
+  Annotations MainFile(R"cpp(
+    #include "all.h"
+    FOO([[Foo]]);)cpp");
+
+  TestTU TU;
+  TU.AdditionalFiles["foo.h"] = guard("struct Foo {};");
+  TU.AdditionalFiles["all.h"] = guard(R"(
+    #include "foo.h"
+    #define FOO(X) X y; X z)");
+
+  TU.Code = MainFile.code();
+  ParsedAST AST = TU.build();
+
+  auto Findings = computeIncludeCleanerFindings(AST).MissingIncludes;
+  EXPECT_THAT(Findings, testing::SizeIs(1));
+  auto RefRange = Findings.front().SymRefRange;
+  auto &SM = AST.getSourceManager();
+  EXPECT_EQ(RefRange.file(), SM.getMainFileID());
+  EXPECT_EQ(halfOpenToRange(SM, RefRange.toCharRange(SM)), MainFile.range());
 }
 
 TEST(IncludeCleaner, NoCrash) {
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -40,6 +40,7 @@
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/GenericUniformityImpl.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/STLFunctionalExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -415,6 +416,20 @@
             Ref.Target, TouchingTokens.back().range(SM), Providers};
         MissingIncludes.push_back(std::move(DiagInfo));
       });
+  // Put possibly equal diagnostics together for deduplication.
+  // The duplicates might be from macro arguments that get expanded multiple
+  // times.
+  llvm::stable_sort(MissingIncludes, [](const MissingIncludeDiagInfo &LHS,
+                                        const MissingIncludeDiagInfo &RHS) {
+    if (LHS.Symbol == RHS.Symbol) {
+      // We can get away just by comparing the offsets as all the ranges are in
+      // main file.
+      return LHS.SymRefRange.beginOffset() < RHS.SymRefRange.beginOffset();
+    }
+    // If symbols are different we don't care about the ordering.
+    return true;
+  });
+  MissingIncludes.erase(llvm::unique(MissingIncludes), MissingIncludes.end());
   std::vector<const Inclusion *> UnusedIncludes =
       getUnused(AST, Used, /*ReferencedPublicHeaders*/ {});
   return {std::move(UnusedIncludes), std::move(MissingIncludes)};


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D149165.516831.patch
Type: text/x-patch
Size: 3200 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230425/6923663c/attachment.bin>