Monads everywhere #501

Open
wants to merge 21 commits into
from

Conversation

Projects
None yet
9 participants
Member

c-cube commented Jan 7, 2014

Ok, now this is a bigger PR than usual. I started it after I looked at #10, for monads like 'a Lwt.t. It adds monad instances to batSeq, batList and batLazyList, adds a BatInterfaces.Traversable module signature that looks like

module type Traversable = sig
  type 'a t  (* container *)

  type 'a m (* monad *)

  val mapM : ('a -> 'b m) -> 'a t -> 'b t m

  val foldM : ('a -> 'b -> 'a m) -> 'a -> 'b t -> 'a m

  val sequence : 'a m t -> 'a t m
end

and functors Traversable that take a BatInterfaces.Monad and return a traversable for this monad and container, to aforementioned modules as well as BatEnum. For BatEnum, a similar WithMonad functor was already present so I modified it, but didn't rename it to preserve backward compatibility.

Member

UnixJunkie commented Jan 7, 2014

I'm very interested by all this.
I'll try to read all the code for lists when I have some time.

Contributor

hcarty commented Jan 7, 2014

The mapM naming scheme breaks the general standard of using underscores rather than camel case for value names. Would map_m be better, or, since the functions are in their own submodules anyway, use map without the suffix?

Member

c-cube commented Jan 7, 2014

@hcarty map_m and fold_m seem reasonable, but I'm not sure we should abbreviate to map because it's not to be confused with the regular map function. The names are inspired from Haskell's library, but underscores seem fine.

Member

UnixJunkie commented Jan 8, 2014

Maybe momap for monadic map? :)

Member

UnixJunkie commented Jan 8, 2014

mmap is for system programming on Linux so that's not advisable
(mmap, munmap - map or unmap files or devices into memory)

Contributor

hcarty commented Jan 8, 2014

@UnixJunkie I prefer the _m suffix over a prefix for readability purposes.

@c-cube Given that the functions are already in their own module is name confusion a significant issue? Or do you intend for these functions to be included directly in the affected modules (ex. Lwt_seq.map_m rather than Lwt_seq.Traverse.map_m)?

Member

c-cube commented Jan 8, 2014

well, map_m and fold_m don't have exactly the same signature as usual map and fold functions, so I think it's fair they have a slightly different name. Typing two more chars shouldn't be too much of a problem ;)

Contributor

hcarty commented Jan 8, 2014

Fair point. I'm sold on the _m suffix now, for what that's worth!

@c-cube c-cube referenced this pull request Jan 12, 2014

Open

LWT support #10

Member

c-cube commented Jan 15, 2014

I have no idea why List did not contain a monad instance before, it seems very strange (concat_map or bind is a very useful combinator!). I think this should be in the next release, if possible, at least the Monad instances (maybe not Traversable)...

Member

c-cube commented Mar 2, 2014

Well, I still think having monadic bind instances for containers like List is very important. Should'nt we merge this?

Member

rgrinberg commented Mar 2, 2014

+1 from me. Where's filter_m though?

Member

c-cube commented Mar 2, 2014

@rgrinberg what would its type be? Something like this, I guess:

val filter_m : ('a -> bool) -> 'a t -> 'a t
Member

rgrinberg commented Mar 2, 2014

I was thinking more of:

val filter_m: ('a -> bool m) -> 'a t -> 'a t

But looks like I'm full of shit because for a second I confused your functions with the usual generic monadic functions (from Control.Monad) with your monad derived traversable instances. Ignore me...

Member

UnixJunkie commented Apr 17, 2014

Can't this be merged?

Member

c-cube commented Apr 17, 2014

I hope so! :)
I merged it in my own branch, but of course that's not the same thing at all.

Owner

gasche commented Apr 17, 2014

Can't this be merged?

Did someone (else than @c-cube) review the code, and do all the new or modified functions have tests?

Member

UnixJunkie commented Apr 17, 2014

It would have been nice if the pull request was cut into smaller pieces which would have been smaller,
hence faster to review and easier to accept.

Member

c-cube commented Apr 17, 2014

Well, I can still split it into smaller PRs if that's required (starting with the modification of Enum, then add List, Seq and LazyList separately).

Member

UnixJunkie commented Apr 17, 2014

From what I read and understand, only BatList has new tests.
And not even for all the new functionalities: only the monadic bind and return
are exercised by tests.
I suggest the following pull request should be closed, and reopened into several smaller ones
that we can polish and integrate one after the other.

Owner

gasche commented Apr 17, 2014

For what it's worth, I don't think the patch is that large. But I don't have time for a review, so it's up to whoever is ready to do the work (possibly adding tests as well).

Contributor

hcarty commented Apr 17, 2014

A visual review looks ok to me. I have not tested the path itself though.

bluddy commented Nov 18, 2014

Um... nothing's happened with this since April?

Owner

gasche commented Nov 18, 2014

@bluddy if you want to help the PR move forward, there is a rather easy way to do so: write tests for (some of the) extra functions, to help the PR satisfy the requirement that patches come with sufficient testing.

herry13 and others added some commits Jan 23, 2015

Add pp_print_list and pp_print_text
These were introduced in 4.02

Signed-off-by: Christoph Höger <christoph.hoeger@tu-berlin.de>
[ bug ] modify_opt randomly drops parts of the map
  * This is a naïve fix: I'm not sure whether one could use
    [create] rather than [make_tree] in order to avoid the
    rebalancing operation.
harden the implementation of BatSplay against immutable-data compiler…
… optims

BatSplay uses a bit of type-checking magic to respect the Map
interface (which requires map types to be covariant) while internally
keeping a mutable reference for (inobservable) rebalancing
mutations. The previous implementation is safe for the current
versions of the OCaml compiler, but has the fundamental issue of
mutating (through Obj magic) constructed values of a known-immutable
type.

The new implementation creates values at an immutable type, and then
magically coerce them into the covariant immutable type. This ensures
that the compiler knows (at value-creation time) that the allocated
memory is *not* immutable -- this is the same technique we use for
destination-passing-style lists.

This trick of "covariant mutable references" can be isolated as
a small submodule, which is then used (internally) throughout the
BatSplay implementation.

Thanks to Pierre Chambart for the discussion that led to this change.
provide implementations of new pp_* functions for older OCaml versions
The implementations are inspired by the original proposal of Daniel Bünzli in
  http://caml.inria.fr/mantis/view.php?id=6009
Add comment again
Signed-off-by: Christoph Höger <christoph.hoeger@tu-berlin.de>
Owner

gasche commented Apr 17, 2015

For the record, I just started looking at this PR again, writing tests to eventually be done with it. I'm in the List Traversable instance, and I found a small bug:

let rec map_m f l = match l with
| [] -> M.return []
| x::l' ->
  map_m f l' >>= fun l' ->
  f x >>= fun x ->
  M.return (x::l')

This will perform the side-effects right-to-left instead of the expected left-to-right (be it only to match with List.map's semantics). It needs to be rewritten as (I did this in my local branch):

  f x >>= fun x ->
  map_m f l' >>= fun l' ->
  M.return (x::l')

(and then probably stick an accumulator and List.rev to make this tailrec)

c-cube and others added some commits Jan 6, 2014

Seq.Traverse: add tests
Note that the behavior of Traverse functors on lazy sequences is still
not satisfying, because they tend to force the sequence (when applying
the `sequence` function for example) more than may appear necessary.

The instance of Traverse with the Option monad
  val sequence : 'a option BatSeq.t -> 'a BatSeq.option
must necessarily force the sequence to know whether there is a None somewhere,
but there is no interface reason why the Env monad
  val sequence : ('env -> 'a) BatSeq.t -> ('env -> 'a BatSeq.t)
should force the sequence (but it does).

This is not easy to fix. I think the Monad interface is not rich
enough to allow to preserve lazyness of the container (and interleave
it with the lazyness of the monad instance). Consider the map_m
implementation:

  let rec map_m f e = match e () with
    | Nil -> M.return nil
    | Cons (x, e') ->
      f x >>= fun x ->
      map_m f e' >>= fun e' ->
      M.return (cons x e')

The reason why the sequence is forced is that (map_m f e') is
evaluated before the (cons), instead of being in the cons
right-hand-side (to be computed only when subsequent elements
are requested). It is not quite possible to have the cons first,
because it needs to be inside a M.return, and once the M.return is
done it is not possible to call (map_m) recursively.

It is not clear which interface should be provided if we wanted to
allow this. Maybe something like
  val deep_bind : 'a m -> ('a -> (forall 'c. 'c m -> 'c) -> 'b m) -> 'b m
?
traversable instance for Enum
Traversable for Enum, albeit with a different functor name, for retrocompatibility reasons
Owner

gasche commented Apr 20, 2016

@c-cube , could you force-push the content of my monads branch (which includes your patches) into your own branch, so that we can keep the commits and discussion in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment