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.

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: improve error messages when merlin is absent #608

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
- Fix infer interface code action crash when implementation source does not
exist (#597)

- Improve error message when the reason plugin for merlin is absent (#608)

# 1.9.1

## Fixes
Expand Down
1 change: 1 addition & 0 deletions ocaml-lsp-server/src/dune
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
lsp
lsp_fiber
dot-merlin-reader.merlin_dot_protocol
merlin.extend
merlin.analysis
merlin.kernel
merlin.merlin_utils
Expand Down
77 changes: 64 additions & 13 deletions ocaml-lsp-server/src/ocaml_lsp_server.ml
Original file line number Diff line number Diff line change
Expand Up @@ -184,14 +184,22 @@ let set_diagnostics rpc doc =
async (fun () -> Diagnostics.send state.diagnostics (`One uri))
| Reason
| Ocaml ->
async (fun () ->
let* diagnostics =
let command =
Query_protocol.Errors
{ lexing = true; parsing = true; typing = true }
in
Document.with_pipeline_exn doc (fun pipeline ->
let errors = Query_commands.dispatch pipeline command in
let send () =
let* diagnostics =
let command =
Query_protocol.Errors { lexing = true; parsing = true; typing = true }
in
Document.with_pipeline_exn doc (fun pipeline ->
match Query_commands.dispatch pipeline command with
| exception Extend_main.Handshake.Error error ->
let message =
sprintf
"%s.\n\
Hint: install the following packages: merlin-extend, reason"
error
in
[ create_diagnostic ~range:Range.first_line ~message () ]
| errors ->
let merlin_diagnostics =
List.rev_map errors ~f:(fun (error : Loc.error) ->
let loc = Loc.loc_of_report error in
Expand Down Expand Up @@ -244,9 +252,11 @@ let set_diagnostics rpc doc =
|> List.sort
~compare:(fun (d1 : Diagnostic.t) (d2 : Diagnostic.t) ->
Range.compare d1.range d2.range))
in
Diagnostics.set state.diagnostics (`Merlin (uri, diagnostics));
Diagnostics.send state.diagnostics (`One uri))
in
Diagnostics.set state.diagnostics (`Merlin (uri, diagnostics));
Diagnostics.send state.diagnostics (`One uri)
in
async send

let on_initialize server (ip : InitializeParams.t) =
let state : State.t = Server.state server in
Expand Down Expand Up @@ -311,6 +321,33 @@ let on_initialize server (ip : InitializeParams.t) =
in
(resp, state)

module Code_action_error = struct
type t =
| Initial
| Need_merlin_extend of string
| Exn of Exn_with_backtrace.t

let empty = Initial

let combine x y =
match (x, y) with
| Exn _, Exn _ -> y
| Exn _, _
| _, Exn _ ->
x
Comment on lines +336 to +337
Copy link
Collaborator

Choose a reason for hiding this comment

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

On a second thought, this results in Initial, Exn -> Initial, which shouldn't be the case.

I had a throwing code action now, which resulted in assertion failure from | Error Initial -> assert false

I think it would also make sense if the monoid combined all exceptions rather than taking the latest one.

Irrelevant but curious, the code action threw this

[Trace - 2:10:58 AM] Received response 'textDocument/codeAction - (39)' in 2ms. Request failed: uncaught exception (-32603).
Error data: {
    "exn": "File \"ocaml-lsp-server/vendor/merlin/src/ocaml/utils/local_store.ml\", line 53, characters 2-8: Assertion failed",
    "backtrace": "Raised at Ocaml_utils__Local_store.with_store in file \"ocaml-lsp-server/vendor/merlin/src/ocaml/utils/local_store.ml\", line 53, characters 2-39\nCalled from Mocaml.new_state in file \"ocaml-lsp-server/vendor/merlin/src/kernel/mocaml.ml\", line 12, characters 2-70\nCalled from Mpipeline.Cache.get in file \"ocaml-lsp-server/vendor/merlin/src/kernel/mpipeline.ml\", line 60, characters 18-37\nCalled from Mpipeline.process in file \"ocaml-lsp-server/vendor/merlin/src/kernel/mpipeline.ml\", line 141, characters 14-30\nCalled from Ocaml_lsp_server__Action_type_annotate.code_action.(fun) in file \"ocaml-lsp-server/src/code_actions/action_type_annotate.ml\", line 57, characters 25-68\nCalled from Merlin_utils__Std.let_ref in file \"ocaml-lsp-server/vendor/merlin/src/utils/std.ml\", line 686, characters 8-12\nRe-raised at Merlin_utils__Std.let_ref in file \"ocaml-lsp-server/vendor/merlin/src/utils/std.ml\", line 688, characters 30-39\nCalled from Merlin_utils__Misc.try_finally in file \"ocaml-lsp-server/vendor/merlin/src/utils/misc.ml\", line 45, characters 8-15\nRe-raised at Merlin_utils__Misc.try_finally in file \"ocaml-lsp-server/vendor/merlin/src/utils/misc.ml\", line 62, characters 10-24\nCalled from Stdlib__Fun.protect in file \"fun.ml\", line 33, characters 8-15\nRe-raised at Stdlib__Fun.protect in file \"fun.ml\", line 38, characters 6-52\nCalled from Mocaml.with_state in file \"ocaml-lsp-server/vendor/merlin/src/kernel/mocaml.ml\", line 18, characters 8-38\nRe-raised at Mocaml.with_state in file \"ocaml-lsp-server/vendor/merlin/src/kernel/mocaml.ml\", line 20, characters 42-53\nCalled from Stdune__Exn_with_backtrace.try_with in file \"submodules/dune/otherlibs/stdune/exn_with_backtrace.ml\", line 9, characters 8-12\nRe-raised at Stdune__Exn.raise_with_backtrace in file \"submodules/dune/otherlibs/stdune/exn.ml\", line 36, characters 27-56\nCalled from Fiber.O.(>>|).(fun) in file \"submodules/dune/src/fiber/fiber.ml\", line 338, characters 36-41\nCalled from Fiber.Execution_context.run_jobs in file \"submodules/dune/src/fiber/fiber.ml\", line 218, characters 8-13\nRe-raised at Stdune__Exn.raise_with_backtrace in file \"submodules/dune/otherlibs/stdune/exn.ml\", line 36, characters 27-56\nCalled from Fiber.O.(>>|).(fun) in file \"submodules/dune/src/fiber/fiber.ml\", line 338, characters 36-41\nCalled from Fiber.Execution_context.run_jobs in file \"submodules/dune/src/fiber/fiber.ml\", line 218, characters 8-13\n"
}

Copy link
Collaborator

@ulugbekna ulugbekna Feb 7, 2022

Choose a reason for hiding this comment

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

Something like this perhaps?

  let combine x y =
    match (x, y) with
    | Initial, _ -> y (* [Initial] cedes to any *)
    | _, Initial -> x
    | _, Exn _ -> y (* [Exn] takes over any; the right one wins - why? *) 
    | Exn _, _ -> x
    | Need_merlin_extend _, Need_merlin_extend _ -> y

Another easy way would be to define comparison function on variants and simply always take max of them

Copy link
Member Author

Choose a reason for hiding this comment

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

Something like this perhaps?

Yeah that looks fine.

Another easy way would be to define comparison function on variants and simply always take max of them

Writing the comparison function is just as much work so I think the current approach is better because it's more direct.

| (Need_merlin_extend _ as r), Initial
| Initial, (Need_merlin_extend _ as r) ->
r
| Need_merlin_extend _, Need_merlin_extend _ -> y
| Initial, Initial -> Initial
end

module Code_action_error_monoid = struct
type t = Code_action_error.t

include Stdune.Monoid.Make (Code_action_error)
end

let code_action (state : State.t) (params : CodeActionParams.t) =
let doc =
let uri = params.textDocument.uri in
Expand All @@ -322,8 +359,22 @@ let code_action (state : State.t) (params : CodeActionParams.t) =
| Some set when not (List.mem set ca.kind ~equal:Poly.equal) ->
Fiber.return None
| Some _
| None ->
ca.run doc params
| None -> (
let+ res =
Fiber.map_reduce_errors
~on_error:(fun (exn : Exn_with_backtrace.t) ->
match exn.exn with
| Extend_main.Handshake.Error error ->
Fiber.return (Code_action_error.Need_merlin_extend error)
| _ -> Fiber.return (Code_action_error.Exn exn))
(module Code_action_error_monoid)
(fun () -> ca.run doc params)
in
match res with
| Ok res -> res
| Error Initial -> assert false
| Error (Need_merlin_extend _) -> None
| Error (Exn exn) -> Exn_with_backtrace.reraise exn)
in
let+ code_action_results =
(* XXX this is a really bad use of resources. we should be batching all the
Expand Down