[clangd] Rename constructors and destructors in cross-file case

[kbobyrev]

Summary

• Use ad-hoc Decl canonicalization from Clang-Rename to allow renaming constructors and destructors while using cross-file rename.
• Manually handle the destructor selection
• Add unit tests to prevent regressions and ensure the correct behaviour

[kbobyrev]

Improve wording in the comment I moved.

[kadircet]

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

754

could you also add a case with ~^Foo()

[kbobyrev]

Add another test for ~^Foo and rebase on top of master.

[sammccall]

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

86–87

Braindump from an offline conversation...
This seems like an unfortunate place to put a complicated special case and it doesn't generalize well.

The TokenStartLoc check is a bit unfortunate to begin with - it's subject to all the heuristic-token-rewinding problems we've encountered in the past. Its one advantage over plain selectiontree is that it works at the end of the identifier.

Better approach seems to be:

• we only allow rename targeted at an identifier token
• there can be at most one identifier touching the location (if there's one before and after, they'd be a single identifier)
• so find that identifier spelled token using TokenBuffer
• then look it up as a macro (we get its start location for free)
• if that fails, create a selectiontree for the token's range and rename what we find

I don't think there's a better TokenBuffer API than calling spelledTokens() and binary-searching, at the moment.

This touches more code, but is less special-case-y overall. I'd consider putting the getIdentifierTouching(SourceLocation, TokenBuffer, SourceManager) in SourceCode.h

[kbobyrev]

Need to address the comments.

[kbobyrev]

Address the comments and simplify the code.

[sammccall]

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

91

why is this code being moved?

453

so getting an invalid location from the editor isn't the same thing as the symbol being used outside the file! Here we already have an Error to return.

if (!Offset)
  return Offset.takeError();

but really there's a helper to do this:

SourceLocation Loc = sourceLocationInMainFile(SM, RInputs.Pos);
if (!Loc)
  return Loc.takeError();
const syntax::Token *IdentifierToken = spelledIdentifierTouching(Loc, AST.getTokens());

456

nit: auto* for pointers, but spelling out the type here would be clearer I think

458

this is an invariant of the function we're calling, not this one - no need to document it (next line is good though)

462

This unwrapping doesn't seem necessary, - it doesn't make a difference for macros (which we don't support anyway), nor for selectiontree. Just inline usages of IdentifierToken->location() below?

[kbobyrev]

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

91

So, this code was previously only in the within-file rename path and hence triggering rename from a constructor/destructor-s was only possible there. Moving the code here improves overall "symbol selection" and fixes it for global renames, too.

[sammccall]

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

91

Clangd as a whole actually canonicalizes in the other direction (templatedecl -> templated decl).
So I think this change introduces a bug (the references you seek won't be in the index).

Certainly up for discussion which is better (behavior is historical) but that's a larger change.

96

dyn_cast then unconditionally inserting seems dodgy - if it can be null, it must be checked, and if it can't, it should be plain cast

[sammccall]

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

91

OK, this can definitely fail though - just select 'friend X' in a class body or so. Need to check for ND and bail out.

223

you removed this comment and the fixme, but it still applies?

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

732

nit: put these after "class methods" rather than at the top?

750

4 is too many cases for constructor/destructor.

What about 2: Constructor at start, destructor at end?
Renaming in the middle of the token is covered by existing cases.

[sammccall]

LG but the cast needs to be fixed

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

 #include "ParsedAST.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "index/SymbolCollector.h"
 #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"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include <algorithm>
 
 namespace clang {
 namespace clangd {
llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
                                           SourceLocation TokenStartLoc) {
   unsigned Offset =
       AST.getSourceManager().getDecomposedSpellingLoc(TokenStartLoc).second;
 
   SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
   const SelectionTree::Node *SelectedNode = Selection.commonAncestor();
   if (!SelectedNode)
     return {};
 
-  // If the location points to a Decl, we check it is actually on the name
-  // range of the Decl. This would avoid allowing rename on unrelated tokens.
-  //   ^class Foo {} // SelectionTree returns CXXRecordDecl,
-  //                 // we don't attempt to trigger rename on this position.
-  // FIXME: Make this work on destructors, e.g. "~F^oo()".
-  if (const auto *D = SelectedNode->ASTNode.get<Decl>()) {
-    if (D->getLocation() != TokenStartLoc)
-      return {};
-  }
-
   llvm::DenseSet<const Decl *> Result;
   for (const auto *D :
        targetDecl(SelectedNode->ASTNode,
-                  DeclRelation::Alias | DeclRelation::TemplatePattern))
-    Result.insert(D);
+                  DeclRelation::Alias | DeclRelation::TemplatePattern)) {
+    // 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 maunally.
+    // FIXME: Do proper and well-defined canonicalization.
+    D = D->getDescribedTemplate() ? D->getDescribedTemplate() : D;
+    const auto *ND = llvm::dyn_cast<NamedDecl>(D);
+    // Get to CXXRecordDecl from constructor or destructor.
+    ND = tooling::getCanonicalSymbolDeclaration(ND);
+    Result.insert(ND);
+  }
   return Result;
 }
 
 enum ReasonToReject {
   NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile, // for within-file rename only.
llvm::Error makeError(ReasonToReject Reason) {
   return llvm::make_error<llvm::StringError>(
       llvm::formatv("Cannot rename symbol: {0}", Message(Reason)),
       llvm::inconvertibleErrorCode());
 }
 
 // Return all rename occurrences in the main file.
 std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
                                                       const NamedDecl &ND) {
-  // In theory, locateDeclAt should return the primary template. However, if the
-  // cursor is under the underlying CXXRecordDecl of the ClassTemplateDecl, ND
-  // will be the CXXRecordDecl, for this case, we need to get the primary
-  // template maunally.
-  const auto &RenameDecl =
-      ND.getDescribedTemplate() ? *ND.getDescribedTemplate() : ND;
-  // 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.
-  std::vector<std::string> RenameUSRs = tooling::getUSRsForDeclaration(
-      tooling::getCanonicalSymbolDeclaration(&RenameDecl), AST.getASTContext());
+  std::vector<std::string> RenameUSRs =
+      tooling::getUSRsForDeclaration(&ND, AST.getASTContext());
   llvm::DenseSet<SymbolID> TargetIDs;
   for (auto &USR : RenameUSRs)
     TargetIDs.insert(SymbolID(USR));
 
   std::vector<SourceLocation> Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
     findExplicitReferences(TopLevelDecl, [&](ReferenceLoc Ref) {
       if (Ref.Targets.empty())
if (!Content)
                         Content.getError().message()));
     if (!*Content)
       return llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           llvm::formatv("Got no buffer for file {0}", AbsPath));
 
     return (*Content)->getBuffer().str();
   };
-  SourceLocation SourceLocationBeg = SM.getMacroArgExpandedLocation(
-      getBeginningOfIdentifier(RInputs.Pos, SM, AST.getLangOpts()));
+  // Try to find the tokens adjacent to the cursor position.
+  auto Loc = sourceLocationInMainFile(SM, RInputs.Pos);
+  if (!Loc)
+    return Loc.takeError();
+  const syntax::Token *IdentifierToken =
+      spelledIdentifierTouching(*Loc, AST.getTokens());
+  // Renames should only triggered on identifiers.
+  if (!IdentifierToken)
+    return makeError(ReasonToReject::NoSymbolFound);
   // FIXME: Renaming macros is not supported yet, the macro-handling code should
   // be moved to rename tooling library.
-  if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
+  if (locateMacroAt(IdentifierToken->location(), AST.getPreprocessor()))
     return makeError(ReasonToReject::UnsupportedSymbol);
 
-  auto DeclsUnderCursor = locateDeclAt(AST, SourceLocationBeg);
+  auto DeclsUnderCursor = locateDeclAt(AST, IdentifierToken->location());
   if (DeclsUnderCursor.empty())
     return makeError(ReasonToReject::NoSymbolFound);
   if (DeclsUnderCursor.size() > 1)
     return makeError(ReasonToReject::AmbiguousSymbol);
 
   const auto *RenameDecl = llvm::dyn_cast<NamedDecl>(*DeclsUnderCursor.begin());
   if (!RenameDecl)
     return makeError(ReasonToReject::UnsupportedSymbol);

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

Case Cases[] = {
       {
           R"cpp(
          #define MACRO 1
          int s = MAC^RO;
        )cpp",
           "not a supported kind", HeaderFile, Index},
 
       {
-
           R"cpp(
         struct X { X operator++(int); };
         void f(X x) {x+^+;})cpp",
-          "not a supported kind", HeaderFile, Index},
+          "no symbol", HeaderFile, Index},
 
       {R"cpp(// foo is declared outside the file.
         void fo^o() {}
       )cpp",
        "used outside main file", !HeaderFile /*cc file*/, Index},
 
       {R"cpp(
          // We should detect the symbol is used outside the file from the AST.
TEST(CrossFileRenameTests, WithUpToDateIndex) {
   void onDiagnosticsReady(PathRef File,
                           std::vector<Diag> Diagnostics) override {}
   } DiagConsumer;
   // rename is runnning on the "^" point in FooH, and "[[]]" ranges are the
   // expcted rename occurrences.
   struct Case {
     llvm::StringRef FooH;
     llvm::StringRef FooCC;
     } Cases [] = {
       {
+        // Constructor.
+      R"cpp(
+        class [[Foo]] {
+          [[Foo^]]();
+          ~[[Foo]]();
+        };
+      )cpp",
+      R"cpp(
+        #include "foo.h"
+        [[Foo]]::[[Foo]]() {}
+        [[Foo]]::~[[Foo]]() {}
+
+        void func() {
+          [[Foo]] foo;
+        }
+      )cpp",
+    },
+    {
+      // Destructor (selecting before the identifier).
+      R"cpp(
+        class [[Foo]] {
+          [[Foo]]();
+          ~[[^Foo]]();
+        };
+      )cpp",
+      R"cpp(
+        #include "foo.h"
+        [[Foo]]::[[Foo]]() {}
+        [[Foo]]::~[[Foo]]() {}
+
+        void func() {
+          [[Foo]] foo;
+        }
+      )cpp",
+    },
+    {
+      // Destructor (selecting within the identifier).
+      R"cpp(
+        class [[Foo]] {
+          [[Foo]]();
+          ~[[F^oo]]();
+        };
+      )cpp",
+      R"cpp(
+        #include "foo.h"
+        [[Foo]]::[[Foo]]() {}
+        [[Foo]]::~[[Foo]]() {}
+
+        void func() {
+          [[Foo]] foo;
+        }
+      )cpp",
+    },
+    {
+      // Destructor (selecting after the identifier).
+      R"cpp(
+        class [[Foo]] {
+          [[Foo]]();
+          virtual ~ /*~Foo?*/[[Foo^]]() {
+            int a = 4;
+          }
+        };
+      )cpp",
+      R"cpp(
+        #include "foo.h"
+        [[Foo]]::[[Foo]]() {}
+        [[Foo]]::~[[Foo]]() {}
+
+        void func() {
+          [[Foo]] foo;
+        }
+      )cpp",
+    },
+    {
       // classes.
       R"cpp(
         class [[Fo^o]] {
           [[Foo]]();
           ~[[Foo]]();
         };
       )cpp",
       R"cpp(