Make `IO#onError` consistent with `ApplicativeError` #4121

lenguyenthanh · 2024-08-20T15:53:22Z

This also deprecates the old IO.onError function and replace it's usage with the new one.

This also deprecates the old `IO.onError` function.

As it requires to annotate anonymous function: The argument types of an anonymous function must be fully known. (SLS 8.5)

armanbilge · 2024-08-20T19:20:02Z

core/shared/src/main/scala/cats/effect/IO.scala

@@ -588,9 +589,19 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] {
  def onCancel(fin: IO[Unit]): IO[A] =
    IO.OnCancel(this, fin)

+  @deprecated("Use onError with PartialFunction argument", "3.6.0")
  def onError(f: Throwable => IO[Unit]): IO[A] =
    handleErrorWith(t => f(t).voidError *> IO.raiseError(t))


Can we delegate this to the other method?

Suggested change

handleErrorWith(t => f(t).voidError *> IO.raiseError(t))

onError(f: PartialFunction[Throwable, IO[Unit]])

mm, I'm not sure I understand, I think your suggestion shouldn't be compiled?
Or do you mean use the new onError?
btw, I'm about to go to bed, will think more and answer tomorrow.

I think your suggestion shouldn't be compiled? Or do you mean use the new onError?

Oops, I made a typo. But I think you are right, this won't compile as-is 🤔

Basically we should avoid to duplicate the onError implementation. We can share the implementation by delegating one onError to the other onError, by converting Function to PartialFunction or vice versa.

Fixed this in f4f505c

Another way would be

- handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit) *> IO.raiseError(t)) + onError(t => pf.applyOrElse(t, (_: Throwable) => IO.unit))

but it feels awkward as We used a function that We just deprecated it.

armanbilge · 2024-08-22T05:28:04Z

core/shared/src/main/scala/cats/effect/IO.scala

+   * Implements `ApplicativeError.onError`.
+   */
+  def onError(pf: PartialFunction[Throwable, IO[Unit]]): IO[A] =
+    handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit) *> IO.raiseError(t))


To match the other implementation we need this voidError. But this raises an interesting question about the semantics: if the onError callback raises an error, which error should be propagated, that error or the original one? Also the voidError completely discards the error without reporting it, which seems really weird 😕

Suggested change

handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit) *> IO.raiseError(t))

handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit).voidError *> IO.raiseError(t))

I think what we should do is extract this into a helper method.

cats-effect/core/shared/src/main/scala/cats/effect/IO.scala

Line 493 in 7168625

IO.executionContext.flatMap(ec => IO(ec.reportFailure(t)))

Then instead of voidError we should redirect errors within the onError callback to that? And then re-raise the original error.

thanks @armanbilge, I'm completely missed that voidError. I added reportError helper method as your suggestion. I think this is a great improvement. The name is subjecting for bike-shedding. We can also use this reportError method in the two other places. I can do that in this pr.

I'm completely missed that voidError

Yes, I almost missed it as well. We should add a test to cover this case (what happens if onError callback raises an error), that would have helped us. Actually we might even consider adding this as a law to Cats, so that onError semantics are the same across all implementations.

We can also use this reportError method in the two other places. I can do that in this pr.

🚀 it looks good! I think name is reasonable and it is private internal anyway.

cool, I'll add a test case for in this first. And think about a law for cats later on.

Hmm, actually, now I am hesitating 😅

The goal of this PR is to make IO#onError consistent with ApplicativeError#onError. If we use voidError/reportError then the semantics are inconsistent again with ApplicativeError 😕

https://github.com/typelevel/cats/blob/8e8724a29881a394bc39357a7f0cd21124573f30/core/src/main/scala/cats/ApplicativeError.scala#L261-L262

Unless we change it in Cats (and this may be too drastic of a change) I think we just have to let the callback's error propagate instead.

ahhhh, you're right. So, my first implemenaton was accidentally correct :p.

With this change, if the onError callback raises any error, We'll report that error and then re-raise the original error.

armanbilge · 2024-08-22T18:13:51Z

core/shared/src/main/scala/cats/effect/IO.scala

+  @deprecated("Use onError with PartialFunction argument", "3.6.0")
+  def onError(f: Throwable => IO[Unit]): IO[A] = {
+    val pf: PartialFunction[Throwable, IO[Unit]] = { case t => f(t) }
+    onError(pf)
+  }
+
+  /**
+   * Execute a callback on certain errors, then rethrow them. Any non matching error is rethrown
+   * as well.
+   *
+   * Implements `ApplicativeError.onError`.
+   */
+  def onError(pf: PartialFunction[Throwable, IO[Unit]]): IO[A] =
+    handleErrorWith(t =>
+      pf.applyOrElse(t, (_: Throwable) => IO.unit).reportError *> IO.raiseError(t))


I guess we should use reportError for the deprecated onError, but for the onError consistent with ApplicativeError we have to match the semantics defined in Cats.

Suggested change

@deprecated("Use onError with PartialFunction argument", "3.6.0")

def onError(f: Throwable => IO[Unit]): IO[A] = {

val pf: PartialFunction[Throwable, IO[Unit]] = { case t => f(t) }

onError(pf)

}

/**

* Execute a callback on certain errors, then rethrow them. Any non matching error is rethrown

* as well.

*

* Implements `ApplicativeError.onError`.

*/

def onError(pf: PartialFunction[Throwable, IO[Unit]]): IO[A] =

handleErrorWith(t =>

pf.applyOrElse(t, (_: Throwable) => IO.unit).reportError *> IO.raiseError(t))

@deprecated("Use onError with PartialFunction argument", "3.6.0")

def onError(f: Throwable => IO[Unit]): IO[A] = {

val pf: PartialFunction[Throwable, IO[Unit]] = { case t => f(t).reportError }

onError(pf)

}

/**

* Execute a callback on certain errors, then rethrow them. Any non matching error is rethrown

* as well.

*

* Implements `ApplicativeError.onError`.

*/

def onError(pf: PartialFunction[Throwable, IO[Unit]]): IO[A] =

handleErrorWith(t =>

pf.applyOrElse(t, (_: Throwable) => IO.unit)*> IO.raiseError(t))

fixed in: 868451b

For the ApplicativeError.onError method, We let the callback's error propagate to match with cats's ApplicativeError semantic. For the old onError, We report callback's error and raise the original error instead.

armanbilge

Nice, this looks good to me. It turns out the old onError was inconsistent both in signature and in behavior, so makes sense to deprecate.

armanbilge · 2024-08-22T20:38:13Z

@lenguyenthanh if you want a small follow-up PR, I think it would be worthwhile to add a law to Cats testing this behavior when onError raises an error.

lenguyenthanh · 2024-08-23T10:55:42Z

thanks @armanbilge for your guidance, It helped me a lot!

For the onError laws, iiuc, We already have it: https://github.com/typelevel/cats/blob/8e8724a29881a394bc39357a7f0cd21124573f30/laws/src/main/scala/cats/laws/ApplicativeErrorLaws.scala#L70-L71C63

Not sure why We didn't catch it here, maybe because of the generator?

armanbilge · 2024-08-25T22:39:43Z

For the onError laws, iiuc, We already have it:

@lenguyenthanh ah yes, you're right. Even though there is no explicit test for this case, it should be covered by the generator.

Not sure why We didn't catch it here, maybe because of the generator?

I was confused about this as well, but I think I know why. We actually need to do one more thing in your PR: we need to add an onError override to the IOAsync instances, because otherwise your implementation is actually not being law tested.

cats-effect/core/shared/src/main/scala/cats/effect/IO.scala

Line 1965 in 868451b

    
           private[this] final class IOAsync extends kernel.Async[IO] with StackSafeMonad[IO] {

Actually, we should make sure that every custom method on IO is overridden in the IOAsync instances. If we had done this at the beginning for onError we would have realized that the signature and implementation are wrong.

lenguyenthanh · 2024-08-26T17:52:29Z

If we had done this at the beginning for onError we would have realized that the signature and implementation are wrong.

@armanbilge added the onError override to IOAsync. But it seems tests are passed even with the old onError method. I guess it's because a partial function is a function and our test suite doesn't work well with throwable 🤷. I'll dig more into this after #3757

armanbilge · 2024-08-28T00:12:38Z

But it seems tests are passed even with the old onError method.

Hmm, if I restore the semantics mismatch/bug, then the laws fail. So seems to be working.

diff --git a/core/shared/src/main/scala/cats/effect/IO.scala b/core/shared/src/main/scala/cats/effect/IO.scala
index ee3a9da1f..408b2a5e9 100644
--- a/core/shared/src/main/scala/cats/effect/IO.scala
+++ b/core/shared/src/main/scala/cats/effect/IO.scala
@@ -596,7 +596,7 @@ sealed abstract class IO[+A] private () extends IOPlatform[A] {
    * Implements `ApplicativeError.onError`.
    */
   def onError(pf: PartialFunction[Throwable, IO[Unit]]): IO[A] =
-    handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit) *> IO.raiseError(t))
+    handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit).voidError *> IO.raiseError(t))
 
   /**
    * Like `Parallel.parProductL`

failing seed for async.applicativeError onError raise is 7OQiZWdn0skINxhWMBdm5dKBPdF49wcLmELPfFNLY_D=
[error] x async.applicativeError onError raise
[error]  Falsified after 4 passed tests.
[error]  > Labels of failing property:Expected: IO(...)
[error]  Received: IO(...)> ARG_0: IO(...)
[error]  > ARG_1: java.lang.Exception
[error]  > ARG_2: IO(...)
[error]  The seed is 7OQiZWdn0skINxhWMBdm5dKBPdF49wcLmELPfFNLY_D=

lenguyenthanh added 2 commits August 20, 2024 17:47

Make IO.onError consistent with Applicative.onError

932fa7d

This also deprecates the old `IO.onError` function.

Replace usage of the old onError with the new one

b4cd5e5

lenguyenthanh force-pushed the make-io-onError-consistent-with-applicative-error branch from f9ae95c to b4cd5e5 Compare August 20, 2024 15:56

lenguyenthanh marked this pull request as draft August 20, 2024 16:13

Make scala 2.12 happy

7825274

As it requires to annotate anonymous function: The argument types of an anonymous function must be fully known. (SLS 8.5)

lenguyenthanh force-pushed the make-io-onError-consistent-with-applicative-error branch from 9aedbfe to 7825274 Compare August 20, 2024 17:15

lenguyenthanh marked this pull request as ready for review August 20, 2024 17:36

armanbilge changed the title ~~Make io on error consistent with applicative error~~ Make IO#onError consistent with ApplicativeError Aug 20, 2024

armanbilge reviewed Aug 20, 2024

View reviewed changes

armanbilge added the 🪲 bug label Aug 20, 2024

armanbilge added this to the v3.6.0 milestone Aug 20, 2024

Reuse logic between IO.onError

f4f505c

armanbilge reviewed Aug 22, 2024

View reviewed changes

Add reportError helper and use it for onError

40a7dde

With this change, if the onError callback raises any error, We'll report that error and then re-raise the original error.

armanbilge reviewed Aug 22, 2024

View reviewed changes

lenguyenthanh added 2 commits August 22, 2024 20:54

Use reportError when applicable

4764d05

Fix onError callback error handling semantic

868451b

For the ApplicativeError.onError method, We let the callback's error propagate to match with cats's ApplicativeError semantic. For the old onError, We report callback's error and raise the original error instead.

armanbilge previously approved these changes Aug 22, 2024

View reviewed changes

Override onError for IOAsync

c6026b3

lenguyenthanh dismissed armanbilge’s stale review via c6026b3 August 26, 2024 17:18

armanbilge approved these changes Aug 28, 2024

View reviewed changes

armanbilge merged commit dab9d23 into typelevel:series/3.x Aug 28, 2024
29 of 33 checks passed

lenguyenthanh deleted the make-io-onError-consistent-with-applicative-error branch August 28, 2024 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `IO#onError` consistent with `ApplicativeError` #4121

Make `IO#onError` consistent with `ApplicativeError` #4121

lenguyenthanh commented Aug 20, 2024

armanbilge Aug 20, 2024 •

edited

Loading

lenguyenthanh Aug 20, 2024

armanbilge Aug 20, 2024

lenguyenthanh Aug 21, 2024

armanbilge Aug 22, 2024

armanbilge Aug 22, 2024

lenguyenthanh Aug 22, 2024

armanbilge Aug 22, 2024

lenguyenthanh Aug 22, 2024

armanbilge Aug 22, 2024

lenguyenthanh Aug 22, 2024

armanbilge Aug 22, 2024

lenguyenthanh Aug 22, 2024

armanbilge left a comment

armanbilge commented Aug 22, 2024

lenguyenthanh commented Aug 23, 2024 •

edited

Loading

armanbilge commented Aug 25, 2024

lenguyenthanh commented Aug 26, 2024

armanbilge commented Aug 28, 2024

	handleErrorWith(t => f(t).voidError *> IO.raiseError(t))
	onError(f: PartialFunction[Throwable, IO[Unit]])

	handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit) *> IO.raiseError(t))
	handleErrorWith(t => pf.applyOrElse(t, (_: Throwable) => IO.unit).voidError *> IO.raiseError(t))

Make IO#onError consistent with ApplicativeError #4121

Make IO#onError consistent with ApplicativeError #4121

Conversation

lenguyenthanh commented Aug 20, 2024

armanbilge Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

armanbilge commented Aug 22, 2024

lenguyenthanh commented Aug 23, 2024 • edited Loading

armanbilge commented Aug 25, 2024

lenguyenthanh commented Aug 26, 2024

armanbilge commented Aug 28, 2024

Make `IO#onError` consistent with `ApplicativeError` #4121

Make `IO#onError` consistent with `ApplicativeError` #4121

armanbilge Aug 20, 2024 •

edited

Loading

lenguyenthanh commented Aug 23, 2024 •

edited

Loading