ocsigen / lwt

OCaml promises and concurrent I/O
https://ocsigen.org/lwt
MIT License
709 stars 175 forks source link

Remove >> syntax from the PPX #495

Closed Sventimir closed 6 years ago

Sventimir commented 6 years ago

I have recently spent half an afternoon wondering, why an operator >> I defined in a module does not work properly in my code, only to find out that the reason was lwt.ppx used in the project. Why is >> defined as a syntax extension, when it's just a simple function? It could (and certainly should!) be defined in Lwt.Infix as simple as this:

let (>>) a b = a >>= fun _ -> b

Having it as a syntax extension instead denies users the possibility to define their own >> operator, which is idiomatic for any monad, not only Lwt. Besides it's just confusing when somebody (like myself) does not know about it. I would gladly see it changed. In fact I could probably do it myself unless there is a good reason not to, which is why I post here instead of submitting a pull-request directly.

hcarty commented 6 years ago

If you do submit a PR to add >> please define it as fun () -> ... rather than fun _ -> ... to help catch bugs!

>> from lwt.ppx is considered deprecated, in case that helps, in part because of the issues you mention.

fdopen commented 6 years ago

It's defined as a syntax extension because otherwise the evaluation order would be unspecified - and surprising with the current implementation and impure code. Just compare the output of Lwt_io.printl "1" >> Lwt_io.printl "2" with your definition and the syntax extension.

Sventimir commented 6 years ago

How is it possible that bind does not have that problem, but >> defined in terms of bind does? It doesn't make much sense to me. Also if it's important to keep >> a syntax extension, then why it's already deprecated?

aantron commented 6 years ago

@Sventimir The difference is that with bind, you have:

Lwt.bind (e1) (fun () -> e2)

whereas with >>, you have

e1 >> e2

Because e2 is not inside a function abstraction (thunk, closure, whatever) in the >> case, Lwt can't control its order of evaluation. OCaml decides that, and leaves it unspecified.

And it's not important to keep >> as a syntax extension. Everyone, including the authors of it, wants to remove it. That's why we deprecated it, and intend to delete it.

I will link to issues when I return to the computer.

Sventimir commented 6 years ago

Thank you for the explanation. Indeed, replacing the syntax extension with a function does break a test for sequencing Lwt operations. I suppose I know understand why.

aantron commented 6 years ago

Yep, so we plan to just delete the whole thing completely. It turned out to be not as good an idea as originally hoped. If you don't mind, I'll rename this issue accordingly, since it has a nice discussion, and tag it for getting done for 4.0.0 (which is when we'd do this small API breakage).

Sventimir commented 6 years ago

Of course I don't mind. Is there any workaround to the issue I mentioned that would temporarily let me define >> function until the syntax extension is removed from the API?

aantron commented 6 years ago

I believe you can pass -no-sequence to the PPX to disable >>. If not sure how this is done with your build system, let us know what you're using, and we should be able to say.

Sventimir commented 6 years ago

I am using Oasis and indeed I don't think I know how to handle ppx flags in it.

aantron commented 6 years ago

I actually don't know what is a good way of doing it with OASIS, short of tweaking myocamlbuild.ml. I found an example here: https://github.com/c-cube/cconv/blob/5ffbc8b77f427b5f90985a4c4ed310ee2e6d4257/myocamlbuild.ml#L811, the expression you'd want is something like

flag
  ["ocaml"; "compile"] &
  S [A "-ppxopt"; A "lwt.ppx,-no-sequence"];
Sventimir commented 6 years ago

Thank you very much.

aantron commented 6 years ago

I myself recall using a ppxopt tag, so it would be something like

<**/*.ml>: ppxopt(lwt.ppx,-no-sequence)

in your _tags file, but I found no information on it, nor mention of it in the Ocamlbuild code, so it might be a false memory.

aantron commented 6 years ago

And the links are:

Drup commented 6 years ago

>> seems to have been in the PPX since it was originally added, in #75. I don't see any specific discussion of it, but the rationale was, I believe, compatibility with the Camlp4 syntax.

I can confirm. It was entirely for the purpose of compatibility. I already though it was an horrible idea at that time.

ygrek commented 6 years ago

ftr marking -no-sequence option itself as deprecated is not very helpful, because it means we cannot have this option enabled (not clean build) to track that >> is not introduced in codebase. I would suggest keeping this option warning-free as long as >> is accepted by ppx, and as noop after >> is removed.

aantron commented 6 years ago

@ygrek @jorisgio Indeed, reservations about deprecating -no-sequence are included here https://github.com/ocsigen/lwt/pull/534#issue-285514460.

However, any release of Lwt/the PPX that has deprecated -no-sequence and emits warnings on using it, also emits warnings directly on using >>. The warning number should be the same (29 IIRC), and IIRC there is no way to suppress it locally for only some of the language constructs in any file.

That should mean that if any modern Lwt/PPX is part if your build, -no-sequence is completely redundant and cannot provide more information than just the >> warning. If warnings are problematic in your build, the >> warning itself will provide more accurate information and/or stop your build.

Please let me know if there is any oversight/misunderstanding in the reasoning above :)

ygrek commented 6 years ago

-no-sequence is completely redundant and cannot provide more information than just the >> warning

Indeed, this is true. Please disregard my previous comment.