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

[tom-anders]

Summary

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.

[tom-anders]

clang-format

clang-tools-extra/clangd/refactor/Rename.cpp

if (impliesSimpleEdit(IndexedRest.front().start, LexedRest.front().start)) {
     findNearMiss(PartialMatch, IndexedRest.drop_front(), LexedRest.drop_front(),
                  LexedIndex + 1, Fuel, MatchedCB);
     PartialMatch.pop_back();
   }
   findNearMiss(PartialMatch, IndexedRest, LexedRest.drop_front(),
                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) {
   assert(!RInputs.Index == !RInputs.FS &&
          "Index and FS must either both be specified or both null.");
   trace::Span Tracer("Rename flow");
   const auto &Opts = RInputs.Opts;
   ParsedAST &AST = RInputs.AST;
Range CurrentIdentifier = halfOpenToRange(
       SM, CharSourceRange::getCharRange(IdentifierToken->location(),
                                         IdentifierToken->endLocation()));
   // FIXME: Renaming macros is not supported yet, the macro-handling code should
   // be moved to rename tooling library.
   if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
     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);
     RenameDecls.insert(CD.begin(), CD.end());
   }
   
   for (const NamedDecl* D : RenameDecls) {
     auto *ID = D->getIdentifier();

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

llvm::StringRef Tests[] = {
       R"cpp(
         namespace ns { int [[^foo]](int); char foo(char); }
         using ns::[[foo]];
         void f() {
           [[^foo]](42);
           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) {
     SCOPED_TRACE(T);
     Annotations Code(T);
     auto TU = TestTU::withCode(Code.code());
     TU.ExtraArgs.push_back("-xobjective-c++");
     auto AST = TU.build();
Case Cases[] = {
        "Cannot rename symbol: there is no symbol at the given location", false},
       {R"cpp(// Ensure it doesn't associate base specifier with base name.
         /*error-ok*/
         struct A {
           A() : inva^lid(0) {}
         };
       )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) {
     SCOPED_TRACE(Case.Code);
     Annotations T(Case.Code);
     TestTU TU = TestTU::withCode(T.code());
     TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
     if (Case.IsHeaderFile) {