ocaml / merlin

Context sensitive completion for OCaml in Vim and Emacs
https://ocaml.github.io/merlin/
MIT License
1.59k stars 233 forks source link

LSP: replace ppx_deriving_yojson? #937

Closed Khady closed 5 years ago

Khady commented 5 years ago

merlin-lsp relies on ppx_deriving_yojson for the (de-)serialization of messages in the protocol. One downside is that is not possible to use dune + ppx_deriving_yojson on ocaml 4.02.3, at least with the constraints that are in opam-repository (I have a branch that compiles on 4.02.3 https://github.com/Khady/merlin/commit/9325d1d9e4f2d3de0106a1a51d9479c0326bebb0).

Should merlin-lsp be able to support 4.02.3 and bucklescript? If yes, what is the best solution?

  1. The messages could be parsed by hand, if the goal is to limit the number of dependencies. The json objects are not terribly complicated. But it is tedious, repetitive and error prone.
  2. A tool like atdgen could be used. One good point with atdgen is that the output could be committed to the repository. In this situation, atdgen would only be a dev dependency.
  3. Something I didn't think about?

Maybe it's just better to stick to the current situation and wait for bucklescript to add support for 4.06.

cc @andreypopp @bryphe @rvantonder @rusty-key who might have an opinion.

andreypopp commented 5 years ago

I think even if BS adds support for 4.06 we still want to get rid of ppx_deriving_yojson simply because it introduces so many deps into opam switch / esy project which has merlin (and merlin is used in 99% projects I've seen).

I'm voting for atdgen — #936 does something similar with menhir and thanks to dune's promote feature it doesn't sound too difficult to do (maybe I'm missing something though as I haven't used atdgen yet).

trefis commented 5 years ago

The other option you could consider is keep using ppx_deriving_yojson, but only in dev mode, using [@@deriving_inline].

Base uses that workflow, it is described on its README. (EDIT: actually, it's not so well described, and I wonder if it might not rely on tooling internal to janestreet. Perhaps adtgen would be simpler then...)

Khady commented 5 years ago

deriving_inline would be great. Support is actually available in dune. But it seems to to only handle plugins based on ppxlib. So not ppx_deriving_yojson. I'm almost tempted to write a yojson deriver based on ppxlib at this point. But it's a bit a waste of time.

hcarty commented 5 years ago

I'm almost tempted to write a yojson deriver based on ppxlib

There is https://github.com/andersfugmann/ppx_protocol_conv/tree/master/drivers/json

I'm not sure how it compares to ppx_deriving_yojson in practice.

Khady commented 5 years ago

We discussed with @andreypopp about this issue. We would like to give a try to deriving_inline and ppx_protocol_conv. And switch to Yojson.Basic.jon later. So here is the plan:

Khady commented 5 years ago

So deriving inline works well. But my branch using ppx_protocol_conv introduces a dependency on compiler libs that I don’t manage to remove.

Also ppxlib is about to change again: https://github.com/ocaml-ppx/ppxlib/wiki/roadmap-v1

So if someone wants to experiment more with ppx, I support you. But I’ll stay away from those tools for now.

XVilka commented 5 years ago

Can be this closed then? Since BuckleScript was updated (see 6.2.0 version) and there is no reason to rely on the old 4.02 anymore. And ppx_deriving_yojson was updated to support latest OCaml as well. ppxlib will not change, but rather will be required to use ppx instead.

andreypopp commented 5 years ago

This can be closed because it was implemented!