I forgot if this is still an issue, but wasn't the...

# arrow-contributors

[Youssef Shoaib [MOD]]

I forgot if this is still an issue, but wasn't there problems with runCatching swallowing `RaiseCancellationException`s and hence not playing nicely with Raise? Should Arrow maybe offer an alternative runNonFatalCatching or a Raise.runCatching shadowed function that rethrows cancellation exceptions?

[simon.vergauwen]

Hey @Youssef Shoaib [MOD],

Yes, since runCatching swallows CancellationExeption it also swallows RaiseCancellationException 😞 This is inherit to Result since it's meant for the intrinsics of Kotlin Coroutines.

There is a top-level catch function, and Either.catch that allow avoiding this issue. We could also add something for Result 🤔 Currently:

val x: Either<Throwable, Int> = Either.catch { 1 }

val y: Either<PSQLException, Int> = Either.catchOrThrow { 1 }

val i: Int = catch({ 1 }) { _: Throwable -> fallback() }

fun Raise<E>.example(): Int =
  catch({ 1 }) { _: Throwable -> raise(e) }

fun Raise<E>.example(): Int =
    catch({ 1 }) { _: PSQLException -> raise(e) }

[Youssef Shoaib [MOD]]

I was thinking maybe a fun Raise<E>.runCatching(block: () -> R): Result<R> so that if someone takes pre-existing code that uses runCatching and then wraps some of it in effect the code would still work. Maybe even with a @Deprecated that tells them to use Either instead of Result

[simon.vergauwen]

Hmm, we've considered leveraging @Deprecated with level = Deprecation.ERROR to prevent incorrect usages 🤔 https://github.com/arrow-kt/arrow/issues/2838

I'm still awaiting FIR to be-able to do add IDEA inspections through a compiler plugin, but that seems quite far away. Building a IDEA plugin seems a bad workaround.

[Youssef Shoaib [MOD]]

Yes I'm thinking maybe a Deprecation.ERROR runCatching function that tells the person to use either. Shall I make a PR for that, and perhaps even for bind errors? Only issue is, I'm not sure how we'd write tests to test that those functions are doing their job correctly...

[simon.vergauwen]

Thinking about this a bit more. We can also just declare runCatching inside Raise that doesn't catch the RaiseCancellationException but still captures the more general CancellationException. Or even not catch it at all.

It's possible since it'll prefer the version declared on the receivers, not sure how it would behave with context receivers though.. They fix the problem of scope pollution, but not sure how it works with a function defined on a context vs top-level. I guess it would still prefer the context variant.

[Youssef Shoaib [MOD]]

I've tested the top-level Vs context resolution before, and the context version always gets preferred. It's not considered the best of practices according to the KEEP, but in this case only functions that take a Raise as some form of receiver (extension, dispatch, or context) would ever be able to throw a RaiseCancellationException, and hence every erroneous instance of runCatching { somethingThatUsesRaise() } will always have a Raise receiver in scope, and any other runCatching invocation outside of a Raise receiver would never throw RaiseCancellationException anyways.

[simon.vergauwen]

in this case only functions that take a Raise as some form of receiver (extension, dispatch, or context) would ever be able to throw a RaiseCancellationException, and hence every erroneous instance of runCatching { somethingThatUsesRaise() } will always have a Raise receiver in scope, and any other runCatching invocation outside of a Raise receiver would never throw RaiseCancellationException anyways.

Yes, that was exactly my train of thought

[Youssef Shoaib [MOD]]

Only catch is that declaring it as a `context(Raise<E>) fun runCatching`means that it'd have to be imported, while if it was a member of the Raise interface, then it doesn't need to be imported

[simon.vergauwen]

Well, it's not a 100% fix but you'd have to write some pretty obscure code 😄

[Youssef Shoaib [MOD]]

Yes true, because you could do:

fun myStupidFun(raise: Raise<Blah>) = with(raise) { ... }

[simon.vergauwen]

while if it was a member of the Raise interface, then it doesn't need to be imported

I meant as a member on Raise, I thought we already came to the conclusion that was a requirement 😜

[simon.vergauwen]

Worse,

fun myStupidFun(raise: Raise<Blah>) =
  runCatching { raise.raise(blah) }

[Youssef Shoaib [MOD]]

Oh I misread sorry since you had mentioned contexts so my mind went to context(Raise) fun runCatching

[simon.vergauwen]

Ideally, I think we would have a gradle plugin for Arrow that builds these kind of checks in FIR with IDEA support, highlighting, quick-fixes, etc

[Youssef Shoaib [MOD]]

Yes. I think that that's a very unlikely misuse though, and especially when context receivers come out, I wouldn't imagine any reason to ever pass Raise as a parameter

[simon.vergauwen]

Yes, 100% agree. It's extremely unlikely, but things sometimes get crazy in the real world 😂