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 different we don't care about the ordering. + return true; + }); + MissingIncludes.erase(llvm::unique(MissingIncludes), MissingIncludes.end()); std::vector 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) {