https://kotlinlang.org logo
Join Slack
Powered by
I forgot if this is still an issue, but wasn't the...
# arrow-contributors
y
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?
s
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:
Copy code
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) }
y
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
s
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.
y
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...
s
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.
y
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.
s
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
y
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
s
Well, it's not a 100% fix but you'd have to write some pretty obscure code 😄
y
Yes true, because you could do:
Copy code
fun myStupidFun(raise: Raise<Blah>) = with(raise) { ... }
s
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 😜
Worse,
Copy code
fun myStupidFun(raise: Raise<Blah>) =
  runCatching { raise.raise(blah) }
y
Oh I misread sorry since you had mentioned contexts so my mind went to 
context(Raise) fun runCatching
s
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
y
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
s
Yes, 100% agree. It's extremely unlikely, but things sometimes get crazy in the real world 😂
34 Views
Open in Slack
PreviousNext
Powered by