Reimplemented MonadError[FreeT[...]] to be correct

[djspiewak]

There's an explanation of this in the comments in the code. I retained the old implementation for binary compatibility, but it is somewhat unfortunate since any code which is hitting that is outright-broken and people may not know it. Then again, I'm not sure how many people (other than me) are using FreeT, so maybe it's okay either way.

Fixes #3240

[LukaJCB]

typo: ...retained **above** for...

[kubukoz]

Is it possible to adjust the generators / equalities used in tests to verify that the old implementation does indeed break a law?

[codecov-io]

Codecov Report

Merging #3241 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3241      +/-   ##
==========================================
+ Coverage   93.05%   93.08%   +0.02%     
==========================================
  Files         376      376              
  Lines        7413     7428      +15     
  Branches      201      196       -5     
==========================================
+ Hits         6898     6914      +16     
+ Misses        515      514       -1

Flag	Coverage Δ
#scala_version_212	93.38% <100%> (-0.01%)	⬇️
#scala_version_213	92.85% <100%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
...a/cats/laws/discipline/ApplicativeErrorTests.scala	100% <100%> (ø)	⬆️
free/src/main/scala/cats/free/FreeT.scala	92.68% <100%> (+2.01%)	⬆️
...rc/main/scala/cats/laws/ApplicativeErrorLaws.scala	100% <100%> (ø)	⬆️
core/src/main/scala/cats/syntax/list.scala	100% <0%> (ø)	⬆️

---

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 f839716...9d5542f. Read the comment docs.

[kailuowang]

Thanks for spotting and fixing this bug.
A couple questions if you don't mind:
It seems that making it private[this] still breaks BC, probably need to stick to package private.
Why keeping the old faulty implementation (v.s. letting the old method delegate to the new correct one)?
It kind of itches that a faulty handleErrorWith implementation doesn't break the ApplicativeErrorLaws. There is no way to add a new law to capture this right?

[djspiewak]

@kubukoz I've been wondering the same thing myself. I tried fiddling around with some sort of distributivity law for handleErrorWith but it's surprisingly subtle, because you still need to ensure ordered short-circuiting. I would definitely like to codify this better.

@kailuowang See above. :-) I want to add a law. Thoughts welcome. My attempt was something like this:

def handleErrorWithDistributesOverAp[A, E](fa1: F[A], fa2: F[A], h: E => F[A]): F[A] =
  F.handleErrorWith(fa1 *> fa2)(h) <-> (F.handleErrorWith(fa1)(h) *> F.handleErrorWith(fa2)(h))

That's not actually right, though, because fa1 should short-circuit fa2 if it errors, whereas it will not in the right side.

Why keeping the old faulty implementation (v.s. letting the old method delegate to the new correct one)?

The constraints are different. The new requires a Functor[S]. I am tempted to just add a sys.error though, because any code using the old implementation is broken in a very dangerous way.

It seems that making it private[this] still breaks BC, probably need to stick to package private.

Actually it has to be public. The problem is the static forwarder. I've fixed it now.

[kailuowang]

Thanks @djspiewak I missed the new constraint. This is source breaking then, I am adding the label just so that it doesn't surprise users.

[djspiewak]

How about this as a law?

def attemptDistributivity[A, B](fa: F[A], f: A => F[B]): F[Either[E, B]] =
  F.attempt(F.flatMap(fa)(f)) <-> F.flatMap(F.attempt(fa))(_.traverse(f.andThen(F.attempt)))

[djspiewak]

(btw, please don't merge this just yet; even aside from the discussion about a new law, which is important, I have some ancillary evidence that this isn't appropriately stack-safe.

[djspiewak]

@kailuowang @kubukoz Okay, I've added a pair of laws in ApplicativeError which capture this case. I've verified that they do indeed fail on FreeT in master. They aren't quite as strong as what I would like (explained in a comment), but I think they should appropriately constrain any non-pathological implementations.

I've also added a Defer for FreeT, which was previously absent, and used it to ensure the stack-safety of handleErrorWith even when the suspension monad does not provide a stack-safe flatMap. An obvious objection here is that, if the suspension monad is not stack-safe, then presumably stack-safety in handleErrorWith is just delaying the inevitable. However, it's entirely possible to run handleErrorWith on FreeT when the suspension monad is not stack-safe, and then subsequently mapK into a stack-safe context prior to interpretation. That use-case was impossible with my initial implementation.

Edit: Also to be clear, feel free to merge once everyone approves, particularly the new laws. I'm not aware of any further changes that are necessary, excepting what I'll need to change in response to review.

[kubukoz]

Another way might be Arbitrary[F[_]].map(_.attempt)... But that's probably problematic in other ways (depending on the behavior of attempt)

[djspiewak]

Well, the problem is then you need a function E => A. That's not doable without breaking binary compatibility.

[kubukoz]

Why binary compatibility? I fail to see that here...

[djspiewak]

In order to have an Arbitrary[E => A], we need a Cogen[E] and an Arbitrary[A]. We have the former, but not the latter (in ApplicativeErrorTests). In order to get the latter, we would need to add it as a parameter to the applicativeError function, which breaks binary compatibility.

[kubukoz]

Ohhh. Yeah, you're absolutely right.

[kailuowang]

Thanks very much!