[rename] Deduplicate symbol occurrences

[hokein]

Summary

SymbolOccurrences is not guaranteed to be unique -- as some parts of the
AST may get traversed twice, we may add duplicated occurrences, we
should deduplicate them.

[kadircet]

Thanks

clang-tools-extra/unittests/clangd/ClangdTests.cpp

1160

maybe put the test under clang/unittests/Rename?

[hokein]

clang-tools-extra/unittests/clangd/ClangdTests.cpp

1160

I considered it before, but it is not trivial to add the test there unfortunately :( I'd keep the test as-is.

[kadircet]

Note that the bug has been fixed with D61596 incidentally

clang-tools-extra/unittests/clangd/ClangdTests.cpp

TEST_F(ClangdVFSTest, FallbackWhenWaitingForCompileCommand) {
   ASSERT_TRUE(Server.blockUntilIdleForTest());
   EXPECT_THAT(cantFail(runCodeComplete(Server, FooCpp, Code.point(),
                                        clangd::CodeCompleteOptions()))
                   .Completions,
               ElementsAre(AllOf(Field(&CodeCompletion::Name, "xyz"),
                                 Field(&CodeCompletion::Scope, "ns::"))));
 }
 
+TEST_F(ClangdVFSTest, NoDuplicatedTextEditsOnRename) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Annotations Code(R"cpp(
+    struct [[Foo]] {};
+    [[Foo]] test() {
+     [[F^oo]] x;
+     return x;
+    }
+  )cpp");
+
+  runAddDocument(Server, FooCpp, Code.code());
+
+  auto TextEdits =
+      llvm::cantFail(runRename(Server, FooCpp, Code.point(), "new_name"));
+  std::vector<Range> ReplacementRanges;
+  for (const auto& TestEdit : TextEdits)
+    ReplacementRanges.push_back(TestEdit.range);
+  EXPECT_THAT(ReplacementRanges,
+              testing::UnorderedElementsAreArray(Code.ranges()));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

if (USRSet.find(getUSRForDecl(ND)) != USRSet.end()) {
         Loc = SM.getSpellingLoc(Loc);
       checkAndAddLocation(Loc);
     }
     return true;
   }
 
   // Non-visitors:
 
-  /// Returns a set of unique symbol occurrences. Duplicate or
-  /// overlapping occurrences are erroneous and should be reported!
+  /// Returns a set of unique symbol occurrences. Overlapping occurrences are
+  /// erroneous and should be reported!
   SymbolOccurrences takeOccurrences() { return std::move(Occurrences); }
 
 private:
   void checkAndAddLocation(SourceLocation Loc) {
     const SourceLocation BeginLoc = Loc;
     const SourceLocation EndLoc = Lexer::getLocForEndOfToken(
         BeginLoc, 0, Context.getSourceManager(), Context.getLangOpts());
     StringRef TokenName =
         Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
                              Context.getSourceManager(), Context.getLangOpts());
     size_t Offset = TokenName.find(PrevName.getNamePieces()[0]);
 
     // The token of the source location we find actually has the old
     // name.
-    if (Offset != StringRef::npos)
-      Occurrences.emplace_back(PrevName, SymbolOccurrence::MatchingSymbol,
-                               BeginLoc.getLocWithOffset(Offset));
+    if (Offset != StringRef::npos) {
+      SymbolOccurrence NewOccurrence = {PrevName,
+                                        SymbolOccurrence::MatchingSymbol,
+                                        BeginLoc.getLocWithOffset(Offset)};
+      // Don't insert duplicated occurrences.
+      auto It = llvm::find_if(
+          Occurrences, [&NewOccurrence](const SymbolOccurrence &L) {
+            return std::make_pair(L.getKind(), L.getNameRanges().vec()) ==
+                   std::make_pair(NewOccurrence.getKind(),
+                                  NewOccurrence.getNameRanges().vec());
+          });
+      if (It == Occurrences.end())
+        Occurrences.push_back(std::move(NewOccurrence));
+    }
   }
 
   const std::set<std::string> USRSet;
   const SymbolName PrevName;
   SymbolOccurrences Occurrences;
   const ASTContext &Context;
 };