Page MenuHomePhabricator

[clangd] Implement "prepareRename"

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


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


Diff Detail


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
393 ↗(On Diff #203991)

why set this only if the client advertised support?

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...

412 ↗(On Diff #203991)

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

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
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.

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