[PATCH] D135544: [clangd] Rename: Allow renaming using declaration introducing multiple symbols [3/3]

[Tom Praschan via Phabricator via cfe-commits]

tom-anders created this revision.
tom-anders added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
tom-anders updated this revision to Diff 466393.
tom-anders added a comment.
tom-anders edited the summary of this revision.
tom-anders published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

clang-format


I needed to add a helper function to make sure that this is not reported as AmbiguousSymbol.
I think filterRenameTargets() now technically is not needed anymore at the moment, but dropping the UsingDecl probably is better for performance down the line.
Also, as discussed filterRenameTargets() might be extended with more conditions in the future, so let's keep it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135544

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp


Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -840,6 +840,17 @@
           foo('x');
         }
       )cpp",
+
+      // Renaming a using-declaration that introduces multiple symbols
+      R"cpp(
+        namespace ns { struct [[foo]] {}; int [[foo]](int); char [[foo]](char); }
+        using ns::[[f^oo]];
+        void bar() {
+          [[foo]](1);
+          [[foo]]('x');
+          struct [[foo]] f;
+        }
+      )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
@@ -1085,14 +1096,6 @@
         };
       )cpp",
        "no symbol", false},
-
-      {R"cpp(// FIXME we probably want to rename both overloads here,
-             // but renaming currently assumes there's only a 
-             // single canonical declaration.
-        namespace ns { int foo(int); char foo(char); }
-        using ns::^foo;
-      )cpp",
-       "there are multiple symbols at the given location", !HeaderFile},
   };
 
   for (const auto& Case : Cases) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -737,6 +737,21 @@
                LexedIndex + 1, Fuel, MatchedCB);
 }
 
+/// In general, if we have multiple targets under the cursor we consider them
+/// ambiguous and return an error. However, there are some exceptions.
+bool areRenameTargetsAmbiguous(
+    const llvm::DenseSet<const NamedDecl *> &Targets) {
+  // If the cursor is on a using declaration, we may get multiple targets, e.g.
+  //    namespace ns { void foo(int); void foo(char); }
+  //    using ns::f^oo
+  // We want to rename both declarations here, so allow this.
+  if (llvm::any_of(Targets,
+                   [](const NamedDecl *D) { return llvm::isa<UsingDecl>(D); }))
+    return false;
+
+  return Targets.size() > 1;
+}
+
 } // namespace
 
 llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
@@ -766,12 +781,13 @@
     return makeError(ReasonToReject::UnsupportedSymbol);
 
   auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
-  filterRenameTargets(DeclsUnderCursor);
   if (DeclsUnderCursor.empty())
     return makeError(ReasonToReject::NoSymbolFound);
-  if (DeclsUnderCursor.size() > 1)
+  if (areRenameTargetsAmbiguous(DeclsUnderCursor))
     return makeError(ReasonToReject::AmbiguousSymbol);
 
+  filterRenameTargets(DeclsUnderCursor);
+
   llvm::DenseSet<const NamedDecl *> RenameDecls;
   for (const auto *D : DeclsUnderCursor) {
     auto CD = canonicalRenameDecls(D);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D135544.466393.patch
Type: text/x-patch
Size: 2807 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221009/b7fca6bb/attachment.bin>