OCamlformat RPC

[voodoos]

This PR add support for formatting of types in popups. Types provided by Merlin are not always nicely formatted and module signatures often wrap because it exceeds the maximum number of columns of a VSCode popup (64 characters on my setup). The idea is to use OCamlformat to format these types, but starting a new process would be to heavy for that usage.

In a joint effort with the OCamlformat team this PR acheive type formatting by spawning an "OCamlformat RPC server" and then communicating with it using a simple protocol. The protocol is vendored for now (in the file ocamlformat_rpc_lib.ml but will be made available as a separate and common package.

In case of failure to start the server, negotiate a version or receive a response we log some information and fallback to the non-formatted type.

Todo

• Wait for OCaml-RPC lib to be published as a separate package and remove the vendored copy.
• Change the program name to the definitive name of the OCamllsp-RPC server when known. (for now it is a hard path to my copy of OCamlformat).

Related works

• This OCamlformat PR tracks the progress on the RPC library and the server implementation.
• This mdx PR tracks the progress on implementing a client similar to the one of this PR to format code blocks in mdx.

[gpetiot]

ocamlformat-rpc-lib is now available on opam!

[voodoos]

About error messages, I think most of them will stay as debug messages.
The only user-facing message should be an information diagnostic showing when OCaml-LSP cannot find the ocamlformat-rpc binary and the user should install it for full functionality.

OCamlformat crashes and errors should be kept in the debug log: there are useful for devs to debug issues but useless to the users and unrelated to their programs.

[voodoos]

As you suggested it in a comment I decided to stick more closely to how Dune RPC process is handled (in
42b0971):
I start the OCamllsp_rpc server and wait for it to finish in Ocaml_lsp_server.start:

Fiber.parallel_iter
          ~f:(fun f -> f ())
          [ (fun () -> Server.start server)
          ; (fun () ->
              let* _state = Ocamlformat_rpc.run ocamlformat_rpc in
              (* TODO ulysse Handle errors*)
              Fiber.return ())
          ; (fun () -> ...  Dune.run dune  ... )
          ]

However I have a problem to stop it: you appear to stop the DRPC client after this first fiber returns:

      match !dune with
      | None -> Fiber.return ()
      | Some dune -> Dune.stop dune)

But I don't understand why the first fiber would return before the process is stopped because it is waiting for the process to stop to complete ? In any case I had that problem with OCamlformat server and decided to stop it explicitely when the Server does:

Fiber.parallel_iter
          ~f:(fun f -> f ())
          [ (fun () ->
              let* () = Server.start server in
              (* When server exits we also stop ocamlformat rpc *)
              Ocamlformat_rpc.stop ocamlformat_rpc)
          ; (fun () ->
              let* _state = Ocamlformat_rpc.run ocamlformat_rpc in
              (* TODO ulysse Handle errors*)
              Fiber.return ())
          ; (fun () -> ...  Dune.run dune  ... )
          ]

This way the second item in the parallel iter does complete because it watches the process' exit and the process is exited after stopping the server in the first item of the parallel iter.

Is that the right thing to do or did I miss something essential ?

(Also when I use Dune master the ocamllsp process does not exits when asking a reload of the server in vscode, process are multiplying , which does not happen with a version of Dune without the rpc server)

[rgrinberg]

Is that the right thing to do or did I miss something essential ?

You aren't missing anything. I already had this fixed in a branch that I merged yesterday.

[voodoos]

Is that the right thing to do or did I miss something essential ?

You aren't missing anything. I already had this fixed in a branch that I merged yesterday.

Great, I will have one last pass and it will be ready for a (hopefully) final review.

[voodoos]

I refactored the module a bit.

I also added a user notification (ShowNotification) showing when the ocamlformat-rpc binary is not found.
However I do not show other errors, they are logged only if debug is enabled.

Some types will fail to be formatted by ocamlformat: Merlin prints types which may be disambiguated with a stamp like in t/1 -> t/2 and this is not valid syntax.
In this case a non-formatted type will be showed to the user as before.

@gpetiot is working on supporting this syntax in ocamlformat.

[rgrinberg]

Looks much better now! There are still some opaque error messages that need fixing (or removing)

[voodoos]

I reworked the logging in ead0825.

Edit: running the tests spawns a huge number of hard-to-kill ocamlformat-rpc instances, I will have to investigate that. Tests also fail on CI because ocamlformat-rpc is not installed.

[rgrinberg]

running the tests spawns a huge number of hard-to-kill ocamlformat-rpc instances, I will have to investigate that. Tests also fail on CI because ocamlformat-rpc is not installed.

That's definitely problematic. I forgot to mention that we should delay launching the process until we actually request it to format something.

[voodoos]

I decided to use a Fiber.Mvar.t in 98d4ca7 to only start the process when the first query is done and then perform that query as soon as the process started. The lifecycle of this Mvar is described in a comment.

Along with some marginal tweaks this does solve the tests. No more zombies

[rgrinberg]

Looks good. I modified the code to us two ivars instead of the mvar. I'm more confident with an ivar that we won't double write when we shouldn't.