Add a new custom command ocamllsp/hoverExtended with support for variable verbosity

[Khady]

This is a proposal to emulate merlin's behavior when multiple hover commands are received in a row on a single position. Each new call should increase the verbosity counter by 1. The PR is not ready. But I'd like to know if the idea looks fine before to invest more time on it.

https://discuss.ocaml.org/t/merlin-vs-ocaml-lsp/8887

This work is sponsored by Ahrefs

[ulugbekna]

Wow, this works like a charm! Thanks!

Regarding implementation:

1. Am I missing something or could we could use a single option to represent the last hover that happened and verbosity with which it happened?
2. Maybe reset verbosity to 0 when increasing it doesn't change the type? I'd imagine this would eliminate the need to hover something to get the initial type (with verbosity 0)

https://user-images.githubusercontent.com/16353531/143781598-f539ec34-b372-4fca-bb93-9164c65723eb.mov

[ulugbekna]

There is a somewhat related PR in merlin repo btw: https://github.com/ocaml/merlin/pull/1374

[rgrinberg]

I understand the appeal of doing it on the server - we do it once and it automatically works everywhere. Unfortunately, there's also a serious downside. The LSP server is a basic block for editors to build functionality on top of, and this behavior is making a pretty basic request unpredictable. It might not matter for users of vscode, but in other editors, lsp requests can be scripted to provide higher level functionality and this is a wrench thrown in that work.

So I suggest we leave basic requests alone and do one of:

• Create a new request that automatically handles verbosity if you insist on handling the state on the server.
• Save the state on the client side and provide verbosity as a parameter in a custom request.

[Khady]

Regarding implementation:

1. Am I missing something or could we could use a single option to represent the last hover that happened and verbosity with which it happened?
2. Maybe reset verbosity to 0 when increasing it doesn't change the type? I'd imagine this would eliminate the need to hover something to get the initial type (with verbosity 0)

1. sure no problem
2. because the position stored is the one of the cursor, not the one of the value, I think that getting the original hover is not a real problem, it's a matter of moving the cursor by one. But I'm fine with implementing this behavior if there's a consensus on it

Create a new request that automatically handles verbosity if you insist on handling the state on the server.

Ok I can do that. Any suggestion on the name? HoverVerbose? HoverSmart?

Thanks to both of you for the quick response

[ulugbekna]

Regarding designing a custom hover request, I think this may be an opportunity to design the request to also support selections (ie show types for selections) that was also discussed in the ocaml discuss thread mentioned earlier I think. Reference of

• Scala metals implementation if interesting: https://github.com/scalameta/metals/pull/3060
• Rust analyzer: https://github.com/microsoft/language-server-protocol/issues/377#issuecomment-889898650

[Khady]

I've started to work on adding a new command. I'm not sure exactly how much work it would require in the client to become usable. I am scared that it becomes very troublesome to use. But in the process I found that it is possible to have multiple hover providers, which could be an alternative way to have this feature. One provider could give the normal type, one could give the doc, one could give a more and more verbose answer. But I don't know if one server can have multiple hover providers.

Anyway, in the process of adding a new command I had to duplicate quite a lot of code from ocaml_lsp_server.ml to req_hover_extended.ml. Because the later is called from the former. So all the utils can't be called from the custom commands. I wonder where I should move that code to avoid duplication.

https://github.com/Khady/ocaml-lsp/blob/c2bec75749a5d2c3968d2364e86ca2fb1fa196e5/ocaml-lsp-server/src/custom_requests/req_hover_extended.ml#L31-L80

[ulugbekna]

I've started to work on adding a new command. I'm not sure exactly how much work it would require in the client to become usable. I am scared that it becomes very troublesome to use. But in the process I found that it is possible to have multiple hover providers, which could be an alternative way to have this feature. One provider could give the normal type, one could give the doc, one could give a more and more verbose answer. But I don't know if one server can have multiple hover providers.

I haven't looked into this too closely, but I was under assumption that for vscode client, we simply add our own hover provider that uses the custom hover request-response and turn off the provider that uses the default LSP request for hovers. Does this make sense?

Anyway, in the process of adding a new command I had to duplicate quite a lot of code from ocaml_lsp_server.ml to req_hover_extended.ml. Because the later is called from the former. So all the utils can't be called from the custom commands. I wonder where I should move that code to avoid duplication.

This PR should help with this.

[Khady]

A few months later, I'm back with an updated version of this PR. There's no state on the server anymore, it is left to the client. I'm not handling selections (solving one problem at a time). And I haven't tried the change in an editor yet. Sadly it looks like my change to send the verbosity to merlin no longer works

https://github.com/ocaml/ocaml-lsp/blob/259bf6e0a016d209c719ab9adecf1f88c072e71a/ocaml-lsp-server/src/document.ml#L281-L291

It was fine it my original commit https://github.com/Khady/ocaml-lsp/commit/a25a778bad222d1db3790ff16c535451f29759e2

Summary of all failing tests
 FAIL  __tests__/ocamllsp-hoverExtended.ts
  ● ocamllsp/hoverExtended › 

    expect(received).toMatchInlineSnapshot(snapshot)

    Snapshot name: `ocamllsp/hoverExtended  2`

    - Snapshot  - 1
    + Received  + 1

    @@ -1,9 +1,9 @@
      Object {
        "contents": Object {
          "kind": "plaintext",
    -     "value": "int",
    +     "value": "foo",
        },
        "range": Object {
          "end": Object {
            "character": 5,
            "line": 2,

      567 |     });
      568 |
    > 569 |     expect(result1).toMatchInlineSnapshot(`
          |                     ^
      570 |       Object {
      571 |         "contents": Object {
      572 |           "kind": "plaintext",

      at __tests__/ocamllsp-hoverExtended.ts:569:21
      at fulfilled (__tests__/ocamllsp-hoverExtended.ts:28:58)

  ● ocamllsp/hoverExtended › 

    expect(received).toMatchInlineSnapshot(snapshot)

    Snapshot name: `ocamllsp/hoverExtended  3`

    - Snapshot  - 1
    + Received  + 1

    @@ -1,9 +1,9 @@
      Object {
        "contents": Object {
          "kind": "plaintext",
    -     "value": "int",
    +     "value": "foo",
        },
        "range": Object {
          "end": Object {
            "character": 5,
            "line": 2,

      592 |     });
      593 |
    > 594 |     expect(result2).toMatchInlineSnapshot(`
          |                     ^
      595 |       Object {
      596 |         "contents": Object {
      597 |           "kind": "plaintext",

      at __tests__/ocamllsp-hoverExtended.ts:594:21
      at fulfilled (__tests__/ocamllsp-hoverExtended.ts:28:58)

[Khady]

actually if I hardcode a verbosity of 100 it seems to do something (it breaks other tests). So maybe it is my test that isn't correctly written?

[ulugbekna]

Since we're creating a new request there should be no harm in keeping the state on the server side. I would hate for every client to have to implement the same thing.

AFAIU, Rudi is not against it as long as we have a custom request for it:

So I suggest we leave basic requests alone and do one of:

Create a new request that automatically handles verbosity if you insist on handling the state on the server. Save the state on the client side and provide verbosity as a parameter in a custom request.

[Khady]

At this point I don't have a strong opinion anymore. As soon as it's a new request it requires some extra work in every client anyway. I guess the new request can even support both. If verbosity is passed as an argument then it is forwarded to merlin. If it is omitted some server side state is used instead. Pleases everyone?

[rgrinberg]

At this point I don't have a strong opinion anymore. As soon as it's a new request it requires some extra work in every client anyway. I guess the new request can even support both. If verbosity is passed as an argument then it is forwarded to merlin. If it is omitted some server side state is used instead.

Sounds fine to me.

Pleases everyone?

It's a new and custom request, and you're the one taking the initiative, so feel free to make the final call.

[Khady]

the last commit factorizing the code of the two hover requests was easy to write so I thought it was worth raising the idea. But I'm happy to drop it if we prefer to keep the code of the two requests more separated

[Khady]

gentle ping on this one, I'd like to put a final effort so that it can get merged

[rgrinberg]

Looks OK apart from some minor issues. By the way, we've deprecated the typescript test suite and are writing tests in OCaml from now on. Do you mind porting your tests to OCaml? There's plenty of examples in e2e-new.

[Khady]

I think I addressed all your comments.

For the ocaml e2e tests, I've been able to write something and get it running. But I'm a bit lost. It works fine for the default hover request, even when in extended mode thanks to the environment variable. But for hoverExtended I always receive a null answer. Am I missing some kind of special wait that must be done for custom requests only?

[ulugbekna]

But for hoverExtended I always receive a null answer. Am I missing some kind of special wait that must be done for custom requests only?

Req_hover_extended.on_request is returning Null (I put an exception in that pattern matching branch, which throws), so it must be Hover_req.handle that is returning None somehow.

[Khady]

Ok I figured out the problem.There was a hardcoded position that I forgot to remove...

[rgrinberg]

@Khady since the PR is approved you can squash & merge at your convenience.

[Khady]

Thanks to both of you for the help!

[copy]

In case someone is wondering how to call this with nvim's lsp:

    vim.keymap.set("n", "<f2>", function()
        local params = vim.lsp.util.make_position_params()
        params["verbosity"] = 1
        vim.lsp.buf_request(0, "ocamllsp/hoverExtended", params, vim.lsp.handlers.hover)
    end, bufopts)