Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify responses to rename request. #616

Closed
wants to merge 1 commit into from

Conversation

doriath
Copy link

@doriath doriath commented Nov 28, 2018

The main goal is to clarify how the null value should be used.
Currently it is not clear in what cases server is allowed to return it,
and how clients should interpret it. #566

The main goal is to clarify how the `null` value should be used.
Currently it is not clear in what cases server is allowed to return it,
and how clients should interpret it.
@doriath
Copy link
Author

doriath commented Nov 28, 2018

Why?

First, let me explain why I would like to add this clarification here. ~2 months ago I was playing with RLS (Rust language server) and vim-lsp (client for vim) and I found that rename was not a great user experience.

Here is why:

  • RLS returns null when rename did not succeed (for any reason). This also included cases like the code doesn't compile, so rename cannot be performed
  • vim-lsp does nothing when null is returned

This means that when I tried to rename something and it failed, I had no message telling me that something failed. From the client (vim-lsp) perspective, it is not clear how to interpret the null response - was there just nothing to do, or was there some error and rename was not performed.

Alternative

In my Pull Request, I went with the proposal of treating null as "nothing has to be done". The alternative approach could be to use it as generic "rename failed" indication.

If this one is preferred, I would propose:

  • clarify that clients can treat this value as "can't perform rename at this position"
  • generally discourage server implementations from returning null, in favor of returning clear error message. For example: the position is not at any symbol (e.g. space or brackets), current symbol does not support renames or the code doesn't compile.

After writing this explanation, I am slightly starting to lean toward the alternative proposal, but I will wait for the feedback first.

@@ -4063,8 +4063,8 @@ interface RenameParams {
```

_Response_:
* result: [`WorkspaceEdit`](#workspaceedit) \| `null` describing the modification to the workspace.
* error: code and message set in case an exception happens during the rename request.
* result: [`WorkspaceEdit`](#workspaceedit) \| `null` describing the modification to the workspace. `null` should be treated the same was as [`WorkspaceEdit`](#workspaceedit) with no changes, and should only be returned when `newName` is the same as the old name (no change was required).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "the same way as".

* result: [`WorkspaceEdit`](#workspaceedit) \| `null` describing the modification to the workspace.
* error: code and message set in case an exception happens during the rename request.
* result: [`WorkspaceEdit`](#workspaceedit) \| `null` describing the modification to the workspace. `null` should be treated the same was as [`WorkspaceEdit`](#workspaceedit) with no changes, and should only be returned when `newName` is the same as the old name (no change was required).
* error: code and message set in case when rename could not be performed for any reason. Examples include: there is nothing at given `position` to rename (like a space), given symbol does not support renaming by the server or the code is invalid (e.g. does not compile).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use either "in case" or "when", not both.

@dbaeumer
Copy link
Member

@doriath there is now a PrepareRenameRequest that can be sent from the client to the server to test if a position can be renamed. So I would like to only clarify the spec that null should be treated the same way as an empty edit. I would not bind this to newName is the same than oldName.

Can you adjust the PR accordingly ?

@doriath
Copy link
Author

doriath commented Dec 12, 2018

dbaeumer@ I agree that there is now PrepareRenameRequest that mostly solves this issue, but even if I control one side (client or server), the other side can still opt-out. That means that sometimes I will need to use just the RenameRequest and I want to make sure I can handle it properly on both sides.

Just so that I understand, is there another case where null / empty edit could be returned, other than newName equal oldName?

@dbaeumer
Copy link
Member

@doriath there is a difference between a rename request at an invalid position. In this case a server should return an error. If it did nothing (for whatever reason) then it should return null or an [] array.

@dbaeumer
Copy link
Member

dbaeumer commented Nov 5, 2021

Merged by hand into 3.17.

@dbaeumer dbaeumer closed this Nov 5, 2021
@dbaeumer dbaeumer added this to the 3.17 milestone Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants