Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in t_of_yojson for merlin call compatible request params #1428

Closed
wants to merge 1 commit into from
Closed

Fix bug in t_of_yojson for merlin call compatible request params #1428

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ module Request_params = struct
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 text_document = TextDocumentIdentifier.t_of_yojson json in
let text_document =
json |> member "textDocument" |> TextDocumentIdentifier.t_of_yojson
in
Comment on lines +73 to +75
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{ text_document; result_as_sexp; command; args }
;;

Expand Down
Loading