[clang-tools-extra] 08f0725 - [clangd] Deduplicate missing-include findings

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2023-04-26T10:33:55+02:00
New Revision: 08f0725a3a5471d16b8a1e9d1d55b9952300ac41

URL: https://github.com/llvm/llvm-project/commit/08f0725a3a5471d16b8a1e9d1d55b9952300ac41
DIFF: https://github.com/llvm/llvm-project/commit/08f0725a3a5471d16b8a1e9d1d55b9952300ac41.diff

LOG: [clangd] Deduplicate missing-include findings

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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index d15dd70efdb70..b88ad0ff5c5b5 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/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 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
             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 
diff erent 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)};

diff  --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
index 39901fda31656..32b7c5444e06f 100644
--- a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -403,15 +403,36 @@ TEST(IncludeCleaner, MacroExpandedThroughIncludes) {
   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"cpp(
+    #include "foo.h"
+    #define FOO(X) X y; X z
+  )cpp");
+
+  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) {