catch for EffectScope

[serras]

This PR adds a small utility method for "simulating" catch as we have for exceptions. If this is useful, a bunch of other utility methods can be implemented on top of it, but first I wanted to know whether you like this catch.

[nomisRev]

Needs to be an extension due to variance?

This duplicates behaviour with handleErrorXXX but it exposes a nicer lambda for handler where you can leverage the DSL again by exposing EffectScope<E> on the receiver of the lambda.

I am absolutely up for improving these APIs, and renaming them but we should probably avoid duplicated APIs as much as possible to keep the API surface reasonable.

That being said this signatures reminds me a lot of Flow#catch, and it uses the same naming so I am in favour of catch over the current handleErrorXXX & redeemXXX methods. This seems to cover all 4 APIs with a single nice API.

[serras]

For me the question is how much of the API should be exposed via EffectScope<E> as opposed to Effect. Personally, I'd prefer to stay within the realm of EffectScope as much as I could, and only refer to Effect to "run" the action at the very top level. But I also see this is very much a design choice, so I'll be happy with whatever decision you take.

[serras]

About not being an extension, I just wrote it directly like that, although it could also be part of EffectScope itself. In fact, this opens the decades-old debate: it catch part of the "error effect", or is it a "effect transformer" (my definition of catch says that the latter).

[i-walker]

This also an alternative, using the UnsafeVariance, which is fine here.

// in EffectScope interface

public suspend fun <B> Effect<R, B>.catch(recover: suspend EffectScope<R>.(@UnsafeVariance R) -> B): B =
    fold({ recover(it) }, ::identity)

we can also have the Effect be a parameter, if this is a better encoding

[nomisRev]

If we speak in terms of catch then I think we also revisit renaming shift to raiseError or similar. WDYT?

[serras]

If we speak in terms of catch then I think we also revisit renaming shift to raiseError or similar. WDYT?

Or we can go with reset instead of catch if you prefer the delimited continuations terminology. In my perfect world we would keep both: EffectScope/shift/reset speak of the general concept, wheread ErrorScope/throw/catch speak of the most important use case.

[i-walker]

If we speak in terms of catch then I think we also revisit renaming shift to raiseError or similar. WDYT?

I am also in favour of remaining with one naming convention - here in favour of shift, instead of multiple ones. Though reset, might not fit within the worlds of the Kotlin Stdlib.

[nomisRev]

Or we can go with reset instead of catch if you prefer the delimited continuations terminology. In my perfect world we would keep both: EffectScope/shift/reset speak of the general concept

@serras I thought reset had a different meaning in delimited continuations 🤔 Since from a reset block you can typically shift multiple values, no? I am not sure we have to stick to the delimited continuations terminology perse, from, from a Kotlin user point of view I don't think it brings any benefit.

Personally, I'd prefer to stay within the realm of EffectScope as much as I could, and only refer to Effect to "run" the action at the very top level.

While I personally agree with this, I don't expect this to become the most popular usage in Kotlin/Arrow. That heavily depends though on how we want to model Arrow towards 2.0. Which I think with all these new changes is on the horizon. For example, a really cool usage which I think would be popular is the following.

suspend fun fetchUser(id: UUID): Either<DbError, User> = ...

// This pattern covers handleErrorWith, handleError, redeem & redeemWith
either<ServiceError, ServiceUser> {
  catch({ fetchUser(uuid) }) { dbError ->
     shift(dbError.toServiceError())
  }
}

This is also another example for me that people will not feel that shift is an idiomatic name in Kotlin.
And that the signature that your propose absolutely makes sense!

It's also 100% in line with what KotlinX Coroutines already exposes in its datatypes (Flow).
It would drastically reduce API surface, which decreases learning curve etc.
1 pattern/api for all effect-style code / data types.

[serras]

One more design question, after looking at Flow's catch: to completely mirror that API we should provide a catch that can be used as:

{ fetchUser(uuid) }.catch { dbError -> shift(dbError.toServiceError()) }

However, I don't know how to write such function (since there's already a receiver), nor whether that would be good for our use case. Another possibility is to mark the function infix, so we can write:

{ fetchUser(uuid) } catch { dbError -> shift(dbError.toServiceError()) }

but maybe then we get a conflict due to a keyword being used.

[serras]

@serras I thought reset had a different meaning in delimited continuations 🤔 Since from a reset block you can typically shift multiple values, no? I am not sure we have to stick to the delimited continuations terminology perse, from, from a Kotlin user point of view I don't think it brings any benefit.

My understanding was that reset was a way to mark the "scope" of a shift, in the sense that a call to shift would bubble up to the nearest reset. In that sense, this works a bit like this, since we force the action to run and capture everything bubbling up. But I agree that it gives no benefit for Kotlin users.

[nomisRev]

One more design question, after looking at Flow's catch: to completely mirror that API we should provide a catch that can be used as:

{ fetchUser(uuid) }.catch { dbError -> shift(dbError.toServiceError()) }

I'm personally not a big fun of these APIs, since they don't play well with inference and they most of the times don't show up in auto-completion. Maybe @BuilderInference solves the inference issue, but not sure.
In that regard I would say that Effect<E, A>.catch(...) would be a better signature, but I prefer your original signature and I think it makes perfect sense in our DSL style API.

KotlinX Coroutines in also takes similar approach of DSL-like blocks for suspend () -> A signatures. Even more so when they can be written using inline fun since then the DSL works for both non-suspend and suspending code. Such as withContext, timeoutOrNull.

[pakoito]

NIT: Recover first, operation last.

[nomisRev]

Also for a DSL like this? I know it's the convention for bimap, fold etc but to me it feels counter-intuitive here since this models an operation like try { } catch { } but for typed errors.

[serras]

Looking at things like Flow's catch, I think that the convention is action, then recovery.

[i-walker]

Thanks @serras, going through our discussion I prefer the original encoding too. And it gives us room in case we come up with improvements.

I corrected an earlier review above

[nomisRev]

About not being an extension, I just wrote it directly like that, although it could also be part of EffectScope itself. In fact, this opens the decades-old debate: it catch part of the "error effect", or is it a "effect transformer" (my definition of catch says that the latter).

Sorry @serras I don't want to drag this out 😅 These APIs are paving the way to new great stuff in Arrow so I think they deserve a little bit of extra attention (and nit picky 🙈)

Currently as it stands it requires import arrow.core.continuations.catch, and it'll be listed last when suggesting APIs in IntelliJ IIRC since members get preference and don't require an import in Kotlin.

So IMO the ergonomics are better as a member of EffectScope, but since R is defined as out it would require @UnsafeVariance as @i-walker suggested. And I think that hinders other ergonomics. I.e. that you cannot "upcast"? (right? 🤔)

[serras]

@nomisRev and I have been working a bit on the ergonomics, and came to a two-step blocks,

attempt { /* something */ } catch { e -> /* handle e */ }

We found no other (easy) way for the compiler to infer the types of every block, a problem that surfaced once I started adding tests.

[serras]

@nomisRev @i-walker any further comments?

[i-walker]

Using attempt might introduce confusion since, we're using Effect#attempt for instance here and EagerEffect#attempt to run continuations. Is there another descriptor name we can use

[serras]

The only other name appart from attempt and try that I can think of is strive, but I'm not sure it sounds any good...

[nomisRev]

Thanks for this great new API @serras 👏 🥳
This is another huge improvement towards Arrow 2.0 🎉

[nomisRev]

Should this not be recover: suspend EffectScope<R>.(E) -> A? My eye only fell on this just now.

[serras]

No, the handler executes in the same context available to the outer thing (which is called R here). The idea is that you “close over” errors of type E, and may turn them into errors of type R, if that makes sense.

[nomisRev]

Yes, makes sense but not sure I understand why suspend is not allowed in recover 🤔

[nomisRev]

Doesn’t this prevent you from calling shift or bind?

[serras]

Oh, I forgot it when I moved it from being an extension function! I’ll push an update tonight.

[nomisRev]

Awesome, feel free to merge after! Great contribution, excited to see this getting merged 🥳

[nomisRev]

Can we add these tests for EagerEffect too? I think we’ll run into the issue that shift cannot be called from the EagerEffect catch method, but I’m not 100% sure.

[serras]

Done!

[i-walker]

Looks like we're able to use shift since attempt is within a suspend EagerEffectScope.() -> A ☺️

[nomisRev]

Thank you @serras! I was wrong 😁

[serras]

@i-walker after our meeting today I'm not sure whether I should merge this or not. If you say yes, I'll do it :)

[i-walker]

No, go ahead this is fine to be merged :) @serras