Error handle without panic #1042

johnhungerford · 2025-01-21T15:39:11Z

Result.Partial[E, A] type alias for Success[A] | Failure[E]
Abort.runPartial to run to Result.Partial and not handle Panic cases
Abort.fold to handle either all three cases or (overloaded) just Success and Failure.
partialResult combinator wrapping Abort.runPartial
foldAbort combinator wrapping Abort.fold
fold and partialResult on ForAbortOps to handle a subset of the Abort union.

…mbinators

johnhungerford · 2025-01-21T15:39:59Z

Closes #1014

kyo-prelude/shared/src/main/scala/kyo/Abort.scala

kyo-data/shared/src/main/scala/kyo/Result.scala

johnhungerford · 2025-01-21T20:58:43Z

Need to add tests for Result.Partial API now that it's an opaque type

fwbrasil · 2025-01-21T22:50:12Z

FYI there's something going on in CI making builds fail. I thought it could be the build caching I had introduced but I reverted it and it's still happening. I'm looking into it.

kyo-prelude/shared/src/main/scala/kyo/Abort.scala

kyo-data/shared/src/main/scala/kyo/Result.scala

…t-panic

fwbrasil · 2025-01-28T17:49:53Z

kyo-prelude/shared/src/main/scala/kyo/Abort.scala

+            reduce: Reducible[Abort[ER]]
+        ): Result.Partial[E, A] < (S & reduce.SReduced) =
+            Abort.runWith[E](v):
+                case Panic(thr)                      => throw thr


This doesn't seem correct. The returned computation should have a pending Abort[Nothing] given that panics aren't handled. I think we can remove the Reducible evidence and return Result.Partial[E, A] < (S & Abort[ER]), which will infer to Abort[Nothing] if it's the only failure pending.

I've added some additional ...OrThrow methods to handle completely and throwing in the case of Panic.

kyo-prelude/shared/src/main/scala/kyo/Abort.scala

johnhungerford · 2025-01-28T20:51:33Z

kyo-prelude/shared/src/test/scala/kyo/AbortTest.scala

@@ -267,7 +267,7 @@ class AbortsTest extends Test:
                "converts matching Panic to Fail" in {
                    val ex     = new RuntimeException("Test exception")
                    val result = Abort.run[RuntimeException](Abort.panic(ex)).eval
-                    assert(result == Result.panic(ex))
+                    assert(result == Result.Failure(ex))


This test did not assert what it purported to be testing. Note that I made changes to Abort.run (now Abort.runWith) to produce the behavior this test seems to want. This is now producing a bunch of failed kyo-core tests, however.

Do we actually want Panic to be lifted to Failure or not? If yes, I'll go and fix the other tests.

I think I had introduced this behavior initially and changed it later based on @hearnadam's feedback but I forgot to update the test. We should probably remove it.

Ok -- that's where I was leaning after making the changes.

Do we want to continue to convert caught exceptions to Failure when they are subtypes? This doesn't seem to be tested, but it's implemented in Abort.run (now Abort.runWith). This behavior seems similarly problematic in that it means potentially handling failures from some other part of your application unexpectedly:

// Say you are given a program that gets some text, and it throws an unexpected (i.e., no Abort) `IOException` val effect1: String < Async = ??? // Now say you have an method to persist text, which expects to sometimes throw an `IOException`. // Accordingly, it tracks these failures with `Abort`. def persistValue(int: Int): Unit < (Async & Abort[IOException]) = ??? // Then you combine them retrieve and persist text val effect2: Int < (Async & Abort[IOException]) = effect1.map(someMethod) // Finally, you handle the error, assuming it's from persistence, not retrieval val effect3 = Abort.fold[IOException[(effect2): case Result.Success(_) => Console.printLine("Successfully retrieved and persisted text") case Result.Failure(_) => Console.printLine("Failed to persist text") case Result.Panic(e) => Console.printLine(s"Failed to retrieve or persist text with unexpected exception: $e")

In the above scenario, effect3, when run, would report that it failed to persist the text when in cases where it in fact failed to retrieve the text.

I feel this is a reasonable behavior. If we don't catch exceptions in Abort operations, we would need to provide method variations that include catching since it's a common need. I normally use it to handle failures coming from other libraries for example. @hearnadam do you mind sharing your thoughts on this?

I think we should avoid over-catching exceptions the user does not wrap. Obviously the machinery for effect handling may need to catch this for enriched traces, but I doubt we should be putting that into the 'expected' error channel.

The internal implementation could not catch, that's possible. My concern is the semantics for the user. I think this code not catching the exception would be surprising and inconvenient:

Abort.run[SomeException](someCodeMayThrowTheException) // would have to become Abort.run[SomeException](Abort.catching[SomeException](someCodeMayThrowTheException))

It seems your mental model is that Abort should be concerned mainly with expected failures? I'd say the majority of the users not used to type-level failure tracking would find the strictness unnecessary. Are there scenarios where it would be essential?

I think the scenario I gave above is an telling case. While some users would probably want to lift thrown exceptions to failures, those who do not would not have any way to distinguish between expected exceptions and those of the same type thrown unexpectedly by some other part of their code base.

For the former user, we could add a method runCatching.

I still have trouble to see the value of such behavior considering the potential drawback in usability but I might be missing something. Can we follow up on in a new issue?

johnhungerford · 2025-01-28T20:54:34Z

kyo-combinators/shared/src/main/scala/kyo/Combinators.scala

+        fl2: Flat[B],
+        fr: Frame
+    ): B < (S & S1) =
+        Abort.foldOrThrow(onSuccess, onFail)(effect)


Note that in kyo-combinators, I'm throwing Panic exceptions by default for all error-handling methods that ignore panic when handling for the full Abort type. When using forAbort[E], these methods will leave Panic unhandled and eventually reduce to Abort[Nothing].

Is it a usability decision? It'd be nice to keep a similar behavior to the Abort APIs

Yeah usability. The point of this PR is to have a way to deal with Abort where you don't have to think about panic. Leaving an Abort[Nothing] in the pending intersection when you handle success and failure means you have to handle Abort a second time which is not great.

On the other hand, if you are only handling a single subtype within the Abort, you probably don't want to throw Panic exceptions. For this reason, I broke the Abort methods into two forms -- one that leaves Abort[Nothing] and one that throws (Abort.fold vs Abort.foldOrThrow, and Abort.runPartial and Abort.runPartialOrThrow). The ...OrThrow methods operate on only the whole Abort type rather than allowing you to specify a type. This is essentially the same behavior as kyo-combinators.

Sounds reasonable. Something to keep in mind is that Abort[Nothing] is included in IO and Async but I think the compiler doesn't automatically de-duplicate the pending effect.

Hmm I didn't realize that. In that case, it probably makes more sense to have all of these methods not throw and just handle to Abort[Nothing] and to add separate ...OrThrow methods. Most cases where a user will want to handle Abort will be in conjunction with Async or IO anyways, and this could result in repeatedly throwing and catching the same exceptions, which seems bad.

I'll make the combinators work the same way as Abort: fold/foldOrCatch and partialResult/partialResultOrThrow.

I'll make orPanic reduce Abort[X] to Abort[Nothing], and I'll add an orThrow method to eliminate Abort entirely by throwing exceptions.

Cool! If we're able to keep similar semantics, it'd be easier for people to start with simplified APIs and eventually migrate to rich combinators as they become more proficient

Done. I feel happy with the current API.

…t-panic

…d/kyo into error-handle-without-panic

kyo-prelude/shared/src/main/scala/kyo/Abort.scala

fwbrasil

This is a great improvement! Thanks for making the changes across the modules :)

fwbrasil · 2025-02-01T23:13:48Z

Let's wait for @hearnadam's feedback to merge

hearnadam

I'm very happy to see these methods being added, though I'm not in love with much of the naming. We can always change them later... but I don't like doing so 😅

hearnadam · 2025-02-01T23:20:53Z

kyo-combinators/shared/src/main/scala/kyo/Combinators.scala

@@ -405,7 +512,7 @@ extension [A, S, E](effect: A < (Abort[E] & S))
        handled.map((v: Result[E, A]) => Abort.get(v.swap))
    end swapAbort

-    /** Catches any Aborts and panics instead
+    /** Converts any Aborts to Panic, wrapping non-Throwable Failures in PanicException


Suggested change

/** Converts any Aborts to Panic, wrapping non-Throwable Failures in PanicException

/** Converts all Aborts to Panic, wrapping non-Throwable Failures in PanicException

hearnadam · 2025-02-01T23:22:32Z

kyo-combinators/shared/src/main/scala/kyo/Combinators.scala

            case Result.Success(v)              => v
-            case Result.Failure(thr: Throwable) => Abort.panic(thr).asInstanceOf[Nothing < Any]
+            case Result.Failure(thr: Throwable) => Abort.panic(thr)
            case Result.Failure(other)          => Abort.panic(PanicException(other)).asInstanceOf[Nothing < Any]


Suggested change

case Result.Failure(other) => Abort.panic(PanicException(other)).asInstanceOf[Nothing < Any]

case Result.Failure(other) => Abort.panic(PanicException(other))

hearnadam · 2025-02-01T23:26:12Z

kyo-prelude/shared/src/test/scala/kyo/AbortTest.scala

@@ -267,7 +267,7 @@ class AbortsTest extends Test:
                "converts matching Panic to Fail" in {
                    val ex     = new RuntimeException("Test exception")
                    val result = Abort.run[RuntimeException](Abort.panic(ex)).eval
-                    assert(result == Result.panic(ex))
+                    assert(result == Result.Failure(ex))


I think we should avoid over-catching exceptions the user does not wrap. Obviously the machinery for effect handling may need to catch this for enriched traces, but I doubt we should be putting that into the 'expected' error channel.

hearnadam · 2025-02-01T23:29:20Z

kyo-data/shared/src/main/scala/kyo/Result.scala

+              * @return
+              *   An Either with Left containing the error or exception, and Right containing the successful value
+              */
+            def toEitherPartial: Either[E, A] =


Do the methods need to have distinct names?

Yes, because Partial is a subtype of Result so the methods conflict otherwise. fold ought to work without renaming it since the signature is different, but I couldn't get it to work.

hearnadam · 2025-02-01T23:29:51Z

kyo-data/shared/src/main/scala/kyo/Result.scala

+      * @tparam A
+      *   The type of the successful value
+      */
+    opaque type Partial[+E, +A] >: Success[A] | Failure[E] <: Result[E, A] = Success[A] | Failure[E]


My only real hesitation is that a Partial result sounds almost the opposite of what this is. This is the Expected result, but I don't think that's great either...

Partial made sense to me given that it partially represents a Result but I agree it doesn't communicate much meaning. I can't think of good suggestions. Maybe something like NoPanic or WithoutPanic?

hearnadam · 2025-02-01T23:30:48Z

kyo-prelude/shared/src/main/scala/kyo/Abort.scala

+            ct: SafeClassTag[E],
+            reduce: Reducible[Abort[ER]]
+        ): Result[E, A] < (S & reduce.SReduced) =
+            runWith[E](v)(identity)
        end apply
    end RunOps

    /** Runs an Abort effect. This operation handles the Abort effect, converting it into a Result type.
      */
    inline def run[E]: RunOps[E] = RunOps(())


I'm tempted to suggest that run should return Partial by default. If we don't expect users to manipulate panics that is...

run & runAll?

If that's the case, then really what is now called Partial should be called Result, and what is now called Result should be Result.OrPanic or something like that. (That would be my preference, FWIW, given how I tend to use Abort.)

What would be the behavior of Result.flatMap in case there's a failure during transformation? It seems the only option is throwing if panics aren't represented

Yeah good point. Presumably we'd have a flatMapAll for Result.OrPanic and restrict flatMap to the narrower Result. This is all on the premise that users generally won't be dealing with the Panic case.

hearnadam · 2025-02-01T23:32:25Z

kyo-data/shared/src/main/scala/kyo/Result.scala

@@ -269,6 +271,13 @@ object Result:
                throw exception
    end Panic

+    /** Provides extension methods for Result type */
+    extension [A](self: Success[A])
+        def successValue: A = self match


is value taken?

Yes, it's an extension on Result that returns Maybe[A].

hearnadam · 2025-02-01T23:33:25Z

kyo-data/shared/src/main/scala/kyo/Result.scala

          *   Function to apply if the Result is a Success
          * @return
          *   The result of applying the appropriate function
          */
-        inline def foldError[B](ifError: Error[E] => B)(inline ifSuccess: A => B): B =
+        inline def foldError[B](inline onSuccess: A => B, ifError: Error[E] => B): B =


Perhaps I missed a comment, but doesn't this make the inference worse?

This change was made to make all the fold... methods more consistent. fold is not curried now and begins with onSuccess so that it can be overloaded to include or exclude onPanic. I haven't noticed any issues with type inference in the tests at least, but maybe I'm missing something?

fwbrasil · 2025-02-03T20:34:50Z

@johnhungerford @hearnadam are we ok to merge this?

johnhungerford added 4 commits January 20, 2025 20:44

Added Result.Partial type alias, Abort.runPartial, Abort.fold, and co…

22f7e6f

…mbinators

Fixed tests

3f071b1

Merged main

647a2b0

Added note about runPartial and fold to README

60a89df

Updated readme

e769aea

johnhungerford commented Jan 21, 2025

View reviewed changes

kyo-prelude/shared/src/main/scala/kyo/Abort.scala Outdated Show resolved Hide resolved

fwbrasil reviewed Jan 21, 2025

View reviewed changes

kyo-data/shared/src/main/scala/kyo/Result.scala Outdated Show resolved Hide resolved

johnhungerford added 2 commits January 21, 2025 15:54

Using opaque type for Result.Partial

50b6e3f

Removed Partial#toResult since its a subtype

a96d948

johnhungerford marked this pull request as draft January 21, 2025 20:58

johnhungerford added 2 commits January 21, 2025 16:11

Improved CanEqual givens

9073dd5

Reverted CanEqual nonsense

1611e65

Added Result.Partial tests

3e88f30

johnhungerford marked this pull request as ready for review January 22, 2025 03:06

Fixed README

5799aaf

fwbrasil reviewed Jan 22, 2025

View reviewed changes

kyo-prelude/shared/src/main/scala/kyo/Abort.scala Outdated Show resolved Hide resolved

fwbrasil reviewed Jan 22, 2025

View reviewed changes

kyo-prelude/shared/src/main/scala/kyo/Abort.scala Show resolved Hide resolved

fwbrasil reviewed Jan 22, 2025

View reviewed changes

kyo-data/shared/src/main/scala/kyo/Result.scala Show resolved Hide resolved

johnhungerford added 3 commits January 28, 2025 11:45

Using runWith for run, runPartial, and runFold

893a314

Update Result fold API to match abort

3b2a1e6

Merge remote-tracking branch 'upstream/main' into error-handle-withou…

35dc485

…t-panic

fwbrasil reviewed Jan 28, 2025

View reviewed changes

kyo-prelude/shared/src/main/scala/kyo/Abort.scala Outdated Show resolved Hide resolved

johnhungerford added 4 commits January 28, 2025 14:09

Updated Result

fc1cda6

minor fix

f030dcb

Starting to fix tests

d2cbd12

runwith private

0bc9d42

johnhungerford commented Jan 28, 2025

View reviewed changes

johnhungerford and others added 16 commits January 28, 2025 16:03

Updated abort combinators

8e489e2

Fixed combinators tests

7c595a9

Fixed some tests

b13da54

Updated Result scaladoc

0371e98

Fixed cats test

8ea2c37

Fixed retry test

fe68cf1

Reverted test changes

f9f1cbc

Reverted more tests

24ec280

Reverted and cleaned up

1baab4d

Fixed sttp JS

ee0e788

Merge branch 'main' into error-handle-without-panic

65ba948

Merge remote-tracking branch 'upstream/main' into error-handle-withou…

370328e

…t-panic

Added ...OrThrow methods to Abort combinators

224f397

Merge branch 'error-handle-without-panic' of github.com:johnhungerfor…

fdc8641

…d/kyo into error-handle-without-panic

Updated README

6b569a2

Fixed lingering foldError

356c18a

fwbrasil reviewed Feb 1, 2025

View reviewed changes

kyo-prelude/shared/src/main/scala/kyo/Abort.scala Outdated Show resolved Hide resolved

Made RunWithOps private

ce0121b

fwbrasil approved these changes Feb 1, 2025

View reviewed changes

hearnadam reviewed Feb 1, 2025

View reviewed changes

Minor change to docstring

787cd7c

hearnadam merged commit 4c922cc into getkyo:main Feb 3, 2025
3 checks passed

johnhungerford mentioned this pull request Feb 3, 2025

Error handling without panic #1014

Closed

	/** Converts any Aborts to Panic, wrapping non-Throwable Failures in PanicException
	/** Converts all Aborts to Panic, wrapping non-Throwable Failures in PanicException

	case Result.Failure(other) => Abort.panic(PanicException(other)).asInstanceOf[Nothing < Any]
	case Result.Failure(other) => Abort.panic(PanicException(other))

Error handle without panic #1042

Error handle without panic #1042

Conversation

johnhungerford commented Jan 21, 2025 • edited Loading

johnhungerford commented Jan 21, 2025

johnhungerford commented Jan 21, 2025

fwbrasil commented Jan 21, 2025 • 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

fwbrasil Feb 2, 2025 • 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

johnhungerford Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwbrasil left a comment

Choose a reason for hiding this comment

fwbrasil commented Feb 1, 2025

hearnadam left a 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

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

fwbrasil commented Feb 3, 2025

johnhungerford commented Jan 21, 2025 •

edited

Loading

fwbrasil commented Jan 21, 2025 •

edited

Loading

fwbrasil Feb 2, 2025 •

edited

Loading

johnhungerford Jan 31, 2025 •

edited

Loading