Fix bug in t_of_yojson for merlin call compatible request params

[awilliambauer]

No description provided.

[rgrinberg]

What about just switching to the ppx based converters?

[voodoos]

I am not sure I understand why this change is required, the current implementation does respect the spec in https://github.com/ocaml/ocaml-lsp/blob/master/ocaml-lsp-server/docs/ocamllsp/merlinCallCompatible-specs.md and this change is not coherent with the dual yojson_of_t.

Looking at the LSP spec, in most places the TextDocumentIdentifier.t is usually given a key named textDocument and making this change would be more "LSP idiomatic", however inlining the sole field in TextDocumentIdentifier is not really "wrong" either...

[voodoos]

The clean way to decode would in fact be:

  let t_of_yojson json =
    let open Yojson.Safe.Util in
    let result_as_sexp = json |> member "resultAsSexp" |> to_bool in
    let command = json |> member "command" |> to_string in
    let args = args_of_yojson json in
    let uri = json |> member "uri" |> DocumentUri.t_of_yojson in
    { text_document = uri; result_as_sexp; command; args }
  ;;

And we should also probably rename the misleading field text_document to uri.

[awilliambauer]

Some change is required, since the current t_of_yojson certainly has a bug (TextDocumentIdentifier.t_of_yojson json is clearly wrong in this context). I think you're correct that the current implementation does not match the spec.

It's also awkward that the custom requests are not uniform in whether their parameters inline the URI or contain a TextDocumentIdentifier.t. I'd be happy with a bigger change to make them all the same, and use ppx-based converters (which the hoverExtended request already does).

[xvw]

Yes, it was noticed during ocaml-eglot implementation (and this is why the current implementation duplicate a field uri additionned to textDocumentIdentifier.

[xvw]

Just out of curiosity (and because I can't see the issue, sorry!), I can't understand why TextDocumentIdenfier.t_of_yojson json is wrong?

[awilliambauer]

Can you point me to the duplicate uri field? I don't see it in the code...

Separately, I don't think a duplicate field is a very nice solution—wouldn't it be better to have the URI appear once in the params rather than twice?

[xvw]

Here: https://github.com/tarides/ocaml-eglot/blob/main/ocaml-eglot-req.el#L57

but yeah. I made that fix (on ocaml-eglot) with the goal of making a global pass on every query.

But even I totally agree with you about having uniform way to denote TextDocumentIdentifier, I do not understand the issue with TextDocumentIdenfier.t_of_yojson json (maybe I am missing something)

[awilliambauer]

Ah, I see. I was confused by the fact that the definition of RequestParams.t doesn't match the request spec (i.e., the request spec has a uri field, but RequestParams.t has a textDocument field).

This PR is not needed if I change our internal integration to follow the spec, and not the type definition in the ocaml-lsp code.