add ifElseM

[mtomko]

The actual implementation is from @djspiewak:

https://gist.github.com/djspiewak/1b92a6e338f4e1537692e748c54b9743

We discussed this function briefly on the cats-effect gitter channel:

https://gitter.im/typelevel/cats-effect?at=5f297e4314c413356f56d230

As indicated in the discussion there, it wasn't totally clear where it belonged. There is no package object in cats.syntax.flatMap and I'm not sure adding a package object now makes sense, with Scala 3 on the horizon. This is my first code contribution to cats and I'm still learning the code layout so I'm happy to move things around to suit people's preferences.

Also, I did see some failures locally in the alleycats JS tests, but a clean checkout of the master branch also had those, and I don't see how I could have broken them or made the situation any worse with the changes I've made here. I'm curious to see what the CI build does - if it has problems I'll investigate further.

[codecov-commenter]

Codecov Report

Merging #3553 into master will decrease coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3553      +/-   ##
==========================================
- Coverage   91.30%   91.04%   -0.26%     
==========================================
  Files         386      385       -1     
  Lines        8565     8554      -11     
  Branches      248      251       +3     
==========================================
- Hits         7820     7788      -32     
- Misses        745      766      +21     

[barambani]

I see from the discussion that calling it from the object (FlatMap.ifElseM(...)) is the preferred solution from the ergonomics point of view when compared to the class/syntax alternative (fa.ifElseM(p1 -> a1, p2 -> a2, ...)(else)) so the location seems ok to me. I don't use this pattern much so I don't have preferences. 👍

[johnynek]

I'd rather see this directly on the Monad (or FlatMap) trait since that is binary compatible on 2.12 and later.

[johnynek]

PS: I can see why the the ergonomics is nicer on the object (in fact, we explored this pattern many years ago, but cats never seemed to like it, it makes type inference better. See e.g.:

cats/kernel/src/main/scala/cats/kernel/Monoid.scala

Line 98 in a26f161

abstract class MonoidFunctions[M[T] <: Monoid[T]] extends SemigroupFunctions[M] {

which we copied from algebird. Being able to do Monoid.combine(a, b) is great for type inference, but copying each method to the companion object is a bit of a pain, so I eventually buckled and felt that Monoid[Int].combine was a fine compromise in general. When the type is really ugly, that isn't so great of course).

[mtomko]

Will do, running tests now...

[johnynek]

I thought I left this comment but I don't see it now.

If we have a long list of branches, this will stack overflow. We can use tailRecM to address that if we work on Monad:

type Branches = List[(F[Boolean], F[A])]
def step(branches: Branches): F[Either[Branches, A]] =
  branches match {
    case (cond, conseq) :: tail =>
      cond.flatMap { b => if (b) conseq.map(Right(_)) else pure(Left(tail)) }
    case Nil => els.map(Right(_))
}

tailRecM(branches)(step(_))

[mtomko]

Thanks for pointing that out. I have a couple of questions. I'm not sure how to spot when a flatMap isn't stack-safe - is it dependent on the underlying monad's own stack-safety or is there something I can see in the chain of calls to flatMap itself?

Also, if simply using tailRecM is the solution, there is a tailRecM for FlatMap. I'm not averse to moving this code over to Monad but if we don't need the full power of Monad for stack-safety, then I guess I'd like to save the effort.

Thanks again for your comments and suggestions!

[mtomko]

Ah, I just reread the cats FAQ on tailRecM. So it's nothing particular to this implementation, it depends on the underlying Monad, and using tailRecM avoids that problem. I'm going to reframe the implementation in terms of tailRecM.

[mtomko]

And, now I think I see why it has to be on Monad...

[mtomko]

Just a wild thought, too, we could call this condM as a nod to our FP roots (Lisp's cond form)... It almost has that feel.

[johnynek]

This looks good to me. Thanks for making the stack safety changes.

BTW: I don't have a strong opinion about the name. Cond is a cute reference. But personally I'd lean to what you have which I think will be more intuitive for newcomers.

[mtomko]

@johnynek thanks so much for your help. I'm digging into some more resources on stack safety now - I need to learn a few things before I can properly understand the Phil Freeman paper but I think it'll be good for me to build that background.

I think ultimately the shape of this method isn't quite the same as cond because the els has to be in a separate argument list. I'll just stick with ifElseM for that reason and for the reason you suggested (approachability).