[clangd] Use raw rename functions to implement the rename.

[hokein]

Summary

The API provided by refactoring lib doesn't provide enough flexibility
to get clangd's rename to behave as we expect. Instead, we replace it
with the low-level rename functions, which give us more control.

Bonus:

• performance, previously we visit the TU to find all occurrences, now we just visit top-level decls from main file;
• fix a bug where we wrongly filter out the main file replacement due to the different relative/absolute file path;

[sammccall]

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

141 ↗

(On Diff #214101)

nit: most of the new uses of auto in this patch don't have a type that's obvious from the RHS, nor one that's hard to read, and should probably be expanded

175 ↗

(On Diff #214101)

is this a regression in this patch, or did the limitation already exist?

178 ↗

(On Diff #214101)

why can't this fail?

[hokein]

Address comments.

[hokein]

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

175 ↗

(On Diff #214101)

this is not a regression, this is a limitation in clangd.

178 ↗

(On Diff #214101)

I just kept the previous behavior. Made the error emit to the client rather than crashing here.

[sammccall]

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

182 ↗

(On Diff #214112)

for actual error handling behavior (if this can actually fail): is this a deliberate choice to refuse to perform any of the edits if there are conflicts? Is this better than applying some non-conflicting set?

Unlike most error-propagation, this is a nontrivial policy choice and needs a comment.

175 ↗

(On Diff #214101)

can you point me to where this behavior (bailing out when there's more than one name range) occurred in the old version of the code?

(by regression I would mean e.g. if objective-c selector renames worked before this change, but don't work afterwards. But either way, it'd be useful to understand why)

178 ↗

(On Diff #214101)

I'm not sure what you meant by "kept the previous behavior".
Yes, before this change there was a call cantFail, but the implementation was different, so it's not clear the set of errors it is handling are the same.

Previously, I guess it couldn't fail because we assumed that the underlying layer (which returned a set of edits) wouldn't produce conflicting ones.

Here, we're producing the set of edits ourselves, so any global property (e.g. "no pair of these conflict") needs to be established by us. So we should document whether/under what circumstances it can fail.

[hokein]

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

182 ↗

(On Diff #214112)

I think if there are conflicts, it is a signal that indicates we have bugs in the code (either in clangd, or rename library), applying parts of them may be not valuable (like generating uncompilable code)

I'd prefer to refuse to perform renaming if we have conflicts, WDYT?

175 ↗

(On Diff #214101)

There is a FIXME states that.

can you point me to where this behavior (bailing out when there's more than one name range) occurred in the old version of the code?

I'm pretty sure that the old version of the code will trigger the assertion when doing multiple-piece rename, as the name pieces of the NewName is always 1 according to the code.

178 ↗

(On Diff #214101)

Previously, I guess it couldn't fail because we assumed that the underlying layer (which returned a set of edits) wouldn't produce conflicting ones.

This is true, at this point we shouldn't see conflicts, because if we have conflicts, we have caught an error (in the RefactoringResultConsumer::handleError) when invoking the RenameOccurrences.

To sum up, in the previous version, we will report the error and refuse to apply any edits if there are conflicts, which is the same behavior we keep in current patch.
The only difference I see is that we traversal main file decls (rather than TUDecl), if we have bugs in the main file decls, we may generate conflicting edits, but I wouldn't too worry about this (as top level decls are a small subset of decls inside TUDecl).

[sammccall]

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

182 ↗

(On Diff #214112)

That sounds OK to me, can you add a comment?

[hokein]

Add a comment.

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

 //===--- Rename.cpp - Symbol-rename refactorings -----------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "refactor/Rename.h"
 #include "AST.h"
 #include "Logger.h"
 #include "index/SymbolCollector.h"
-#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRFinder.h"
+#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
+#include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
-class RefactoringResultCollector final
-    : public tooling::RefactoringResultConsumer {
-public:
-  void handleError(llvm::Error Err) override {
-    assert(!Result.hasValue());
-    Result = std::move(Err);
-  }
-
-  // Using the handle(SymbolOccurrences) from parent class.
-  using tooling::RefactoringResultConsumer::handle;
-
-  void handle(tooling::AtomicChanges SourceReplacements) override {
-    assert(!Result.hasValue());
-    Result = std::move(SourceReplacements);
-  }
-
-  llvm::Optional<llvm::Expected<tooling::AtomicChanges>> Result;
-};
-
-// Expand a DiagnosticError to make it print-friendly (print the detailed
-// message, rather than "clang diagnostic").
-llvm::Error expandDiagnostics(llvm::Error Err, DiagnosticsEngine &DE) {
-  if (auto Diag = DiagnosticError::take(Err)) {
-    llvm::cantFail(std::move(Err));
-    SmallVector<char, 128> DiagMessage;
-    Diag->second.EmitToString(DE, DiagMessage);
-    return llvm::make_error<llvm::StringError>(DiagMessage,
-                                               llvm::inconvertibleErrorCode());
-  }
-  return Err;
-}
-
 llvm::Optional<std::string> filePath(const SymbolLocation &Loc,
                                      llvm::StringRef HintFilePath) {
   if (!Loc)
     return None;
   auto Uri = URI::parse(Loc.FileURI);
   if (!Uri) {
     elog("Could not parse URI {0}: {1}", Loc.FileURI, Uri.takeError());
     return None;
if (auto RefFilePath = filePath(R.Location, /*HintFilePath=*/MainFile)) {
       if (*RefFilePath != MainFile)
         OtherFile = *RefFilePath;
     }
   });
   return OtherFile;
 }
 
 enum ReasonToReject {
+  NoSymbolFound,
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile,
   UnsupportedSymbol,
 };
 
 // Check the symbol Decl is renameable (per the index) within the file.
 llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
llvm::Optional<ReasonToReject> renamableWithinFile(const Decl &RenameDecl,
   // If the symbol is indexable and has refs from other files in the index,
   // we disallow rename.
   return ReasonToReject::UsedOutsideFile;
 }
 
 llvm::Error makeError(ReasonToReject Reason) {
   auto Message = [](ReasonToReject Reason) {
     switch (Reason) {
+    case NoSymbolFound:
+      return "there is no symbol at the given location";
     case NoIndexProvided:
       return "symbol may be used in other files (no index available)";
     case UsedOutsideFile:
       return "the symbol is used outside main file";
     case NonIndexable:
       return "symbol may be used in other files (not eligible for indexing)";
     case UnsupportedSymbol:
       return "symbol is not a supported kind (e.g. namespace, macro)";
     }
     llvm_unreachable("unhandled reason kind");
   };
   return llvm::make_error<llvm::StringError>(
       llvm::formatv("Cannot rename symbol: {0}", Message(Reason)),
       llvm::inconvertibleErrorCode());
 }
 
+// Return all rename occurrences in the main file.
+tooling::SymbolOccurrences
+findOccurrencesWithinFile(ParsedAST &AST, const NamedDecl *RenameDecl) {
+  const NamedDecl *CanonicalRenameDecl =
+      tooling::getCanonicalSymbolDeclaration(RenameDecl);
+  assert(CanonicalRenameDecl && "RenameDecl must be not null");
+  std::vector<std::string> RenameUSRs =
+      tooling::getUSRsForDeclaration(CanonicalRenameDecl, AST.getASTContext());
+  std::string OldName = CanonicalRenameDecl->getNameAsString();
+  tooling::SymbolOccurrences Result;
+  for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
+    tooling::SymbolOccurrences RenameInDecl =
+        tooling::getOccurrencesOfUSRs(RenameUSRs, OldName, TopLevelDecl);
+    Result.insert(Result.end(), std::make_move_iterator(RenameInDecl.begin()),
+                  std::make_move_iterator(RenameInDecl.end()));
+  }
+  return Result;
+}
+
 } // namespace
 
 llvm::Expected<tooling::Replacements>
 renameWithinFile(ParsedAST &AST, llvm::StringRef File, Position Pos,
                  llvm::StringRef NewName, const SymbolIndex *Index) {
-  RefactoringResultCollector ResultCollector;
-  ASTContext &ASTCtx = AST.getASTContext();
   SourceLocation SourceLocationBeg = clangd::getBeginningOfIdentifier(
       AST, Pos, AST.getSourceManager().getMainFileID());
   // FIXME: renaming macros is not supported yet, the macro-handling code should
   // be moved to rename tooling library.
   if (locateMacroAt(SourceLocationBeg, AST.getPreprocessor()))
     return makeError(UnsupportedSymbol);
-  tooling::RefactoringRuleContext Context(AST.getSourceManager());
-  Context.setASTContext(ASTCtx);
-  auto Rename = clang::tooling::RenameOccurrences::initiate(
-      Context, SourceRange(SourceLocationBeg), NewName);
-  if (!Rename)
-    return expandDiagnostics(Rename.takeError(), ASTCtx.getDiagnostics());
 
-  const auto *RenameDecl = Rename->getRenameDecl();
-  assert(RenameDecl && "symbol must be found at this point");
+  const auto *RenameDecl =
+      tooling::getNamedDeclAt(AST.getASTContext(), SourceLocationBeg);
+  if (!RenameDecl)
+    return makeError(NoSymbolFound);
+
   if (auto Reject =
           renamableWithinFile(*RenameDecl->getCanonicalDecl(), File, Index))
     return makeError(*Reject);
 
-  Rename->invoke(ResultCollector, Context);
-
-  assert(ResultCollector.Result.hasValue());
-  if (!ResultCollector.Result.getValue())
-    return expandDiagnostics(ResultCollector.Result->takeError(),
-                             ASTCtx.getDiagnostics());
-
-  tooling::Replacements FilteredChanges;
-  // Right now we only support renaming the main file, so we
-  // drop replacements not for the main file. In the future, we might
-  // also support rename with wider scope.
   // Rename sometimes returns duplicate edits (which is a bug). A side-effect of
   // adding them to a single Replacements object is these are deduplicated.
-  for (const tooling::AtomicChange &Change : ResultCollector.Result->get()) {
-    for (const auto &Rep : Change.getReplacements()) {
-      if (Rep.getFilePath() == File)
-        cantFail(FilteredChanges.add(Rep));
-    }
+  tooling::Replacements FilteredChanges;
+  for (const tooling::SymbolOccurrence &Rename :
+       findOccurrencesWithinFile(AST, RenameDecl)) {
+    // Currently, we only support normal rename (one range) for C/C++.
+    // FIXME: support multiple-range rename for objective-c methods.
+    if (Rename.getNameRanges().size() > 1)
+      continue;
+    // We shouldn't have conflicting replacements. If there are conflicts, it
+    // means that we have bugs either in clangd or in Rename library, therefore
+    // we refuse to perform the rename.
+    if (auto Err = FilteredChanges.add(tooling::Replacement(
+            AST.getASTContext().getSourceManager(),
+            CharSourceRange::getCharRange(Rename.getNameRanges()[0]), NewName)))
+      return std::move(Err);
   }
   return FilteredChanges;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/test/rename.test

 # CHECK-NEXT:        "character": 4,
 # CHECK-NEXT:        "line": 0
 # CHECK-NEXT:      }
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
 #      CHECK:  "error": {
 # CHECK-NEXT:    "code": -32001,
-# CHECK-NEXT:    "message": "there is no symbol at the given location"
+# CHECK-NEXT:    "message": "Cannot rename symbol: there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0"
 ---
 {"jsonrpc":"2.0","id":4,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #      CHECK:  "error": {
 # CHECK-NEXT:    "code": -32001,
-# CHECK-NEXT:    "message": "there is no symbol at the given location"
+# CHECK-NEXT:    "message": "Cannot rename symbol: there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 4,
 # CHECK-NEXT:  "jsonrpc": "2.0"
 ---
 {"jsonrpc":"2.0","id":3,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":5},"newName":"bar"}}
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {