[clangd] Extend the rename API.

several changes:
- return a structure result in rename API;
- prepareRename now returns more information (main-file occurrences);
- remove the duplicated detecting-touch-identifier code in prepareRename (which is implemented in rename API);

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

Author: hokein

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -793,8 +793,13 @@ void ClangdLSPServer::onWorkspaceSymbol(
 
 void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params,
                                       Callback<llvm::Optional<Range>> Reply) {
-  Server->prepareRename(Params.textDocument.uri.file(), Params.position,
-                        Opts.Rename, std::move(Reply));
+  Server->prepareRename(
+      Params.textDocument.uri.file(), Params.position, Opts.Rename,
+      [Reply = std::move(Reply)](llvm::Expected<RenameResult> Result) mutable {
+        if (!Result)
+          return Reply(Result.takeError());
+        return Reply(std::move(Result->Target));
+      });
 }
 
 void ClangdLSPServer::onRename(const RenameParams &Params,
@@ -806,14 +811,14 @@ void ClangdLSPServer::onRename(const RenameParams &Params,
   Server->rename(
       File, Params.position, Params.newName, Opts.Rename,
       [File, Params, Reply = std::move(Reply),
-       this](llvm::Expected<FileEdits> Edits) mutable {
-        if (!Edits)
-          return Reply(Edits.takeError());
-        if (auto Err = validateEdits(DraftMgr, *Edits))
+       this](llvm::Expected<RenameResult> R) mutable {
+        if (!R)
+          return Reply(R.takeError());
+        if (auto Err = validateEdits(DraftMgr, R->GlobalChanges))
           return Reply(std::move(Err));
         WorkspaceEdit Result;
         Result.changes.emplace();
-        for (const auto &Rep : *Edits) {
+        for (const auto &Rep : R->GlobalChanges) {
           (*Result.changes)[URI::createFile(Rep.first()).toString()] =
               Rep.second.asTextEdits();
         }

clang-tools-extra/clangd/ClangdServer.cpp

@@ -400,46 +400,35 @@ void ClangdServer::formatOnType(PathRef File, llvm::StringRef Code,
 
 void ClangdServer::prepareRename(PathRef File, Position Pos,
                                  const RenameOptions &RenameOpts,
-                                 Callback<llvm::Optional<Range>> CB) {
+                                 Callback<RenameResult> CB) {
   auto Action = [Pos, File = File.str(), CB = std::move(CB), RenameOpts,
                  this](llvm::Expected<InputsAndAST> InpAST) mutable {
     if (!InpAST)
       return CB(InpAST.takeError());
-    auto &AST = InpAST->AST;
-    const auto &SM = AST.getSourceManager();
-    auto Loc = sourceLocationInMainFile(SM, Pos);
-    if (!Loc)
-      return CB(Loc.takeError());
-    const auto *TouchingIdentifier =
-        spelledIdentifierTouching(*Loc, AST.getTokens());
-    if (!TouchingIdentifier)
-      return CB(llvm::None); // no rename on non-identifiers.
-
-    auto Range = halfOpenToRange(
-        SM, CharSourceRange::getCharRange(TouchingIdentifier->location(),
-                                          TouchingIdentifier->endLocation()));
-
-    if (RenameOpts.AllowCrossFile)
-      // FIXME: we now assume cross-file rename always succeeds, revisit this.
-      return CB(Range);
-
-    // Performing the local rename isn't substantially more expensive than
-    // doing an AST-based check, so we just rename and throw away the results.
-    auto Changes = clangd::rename({Pos, "dummy", AST, File, Index, RenameOpts,
-                                   /*GetDirtyBuffer=*/nullptr});
-    if (!Changes) {
+    // prepareRename is latency-sensitive:
+    //  - for single-file rename, performing rename isn't substantially more
+    //    expensive than doing an AST-based check (the index is used to see if
+    //    the rename is complete);
+    //  - for cross-file rename, we deliberately pass a nullptr index to save
+    //    the cost, thus the result may be incomplete as it only contains
+    //    main-file occurrences;
+    auto Results = clangd::rename({Pos, /*NewName*/ "", InpAST->AST, File,
+                                   RenameOpts.AllowCrossFile ? nullptr : Index,
+                                   RenameOpts});
+    if (!Results) {
       // LSP says to return null on failure, but that will result in a generic
       // failure message. If we send an LSP error response, clients can surface
       // the message to users (VSCode does).
-      return CB(Changes.takeError());
+      return CB(Results.takeError());
     }
-    return CB(Range);
+    return CB(*Results);
   };
   WorkScheduler.runWithAST("PrepareRename", File, std::move(Action));
 }
 
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
-                          const RenameOptions &Opts, Callback<FileEdits> CB) {
+                          const RenameOptions &Opts,
+                          Callback<RenameResult> CB) {
   // A snapshot of all file dirty buffers.
   llvm::StringMap<std::string> Snapshot = WorkScheduler.getAllFileContents();
   auto Action = [File = File.str(), NewName = NewName.str(), Pos, Opts,
@@ -457,24 +446,24 @@ void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
         return llvm::None;
       return It->second;
     };
-    auto Edits = clangd::rename(
+    auto R = clangd::rename(
         {Pos, NewName, InpAST->AST, File, Index, Opts, GetDirtyBuffer});
-    if (!Edits)
-      return CB(Edits.takeError());
+    if (!R)
+      return CB(R.takeError());
 
     if (Opts.WantFormat) {
       auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents,
                                          *InpAST->Inputs.TFS);
       llvm::Error Err = llvm::Error::success();
-      for (auto &E : *Edits)
+      for (auto &E : R->GlobalChanges)
         Err =
             llvm::joinErrors(reformatEdit(E.getValue(), Style), std::move(Err));
 
       if (Err)
         return CB(std::move(Err));
     }
-    RenameFiles.record(Edits->size());
-    return CB(std::move(*Edits));
+    RenameFiles.record(R->GlobalChanges.size());
+    return CB(*R);
   };
   WorkScheduler.runWithAST("Rename", File, std::move(Action));
 }

clang-tools-extra/clangd/ClangdServer.h

@@ -273,17 +273,20 @@ class ClangdServer {
                     StringRef TriggerText, Callback<std::vector<TextEdit>> CB);
 
   /// Test the validity of a rename operation.
+  ///
+  /// The returned result describes edits in the main-file only (all
+  /// occurrences of the renamed symbol are simply deleted.
   void prepareRename(PathRef File, Position Pos,
                      const RenameOptions &RenameOpts,
-                     Callback<llvm::Optional<Range>> CB);
+                     Callback<RenameResult> CB);
 
   /// Rename all occurrences of the symbol at the \p Pos in \p File to
   /// \p NewName.
   /// If WantFormat is false, the final TextEdit will be not formatted,
   /// embedders could use this method to get all occurrences of the symbol (e.g.
   /// highlighting them in prepare stage).
   void rename(PathRef File, Position Pos, llvm::StringRef NewName,
-              const RenameOptions &Opts, Callback<FileEdits> CB);
+              const RenameOptions &Opts, Callback<RenameResult> CB);
 
   struct TweakRef {
     std::string ID;    /// ID to pass for applyTweak.

clang-tools-extra/clangd/SourceCode.h

@@ -181,6 +181,8 @@ struct Edit {
   tooling::Replacements Replacements;
   std::string InitialCode;
 
+  Edit() = default;
+
   Edit(llvm::StringRef Code, tooling::Replacements Reps)
       : Replacements(std::move(Reps)), InitialCode(Code) {}
 

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

@@ -182,8 +182,6 @@ llvm::Optional<ReasonToReject> renameable(const NamedDecl &RenameDecl,
   }
 
   assert(CrossFile);
-  if (!Index)
-    return ReasonToReject::NoIndexProvided;
 
   // FIXME: Renaming virtual methods requires to rename all overridens in
   // subclasses, our index doesn't have this information.
@@ -427,7 +425,7 @@ void findNearMiss(
 
 } // namespace
 
-llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
+llvm::Expected<RenameResult> rename(const RenameInputs &RInputs) {
   trace::Span Tracer("Rename flow");
   const auto &Opts = RInputs.Opts;
   ParsedAST &AST = RInputs.AST;
@@ -456,9 +454,13 @@ llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
     return Loc.takeError();
   const syntax::Token *IdentifierToken =
       spelledIdentifierTouching(*Loc, AST.getTokens());
+
   // Renames should only triggered on identifiers.
   if (!IdentifierToken)
     return makeError(ReasonToReject::NoSymbolFound);
+  Range CurrentIdentifier = halfOpenToRange(
+      SM, CharSourceRange::getCharRange(IdentifierToken->location(),
+                                        IdentifierToken->endLocation()));
   // FIXME: Renaming macros is not supported yet, the macro-handling code should
   // be moved to rename tooling library.
   if (locateMacroAt(*IdentifierToken, AST.getPreprocessor()))
@@ -489,32 +491,40 @@ llvm::Expected<FileEdits> rename(const RenameInputs &RInputs) {
   auto MainFileRenameEdit = renameWithinFile(AST, RenameDecl, RInputs.NewName);
   if (!MainFileRenameEdit)
     return MainFileRenameEdit.takeError();
+  RenameResult Result;
+  Result.Target = CurrentIdentifier;
+  Edit MainFileEdits = Edit(MainFileCode, std::move(*MainFileRenameEdit));
+  llvm::for_each(MainFileEdits.asTextEdits(), [&Result](const TextEdit &TE) {
+    Result.LocalChanges.push_back(TE.range);
+  });
 
   // return the main file edit if this is a within-file rename or the symbol
   // being renamed is function local.
   if (!Opts.AllowCrossFile || RenameDecl.getParentFunctionOrMethod()) {
-    return FileEdits(
-        {std::make_pair(RInputs.MainFilePath,
-                        Edit{MainFileCode, std::move(*MainFileRenameEdit)})});
+    Result.GlobalChanges = FileEdits(
+        {std::make_pair(RInputs.MainFilePath, std::move(MainFileEdits))});
+    return Result;
   }
 
-  FileEdits Results;
-  // Renameable safely guards us that at this point we are renaming a local
-  // symbol if we don't have index.
-  if (RInputs.Index) {
-    auto OtherFilesEdits = renameOutsideFile(
-        RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
-        Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
-                             : Opts.LimitFiles,
-        GetFileContent);
-    if (!OtherFilesEdits)
-      return OtherFilesEdits.takeError();
-    Results = std::move(*OtherFilesEdits);
+  // If the index is nullptr, we don't know the completeness of the result, so
+  // we don't populate the field GlobalChanges.
+  if (!RInputs.Index) {
+    assert(Result.GlobalChanges.empty() && Opts.AllowCrossFile);
+    return Result;
   }
+
+  auto OtherFilesEdits = renameOutsideFile(
+      RenameDecl, RInputs.MainFilePath, RInputs.NewName, *RInputs.Index,
+      Opts.LimitFiles == 0 ? std::numeric_limits<size_t>::max()
+                           : Opts.LimitFiles,
+      GetFileContent);
+  if (!OtherFilesEdits)
+    return OtherFilesEdits.takeError();
+  Result.GlobalChanges = *OtherFilesEdits;
   // Attach the rename edits for the main file.
-  Results.try_emplace(RInputs.MainFilePath, MainFileCode,
-                      std::move(*MainFileRenameEdit));
-  return Results;
+  Result.GlobalChanges.try_emplace(RInputs.MainFilePath,
+                                   std::move(MainFileEdits));
+  return Result;
 }
 
 llvm::Expected<Edit> buildRenameEdit(llvm::StringRef AbsFilePath,

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

@@ -55,10 +55,20 @@ struct RenameInputs {
   DirtyBufferGetter GetDirtyBuffer = nullptr;
 };
 
+struct RenameResult {
+  // The range of the symbol that the user can attempt to rename.
+  Range Target;
+  // Rename occurrences for the current main file.
+  std::vector<Range> LocalChanges;
+  // Complete edits for the rename, including LocalChanges.
+  // If the full set of changes is unknown, this field is empty.
+  FileEdits GlobalChanges;
+};
+
 /// Renames all occurrences of the symbol. The result edits are unformatted.
 /// If AllowCrossFile is false, returns an error if rename a symbol that's used
 /// in another file (per the index).
-llvm::Expected<FileEdits> rename(const RenameInputs &RInputs);
+llvm::Expected<RenameResult> rename(const RenameInputs &RInputs);
 
 /// Generates rename edits that replaces all given occurrences with the
 /// NewName.

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

@@ -21,9 +21,12 @@
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2}}}
-#      CHECK:  "id": 2,
-# CHECK-NEXT:  "jsonrpc": "2.0",
-# CHECK-NEXT:  "result": null
+#      CHECK:  "error": {
+# CHECK-NEXT:    "code": -32001,
+# 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": {