LSP integration: inconsistent handling of goto-definition for selected text

[DavidGoldman]

• VSCode Version: 1.43.0
• OS Version: macOS Catalina 1.15.3

Triggering goto-definition on selected (e.g. to copy) text depends upon how the text itself was selected.

This is because the goto definition request uses a different text position depending upon the end point of the selection. For a fix, I think it should always use the beginning of the selection as judged by earlier file offset.

Steps to Reproduce:

1. Use any LSP that's available within VSCode. For my example, I'll be using https://github.com/apple/sourcekit-lsp on itself
2. I'll be using the text here:

extension BuildServerBuildSystem: BuildSystem {

Examples:

1. extension BuildServerBuildSystem: BuildSystem { --------------------- ^ ----------- double click to select Goto-definition doesn't work (VSCode appears to send the position at char :)

2. extension BuildServerBuildSystem: BuildSystem { ------------ ^------------------------^ ------------start------------------- end Goto-definition doesn't work (VSCode appears to send the position at char :)

3. extension BuildServerBuildSystem: BuildSystem { ------------ ^------------------------^ ------------end------------------- start Goto-definition does work (VSCode appears to send the position at char )

For these cases VSCode should also send the position at char before the selection

[DavidGoldman]

From Selection this might to be due to getPosition() returning the ending column + line.

There's also SingleCursorState which references cursor information but I'm not sure how that is used

[dbaeumer]

Actually this needs to be addressed in the LSP libs since LSP doesn't have the concept of a selection direction.

[DavidGoldman]

In the meantime a potential fix would be to use the initial position, but long term that makes sense

[dbaeumer]

I looked at the VS Code API again and it looks like VS Code only passes a position.

@jrieken can you comment on what the expected behavior here should be in terms of selection and goto definition. Thanks!

[jrieken]

Every selection has an active position (can be the start or end of the selection) which is the position of the cursor. For all language features we use that, not the selection

[dbaeumer]

Looks like there is nothing LSP can do about it.

@DavidGoldman in which language are you seeing this?

[DavidGoldman]

I was seeing this with Swift (SourceKit-LSP) but it should be reproducible in other LSPs as well. I recommend a fix of using the start of the selection to make this more consistent, especially since when double clicking to select it will default to the error case (see my example in the first post)

[DavidGoldman]

e.g. have VSCode pass the position of the start of the selection instead of the active position

I can re-open this against VSCode if you want.

[rcjsuen]

Looks like there is nothing LSP can do about it.

It feels to me like the protocol should state whether the start or the end of the selection should be sent. I do however agree with @DavidGoldman that the decision should be to send the start of the selection.

[dbaeumer]

I actually disagree. The position that should be used is the one where the cursor blinks. Otherwise it is IMO confusion for the user.

[rcjsuen]

I actually disagree. The position that should be used is the one where the cursor blinks. Otherwise it is IMO confusion for the user.

OK. This should be documented in the protocol then.

[DavidGoldman]

I actually disagree. The position that should be used is the one where the cursor blinks. Otherwise it is IMO confusion for the user.

I disagree, most people who are selecting text to copy don't care about the cursor position - they care about the selection itself unless they're actually trying to edit it. I didn't even notice it exists until you mentioned it. They care about the symbol selected, for which the start position is a better fit. FWIW, I've filed this bug because:

1) Users did not find this intuitive - they filed a bug internally because it did not operate as expected 2) Xcode does not have this issue - it appears to send the start position

[dbaeumer]

At the end this is not a protocol issue. It is a client / editor decision. If for consistency an editor decides to honor the direction of a selection it is free to do so. If XCode decides to operate differently it is fine as well. I am against specing this in the protocol and forcing every client to implement the same behavior. What if an editor has a LSP server powering a language feature and for another feature not. How should a client behave in that case.

[DavidGoldman]

At the end this is not a protocol issue. It is a client / editor decision. If for consistency an editor decides to honor the direction of a selection it is free to do so. If XCode decides to operate differently it is fine as well. I am against specing this in the protocol and forcing every client to implement the same behavior. What if an editor has a LSP server powering a language feature and for another feature not. How should a client behave in that case.

Doesn't the selection need to be in the protocol? Otherwise how can you expect the editor to tell the difference between a selection vs. the user manually trigger requests at that cursor position (you can't). Such that if you were designing an LSP for a specific editor, it might not work properly with another editor due to differences in how the editor handles selection.

At the very least the LSP documentation should include this behavior: selection may occur in different ways and the LSP should receive the active cursor position of the selection or it's intentionally left unspecified.

[rcjsuen]

At the very least the LSP documentation should include this behavior: selection may occur in different ways and the LSP should receive the active cursor position of the selection or it's intentionally left unspecified.

Agreed. The protocol should be clear that the position is not standardized and that servers should account for this if possible.

[dbaeumer]

The protocol should be clear that the position is not standardized and that servers should account for this if possible.

IMO this is not correct. A position is standardized and the server shouldn't account for. It can't in the current design of the protocol.

If we want to give servers a choice I agree that we need to change the API from position to selection. But I am not sure if this is worth it.

I do agree that the documentation should mention that the editor / library converts a selection to a position and that the decision is made by the editor / library.

[rcjsuen]

The protocol should be clear that the position is not standardized and that servers should account for this if possible.

IMO this is not correct. A position is standardized and the server shouldn't account for. It can't in the current design of the protocol.

If we want to give servers a choice I agree that we need to change the API from position to selection. But I am not sure if this is worth it.

I do agree that the documentation should mention that the editor / library converts a selection to a position and that the decision is made by the editor / library.

I think we are ultimately saying the same thing so I'll wait for the changes to be written into the microsoft/language-server-protocol repository before commenting further.

[dbaeumer]

Added this to the spec:

It is up to the client to decide how a selection is converted into a position when issuing a request for a text document. The client can for example honor or ignore the selection direction to make LSP request consistent with features implemented internally.

I think we need a different item if we want to support selections instead of positions.