[PATCH] D63126: [clangd] Implement "prepareRename"

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:393
         }}}};
+  if (Params.capabilities.RenamePrepareSupport)
+    Result["renameProvider"] = llvm::json::Object{{"prepareProvider", true}};
----------------
hokein wrote:
> sammccall wrote:
> > why set this only if the client advertised support?
> The LSP says
> ```     
>  /**
> 	 * The server provides rename support. RenameOptions may only be
> 	 * specified if the client states that it supports
> 	 * `prepareSupport` in its initial `initialize` request.
> 	 */
> 	renameProvider?: boolean | RenameOptions;
> ```
> 
> so we only send `RenameOptions` when the client declares it supports prepareRename.
Ah, OK. this should be documented.

Could we put this logic above instead, for less data structure wrangling?
```
llvm::json::Value RenameProvider = llvm::json::Object{{"prepareProvider", true}};
if (!Params.capabilities.RenamePrepareSupport) // Only boolean allowed per LSP
  RenameProvider = true;
...
{"renameProvider", std::move(RenameProvider)}
```



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:581
+      Params.textDocument.uri.file(), Params.position,
+      Bind(
+          [](decltype(Reply) Reply, llvm::Expected<llvm::Optional<Range>> R) {
----------------
this appears to be the same as just passing std::move(Reply) to preparerename


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:298
+    if (!Changes) {
+      // LSP clients may emit a hard-coded error message (e.g. "can't be
+      // renamed" in VSCode) if we returns null, we use the interl error message
----------------
There's some context missing here - you're explaining *why* we're going against LSP, but you haven't explained that we're doing so!

"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)"

("will" not "may", because there's no way it can result in a detailed message)


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:296
+    // Return null if the "rename" is not valid at the position.
+    auto Changes = renameWithinFile(AST, File, Pos, "dummy", Index);
+    if (!Changes) {
----------------
sammccall wrote:
> High level comment indicating why we use rename to implement preparerename?
> (i.e. why isn't it too expensive)
can you say something like "Performing the rename isn't substantially more expensive than an AST-based check, so we just rename and throw away the results. We may have to revisit this when we support cross-file rename"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63126/new/

https://reviews.llvm.org/D63126