[clangd] Fix non-idempotent cases of canonicalRenameDecl()

[sammccall]

Summary

The simplest way to ensure full canonicalization is to canonicalize
recursively in most cases.

This fixes an assertion failure and presumably correctness bugs.

It does show up that D132797's index-based virtual method renames doesn't handle
templates well (the AST behavior is different and IMO better).
We could choose to disable in this case or change the index behavior,
but this patch doesn't do either.

[ilya-biryukov]

LGTM to unbreak clangd. Agree that a more thorough look at this is needed.
Maybe add a bug to track this?

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

1521

Quick question to help better understand our current behavior. Not requesting to change anything here, just wanted to make sure what we're doing now.

If we run the rename inside the primary template itself, are we going to rename the use in Bar?
I suspect the answer is "yes" because it's in the same file, so we get it from the AST and not from the index. Just to make sure.

[sammccall]

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

1521

Yes, I just checked - this example works in both directions. And indeed it's because the index isn't involved.

Filed https://github.com/clangd/clangd/issues/1325 for the index issue.

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

return canonicalRenameDecl(
         ClassTemplateSpecialization->getSpecializedTemplate()
             ->getTemplatedDecl());
   if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
     if (Method->getDeclKind() == Decl::Kind::CXXConstructor ||
         Method->getDeclKind() == Decl::Kind::CXXDestructor)
       return canonicalRenameDecl(Method->getParent());
     if (const FunctionDecl *InstantiatedMethod =
             Method->getInstantiatedFromMemberFunction())
-      Method = cast<CXXMethodDecl>(InstantiatedMethod);
+      return canonicalRenameDecl(InstantiatedMethod);
     // FIXME(kirillbobyrev): For virtual methods with
     // size_overridden_methods() > 1, this will not rename all functions it
     // overrides, because this code assumes there is a single canonical
     // declaration.
-    while (Method->isVirtual() && Method->size_overridden_methods())
-      Method = *Method->overridden_methods().begin();
-    return Method->getCanonicalDecl();
+    if (Method->isVirtual() && Method->size_overridden_methods())
+      return canonicalRenameDecl(*Method->overridden_methods().begin());
   }
   if (const auto *Function = dyn_cast<FunctionDecl>(D))
     if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
       return canonicalRenameDecl(Template);
   if (const auto *Field = dyn_cast<FieldDecl>(D)) {
     // This is a hacky way to do something like
     // CXXMethodDecl::getInstantiatedFromMemberFunction for the field because
     // Clang AST does not store relevant information about the field that is
     // instantiated.
     const auto *FieldParent =
         dyn_cast_or_null<CXXRecordDecl>(Field->getParent());
     if (!FieldParent)
       return Field->getCanonicalDecl();
     FieldParent = FieldParent->getTemplateInstantiationPattern();
     // Field is not instantiation.
     if (!FieldParent || Field->getParent() == FieldParent)
       return Field->getCanonicalDecl();
     for (const FieldDecl *Candidate : FieldParent->fields())
       if (Field->getDeclName() == Candidate->getDeclName())
         return Candidate->getCanonicalDecl();
     elog("FieldParent should have field with the same name as Field.");
   }
   if (const auto *VD = dyn_cast<VarDecl>(D)) {
     if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
-      VD = OriginalVD;
-    return VD->getCanonicalDecl();
+      return canonicalRenameDecl(OriginalVD);
   }
   return dyn_cast<NamedDecl>(D->getCanonicalDecl());
 }
 
 llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
                                                SourceLocation TokenStartLoc) {
   unsigned Offset =
       AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;

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

llvm::StringRef FooCC;
                   Derived2* d2, NotDerived* nd) {
           b->[[foo]]();
           d1->[[foo]]();
           d2->[[foo]]();
           nd->foo();
         }
       )cpp",
       },
-      // FIXME: triggers an assertion failure due to a bug in canonicalization.
-      // See https://reviews.llvm.org/D132797
-#if 0
       {
           // virtual templated method
           R"cpp(
         template <typename> class Foo { virtual void [[m]](); };
         class Bar : Foo<int> { void [[^m]]() override; };
-      )cpp", ""
+      )cpp",
+          R"cpp(
+          #include "foo.h"
+
+          template<typename T> void Foo<T>::[[m]]() {}
+          // FIXME: not renamed as the index doesn't see this as an override of
+          // the canonical Foo<T>::m().
+          // https://github.com/clangd/clangd/issues/1325
+          class Baz : Foo<float> { void m() override; };
+        )cpp"
       },
-#endif
       {
           // rename on constructor and destructor.
           R"cpp(
         class [[Foo]] {
           [[^Foo]]();
           ~[[Foo^]]();
         };
       )cpp",