Page MenuHomePhabricator

[clangd] Implement "prepareRename"
ClosedPublic

Authored by hokein on Jun 11 2019, 2:30 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Jun 11 2019, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 2:30 AM
hokein updated this revision to Diff 211080.Jul 22 2019, 6:47 AM

Update the patch, and add tests.

hokein edited the summary of this revision. (Show Details)Jul 22 2019, 6:48 AM
sammccall added inline comments.Jul 23 2019, 4:42 AM
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 updated this revision to Diff 211288.Jul 23 2019, 6:55 AM
hokein marked 6 inline comments as done.

Address review comments.

hokein added inline comments.Jul 23 2019, 6:55 AM
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 accepted this revision.Jul 23 2019, 7:24 AM
sammccall added inline comments.
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"

This revision is now accepted and ready to land.Jul 23 2019, 7:24 AM
hokein updated this revision to Diff 211302.Jul 23 2019, 7:43 AM
hokein marked 4 inline comments as done.

Address comments

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2019, 12:51 AM