Send range with textDocument/hover when possible

[ayoub-benali]

This relies on experimental.rangeHoverProvider which so far only Metals implement. But this workaround would be removed once microsoft/language-server-protocol#377 is part of the LSP specification.

Fixes scalameta/metals-sublime#44

[rwols]

Sorry, but I still think this is way too complicated for its own good?

I was imagining a server capability path hoverProvider.rangeSupport, which is a boolean, which is true when the server has support for a range in the request.

Client side request:

• If the server has hoverProvider.rangeSupport, and
• If the position is contained in the current selection, and
• If the current selection is not empty,
• then add params["range"] = region_to_range(first_selection_region(view), view)

(I'm unsure if all these additional checks other than the hoverProvider.rangeSupport are needed. I could imagine other servers wanting to have the information about the current position in any case, whether it's contained in the range or not)

Server side:

• Check whether the request contains a "range" and do something clever with that

Client side response:

• Show whatever the server returns

This whole scheme requires no more than 8 lines of diff, no? We're in a position to cook up an additional server capability and enhancement to the textDocument/hover request and I think the above scheme is the least intrusive for the spec.

[rwols]

By the way, whether the server capability is considered experimental or not (experimental.rangeHoverProvider) is an implementation detail for LSP-metals I think.

[ayoub-benali]

Client side request:

If the server has hoverProvider.rangeSupport, and
If the position is contained in the current selection, and
If the current selection is not empty,
then add params["range"] = region_to_range(first_selection_region(view), view)
(I'm unsure if all these additional checks other than the hoverProvider.rangeSupport are needed. I could imagine other servers wanting to have the information about the current position in any case, whether it's contained in the range or not)

Two scenarios are missing:

• lsp_hover is invoked via keyboard and the mouse isn't hovering. textDocument/hover should be sent with selection
• there is a selection but mouse is hovering an other location. textDocument/hover should be sent with mouse position

This is why there are four cases

[rwols]

I mean, maybe I want to see both the expression type of the range as well as type info of the position outside of the range using the mouse? Why must that be excluded?

[ayoub-benali]

yes and it is possible with the current implementation. We just can't send a request with both fields set: https://scalameta.org/metals/docs/integrations/new-editor#textdocumenthover

Why must that be excluded?

it is not excluded but as I said there are multiple combinations and these four checks are there for that.
Are you saying you can handle all those cases with fewer checks ?

[jwortmann]

lsp_hover is invoked via keyboard and the mouse isn't hovering. textDocument/hover should be sent with selection

This still feels wrong to me and I would argue to not send the selection if it is entirely unrelated to mouse hover. Shouldn't it be a separate command which can be used to assign a key binding then? For example lsp_selection_info or so.

[jwortmann]

Well, actually the lsp_hover already takes the optional point argument, and as implemented here it is just extending this concept. So feel free to ignore my concern if it's useful to combine them all into lsp_hover, even though not really related to "hover".

[rchl]

lsp_hover is invoked via keyboard and the mouse isn't hovering. textDocument/hover should be sent with selection

This still feels wrong to me and I would argue to not send the selection if it is entirely unrelated to mouse hover. Shouldn't it be a separate command which can be used to assign a key binding then? For example lsp_selection_info or so.

When using the keyboard the "point" has to follow the caret. It wouldn't make sense to follow the mouse pointer when using keyboard.

Yes, the naming of textDcoument/hover request makes it confusing to think about in terms of keyboard usage but if we want to allow triggering the hover info from the keyboard (which we are and should) then it only makes sense to follow the caret position. It would be unusable otherwise.

[jwortmann]

Right, I think I had this confusion because I don't have this keybinding set up, and because lsp_hover does different things depending on how it was invoked. Should be fine then as it is!

[ayoub-benali]

@rwols I simplified it now and all cases work. Sorry for missing your point last time.

[rwols]

I accidentally approved it :) see above comment.

[rchl]

I personally am pretty OK with those changes.
Just make sure to fix all the type/test issues.

[rchl]

I've just noticed that rust-analyzer supports the same experimental feature but implemented differently and with different capability. https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/lsp-extensions.md#hover-range

It would be ideal if both server's implementation would be unified :)

[ayoub-benali]

It would be ideal if both server's implementation would be unified :)

TBH I have no involvement with the server development so it is gonna be very difficult getting the two servers to agree on the capability and implementation without an official spec :/

[rwols]

Am I the only one nagging too much?

These two comments describe position OR range:

microsoft/language-server-protocol#377 (comment)
microsoft/language-server-protocol#377 (comment)

But both metals and rust-analyzer now want position XOR range. It doesn't make sense to me to necessarily omit the position when there is a range. @tgodzik can't metals be modified to take position OR range? I don't want to get into a situation where we have to maintain an old version of metals for position OR range while a future standardized version of VSCode uses position XOR range. Let us figure out what the correct params look like before committing to this kind of code.

[ckipp01]

can't metals be modified to take position OR range

Just to be super clear, you're asking for it to be changed to what is listed in this comment right?

microsoft/language-server-protocol#377 (comment)

If so, it's actually pretty easy to change this on our end, we just need to make sure the clients that have implemented this are aware of it. If so, it'd be as simple as just changing the logic in this single method:

https://github.com/scalameta/metals/blob/3f211b03d6c2d1ff4d5fa936d50a4aab013100fb/metals/src/main/scala/scala/meta/internal/metals/HoverExtension.scala#L14-L16

We could just check to see if range is there, if so use it, if not, use the fallback to the position. I don't think this would be an issue unless @tgodzik sees something I'm missing. The only negative part of this is that then we still have diverging implementations where Metals would basically be using:

interface HoverParams extends WorkDoneProgressParams {
    textDocument: TextDocumentIdentifier;
    position: Position;
    range?: Range;
}

whereas rust would be using:

interface HoverParams extends WorkDoneProgressParams {
    textDocument: TextDocumentIdentifier;
    position: Range | Position;
}

[ayoub-benali]

I don't want to get into a situation where we have to maintain an old version of metals for position OR range while a future standardized version of VSCode uses position XOR range

Metals haven't reached 1.0 so breaking changes are expected and even more for features under experimental. So I wouldn't worry about that.

If Metals' implementation would be to change then I prefer the first option:

interface HoverParams extends WorkDoneProgressParams {
    textDocument: TextDocumentIdentifier;
    position: Position;
    range?: Range;
}

[tgodzik]

Currently Metals has HoverParams specified as:

case class HoverExtParams(
    textDocument: TextDocumentIdentifier,
    @Nullable position: Position = null,
    @Nullable range: Range = null
) {
  def getPosition: Position =
    if (position != null) position
    else range.getStart()
}

so it can have both position and range, where range will only be used for specific functionality (type on selection).

https://scalameta.org/metals/docs/integrations/new-editor#textdocumenthover

So I don't think any change is really needed on the server side? If I am following the discussion correctly?

[ayoub-benali]

Thanks for the info @tgodzik ! I wasn't aware that both position and range could be set.
The documentation you sent says otherwise

 /** Either `position` or `range` should be specified */

On the Server side there is nothing to change expect fixing the doc.

@rwols is it enough if the implementation is updated to simply include the range when possible to get this PR merged ?

[tgodzik]

The documentation you sent says otherwise

The comment is wrong I think, the spec underneath it doesn't say that only one could be sent. And from the implementation it seem both are fine.

[ckipp01]

I think there are multiple different conversations going on that are sort of mixing. If I'm understanding what @rwols was pointing out, the conversations are referring to position always being sent with an optional range. We could still do this, but then the logic we have would still need to change since we default to position, which would break in the examples above.

def getPosition: Position =
    if (position != null) position
    else range.getStart()

the spec underneath it doesn't say that only one could be sent.

The spec doesn't say it, but we sort of assume it since if the position is there we take it no matter what.

And from the implementation it seem both are fine.

Not really since if a user sends both, they'll never get the range.

[tgodzik]

Not really since if a user sends both, they'll never get the range.

Though in theory this should not be an issue, no? getPosition is used for features that do not use range, so they would not have a use for that anyway. Range is still used for expression type on selection feature

[ckipp01]

Though in theory this should not be an issue, no? getPosition is used for features that do not use range, so they would not have a use for that anyway. Range is still used for expression type on selection feature

Ahhh you're totally right. I was mistaken on how that method was actually used.

[ayoub-benali]

@rwols I modified the request to send position and range when both are available.

[rwols]

Sorry for the lag. I’ll get to this in the weekend!