Add a custom query for raw invocation of Merlin #1265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
from
b96f352 to
18ac5cd
Compare
voodoos
reviewed
May 22, 2024
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
Derivation doesn't seem to be particularly used in the code base (for custom queries), so I didn't want to introduce it in this PR. In an ideal world, I'd probably go for applicative validation, with the combinators that go with it. But I think that could be the subject of another PR. WDYT? |
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
2 times, most recently
from
9f4932f to
77a2fd8
Compare
xvw
changed the title
rgrinberg
reviewed
May 27, 2024
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
rgrinberg
reviewed
May 27, 2024
rgrinberg
reviewed
May 27, 2024
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
rgrinberg
reviewed
May 27, 2024
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
rgrinberg
reviewed
May 27, 2024
| let class_, output = | ||
| match action () with | ||
| | result -> ("return", result) | ||
| | exception Failure message -> ("failure", `String message) |
There was a problem hiding this comment.
When is this exception possible? if this is a user facing error, it should have its own exception type.
There was a problem hiding this comment.
It is a reproduction of the behaviour of the ocamlmerlin binary. (The failure class exists, and is only produced in this case).
rgrinberg
reviewed
May 27, 2024
| match action () with | ||
| | result -> ("return", result) | ||
| | exception Failure message -> ("failure", `String message) | ||
| | exception exn -> |
There was a problem hiding this comment.
There's already a catch-all for exceptions so I think this is not necessary.
There was a problem hiding this comment.
Why is this? Catching the exception returns the result in a dedicated response class and mimics the behaviour of the merlin binary.
There was a problem hiding this comment.
Returning a dedicated response is already the default behavior. It's a matter of choosing the correct error code I suppose. Does merlin guarantee that every Failure that it raises is caused by the caller making an error? Or are some Failure related to the server misbehaving in general?
voodoos
reviewed
Jun 5, 2024
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
from
68cb27e to
2c04293
Compare
voodoos
approved these changes
Jun 6, 2024
ocaml-lsp-server/src/custom_requests/req_merlin_call_compatible.ml
Outdated
Show resolved
Hide resolved
voodoos
reviewed
Jun 6, 2024
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
from
055f23d to
a321b53
Compare
rgrinberg
reviewed
Jun 10, 2024
| import * as Protocol from "vscode-languageserver-protocol"; | ||
| import * as Types from "vscode-languageserver-types"; | ||
|
|
||
| describe("ocamllsp/merlinCallCompatible", () => { |
There was a problem hiding this comment.
I should have mentioned this earlier, but we try not to extend the typescript test suite ever since we've introduced the OCaml LSP client. Do you think you can port these tests to e2e-new?
There was a problem hiding this comment.
Done in 2e506c6, thanks for the feedback!
There was a problem hiding this comment.
in https://github.com/xvw/ocaml-lsp/blob/tunneling-merlin-lsp-p2-use-merlin-lib/ocaml-lsp-server/test/e2e-new/merlin_call_compatible.ml#L116, the last test of the test suite, I randomly face to the following error: "Lev_fiber.Io: already closed". Did I miss something?
@@ -124,4 +124,31 @@ let%expect_test "errors: warning is disabled" =
Helpers.test source request;
[%expect
{|
- { "resultAsSexp": false, "result": "{\"class\":\"return\",\"value\":[]}" } |}]
+ (* CR expect_test_collector: This test expectation appears to contain a backtrace.
+ This is strongly discouraged as backtraces are fragile.
+ Please change this test to not include a backtrace. *)
+
+ { "resultAsSexp": false, "result": "{\"class\":\"return\",\"value\":[]}" }
+ detached: /-----------------------------------------------------------------------
+ | Internal error: Uncaught exception.
+ | ("Lev_fiber.Io: already closed",
+ | { source =
+ | "Raised by primitive operation at Lev_fiber.Io.callstack.(fun) in file \"submodules/lev/lev-fiber/src/lev_fiber.ml\", line 590, characters 31-58\n\
+ | Called from Lev_fiber.Io.create in file \"submodules/lev/lev-fiber/src/lev_fiber.ml\", line 743, characters 17-29\n\
+ | Called from Lev_fiber.Io.Lazy_fiber.force.(fun) in file \"submodules/lev/lev-fiber/src/lev_fiber.ml\", line 1047, characters 23-27\n\
+ | Called from Fiber__Scheduler.exec in file \"fiber/src/scheduler.ml\", line 73, characters 8-11\n\
+ | Called from Fiber__Scheduler.start in file \"fiber/src/scheduler.ml\", line 216, characters 2-42\n\
+ | Called from Fiber.run.(fun) in file \"fiber/src/fiber.ml\" (inlined), line 17, characters 28-47\n\
+ | Called from Lev_fiber.run in file \"submodules/lev/lev-fiber/src/lev_fiber.ml\", line 1264, characters 10-59\n\
+ | Called from Ocaml_lsp_server.run in file \"ocaml-lsp-server/src/ocaml_lsp_server.ml\", lines 987-989, characters 2-51\n\
+ | Called from Stdune__Exn_with_backtrace.try_with in file \"otherlibs/stdune/src/exn_with_backtrace.ml\", line 9, characters 8-12\n\
+ | Called from Dune__exe__Main in file \"ocaml-lsp-server/bin/main.ml\", lines 41-42, characters 6-72\n\
+ | "
+ | })
+ | Raised at Stdune__Code_error.raise in file "otherlibs/stdune/src/code_error.ml", line 10, characters 30-62
+ | Called from Lev_fiber.Io.with_ in file "submodules/lev/lev-fiber/src/lev_fiber.ml", line 983, characters 10-62
+ | Called from Fiber__Scheduler.exec in file "fiber/src/scheduler.ml", line 73, characters 8-11
+ | Re-raised at Stdune__Exn.raise_with_backtrace in file "otherlibs/stdune/src/exn.ml" (inlined), line 38, characters 27-56
+ | Called from Stdune__Exn_with_backtrace.reraise in file "otherlibs/stdune/src/exn_with_backtrace.ml", line 20, characters 33-71
+ | Called from Fiber__Scheduler.exec in file "fiber/src/scheduler.ml", line 73, characters 8-11
There was a problem hiding this comment.
Usually this happens when you're being sent diagnostics (likely) or code actions (less likely) and you are closing the client before they arrive.
Would be good to see what is being sent in this line:
Called from Ocaml_lsp_server.run in file "ocaml-lsp-server/src/ocaml_lsp_server.ml", lines 987-989, characters 2-51\n\
But regardless, the fix is just to wait for what is being sent to arrive before terminating the test.
I wonder if we should fix ocamllsp to just silently shut itself down on such errors.
There was a problem hiding this comment.
Would be good to see what is being sent in this line:
Lev_fiber.run ~sigpipe:`Ignore (fun () ->
let* input, output = stream_of_channel channel in
start (Lsp_fiber.Fiber_io.make input output))
|> Lev_fiber.Error.ok_exnBut regardless, the fix is just to wait for what is being sent to arrive before terminating the test.
I was inspired by other e2e-test suite, did I miss something about properly wait at the end of the test? Thanks in advance!
There was a problem hiding this comment.
Currently https://github.com/ocaml/ocaml-lsp/actions/runs/9548374342/job/26316523866?pr=1265 is failing because of that error.
Merged
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
2 times, most recently
from
e08beb6 to
46e2555
Compare
|
I took the opportunity to rebase on |
Enables a pipeline to be run with a pre-calculated configuration (useful for tunneling when modifying the active configuration with flags).
- add change entry - fix start_stop test
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
from
2f358b7 to
3ecc066
Compare
Pull Request Test Coverage Report for Build 4355Details
💛 - Coveralls |
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
from
131b39d to
d23feec
Compare
xvw
changed the title
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
from
d23feec to
a8c56af
Compare
Pull Request Test Coverage Report for Build 4360Details
💛 - Coveralls |
xvw
force-pushed
the
tunneling-merlin-lsp-p2-use-merlin-lib
branch
from
616db89 to
656a3bd
Compare
Pull Request Test Coverage Report for Build 4361Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 4362Details
💛 - Coveralls |
voodoos
added a commit
to voodoos/opam-repository
that referenced
this pull request
Jul 5, 2024
CHANGES:
## Features
- Introduce a configuration option to control dune diagnostics. The option is
called `duneDiganostics` and it may be set to `{ enable: false }` to disable
diagnostics. (ocaml/ocaml-lsp#1221)
- Support folding of `ifthenelse` expressions (ocaml/ocaml-lsp#1031)
- Improve hover behavior (ocaml/ocaml-lsp#1245)
Hovers are no longer displaye on useless parsetree nodes such as keywords,
comments, etc.
Multiline hovers are now filtered away.
Display expanded ppx's in the hover window.
- Improve document symbols (ocaml/ocaml-lsp#1247)
Use the parse tree instead of the typed tree. This means that document
symbols will work even if the source code doesn't type check.
Include symbols at arbitrary depth.
Differentiate functions / types / variants / etc.
This now includes PPXs like `let%expect_test` or `let%bench` in the outline.
- Introduce a `destruct-line` code action. This is an improved version of the
old `destruct` code action. (ocaml/ocaml-lsp#1283)
- Improve signature inference to only include types for elements that were
absent from the signature. Previously, all signature items would always be
inserted. (ocaml/ocaml-lsp#1289)
- Add an `update-signature` code action to update the types of elements that
were already present in the signature (ocaml/ocaml-lsp#1289)
- Add custom
[`ocamllsp/merlinCallCompatible`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/merlinCallCompatible-spec.md)
request (ocaml/ocaml-lsp#1265)
- Add custom [`ocamllsp/typeEnclosing`](https://github.com/ocaml/ocaml-lsp/blob/109801e56f2060caf4487427bede28b824f4f1fe/ocaml-lsp-server/docs/ocamllsp/typeEnclosing-spec.md) request (ocaml/ocaml-lsp#1304)
## Fixes
- Detect document kind by looking at merlin's `suffixes` config.
This enables more lsp features for non-.ml/.mli files. Though it still
depends on merlin's support. (ocaml/ocaml-lsp#1237)
- Correctly accept the `--clientProcessId` flag. (ocaml/ocaml-lsp#1242)
- Disable automatic completion and signature help inside comments (ocaml/ocaml-lsp#1246)
- Includes a new optional/configurable option to toggle syntax documentation. If
toggled on, allows display of syntax documentation on hover tooltips. Can be
controlled via environment variables and by GUI for VS code. (ocaml/ocaml-lsp#1218)
- For completions on labels that the LSP gets from merlin, take into account
whether the prefix being completed starts with `~` or `?`. Change the label
completions that start with `?` to start with `~` when the prefix being
completed starts with `~`. (ocaml/ocaml-lsp#1277)
- Fix document syncing (ocaml/ocaml-lsp#1278, ocaml/ocaml-lsp#1280, fixes ocaml/ocaml-lsp#1207)
- Stop generating inlay hints on generated code (ocaml/ocaml-lsp#1290)
- Fix parenthesizing of function types in `SignatureHelp` (ocaml/ocaml-lsp#1296)
- Fix syntax documentation rendering (ocaml/ocaml-lsp#1318)
avsm
pushed a commit
to avsm/opam-repository
that referenced
this pull request
Sep 5, 2024
CHANGES:
## Features
- Introduce a configuration option to control dune diagnostics. The option is
called `duneDiganostics` and it may be set to `{ enable: false }` to disable
diagnostics. (ocaml/ocaml-lsp#1221)
- Support folding of `ifthenelse` expressions (ocaml/ocaml-lsp#1031)
- Improve hover behavior (ocaml/ocaml-lsp#1245)
Hovers are no longer displaye on useless parsetree nodes such as keywords,
comments, etc.
Multiline hovers are now filtered away.
Display expanded ppx's in the hover window.
- Improve document symbols (ocaml/ocaml-lsp#1247)
Use the parse tree instead of the typed tree. This means that document
symbols will work even if the source code doesn't type check.
Include symbols at arbitrary depth.
Differentiate functions / types / variants / etc.
This now includes PPXs like `let%expect_test` or `let%bench` in the outline.
- Introduce a `destruct-line` code action. This is an improved version of the
old `destruct` code action. (ocaml/ocaml-lsp#1283)
- Improve signature inference to only include types for elements that were
absent from the signature. Previously, all signature items would always be
inserted. (ocaml/ocaml-lsp#1289)
- Add an `update-signature` code action to update the types of elements that
were already present in the signature (ocaml/ocaml-lsp#1289)
- Add custom
[`ocamllsp/merlinCallCompatible`](https://github.com/ocaml/ocaml-lsp/blob/e165f6a3962c356adc7364b9ca71788e93489dd0/ocaml-lsp-server/docs/ocamllsp/merlinCallCompatible-spec.md)
request (ocaml/ocaml-lsp#1265)
- Add custom [`ocamllsp/typeEnclosing`](https://github.com/ocaml/ocaml-lsp/blob/109801e56f2060caf4487427bede28b824f4f1fe/ocaml-lsp-server/docs/ocamllsp/typeEnclosing-spec.md) request (ocaml/ocaml-lsp#1304)
## Fixes
- Detect document kind by looking at merlin's `suffixes` config.
This enables more lsp features for non-.ml/.mli files. Though it still
depends on merlin's support. (ocaml/ocaml-lsp#1237)
- Correctly accept the `--clientProcessId` flag. (ocaml/ocaml-lsp#1242)
- Disable automatic completion and signature help inside comments (ocaml/ocaml-lsp#1246)
- Includes a new optional/configurable option to toggle syntax documentation. If
toggled on, allows display of syntax documentation on hover tooltips. Can be
controlled via environment variables and by GUI for VS code. (ocaml/ocaml-lsp#1218)
- For completions on labels that the LSP gets from merlin, take into account
whether the prefix being completed starts with `~` or `?`. Change the label
completions that start with `?` to start with `~` when the prefix being
completed starts with `~`. (ocaml/ocaml-lsp#1277)
- Fix document syncing (ocaml/ocaml-lsp#1278, ocaml/ocaml-lsp#1280, fixes ocaml/ocaml-lsp#1207)
- Stop generating inlay hints on generated code (ocaml/ocaml-lsp#1290)
- Fix parenthesizing of function types in `SignatureHelp` (ocaml/ocaml-lsp#1296)
- Fix syntax documentation rendering (ocaml/ocaml-lsp#1318)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is based on #1233 and depends on ocaml/merlin#1758
It introduces the custom query:
ocamllsp/merlinCallCompatible, to call Merlin directly via LSP.