add foldMapA as an alternative to foldMapM that only requires an Applicative rather than a monad

[mberndt123]

Applicative is enough for this method, so it shouldn't require Monad.

[travisbrown]

This could probably be done in a binary-compatible way by making the current implementation package private and adding the new one as an overload. I agree with @LukaJCB that it probably makes more sense to add a new foldMapA with the appropriate constraint instead.

It seems like the bigger issue with this implementation is short circuiting. For example, this will hang forever with the foldMap implementation:

import cats.implicits._

Stream.iterate(0)(_ + 1).foldMapM(b => if (b > 10) Left(b) else Right(b))

I think an implementation like this should be fine both in terms of stack safety and short-circuiting:

def foldMapA[G[_], A, B](fa: F[A])(f: A => G[B])(implicit G: Applicative[G], B: Monoid[B]): G[B] =
  foldRight(fa, Eval.now(G.pure(B.empty)))((a, egb) =>
    G.map2Eval(f(a), egb)(B.combine)
  ).value

For example:

scala> import cats.implicits._
import cats.implicits._

scala> Stream.iterate(0)(_ + 1).foldMapA(b => if (b > 10) Left(b) else Right(b))
res0: scala.util.Either[Int,Int] = Left(11)

scala> Stream.iterate(0)(_ + 1).foldMapA(b => if (b > 1000000) Left(b) else Right(b))
res1: scala.util.Either[Int,Int] = Left(1000001)

[mberndt123]

Hey there,

Thanks for the review! I didn't know about map2Eval, that's very interesting.

I've now added foldMapA as a new method and also a test to make sure the short-circuiting behaviour is preserved.

[codecov-io]

Codecov Report

Merging #3130 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3130      +/-   ##
=========================================
+ Coverage   93.19%   93.2%   +<.01%     
=========================================
  Files         376     376              
  Lines        7323    7324       +1     
  Branches      190     186       -4     
=========================================
+ Hits         6825    6826       +1     
  Misses        498     498

Flag	Coverage Δ
#scala_version_212	93.53% <100%> (ø)	⬆️
#scala_version_213	90.85% <100%> (-0.02%)	⬇️

Impacted Files	Coverage Δ
core/src/main/scala/cats/Foldable.scala	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fb1327...6f9b8cc. Read the comment docs.

[travisbrown]

I think we should merge this, although I don't completely like the idea of having two methods with identical semantics but different constraints and (probably) different performance expectations.

[travisbrown]

Do we actually know that this is true? My intuition is that it generally will be, but there's also some Either overhead involved in the tailRecM implementation.

[travisbrown]

I put together a little benchmark with a vector with 10k elements with examples that both short-circuit in Either and complete successfully, and yeah, the foldMapM implementation is a lot faster:

[info] Benchmark                            Mode  Cnt     Score     Error  Units
[info] FoldMapABench.foldMapA              thrpt   20  2113.567 ± 137.868  ops/s
[info] FoldMapABench.foldMapM              thrpt   20  3445.172 ± 131.904  ops/s
[info] FoldMapABench.foldMapAShortCircuit  thrpt   20  5476.648 ± 289.969  ops/s
[info] FoldMapABench.foldMapMShortCircuit  thrpt   20  6699.230 ± 259.521  ops/s

[LukaJCB]

Let's update the scaladoc and get this merged :)

[mberndt123]

@LukaJCB I'm not quite sure what more I'm supposed to add for scaladoc. Isn't it enough to say that this does the same thing as foldMapM?

[LukaJCB]

Sorry had a brainfart there 😄

[LukaJCB]

Thanks again @mberndt123! 👍