[clangd] Implement "prepareRename"

[hokein]

Summary

• "prepareRename" request is added in LSP v3.12.0
• also update the vscode-client dependency to pick-up the rename bug fix[1]

[1]: https://github.com/microsoft/vscode-languageserver-node/issues/447

[hokein]

Update the patch, and add tests.

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.cpp

393 ↗

(On Diff #203991)

why set this only if the client advertised support?

clang-tools-extra/clangd/ClangdServer.cpp

295 ↗

(On Diff #211080)

move this comment to at/in the !Changes block?

296 ↗

(On Diff #211080)

High level comment indicating why we use rename to implement preparerename?
(i.e. why isn't it too expensive)

298 ↗

(On Diff #211080)

I thought the point of supporting this was to get errors to the user sooner. That can't happen if we throw away the error message...

clang-tools-extra/clangd/Protocol.h

412 ↗

(On Diff #203991)

(i think we shouldn't need to parse this at all)

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

2 ↗

(On Diff #211080)

inline this test into rename.test?

[hokein]

Address review comments.

[hokein]

clang-tools-extra/clangd/ClangdLSPServer.cpp

393 ↗

(On Diff #203991)

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.

clang-tools-extra/clangd/ClangdServer.cpp

298 ↗

(On Diff #211080)

use our own error message instead, luckily, VSCode prompt this error message to users.

[sammccall]

clang-tools-extra/clangd/ClangdLSPServer.cpp

581 ↗

(On Diff #211288)

this appears to be the same as just passing std::move(Reply) to preparerename

393 ↗

(On Diff #203991)

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

clang-tools-extra/clangd/ClangdServer.cpp

298 ↗

(On Diff #211288)

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)

296 ↗

(On Diff #211080)

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"

[hokein]

Address comments

clang-tools-extra/trunk/clangd/ClangdLSPServer.h

private:
   void onSwitchSourceHeader(const TextDocumentIdentifier &,
                             Callback<llvm::Optional<URIForFile>>);
   void onDocumentHighlight(const TextDocumentPositionParams &,
                            Callback<std::vector<DocumentHighlight>>);
   void onFileEvent(const DidChangeWatchedFilesParams &);
   void onCommand(const ExecuteCommandParams &, Callback<llvm::json::Value>);
   void onWorkspaceSymbol(const WorkspaceSymbolParams &,
                          Callback<std::vector<SymbolInformation>>);
+  void onPrepareRename(const TextDocumentPositionParams &,
+                       Callback<llvm::Optional<Range>>);
   void onRename(const RenameParams &, Callback<WorkspaceEdit>);
   void onHover(const TextDocumentPositionParams &,
                Callback<llvm::Optional<Hover>>);
   void onTypeHierarchy(const TypeHierarchyParams &,
                        Callback<llvm::Optional<TypeHierarchyItem>>);
   void onResolveTypeHierarchy(const ResolveTypeHierarchyItemParams &,
                               Callback<llvm::Optional<TypeHierarchyItem>>);
   void onChangeConfiguration(const DidChangeConfigurationParams &);

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onInitialize(const InitializeParams &Params,
   if (Params.capabilities.CompletionItemKinds)
     SupportedCompletionItemKinds |= *Params.capabilities.CompletionItemKinds;
   SupportsCodeAction = Params.capabilities.CodeActionStructure;
   SupportsHierarchicalDocumentSymbol =
       Params.capabilities.HierarchicalDocumentSymbol;
   SupportFileStatus = Params.initializationOptions.FileStatus;
   HoverContentFormat = Params.capabilities.HoverContentFormat;
   SupportsOffsetsInSignatureHelp = Params.capabilities.OffsetsInSignatureHelp;
+
+  // Per LSP, renameProvider can be either boolean or RenameOptions.
+  // RenameOptions will be specified if the client states it supports prepare.
+  llvm::json::Value RenameProvider =
+      llvm::json::Object{{"prepareProvider", true}};
+  if (!Params.capabilities.RenamePrepareSupport) // Only boolean allowed per LSP
+    RenameProvider = true;
+
   llvm::json::Object Result{
       {{"capabilities",
         llvm::json::Object{
             {"textDocumentSync", (int)TextDocumentSyncKind::Incremental},
             {"documentFormattingProvider", true},
             {"documentRangeFormattingProvider", true},
             {"documentOnTypeFormattingProvider",
              llvm::json::Object{
llvm::json::Object Result{
             {"signatureHelpProvider",
              llvm::json::Object{
                  {"triggerCharacters", {"(", ","}},
              }},
             {"declarationProvider", true},
             {"definitionProvider", true},
             {"documentHighlightProvider", true},
             {"hoverProvider", true},
-            {"renameProvider", true},
+            {"renameProvider", std::move(RenameProvider)},
             {"documentSymbolProvider", true},
             {"workspaceSymbolProvider", true},
             {"referencesProvider", true},
             {"executeCommandProvider",
              llvm::json::Object{
                  {"commands",
                   {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
                    ExecuteCommandParams::CLANGD_APPLY_TWEAK}},
Server->workspaceSymbols(
             for (auto &Sym : *Items)
               Sym.kind = adjustKindToCapability(Sym.kind, SupportedSymbolKinds);
 
             Reply(std::move(*Items));
           },
           std::move(Reply)));
 }
 
+void ClangdLSPServer::onPrepareRename(const TextDocumentPositionParams &Params,
+                                      Callback<llvm::Optional<Range>> Reply) {
+  Server->prepareRename(Params.textDocument.uri.file(), Params.position,
+                        std::move(Reply));
+}
+
 void ClangdLSPServer::onRename(const RenameParams &Params,
                                Callback<WorkspaceEdit> Reply) {
   Path File = Params.textDocument.uri.file();
   llvm::Optional<std::string> Code = DraftMgr.getDraft(File);
   if (!Code)
     return Reply(llvm::make_error<LSPError>(
         "onRename called for non-added file", ErrorCode::InvalidParams));
 
ClangdLSPServer::ClangdLSPServer(
   MsgHandler->bind("textDocument/formatting", &ClangdLSPServer::onDocumentFormatting);
   MsgHandler->bind("textDocument/codeAction", &ClangdLSPServer::onCodeAction);
   MsgHandler->bind("textDocument/completion", &ClangdLSPServer::onCompletion);
   MsgHandler->bind("textDocument/signatureHelp", &ClangdLSPServer::onSignatureHelp);
   MsgHandler->bind("textDocument/definition", &ClangdLSPServer::onGoToDefinition);
   MsgHandler->bind("textDocument/declaration", &ClangdLSPServer::onGoToDeclaration);
   MsgHandler->bind("textDocument/references", &ClangdLSPServer::onReference);
   MsgHandler->bind("textDocument/switchSourceHeader", &ClangdLSPServer::onSwitchSourceHeader);
+  MsgHandler->bind("textDocument/prepareRename", &ClangdLSPServer::onPrepareRename);
   MsgHandler->bind("textDocument/rename", &ClangdLSPServer::onRename);
   MsgHandler->bind("textDocument/hover", &ClangdLSPServer::onHover);
   MsgHandler->bind("textDocument/documentSymbol", &ClangdLSPServer::onDocumentSymbol);
   MsgHandler->bind("workspace/executeCommand", &ClangdLSPServer::onCommand);
   MsgHandler->bind("textDocument/documentHighlight", &ClangdLSPServer::onDocumentHighlight);
   MsgHandler->bind("workspace/symbol", &ClangdLSPServer::onWorkspaceSymbol);
   MsgHandler->bind("textDocument/didOpen", &ClangdLSPServer::onDocumentDidOpen);
   MsgHandler->bind("textDocument/didClose", &ClangdLSPServer::onDocumentDidClose);

clang-tools-extra/trunk/clangd/ClangdServer.h

llvm::Expected<tooling::Replacements> formatFile(StringRef Code,
                                                    PathRef File);
 
   /// Run formatting after \p TriggerText was typed at \p Pos in \p File with
   /// content \p Code.
   llvm::Expected<std::vector<TextEdit>> formatOnType(StringRef Code,
                                                      PathRef File, Position Pos,
                                                      StringRef TriggerText);
 
+  /// Test the validity of a rename operation.
+  void prepareRename(PathRef File, Position Pos,
+                     Callback<llvm::Optional<Range>> 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,
               bool WantFormat, Callback<std::vector<TextEdit>> CB);
 

clang-tools-extra/trunk/clangd/ClangdServer.cpp

ClangdServer::formatOnType(llvm::StringRef Code, PathRef File, Position Pos,
 
   std::vector<TextEdit> Result;
   for (const tooling::Replacement &R :
        formatIncremental(Code, *CursorPos, TriggerText, *Style))
     Result.push_back(replacementToEdit(Code, R));
   return Result;
 }
 
+void ClangdServer::prepareRename(PathRef File, Position Pos,
+                                 Callback<llvm::Optional<Range>> CB) {
+  auto Action = [Pos, this](Path File, Callback<llvm::Optional<Range>> CB,
+                            llvm::Expected<InputsAndAST> InpAST) {
+    if (!InpAST)
+      return CB(InpAST.takeError());
+    auto &AST = InpAST->AST;
+    // Performing the rename isn't substantially more expensive than doing 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.
+    auto Changes = renameWithinFile(AST, File, Pos, "dummy", Index);
+    if (!Changes) {
+      // 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());
+    }
+    SourceLocation Loc = getBeginningOfIdentifier(
+        AST, Pos, AST.getSourceManager().getMainFileID());
+    if (auto Range = getTokenRange(AST.getSourceManager(),
+                                   AST.getASTContext().getLangOpts(), Loc))
+      return CB(*Range);
+    // Return null if the "rename" is not valid at the position.
+    CB(llvm::None);
+  };
+  WorkScheduler.runWithAST("PrepareRename", File,
+                           Bind(Action, File.str(), std::move(CB)));
+}
+
 void ClangdServer::rename(PathRef File, Position Pos, llvm::StringRef NewName,
                           bool WantFormat, Callback<std::vector<TextEdit>> CB) {
   auto Action = [Pos, WantFormat, this](Path File, std::string NewName,
                                         Callback<std::vector<TextEdit>> CB,
                                         llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     auto Changes = renameWithinFile(InpAST->AST, File, Pos, NewName, Index);

clang-tools-extra/trunk/clangd/Protocol.h

struct ClientCapabilities {
   bool SemanticHighlighting = false;
 
   /// Supported encodings for LSP character offsets. (clangd extension).
   llvm::Optional<std::vector<OffsetEncoding>> offsetEncoding;
 
   /// The content format that should be used for Hover requests.
   /// textDocument.hover.contentEncoding
   MarkupKind HoverContentFormat = MarkupKind::PlainText;
+
+  /// The client supports testing for validity of rename operations
+  /// before execution.
+  bool RenamePrepareSupport = false;
 };
 bool fromJSON(const llvm::json::Value &, ClientCapabilities &);
 
 /// Clangd extension that's used in the 'compilationDatabaseChanges' in
 /// workspace/didChangeConfiguration to record updates to the in-memory
 /// compilation database.
 struct ClangdCompileCommand {
   std::string workingDirectory;

clang-tools-extra/trunk/clangd/Protocol.cpp

if (auto *Help = TextDocument->getObject("signatureHelp")) {
       R.HasSignatureHelp = true;
       if (auto *Info = Help->getObject("signatureInformation")) {
         if (auto *Parameter = Info->getObject("parameterInformation")) {
           if (auto OffsetSupport = Parameter->getBoolean("labelOffsetSupport"))
             R.OffsetsInSignatureHelp = *OffsetSupport;
         }
       }
     }
+    if (auto *Rename = TextDocument->getObject("rename")) {
+      if (auto RenameSupport = Rename->getBoolean("prepareSupport"))
+        R.RenamePrepareSupport = *RenameSupport;
+    }
   }
   if (auto *Workspace = O->getObject("workspace")) {
     if (auto *Symbol = Workspace->getObject("symbol")) {
       if (auto *SymbolKind = Symbol->getObject("symbolKind")) {
         if (auto *ValueSet = SymbolKind->get("valueSet")) {
           R.WorkspaceSymbolKinds.emplace();
           if (!fromJSON(*ValueSet, *R.WorkspaceSymbolKinds))
             return false;

clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json

 {
     "name": "vscode-clangd",
     "displayName": "vscode-clangd",
     "description": "Clang Language Server",
     "version": "0.0.15",
     "publisher": "llvm-vs-code-extensions",
     "homepage": "https://clang.llvm.org/extra/clangd.html",
     "engines": {
-        "vscode": "^1.30.0"
+        "vscode": "^1.36.0"
     },
     "categories": [
         "Programming Languages",
         "Linters",
         "Snippets"
     ],
     "keywords": [
         "C",

     "main": "./out/src/extension",
     "scripts": {
         "vscode:prepublish": "tsc -p ./",
         "compile": "tsc -watch -p ./",
         "postinstall": "node ./node_modules/vscode/bin/install",
         "test": "node ./node_modules/vscode/bin/test"
     },
     "dependencies": {
-        "vscode-languageclient": "^5.2.0",
-        "vscode-languageserver": "^5.2.0"
+        "vscode-languageclient": "^5.3.0-next.6",
+        "vscode-languageserver": "^5.3.0-next.6"
     },
     "devDependencies": {
         "@types/mocha": "^2.2.32",
         "@types/node": "^6.0.40",
         "mocha": "^5.2.0",
         "typescript": "^2.0.3",
         "vscode": "^1.1.0"
     },

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

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument": {"rename": {"dynamicRegistration": true, "prepareSupport": true}}},"trace":"off"}}
+# CHECK:      "renameProvider": {
+# CHECK-NEXT:    "prepareProvider": true
+# CHECK-NEXT: },
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"int foo;"}}}
 ---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":5},"newName":"bar"}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/prepareRename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":5}}}
 #      CHECK:  "id": 1,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
+# CHECK-NEXT:      "end": {
+# CHECK-NEXT:        "character": 7,
+# CHECK-NEXT:        "line": 0
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "start": {
+# 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:  },
+# 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:  },
+# 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": {
 # CHECK-NEXT:    "changes": {
 # CHECK-NEXT:      "file://{{.*}}/foo.cpp": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "newText": "bar",
 # CHECK-NEXT:          "range": {
 # CHECK-NEXT:            "end": {
 # CHECK-NEXT:              "character": 7
 # CHECK-NEXT:              "line": 0
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "start": {
 # CHECK-NEXT:              "character": 4
 # CHECK-NEXT:              "line": 0
 # CHECK-NEXT:            }
 # CHECK-NEXT:          }
 # CHECK-NEXT:        }
 # CHECK-NEXT:      ]
 # CHECK-NEXT:    }
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"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:  },
-# CHECK-NEXT:  "id": 2,
-# CHECK-NEXT:  "jsonrpc": "2.0"
----
-{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}