Restore FlatMap.map2Eval laziness

[satorg]

Resolves #4858.

[johnynek]

I guess the motivation was that inside the flatMap the .value might not blow the stack and if the F[_] is lazy it would possibly be safe.

The change makes us always evaluate the Eval[F[B]], but it is safe.

The motivation of the map2Eval is for Monads like Try/Either that a failure on the left avoids evaluating the right. This change will evaluate the Eval in those cases and no longer short circuit.

Maybe to mitigate this we want to make sure any shortcircuiting Monads in cats override map2Eval so we don't regress on cases like Try, Future, Either. Maybe others.

A middle ground could be to override StackSafeMonad to use the .value on the idea that the flatMap is always lazy in a StackSafeMonad. That may work or may have other issues that I don't see.

[satorg]

Maybe to mitigate this we want to make sure any shortcircuiting Monads in cats override map2Eval so we don't regress on cases like Try, Future, Either. Maybe others.

Yes, good call. I think that would a correct direction to go. We don't know and can't guess whether current FlatMap instance is for a short-circuiting Monad or not. The goal for generic methods is to be as safe as possible (I believe so). The concrete implementation though does know whether their Monad can short-circuit or not. So, it is more likely their responsibility to optimize for short-circuiting cases.

[johnynek]

It would be nice to have a way to easily audit the code for calls to Eval#value. That's the dangerous thing that was done here unsafely.

It's a bit challenging to formulate the rules of exactly when it is and isn't safe, but something that could be called in a recursive context is definitely not safe, and map2Eval does get called that way on a traverse.

[satorg]

@johnynek , what do you think about these two functions that seem to be designed so that Eval#value is always called from the inside:

• cats/core/src/main/scala/cats/FlatMap.scala

  Line 75 in 086c477

  def productREval[A, B](fa: F[A])(fb: Eval[F[B]]): F[B] = flatMap(fa)(_ => fb.value)

• cats/core/src/main/scala/cats/FlatMap.scala

  Line 99 in 086c477

  def productLEval[A, B](fa: F[A])(fb: Eval[F[B]]): F[A] = flatMap(fa)(a => as(fb.value, a))

[johnynek]

since those have Eval on the input and no Eval on the result you have to discard the Eval or call .value on it i think. No other way.

But if you were to recursively use these I think they could blow the stack as well. That said, since they don't return Eval[F[B]] it seems less likely that someone would think it was safe to use them recursively.

In this case though, I'd say that just using fb: => F[B] might have been the better choice, but maybe there is already a function like that.

Definitely overriding these for the usual short-circuiting monads seems like a good idea (List/Option/Either/Try/Future).