Define let*, and*

[code-ghalib]

Proposal to add a syntax module to both Lwt and Lwt_result now that let* and and* are available in vanilla OCaml without ppx.

I've made an arbitrary choice while defining Lwt_result.both that works for most cases, because I cannot think of a solution that is both general and easy to use. I could avoid the arbitrary choice like so:

val both : ('e -> 'e -> 'e) -> ('a, 'e) Result.t Lwt.t -> ('b, 'e) -> Result.t Lwt.t

let both combine r1 r2 =
  Lwt.map (function
    | Ok p1, Ok p2 -> Ok (p1,p2)
    | Error e1, Error e2 -> Error (combine e1 e2)
    | Error e,_ | _,Error e -> Error e) (Lwt.both a b)

(* In case of string, we would say *)
let (and*) = Lwt_result.both (^)

But then defining the syntax module would probably involve a functor which is not that nice to use.

I'm open to suggestions - the main motivation is to not have to redefine these all the time for being able to use monadic syntax.

[anuragsoni]

Lwt supports ocaml versions starting from 4.02, so these new operators will fail on versions older than 4.08. I think this will need some dune machinery to ensure that this new syntax only gets exposed on ocaml >= 4.08. One example of this that i've seen in the ecosystem is the containers library that supports the new operators on ocaml > 4.08 c-cube/ocaml-containers@bf0227d

[aantron]

I haven't had a chance to look at the PR in detail yet, but the let* syntax support on < 4.08 can be done like this:

1. https://github.com/aantron/luv/blob/0eae7f30ef99157bda77c62e0cb82169410de583/luv.opam#L20

2. https://github.com/aantron/luv/blob/0eae7f30ef99157bda77c62e0cb82169410de583/src/syntax/dune#L4

[aantron]

I think the choice for Lwt_result.both is fine, given that the error type is unconstrained, and Lwt_result.Syntax is meant for use with open. If someone would like a safer both that combines the errors in some useful way, they would have to define (and*) anyway.

A variant of Lwt_result.both that knew something about the error type would be able to solve this more "intelligently." I guess this is an advantage of something like Or_error.

I'll port to < 4.08, add a couple tests, and merge.

[aantron]

Actually, a safer choice is not to define Lwt.result.(and*). Since the purpose of Lwt_result is explicit error handling, we shouldn't mislead users into dropping errors silently.

[raphael-proust]

A variant of Lwt_result.both that knew something about the error type would be able to solve this more "intelligently." I guess this is an advantage of something like Or_error.

I also think that Lwt should just provide Lwt (i.e., promise) monad-like combinators. Combining monads is its own can of worm and each project might want to do it on its own term. It's relatively easy for a project to write promise.ml that basically exports the Lwt primitives and wraps result-promise the way they want to use it.

[code-ghalib]

Actually, a safer choice is not to define Lwt.result.(and*). Since the purpose of Lwt_result is explicit error handling, we shouldn't mislead users into dropping errors silently.

I can live with that:

let (and*) = Lwt_result.both

is not too annoying to duplicate across modules, and makes the choice somewhat more explicitly available for code reviewers. Although, this does limit the utility of having a Syntax module at all.

Also, the error is not dropped per-se, the resulting promise is still wrapped in a Result. The distinction between these two situations is dropped:

• either promise resulted in an error
• both promises resulted in an error

This has practically not mattered to me personally, because in either case, I cannot continue with the remaining computation - it's a small debugging inconvenience to not know which of the two situations happened.

Not sure if you want to add the applicative syntax let+, and+ as well. Personally, I have not used them in practice.

I'll port to < 4.08, add a couple tests, and merge.

That'd be great thanks! Not sure if this involves packaging it as a separate libraries:lwt.syntax, lwt.result.syntax. I will make an attempt to do this at my end too, just to get more familiar with dune.

[aantron]

I can live with that:

I had in mind not to add Lwt.result.both, either. But I'm very open to debating that. Do you often need such a function?

The distinction between these two situations is dropped:

• either promise resulted in an error
• both promises resulted in an error

Apart from the distinction, in the "both" case, one of the errors is also dropped. If we could guarantee that the error type has an operation error -> error -> error (like ^ as you suggested for strings), i.e. some kind of logging, we wouldn't have to drop one of the errors silently. That's what makes me suspect that Lwt_result as it is now is too underconstrained to have a reasonable both with the reasonable type signature, i.e. Lwt_result's original design has missed a certain subtlety. It just seems like it would mislead users about the rigidity of error propagation.

I will make an attempt to do this at my end too, just to get more familiar with dune.

Could you try it in this PR, then? It doesn't require anything fancy, I linked in a prior comment to the only two lines you should have to add for this.

[aantron]

Well, I guess Lwt_result.both's error handling is not more questionable than value handling in Lwt.pick, so maybe it's okay. Perhaps a comment is enough to warn people. Still interested in opinions on this :)

[code-ghalib]

I had in mind not to add Lwt.result.both, either. But I'm very open to debating that. Do you often need such a function?

Yes, in practice, I'm mostly using Lwt to parallelize i/o heavy stuff, and as a result, almost every promise is stateful and can can result in errors, so Lwt_result.t is used much more often than an unwrapped Lwt.t

Apart from the distinction, in the "both" case, one of the errors is also dropped. If we could guarantee that the error type has an operation error -> error -> error (like ^ as you suggested for strings), i.e. some kind of logging, we wouldn't have to drop one of the errors silently. That's what makes me suspect that Lwt_result as it is now is too underconstrained to have a reasonable both with the reasonable type signature, i.e. Lwt_result's original design has missed a certain subtlety. It just seems like it would mislead users about the rigidity of error propagation.

I agree it's not ideal. Not sure how it might be done better though. I'm not familiar enough with haskell to understand some of their monad transformers examples, but I'm sure the situation would have come up there before? Maybe we can seek out opinions on discourse?

I will make an attempt to do this at my end too, just to get more familiar with dune.

Could you try it in this PR, then? It doesn't require anything fancy, I linked in a prior comment to the only two lines you should have to add for this.

Will do!

[code-ghalib]

Looked for inspiration at async. I may be mis-reading this, but I think async is getting the sequential version of both for their Deferred.Result.t:
https://github.com/janestreet/async_kernel/blob/master/src/deferred_result.ml#L17
https://github.com/janestreet/base/blob/master/src/monad.ml#L48

That seems not so useful.

If we were to go with a functorized solution, it would look something like this:

let _ =
  let open Lwt_result.Syntax.Make(struct type t = string let combine_err = (^) end) in
  let* p1 = thing1 ()
  and* p2 = thing2 ()
  and* p3 = thing3 () in
  (* do your thing *)

I would frankly not enjoy using/reading that. I vote to warn people with comments in the absence of a smarter proposal.

[kohlivarun5]

Personally, I would be okay with using both that just picks the first error, since for most of my use cases, that is sufficient and cleaner/easier to use.

If people do want a version with combine, that can be exposed as an additional functor, so clients who prefer to combine can use the functor

[aantron]

I think async is getting the sequential version of both for their Deferred.Result.t

Not in that code. The promises a and b are already constructed, and any underlying operations already running, by the time both is called. So no sequencing of the underlying operations is introduced by waiting first on a and then on b. Lwt is not able to do this because Lwt promises can be rejected with exn, so Lwt.both uses a more complicated definition to ensure commutativity:

lwt/src/core/lwt.ml

Lines 2574 to 2582 in 2412006

	let both p1 p2 =
	let v1 = ref None in
	let v2 = ref None in
	let p1' = bind p1 (fun v -> v1 := Some v; return_unit) in
	let p2' = bind p2 (fun v -> v2 := Some v; return_unit) in
	join [p1'; p2'] |> map (fun () ->
	match !v1, !v2 with
	| Some v1, Some v2 -> v1, v2
	| _ -> assert false)

Here, it relies on Lwt.join's inner semantics to ensure the behavior is the same no matter which promise is written on the left and which on the right.

Actually, we probably want this property for Lwt_result.both, too. See #668 (comment) and the tests:

lwt/test/core/test_lwt.ml

Lines 1948 to 2159 in 2412006

	let both_tests = suite "both" [
	test "both fulfilled" begin fun () ->
	let p = Lwt.both (Lwt.return 1) (Lwt.return 2) in
	state_is (Lwt.Return (1, 2)) p
	end;
	test "both rejected" begin fun () ->
	let p = Lwt.both (Lwt.fail Exception) (Lwt.fail Exit) in
	state_is (Lwt.Fail Exception) p
	end;
	test "rejected, fulfilled" begin fun () ->
	let p = Lwt.both (Lwt.fail Exception) (Lwt.return 2) in
	state_is (Lwt.Fail Exception) p
	end;
	test "fulfilled, rejected" begin fun () ->
	let p = Lwt.both (Lwt.return 1) (Lwt.fail Exception) in
	state_is (Lwt.Fail Exception) p
	end;
	test "both pending" begin fun () ->
	let p = Lwt.both (fst (Lwt.wait ())) (fst (Lwt.wait ())) in
	state_is Lwt.Sleep p
	end;
	test "pending, fulfilled" begin fun () ->
	let p = Lwt.both (fst (Lwt.wait ())) (Lwt.return 2) in
	state_is Lwt.Sleep p
	end;
	test "pending, rejected" begin fun () ->
	let p = Lwt.both (fst (Lwt.wait ())) (Lwt.fail Exception) in
	state_is Lwt.Sleep p
	end;
	test "fulfilled, pending" begin fun () ->
	let p = Lwt.both (Lwt.return 1) (fst (Lwt.wait ())) in
	state_is Lwt.Sleep p
	end;
	test "rejected, pending" begin fun () ->
	let p = Lwt.both (Lwt.fail Exception) (fst (Lwt.wait ())) in
	state_is Lwt.Sleep p
	end;
	test "pending, fulfilled, then fulfilled" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p = Lwt.both p1 (Lwt.return 2) in
	Lwt.wakeup_later r1 1;
	state_is (Lwt.Return (1, 2)) p
	end;
	test "pending, rejected, then fulfilled" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p = Lwt.both p1 (Lwt.fail Exception) in
	Lwt.wakeup_later r1 1;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, fulfilled, then rejected" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p = Lwt.both p1 (Lwt.return 2) in
	Lwt.wakeup_later_exn r1 Exception;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, rejected, then rejected" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p = Lwt.both p1 (Lwt.fail Exception) in
	Lwt.wakeup_later_exn r1 Exit;
	state_is (Lwt.Fail Exception) p
	end;
	test "fulfilled, pending, then fulfilled" begin fun () ->
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both (Lwt.return 1) p2 in
	Lwt.wakeup_later r2 2;
	state_is (Lwt.Return (1, 2)) p
	end;
	test "rejected, pending, then fulfilled" begin fun () ->
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both (Lwt.fail Exception) p2 in
	Lwt.wakeup_later r2 2;
	state_is (Lwt.Fail Exception) p
	end;
	test "fulfilled, pending, then rejected" begin fun () ->
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both (Lwt.return 1) p2 in
	Lwt.wakeup_later_exn r2 Exception;
	state_is (Lwt.Fail Exception) p
	end;
	test "rejected, pending, then rejected" begin fun () ->
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both (Lwt.fail Exception) p2 in
	Lwt.wakeup_later_exn r2 Exit;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, then first fulfilled" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p = Lwt.both p1 (fst (Lwt.wait ())) in
	Lwt.wakeup_later r1 1;
	state_is Lwt.Sleep p
	end;
	test "pending, then first rejected" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p = Lwt.both p1 (fst (Lwt.wait ())) in
	Lwt.wakeup_later_exn r1 Exception;
	state_is Lwt.Sleep p
	end;
	test "pending, then second fulfilled" begin fun () ->
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both (fst (Lwt.wait ())) p2 in
	Lwt.wakeup_later r2 2;
	state_is Lwt.Sleep p
	end;
	test "pending, then second rejected" begin fun () ->
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both (fst (Lwt.wait ())) p2 in
	Lwt.wakeup_later_exn r2 Exception;
	state_is Lwt.Sleep p
	end;
	test "pending, then first fulfilled, then fulfilled" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later r1 1;
	Lwt.wakeup_later r2 2;
	state_is (Lwt.Return (1, 2)) p
	end;
	test "pending, then first fulfilled, then rejected" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later r1 1;
	Lwt.wakeup_later_exn r2 Exception;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, then first rejected, then fulfilled" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later_exn r1 Exception;
	Lwt.wakeup_later r2 2;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, then first rejected, then rejected" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later_exn r1 Exception;
	Lwt.wakeup_later_exn r2 Exit;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, then second fulfilled, then fulfilled" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later r2 2;
	Lwt.wakeup_later r1 1;
	state_is (Lwt.Return (1, 2)) p
	end;
	test "pending, then second fulfilled, then rejected" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later r2 2;
	Lwt.wakeup_later_exn r1 Exception;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, then second rejected, then fulfilled" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later_exn r2 Exception;
	Lwt.wakeup_later r1 1;
	state_is (Lwt.Fail Exception) p
	end;
	test "pending, then second rejected, then rejected" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p2, r2 = Lwt.wait () in
	let p = Lwt.both p1 p2 in
	Lwt.wakeup_later_exn r2 Exception;
	Lwt.wakeup_later_exn r1 Exit;
	state_is (Lwt.Fail Exception) p
	end;
	test "diamond" begin fun () ->
	let p1, r1 = Lwt.wait () in
	let p = Lwt.both p1 p1 in
	Lwt.bind (state_is Lwt.Sleep p) (fun was_pending ->
	Lwt.wakeup_later r1 1;
	Lwt.bind (state_is (Lwt.Return (1, 1)) p) (fun is_fulfilled ->
	Lwt.return (was_pending && is_fulfilled)))
	end;
	]
	let suites = suites @ [both_tests]

[aantron]

If getting commutativity is too much of a pain, we can probably just leave it as a note in this issue. I think you should be able to get it by having both keep track of which promise resolved first, and if both promises resolve with Error, choosing to propagate the Error from the first promise to resolve.

[aantron]

If you're having trouble with Bisect, you can drop support for it. I will work it back in later.

Alternatively, you can test the OCaml version and add (future_syntax) only if it is < 4.08, otherwise add Bisect (conditionally). We don't build with coverage on old versions.

[aantron]

Thanks @code-ghalib! And to all for discussion!

[aantron]

I'll release this in 5.3.0 this week.

[aantron]

This is now out in Lwt 5.3.0.