[clang-tools-extra] 5f7f484 - [clangd] Fix non-idempotent cases of canonicalRenameDecl()

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2022-10-05T21:22:08+02:00
New Revision: 5f7f4846e826f97c7f298fe419c958398d5a0386

URL: https://github.com/llvm/llvm-project/commit/5f7f4846e826f97c7f298fe419c958398d5a0386
DIFF: https://github.com/llvm/llvm-project/commit/5f7f4846e826f97c7f298fe419c958398d5a0386.diff

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

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.

Differential Revision: https://reviews.llvm.org/D133415

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 4e367898c6bfc..22c72d6a4a6a4 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -100,14 +100,13 @@ const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
       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())
@@ -132,8 +131,7 @@ const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
   }
   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());
 }

diff  --git a/clang-tools-extra/clangd/unittests/RenameTests.cpp b/clang-tools-extra/clangd/unittests/RenameTests.cpp
index 451ec37234690..00104ff470e5d 100644
--- a/clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ b/clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -1515,17 +1515,22 @@ TEST(CrossFileRenameTests, WithUpToDateIndex) {
         }
       )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(