[llvm-branch-commits] [clang-tools-extra] cf39bdb - [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via llvm-branch-commits]

Author: Kirill Bobyrev
Date: 2020-11-23T11:42:56+01:00
New Revision: cf39bdb49086350e7178a0a058273907d180e809

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

LOG: [clangd] Implement Decl canonicalization rules for rename

This patch introduces new canonicalization rules which are used for AST-based
rename in Clangd. By comparing two canonical declarations of inspected nodes,
Clangd determines whether both of them belong to the same entity user would
like to rename. Such functionality is relatively concise compared to the
Clang-Rename API that is used right now. It also helps to overcome the
limitations that Clang-Rename originally had and helps to eliminate several
classes of bugs.

Clangd AST-based rename currently relies on Clang-Rename which has design
limitations and also lacks some features. This patch breaks this dependency and
significantly reduces the amount of code to maintain (Clang-Rename is ~2000 LOC,
this patch is just <30 LOC of replacement code).

We eliminate technical debt by simultaneously

* Maintaining feature parity and ensuring no regressions
* Opening a straightforward path to improving existing rename bugs
* Making it possible to add more capabilities to rename feature which would not
  be possible with Clang-Rename

Reviewed By: hokein

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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 33a9c845beaa..d10a7a4574ba 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,58 @@ llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
   return OtherFile;
 }
 
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Template's canonical decl is the templated declaration (i.e.
+//   ClassTemplateDecl is canonicalized to its child CXXRecordDecl,
+//   FunctionTemplateDecl - to child FunctionDecl)
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl because we want to rename both type name and its ctor/dtor.
+// - All specializations are canonicalized to the primary template. For example:
+//
+//    template <typename T, int U>
+//    bool Foo = true; (1)
+//
+//    template <typename T>
+//    bool Foo<T, 0> = true; (2)
+//
+//    template <>
+//    bool Foo<int, 0> = true; (3)
+//
+// Here, both partial (2) and full (3) specializations are canonicalized to (1)
+// which ensures all three of them are renamed.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  if (const auto *VarTemplate = dyn_cast<VarTemplateSpecializationDecl>(D))
+    return canonicalRenameDecl(
+        VarTemplate->getSpecializedTemplate()->getTemplatedDecl());
+  if (const auto *Template = dyn_cast<TemplateDecl>(D))
+    if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
+      return canonicalRenameDecl(TemplatedDecl);
+  if (const auto *ClassTemplateSpecialization =
+          dyn_cast<ClassTemplateSpecializationDecl>(D))
+    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);
+    // 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 dyn_cast<NamedDecl>(Method->getCanonicalDecl());
+  }
+  if (const auto *Function = dyn_cast<FunctionDecl>(D))
+    if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
+      return canonicalRenameDecl(Template);
+  return dyn_cast<NamedDecl>(D->getCanonicalDecl());
+}
+
 llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
                                                SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +142,7 @@ llvm::DenseSet<const NamedDecl *> locateDeclAt(ParsedAST &AST,
   for (const NamedDecl *D :
        targetDecl(SelectedNode->ASTNode,
                   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-    // Get to CXXRecordDecl from constructor or destructor.
-    D = tooling::getCanonicalSymbolDeclaration(D);
-    Result.insert(D);
+    Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -226,19 +275,8 @@ llvm::Error makeError(ReasonToReject Reason) {
 std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
                                                       const NamedDecl &ND) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-      ND.getDescribedTemplate() ? ND.getDescribedTemplate() : &ND;
-  std::vector<std::string> RenameUSRs =
-      tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet<SymbolID> TargetIDs;
-  for (auto &USR : RenameUSRs)
-    TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl(&ND) == &ND &&
+         "ND should be already canonicalized.");
 
   std::vector<SourceLocation> Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -246,11 +284,11 @@ std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
       if (Ref.Targets.empty())
         return;
       for (const auto *Target : Ref.Targets) {
-        auto ID = getSymbolID(Target);
-        if (!ID || TargetIDs.find(ID) == TargetIDs.end())
+        if (canonicalRenameDecl(Target) == &ND) {
+          Results.push_back(Ref.NameLoc);
           return;
+        }
       }
-      Results.push_back(Ref.NameLoc);
     });
   }
 
@@ -557,8 +595,7 @@ llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
     return makeError(ReasonToReject::NoSymbolFound);
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
-  const auto &RenameDecl =
-      llvm::cast<NamedDecl>(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
+  const auto &RenameDecl = **DeclsUnderCursor.begin();
   if (RenameDecl.getName() == RInputs.NewName)
     return makeError(ReasonToReject::SameName);
   auto Invalid = checkName(RenameDecl, RInputs.NewName);