[PATCH] D60827: [rename] Deduplicate symbol occurrences

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: arphaman, kadircet.
Herald added subscribers: dexonsmith, jkorous.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60827

Files:
  clang-tools-extra/unittests/clangd/ClangdTests.cpp
  clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp


Index: clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===================================================================
--- clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -79,8 +79,8 @@
 
   // 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:
@@ -95,9 +95,20 @@
 
     // 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;
Index: clang-tools-extra/unittests/clangd/ClangdTests.cpp
===================================================================
--- clang-tools-extra/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/unittests/clangd/ClangdTests.cpp
@@ -1157,6 +1157,33 @@
                                 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60827.195573.patch
Type: text/x-patch
Size: 2855 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190417/98f27722/attachment.bin>