[clangd] a prototype for triggering rename after extracting.

[hokein]

Summary

This is a prototype patch of implementing a generic mechanism to perform a chain of refactorings.

An example (extract + rename) of how it works:

1. the extract tweak returns a vector of steps ([applyEdit, rename]) during the apply stage.
2. clangd sends a workspace/applyEdit to the editor and waits for the reply;
3. the editor applies the edit and sends back a reply;
4. clangd sends a custom request clangd.triggerRename to the editor (edito needs to implement the command);
5. the editor executes the clangd.triggerRename request (triggers the LSP rename), and replies to clangd;
6. clangd receives the reply, and there is no more steps, so the "apply tweak" workflow is finished;

A demo:

[sammccall]

This is really cool! Hm, formatting is troublesome, well spotted :-)

As you're defining a generic mechanism, it'd be useful to have more examples to verify that this actually generalizes.
We might have to make them up (e.g. replace-auto-with-type-and-go-to-definition?)

If we can't show it generalizes, then I think this would still be useful enough just to dispense with generality and add a "renameAt" extension to CodeAction or something like that.

clang-tools-extra/clangd/Protocol.cpp

544 ↗

(On Diff #211683)

(why would "lsp" be in these names - surely this is just rename?)

clang-tools-extra/clangd/Protocol.h

765 ↗

(On Diff #211683)

(If there's precedent for others using a similar protocol, we should link to it)

777 ↗

(On Diff #211683)

couldn't we send a range here?
That would allow the client to skip prepareRename and still use its native UI.
(In principle. no need to actually implement this in vscode)

807 ↗

(On Diff #211683)

(mark extension here too).

894 ↗

(On Diff #211683)

So there's a few places that the command could be specified:

1. in the code action, as a thing to do at the end (above)
2. as a parameter, when the client executes a command and the server calls applyWorkspaceEdit (here)
3. as the return value from executeCommand

This patch implements 1 & 2. Why this choice? (easiest to prototype is a perfectly sensible answer)

1 is restricted to code actions.
2 & 3 are (practically) restricted to executeCommand.
2 is restricted to when edits are applied.

(To me, implementing just 1 or just 3 seem like the strongest options...)

[hokein]

clang-tools-extra/clangd/Protocol.h

777 ↗

(On Diff #211683)

rename just uses the position as its parameter, not range.

807 ↗

(On Diff #211683)

ah, this is not used, remove.

894 ↗

(On Diff #211683)

sorry, this patch actually implements just 2 (the clientCommand was added accidentally in the CodeAction, removing now).

for 1 -- it has a limitation, at the time when clangd sends CodeAction, we only run the tweak::prepare, at this time we might not get enough information (AST etc) to compute a client command.

for 3 -- LSP doesn't say much about how to use return value from executeCommand, it just suggests servers to send applyEdit request to the server. I believe most of LSP clients (at least VSCode) just throw the return value away.

[ilya-biryukov]

If we are going down that path, could we consider to pre-selecting a name of the extract variable and its usage (using multi-select)?
I.e. the optimal workflow seems to be ([[code]] marks selected regions):

// Input:
foo([[10+10]]);


// 'extract' turns this into the following code (and selects for editors that support this):
int [[dummy]] = 10+10;
foo([[dummy]]);

At this point users can type a new name and get it updated directly.
There are a bunch of reasons why running the full-blown rename is suboptimal:

• rename can fail, e.g. if you had other variables called dummy in this scope or if we actions happen to produce broken AST,
• rename UI is generally clunkier than multi-selection (a new window is opened, this moves focus from the code to the editor).
• rename has to wait for the AST, in general case this might result in high latency for actions that can add an #include or if we any of the included files changed.

The obvious drawback is that this workflow might be hard to implement in some editors, but both Vim and VSCode should be able to provide a decent experience here.

[hokein]

reimplement the mechanism based on the offline discussion.

[hokein]

update the diff

[hokein]

In D65263#1601036, @ilya-biryukov wrote:

If we are going down that path, could we consider to pre-selecting a name of the extract variable and its usage (using multi-select)?

I agree that doing rename after extraction is not an ideal approach (both correctness and latency), there should be other better ways to achieve this.
However as a LSP server, we may want to align with the community and other language servers (typescript, java) in order to provide consistent refactoring experience across languages (vscode prefers to do extract with a default name and rename afterwards). Note that this patch aims to implement a generic mechanism to perform a chain of small refactorings (extract + rename is just one example), we still need this no match which way we're going to perform.

[hokein]

The patch is still missing tests, but should be good for early comments.

[ilya-biryukov]

In D65263#1606371, @hokein wrote:

I agree that doing rename after extraction is not an ideal approach (both correctness and latency), there should be other better ways to achieve this.
However as a LSP server, we may want to align with the community and other language servers (typescript, java) in order to provide consistent refactoring experience across languages (vscode prefers to do extract with a default name and rename afterwards).

Java and Typescript can pretty much guarantee (local) rename is fast and latency is never an issue for those languages.
Either approach seems to require LSP extensions. Pre-select seems to provide better UX , that's the reason I'm proposing to look in that direction.

Note that this patch aims to implement a generic mechanism to perform a chain of small refactorings (extract + rename is just one example), we still need this no match which way we're going to perform.

Totally agree.

clang-tools-extra/clangd/ClangdLSPServer.cpp

if (Params.command == ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND &&
     if (!Code)
       return Reply(llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           "trying to apply a code action for a non-added file"));
 
     auto Action = [this, ApplyEdit](decltype(Reply) Reply, URIForFile File,
                                     std::string Code,
                                     llvm::Expected<Tweak::Effect> R) {
+      // Function object to apply steps for a tweak.
+      // Steps are executed in order: for each step, clangd sends a new request
+      // to the LSP client, waits for the response and continues next step; if
+      // there is any error during the apply process, clangd stops applying the
+      // following steps.
+      //
+      // Example steps [applyEdit, Rename]
+      //  - apply the first step:  send call(applyEdit, Workspace) request
+      //  - ..wait for client response
+      //  - apply the second step: send call(rename, trigger)
+      //  - ..wait for the reply, and finish.
+      struct CallInSequence {
+      public:
+        CallInSequence(ClangdLSPServer *Server, std::vector<Tweak::Step> Steps,
+                       std::string Code, URIForFile MainFile)
+            : Server(Server), Steps(std::move(Steps)), nextStepIndex(0),
+              Code(std::move(Code)), MainFile(std::move(MainFile)) {}
+
+        void operator()(llvm::Expected<llvm::json::Value> Result) {
+          if (!Result) {
+            // FIXME: should send a failed reply to client.
+            log("receive error: {0}", llvm::toString(Result.takeError()));
+            return;
+          }
+          if (nextStepIndex >= Steps.size()) {
+            // FIXME: should send a successful reply to client.
+            log("applied tweak successfully");
+            return;
+          }
+          auto RequestToClient = takeNextStep();
+          nextCall(Server, RequestToClient.first, RequestToClient.second);
+        }
+
+        std::pair<std::string, llvm::json::Value> takeNextStep() {
+          assert(nextStepIndex < Steps.size() && "index out of boundary!");
+          return transform(Steps[nextStepIndex++]);
+        }
+
+      private:
+        // Transform the step into a LSP request.
+        std::pair</* Method */ std::string,
+                  /* MethodParams */ llvm::json::Value>
+        transform(const Tweak::Step &Step) {
+          if (Step.ApplyKind == Tweak::Step::ApplyEdit) {
+            ApplyWorkspaceEditParams Edit;
+
+            Edit.edit.changes.emplace();
+            (*Edit.edit.changes)[MainFile.uri()] =
+                replacementsToEdits(Code, *Step.ApplyEditParams);
+            return {"workspace/applyEdit", std::move(Edit)};
+          } else if (Step.ApplyKind == Tweak::Step::Rename) {
+            // Clangd extension.
+            llvm::json::Object Result;
+            Result["arguments"] = {*Step.RenameParams};
+            return {"clangd.triggerRename", std::move(Result)};
+          }
+          llvm_unreachable("unhandled apply kind");
+        }
+        void nextCall(ClangdLSPServer *LSPServer, llvm::StringRef Method,
+                      llvm::json::Value Params) {
+          LSPServer->call(Method, std::move(Params), std::move(*this));
+        }
+
+        ClangdLSPServer *Server;
+        std::vector<Tweak::Step> Steps;
+        size_t nextStepIndex;
+
+        // datas for transforming step.
+        std::string Code;
+        URIForFile MainFile;
+      };
+
       if (!R)
         return Reply(R.takeError());
 
+      if (R->ApplySteps) {
+        CallInSequence Calls(this, *R->ApplySteps, Code, File);
+        auto Request = Calls.takeNextStep();
+        // Starts the call chain.
+        call(Request.first, std::move(Request.second), std::move(Calls));
+      }
       if (R->ApplyEdit) {
         WorkspaceEdit WE;
         WE.changes.emplace();
         (*WE.changes)[File.uri()] = replacementsToEdits(Code, *R->ApplyEdit);
         ApplyEdit(std::move(WE));
       }
       if (R->ShowMessage) {
         ShowMessageParams Msg;

clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts

clangdClient.onDidChangeState(
                 clangdClient.onNotification(
                     'textDocument/clangd.fileStatus',
                     (fileStatus) => { status.onFileUpdated(fileStatus); });
             } else if (newState == vscodelc.State.Stopped) {
                 // Clear all cached statuses when clangd crashes.
                 status.clear();
             }
         })
+
+    clangdClient.onReady().then(
+        () => {clangdClient.onRequest(
+            "clangd.triggerRename", async (result: any) => {
+              if (result.arguments) {
+                return await vscode.commands
+                    .executeCommand(
+                        'editor.action.rename',
+                        [
+                          vscode.Uri.file(
+                              vscode.window.activeTextEditor.document.fileName),
+                          result.arguments[0]
+                        ])
+                    .then((value) => { return Promise.resolve(true); },
+                          (err) => {
+                            if (err.name == 'Canceled') {
+                              // VSCode will cancel the request automatically
+                              // when it detects there is a change of the file
+                              // (rename) during the process of handling the
+                              // triggerRename request. but we don't want client
+                              // send back a cancel error to clangd, so consume
+                              // the cancel here.
+                              return Promise.resolve(true);
+                            }
+
+                            return Promise.resolve(err);
+                          });
+              }
+              return Promise.reject(new Error("Invalid commend arguments"));
+            })});
     // An empty place holder for the activate command, otherwise we'll get an
     // "command is not registered" error.
     context.subscriptions.push(vscode.commands.registerCommand(
             'clangd-vscode.activate', async () => {}));
 }

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

public:
 
   /// Output of a tweak.
   enum Intent {
     /// Apply changes that preserve the behavior of the code.
     Refactor,
     /// Provide information to the user.
     Info,
   };
+
+  // The class represents a small and focus refactoring. The purpose is to form
+  // a chain of different refactorings, so that we could perform a large and
+  // complicate refactoring.
+  //
+  // Steps are applied in sequence. In LSP layer, each step is transformed to a
+  // call request to the LSP client, LSPServer waits for the client response and
+  // continues to apply next step when the previous step is finished
+  // successfully.
+  //
+  // clangd doesn't track changes of the source files when apply a step, so
+  // the following steps are not aware of the changes, Tweak is corresponding to
+  // pre-calculate the changes for all steps.
+  // FIXME: we should have a mechanism to track the changes for each step.
+  struct Step {
+    enum Kind {
+      ApplyEdit,
+      Rename,
+    };
+    Kind ApplyKind;
+
+    llvm::Optional<tooling::Replacements> ApplyEditParams; // For ApplyEdit
+    llvm::Optional<Position> RenameParams;                 // For Rename
+  };
+
   struct Effect {
     /// A message to be displayed to the user.
     llvm::Optional<std::string> ShowMessage;
     /// An edit to apply to the input file.
     llvm::Optional<tooling::Replacements> ApplyEdit;
+    /// A chain of steps being executed in order when applying the tweak.
+    llvm::Optional<std::vector<Step>> ApplySteps;
 
+    static Effect applySteps(std::vector<Step> Steps) {
+      Effect E;
+      E.ApplySteps = std::move(Steps);
+      return E;
+    }
     static Effect applyEdit(tooling::Replacements R) {
       Effect E;
       E.ApplyEdit = std::move(R);
       return E;
     }
     static Effect showMessage(StringRef S) {
       Effect E;
       E.ShowMessage = S;

clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
   std::string VarName = "dummy";
   SourceRange Range = Target->getExtractionChars();
   // insert new variable declaration
   if (auto Err = Result.add(Target->insertDeclaration(VarName, Range)))
     return std::move(Err);
   // replace expression with variable name
   if (auto Err = Result.add(Target->replaceWithVar(Range, VarName)))
     return std::move(Err);
-  return Effect::applyEdit(Result);
+  // TODO: refine this.
+  // We do the format internally in order to keep the position of the extracted
+  // variable not being changed from the caller.
+  auto &SM = Inputs.AST.getASTContext().getSourceManager();
+  auto *FS = &SM.getFileManager().getVirtualFileSystem();
+  auto Style = getFormatStyleForFile(
+      SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName(),
+      Inputs.Code, FS);
+  auto Formatted = cleanupAndFormat(Inputs.Code, Result, Style);
+  if (!Formatted)
+    return Formatted.takeError();
+  Result = *Formatted;
+  auto NewCode = tooling::applyAllReplacements(Inputs.Code, Result);
+  if (!NewCode)
+    return NewCode.takeError();
+
+  assert(!Result.empty());
+  // Calculate the offset of the dummy variable after applying replacements.
+  size_t ExtractVarOffset = Result.begin()->getOffset();
+  for (auto &R : Result) {
+    auto OffsetInReplacement = R.getReplacementText().find(VarName);
+    if (OffsetInReplacement != llvm::StringRef::npos) {
+      ExtractVarOffset += OffsetInReplacement;
+      break;
+    }
+    ExtractVarOffset += R.getReplacementText().size() - R.getLength();
+  }
+
+  assert(ExtractVarOffset != llvm::StringRef::npos);
+  Step Apply;
+  Apply.ApplyKind = Step::ApplyEdit;
+  Apply.ApplyEditParams = std::move(Result);
+  Step Rename;
+  Rename.ApplyKind = Step::Rename;
+  Rename.RenameParams = offsetToPosition(*NewCode, ExtractVarOffset);
+  // TODO: We should not emit Rename if the client doesn't support it.
+  return Effect::applySteps({std::move(Apply), std::move(Rename)});
 }
 
 // Find the CallExpr whose callee is an ancestor of the DeclRef
 const SelectionTree::Node *getCallExpr(const SelectionTree::Node *DeclRef) {
   // we maintain a stack of all exprs encountered while traversing the
   // selectiontree because the callee of the callexpr can be an ancestor of the
   // DeclRef. e.g. Callee can be an ImplicitCastExpr.
   std::vector<const clang::Expr *> ExprStack;