Allow monadic IO in dot protocol by 3Rafal · Pull Request #1581 · ocaml/merlin

3Rafal · 2023-03-17T13:28:15Z

voodoos · 2023-03-21T13:47:59Z

I think we should use the new interface in Merlin itself and remove the old io functions.
We should also add comments explaining how to use the new interface and maybe point to ocaml-lsp's usage.

src/dot-protocol/merlin_dot_protocol.mli

voodoos

Mostly cosmetic changes...

src/dot-protocol/merlin_dot_protocol.ml

src/dot-protocol/merlin_dot_protocol.mli

src/kernel/mconfig_dot.ml

src/dot-protocol/merlin_dot_protocol.mli

voodoos · 2023-03-28T19:20:18Z

Also we will need a changelog entry :-)

CHANGES.md

voodoos · 2023-03-29T14:32:28Z

Ok, let's stop worrying about the indentation and make this PR the first one to be ocamlformatted.

Can you rebase and clean the history ? Then I will take care of the formatting.

Edit: and factor the functor/module sig

voodoos · 2023-03-29T17:29:10Z

Thanks for the changes @3Rafal !

@rgrinberg , do you confirm that the final interface suits your needs ?

src/dot-protocol/merlin_dot_protocol.mli

rgrinberg · 2023-04-21T14:36:01Z

src/dot-protocol/merlin_dot_protocol.ml

+
+  val read : in_chan -> (Csexp.t, string) result IO.t
+
+  val write : out_chan -> Csexp.t -> unit IO.t


In the ocamllsp fork, this is a list of csexps, why was this changed to a single csexp?

I'm not sure. I think the fork uses a single csexp as well.

merlin/src/dot-protocol/merlin_dot_protocol.mli

Line 105 in 209e5f2

val write : t -> Csexp.t -> unit IO.t

@ddickstein you might be using an old version somewhere.

I think you might be misreading. The functor takes a module whose write is implemented in terms of a single csexp. The module that is provided, Lev_fiber_csexp.Session, implements write in terms of a list, so there's a redefinition of write that implements it for the functor by packaging the csexp into a singleton list and calling Lev_fiber_csexp.Session.write. The incompatibility I was pointing out as an issue was on the read side, where the latest version of Merlin now expects the implementation that returns a result, but Lev_fiber_csexp.Session.read returns an option, and there's no re-implementation to satisfy the functor because ocaml-lsp hasn't yet updated to the latest version of Merlin. But I am now on latest Merlin and on latest ocaml-lsp, so I'm seeing an incompatibility.

Yes, not sure why the result turned into an option, we can easily change that.

However even with that change it is important to note that the current Merlin master should not be expected to be compatible with ocaml-lsp. We still have an upstreaming PR to merge (#1585) and then we will have to make changes to ocaml-lsp to actually use the new api. At that point both projects will we be compatible for the first time.

Implementation used internally by merlin ("Blocking"), uses Csexp library. read is implemented as:

let read = Csexp.input

which has type of

val input : in_channel -> (Sexp.t, string) result

I decided to use result, and then adjust implementation in ocaml-lsp

Whoops, sorry for not reading this closer.

I decided to use result, and then adjust implementation in ocaml-lsp

What would be the purpose of the string in Error case?

Currently, the Error string is not used. I reused the old implementation, which worked that way. Do you think it would be a better idea to change the interface to use option?

Nah, the result type is fine. I'm just curious what you plan to use it for. If it's an error message, It's not really possible to display it in a friendly way to an lsp user through the merlin API.

from 3Rafal/monadic-io-in-dot-protocol

CHANGES: Fri May 26 15:23:42 CEST 2023 + merlin binary - Allow monadic IO in dot protocol (ocaml/merlin#1581) - Add a `scope` option to the `occurrences` command in preparation for the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596) - Construct bool-typed holes as `false` instead of `true` in the `construct` command, for consistency (ocaml/merlin#1599). - Add a hook to configure system command for spawning ppxes when Merlin is used as a library. (ocaml/merlin#1585) - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584) - Cleanup functors caches when backtracking, to avoid memory leaks (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032) - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603) - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575) - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607) - Fix incorrect locations for string literals (ocaml/merlin#1574) - Fixed an issue that caused `errors` to erroneously alert about missing `cmi` files (ocaml/merlin#1577) - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602, fixes ocaml/merlin#1601) - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945) + editor modes - emacs: call the user's configured completion UI in `merlin-construct` (ocaml/merlin#1598) + test suite - Add missing dependency to a test using ppxlib (ocaml/merlin#1583) - Add tests for the new PPX phase cache (ocaml/merlin#1584) - Add and update tests for `construct` ordering (ocaml/merlin#1603)

CHANGES: Fri May 26 15:23:42 CEST 2023 + merlin binary - Allow monadic IO in dot protocol (ocaml/merlin#1581) - Add a `scope` option to the `occurrences` command in preparation for the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596) - Construct bool-typed holes as `false` instead of `true` in the `construct` command, for consistency (ocaml/merlin#1599). - Add a hook to configure system command for spawning ppxes when Merlin is used as a library. (ocaml/merlin#1585) - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584) - Cleanup functors caches when backtracking, to avoid memory leaks (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032) - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603) - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575) - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607) - Fix incorrect locations for string literals (ocaml/merlin#1574) - Fixed an issue that caused `errors` to erroneously alert about missing `cmi` files (ocaml/merlin#1577) - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602, fixes ocaml/merlin#1601) - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945) + editor modes - emacs: call the user's configured completion UI in `merlin-construct` (ocaml/merlin#1598) + test suite - Add missing dependency to a test using ppxlib (ocaml/merlin#1583) - Add tests for the new PPX phase cache (ocaml/merlin#1584) - Add and update tests for `construct` ordering (ocaml/merlin#1603) [new release] merlin, merlin-lib and dot-merlin-reader (4.9-414) CHANGES: Fri May 26 15:23:42 CEST 2023 + merlin binary - Allow monadic IO in dot protocol (ocaml/merlin#1581) - Add a `scope` option to the `occurrences` command in preparation for the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596) - Construct bool-typed holes as `false` instead of `true` in the `construct` command, for consistency (ocaml/merlin#1599). - Add a hook to configure system command for spawning ppxes when Merlin is used as a library. (ocaml/merlin#1585) - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584) - Cleanup functors caches when backtracking, to avoid memory leaks (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032) - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603) - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575) - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607) - Fix incorrect locations for string literals (ocaml/merlin#1574) - Fixed an issue that caused `errors` to erroneously alert about missing `cmi` files (ocaml/merlin#1577) - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602, fixes ocaml/merlin#1601) - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945) + editor modes - emacs: call the user's configured completion UI in `merlin-construct` (ocaml/merlin#1598) + test suite - Add missing dependency to a test using ppxlib (ocaml/merlin#1583) - Add tests for the new PPX phase cache (ocaml/merlin#1584) - Add and update tests for `construct` ordering (ocaml/merlin#1603)

CHANGES: unreleased + merlin binary - Preview support for OCaml 5.1-alpha1. Short path is temporary disabled and inline records might not behave as expected. - Allow monadic IO in dot protocol (ocaml/merlin#1581) - Add a `scope` option to the `occurrences` command in preparation for the upcoming `project-wide-occurrences` feature (ocaml/merlin#1596) - Construct bool-typed holes as `false` instead of `true` in the `construct` command, for consistency (ocaml/merlin#1599). - Add a hook to configure system command for spawning ppxes when Merlin is used as a library. (ocaml/merlin#1585) - Implement an all-or-nothing cache for the PPX phase (ocaml/merlin#1584) - Cleanup functors caches when backtracking, to avoid memory leaks (ocaml/merlin#1609, fixes ocaml/merlin#1529 and ocaml-lsp#1032) - Fix `construct` results ordering for sum types sand poly variants (ocaml/merlin#1603) - Fix object method completion not working (ocaml/merlin#1606, fixes ocaml/merlin#1575) - Improve context detection for package types (ocaml/merlin#1608, fixes ocaml/merlin#1607) - Fix incorrect locations for string literals (ocaml/merlin#1574) - Fixed an issue that caused `errors` to erroneously alert about missing `cmi` files (ocaml/merlin#1577) - Prevent destruct from crashing on closed variant types (ocaml/merlin#1602, fixes ocaml/merlin#1601) - Improve longident parsing (ocaml/merlin#1612, fixes ocaml/merlin#945) + editor modes - emacs: call the user's configured completion UI in `merlin-construct` (ocaml/merlin#1598) + test suite - Add missing dependency to a test using ppxlib (ocaml/merlin#1583) - Add tests for the new PPX phase cache (ocaml/merlin#1584) - Add and update tests for `construct` ordering (ocaml/merlin#1603)

3Rafal changed the title ~~refactor: allow monadic IO in dot protocol~~ Allow monadic IO in dot protocol Mar 17, 2023

3Rafal force-pushed the monadic-io-in-dot-protocol branch 2 times, most recently from e993323 to a329786 Compare March 24, 2023 12:57

voodoos reviewed Mar 24, 2023

View reviewed changes

src/dot-protocol/merlin_dot_protocol.mli Outdated Show resolved Hide resolved

src/dot-protocol/merlin_dot_protocol.mli Outdated Show resolved Hide resolved

3Rafal requested a review from voodoos March 28, 2023 08:41

voodoos reviewed Mar 28, 2023

View reviewed changes

src/dot-protocol/merlin_dot_protocol.mli Outdated Show resolved Hide resolved

voodoos added this to the Next release milestone Mar 28, 2023

3Rafal mentioned this pull request Mar 29, 2023

Ocaml-format the codebase #1586

Closed

refactor: allow monadic IO in dot protocol

ca2421f

3Rafal added a commit to 3Rafal/merlin that referenced this pull request Mar 29, 2023

Add changelog entry for ocaml#1581

84972fc

3Rafal force-pushed the monadic-io-in-dot-protocol branch from 47b51bd to 84972fc Compare March 29, 2023 09:58

voodoos reviewed Mar 29, 2023

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

voodoos approved these changes Mar 29, 2023

View reviewed changes

3Rafal added 2 commits March 29, 2023 18:33

Abstract over IO in Merlin_dot_protocol interface

14556e9

Add changelog entry for ocaml#1581

76ed590

3Rafal force-pushed the monadic-io-in-dot-protocol branch from 84972fc to 76ed590 Compare March 29, 2023 16:35

rgrinberg reviewed Apr 1, 2023

View reviewed changes

src/dot-protocol/merlin_dot_protocol.mli Show resolved Hide resolved

Extract types from functor signature

a963211

voodoos merged commit 9fc6355 into ocaml:master Apr 5, 2023

rgrinberg reviewed Apr 21, 2023

View reviewed changes

voodoos added a commit to voodoos/merlin that referenced this pull request Apr 28, 2023

Allow monadic IO in dot protocol (ocaml#1581)

80b1ca8

from 3Rafal/monadic-io-in-dot-protocol

voodoos mentioned this pull request May 31, 2023

[new release] merlin, merlin-lib and dot-merlin-reader (4.9-501preview) ocaml/opam-repository#23848

Merged


		val read : in_chan -> (Csexp.t, string) result IO.t

		val write : out_chan -> Csexp.t -> unit IO.t

Conversation

3Rafal commented Mar 17, 2023

Uh oh!

voodoos commented Mar 21, 2023

Uh oh!

Uh oh!

Uh oh!

voodoos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voodoos commented Mar 28, 2023

Uh oh!

Uh oh!

voodoos commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voodoos commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rgrinberg Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

3Rafal Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

rgrinberg Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

ddickstein Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

voodoos Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3Rafal Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgrinberg Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

3Rafal Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

rgrinberg Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

voodoos commented Mar 29, 2023 •

edited

Loading

voodoos commented Mar 29, 2023 •

edited

Loading

voodoos Apr 21, 2023 •

edited

Loading

3Rafal Apr 21, 2023 •

edited

Loading