microsoft / microsoft/vscode-languageserver-node
`prepareRename` response `{ defaultBehavior: true }` is treated the same as `null`
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 1.8k
- Forks
- 404
- Avg merge
- 2d 8h
- Merged PRs (30d)
- 14
Description
The LSP 3.17 spec defines distinct semantics for null vs { defaultBehavior: true } from textDocument/prepareRename:
If
{ defaultBehavior: boolean }is returned (since 3.16) the rename position is valid and the client should use its default behavior to compute the rename range. Ifnullis returned then it is deemed that atextDocument/renamerequest is not valid at the given position.
From what I can tell, the current implementation of textDocument/prepareRename packaged in the method client.renameProvider() which treats both null and { defaultBehavior: true } as the same case (i.e., the rename position is invalid which skips textDocument/rename requests from being sent).
[!TIP]
For the time being, I got around the unexpected behavior of{ defaultBehavior: true }being treated asnullby returning the range and placeholder values passed intotextDocument/prepareRename
To make the use case of { defaultBehavior: true } consistent across language-clients, textDocument/prepareRename should be able to determine this case is a valid rename position for clients who use the vscode-languageclient package
Prior discussion of this issue occurred in coc.nvim issue #5583, which importantly points out that coc.nvim's design mirrors the behavior of the client source code in this repo. This makes the fix merged in coc.nvim PR #5591 very easy to translate to this repo, in file
client/src/common/rename.ts#L100-L102:} else if (this.isDefaultBehavior(result)) { - return result.defaultBehavior === true ? null : Promise.reject(new Error(`The element can't be renamed.`)) + return result.defaultBehavior === true ? result : Promise.reject(new Error(`The element can't be renamed.`)) }
I submitted #1748 to reflect this change + I added some new tests for verifing the behavior of textDocument/prepareRename the /client & /client-node-tests for the new rename related features.
Although, I should probably mention that I don't normally use a language client that provides its own defaultBehavior rename provider (neovim user), so maybe the confusion I've encountered and my purposed fix isn't needed here. In which case, I still think it might be beneficial to allow more distinction between defaultBehavior: true vs null prepareRename request responses
Happy to make any adjustments to the PR if needed, just let me know!
Thanks for all the work y'all maintainers have put into this project over the years! It's amazing
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
The affected entry point is client/src/common/rename.ts; start by reading the textDocument/prepareRename handling and the behavior covered by the new tests in /client and /client-node-tests. Run those tests and confirm that null remains invalid while { defaultBehavior: true } is treated as a valid rename position.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- node.js, typescript
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100