Support ;%lwt as sequence syntax -- backwards incompatible in some cases

[hcarty]

This is a particularly useful addition for 4.04.0 and on as it allows non-Lwt ; sequence and Lwt ;%lwt sequence operations to intermingle without extra parens.

Backwards incompatibility

The incompatible change here is that, previously, [%lwt (); Lwt.return_unit] would evaluate just fine but with this change it results in a type error because, within the context of %lwt, both sides of the ; sequence are expected to be promises.

[hcarty]

This has been nibbling at the back of my mind since support for ;%ext was added to the compiler so I thought some holiday evening hacking was in order...

[Drup]

This is a particularly useful addition for 4.04.0 and on as it allows non-Lwt ; sequence and Lwt ;%lwt sequence operations to intermingle without extra parens.

Well, you could always do let%lwt () = .. in ... just fine. ;)

I see the implementation is trivially using the old implementation of >>, so this can be seen as a practical replacement for that (deprecated and disabled by default) syntax. That's a very strong argument in favor that almost completely offset the fact that it's ridiculously hideous.

I'll let @aantron decide how he likes it. :p

[aantron]

Acknowledged. I won't have time to fully evaluate all the issues in this for something like weeks. Comments from others are welcome, however. A thing to consider besides the backwards incompatibility with [%lwt ...] syntax, is that Lwt supports OCaml < 4.04 (>= 4.02) atm.

Given that this is a breaking change, making it would require studying the source code of at least OPAM users (opam list --unavailable --depends-on lwt --depopts --recursive --short, get dev repos, grep -rn '\[%lwt' *; can share a script for this, otherwise I will do it later). Unless made very soon, it would reach release in Lwt 4.0.0 in summer or autumn 2017, and we have to figure out how to warn users.

[hcarty]

The syntax is, admittedly, unpleasant looking. Lots of let%lwt () = ... in ends up being messy too. I haven't found a pretty way to write effectful Lwt (or other monadish) code.

• >> is the least visually offensive to me but it has lots of unpleasant side effects (haha?)
• let%lwt () = is verbose and has the unfortunate side-effect of adding indentation. It also takes more reading to figure out what is going on in the line of code.
• >>= isn't so bad but it looks and feels strange to mix let%lwt and Lwt's >>= in the same block of code. Being consistent is important and I would rather not give up match%lwt!
• ;%lwt looks like the editor session or source code has been corrupted. It's main benefit, from what I've considered so far, is it's consistent with the other Lwt ppx transformations. The translation between Lwt and non-Lwt code makes the most sense here - always add %lwt. Nothing breaks or needs to be rearranged.

(* Using >> *)
do_a_thing () >>
Logs_lwt.info (fun m -> m "Just did that thing") >>
more_code

(* Using let%lwt *)
let%lwt () = do_a_thing () in
let%lwt () = Logs_lwt.info (fun m -> m "Just did that thing") in
more_code

(* Using >>= *)
do_a_thing () >>= fun () ->
Logs_lwt.info (fun m -> m "Just did that thing") >>= fun () ->
more_code

(* Using ;%lwt *)
do_a_thing ();%lwt
Logs_lwt.info (fun m -> m "Just did that thing");%lwt
more_code

[hcarty]

@aantron If this is going to be accepted I'd like to see it happen for the 3.x release so it can be taken advantage of (and breakage will be exposed) more quickly.

I may be able to help with the opam scan if you have a script ready to use.

[aantron]

Here you go: https://raw.githubusercontent.com/ocsigen/lwt/master/src/util/fetch-dependees.py

It'll fetch the source code of each depending package PACKAGE into ./dependees/PACKAGE. You can grep through it there. Can take quite a while to run, downloading everything, but I only do it once every few months.

I'll take a closer look at the PPX in the coming days (tomorrow or day after). A bit burned out at the moment. Worst case, we delay 2.7.0 and 3.0.0 slightly, to make sure this gets a proper look and discussion.

If we have to break things for this, we can probably give enough warning with an opam message. So that shouldn't be a problem. It would be nice to see the results of the scan however. It's some kind of unscientific estimate of the usage in the wild.

[aantron]

In the meantime, if anyone has an opinion on ;%lwt, please do comment :)

[aantron]

Oh, a PPX option can also soften the blow in case we screw some user over, that uses [%lwt ... ; ...] pervasively. Might not be worth it, just a thing to consider.

[aantron]

And here is a verbose and loud way to make this breaking change:

• Add two options, -enable-lwt-sequence and -disable-lwt-sequence. In 2.7.0, -disable-lwt-sequence is the default, however you can use -enable-lwt-sequence to start using ;%lwt in 2.7.0.
• If neither option is supplied, in 2.7.0, the PPX emits warning 3 (deprecated) on the first use of [%lwt ... ; ...] in each file, mentioning the future plans and these two options. See warning of Lwt_unix.bind for an example.
• When releasing 3.0.0, I will make -enable the default, and emit deprecation warnings instead if either option is passed to the PPX.
• After a while (4.0.0?) the options can be removed entirely, and the PPX code will become unconditional, as we all want it to be :)

This is elaborate and perhaps annoying, but it gives people plenty of time to adapt and/or complain. Objections welcome. I haven't fully examined everything, so apologies if some of the above would be redundant.

For fixing the build before 4.04, either omit the test, or I think it has to go into its own executable that will be built only if the OCaml version is high enough. In the former case, I'll do a final test manually and it will be sufficient :)

[aantron]

@hcarty Ok, this is good with the above warnings and flags added (though should probably name the flags -enable-lwt-semicolon/-disable-lwt-semicolon, to avoid confusion with existing flags concerning >>). Or, please propose an alternative way to make this change softly.

[Drup]

(Before making that, let's check it actually break anyone's code. I quite doubt it will)

[aantron]

I doubt anyone deliberately wrote [%lwt ... ; ...], but it's possible to introduce this if not careful while refactoring, for instance. We can't actually check everyone's code, but the OPAM scan would be nice.

[hcarty]

When I tried Reason's refmt I noticed it converts (let|match|etc)%lwt-syntax into [%lwt ...] so it could also be a problem for users converting code between OCaml and Reason syntax.

[Drup]

@hcarty That's normal. let%lwt x = e in e is equivalent to [%lwt let x = e in e].

[aantron]

@Drup I think @hcarty means that using refmt creates syntax that is much more likely to result in a mistaken [%lwt ... ; ...] after further manual refactoring.

@hcarty also, with this change, if I am an Lwt user and my code is supposed to compile both on 4.04 and before 4.04, will it have different meanings if I do end up writing [%lwt ... ; ...] somehow?

(edit: I think the answer is no, but want to be sure).

[Drup]

@aantron Yes

Currently [%lwt <expr> ] for an expression that does not have any other meaning (let, match, try, for, while, if) is expanded to Lwt.catch (fun () -> <expr>) Lwt.fail.

That might have not been such a good idea after all.... The rational is in the ppx's documentation.

[hcarty]

That's normal. let%lwt x = e in e is equivalent to [%lwt let x = e in e].

@Drup - @aantron's response is correct - I was referring to the greater chance of accidental code breakage via refmt.

with this change, if I am an Lwt user and my code is supposed to compile both on 4.04 and before 4.04, will it have different meanings if I do end up writing [%lwt ... ; ...] somehow?

@aantron [%lwt ...] will have a different meaning than it does now but that meaning should be the consistent across OCaml versions. However @Drup's reference to the ppx documentation brings up a good point - nothing else (match, for, etc) would get a special interpretation within [%lwt ...] which is a little strange. I'm not sure what the right solution is for this inconsistency.

[aantron]

Yes, actually I didn't fully realize that, thanks. That makes this change somewhat more severe. I'm also kind of skeptical that the default [%lwt ...] was a good idea.

But my question is: comparing compiling a program under 4.04 and 4.03, both with this change, will there be any difference? If I wrote out [%lwt ... ; ...], I assume the attribute would still be attached to the ; even in 4.03 – is this correct?

[aantron]

Thanks @hcarty, we cross-posted :)

[aantron]

There is the possibility of removing the ability to use [%lwt ...] for general expressions, since it is not future-proof, and replacing it with something else ([%lwt.foo ...]). Attaching an %lwt attribute to anything besides a few special expression kinds would be an error. If doing this, we have to design a gradual approach for doing it.

[aantron]

I guess this would also have the advantage of making the current special behavior of [%lwt ...] orthogonal to whether the ... is (e.g.) a match expression or not.

[hcarty]

@aantron Do you want to try to get this into 3.1.0 in some form? I may be able to spend a little time on it.

[aantron]

Yeah, maybe we should finish it by 4.0.0 – but no rush, I think we will be doing a 3.2.0 anyway. I'll leave a message about that in gitter too, so more people know.

[hcarty]

Closing based on a conversation with @aantron and comments above. There's more important work to do listed in comments above that I'm not likely to get to any time soon. We can re-open later if there's interest. If anyone comes along later and wants to finish the work started in this PR they are more than welcome to use what's here!

[hcarty]

Reopening this based on discussion in #525. I'll rebase onto the latest master branch, hopefully later this week.

[aantron]

Sounds good; let me know if you want me to rebase it instead and save you a bit of work :) We may want to do it after #534 is merged, in case it introduces more conflicts.

[aantron]

And cc @kandu!

[hcarty]

@aantron Agreed regarding merging after #534. If you do the merge and get a chance to rebase this before I do then please feel free! Otherwise I'll take care of it as soon as I'm able.

[hcarty]

@aantron Rebased - though I'm not sure what happened with the ppx test file...

[aantron]

Ok, I'm going to release lwt_ppx today, with the deprecation messages from #534 in it, then merge the PR. We can then release the PPX again in some months, probably less than three like for Lwt. It's nice that it's a separate package now :) I'll also release Lwt 3.2.1, since it still packages lwt.ppx, and people are likely still using that.

Nobody in opam is actually affected by this PR. I really doubt anyone at all would be affected by it anywhere, but let's test the ability to release Lwt quickly...

[aantron]

I suggest we remove this, no need to make the new syntax be leaky :) Especially since -no-strict-sequence is now deprecated. Unless there is a good reason to have non-strict sequence.

[aantron]

I wouldn't recommend this syntax. We are trying to get rid of [%lwt ...] completely because it is fragile to refactoring the inside. We can't actually ban the syntax, but we shouldn't encourage it IMO.

[aantron]

I think the directory was changed to test instead of tests.

[hcarty]

Sorry this took a while! I think I've addressed the comments - do you want to take another look @aantron? Thank you for pointing out the test location change too.

[aantron]

Hey, no worries, there is no rush, and I know you've got other things to do :)

It looks good. We can't merge the test because it unconditionally requires 4.04 to compile. I think there are two "good" ways to address that, one is to use cppo, and the other is to put the ;%lwt test into the PPX expect tests. They are currently disabled, however.

I'm going to merge this without the test for now. I'll take care of fixing the expect tests and add the ;%lwt test to them in a later commit.

Thanks!

[hcarty]

All sounds good - thanks!