https://kotlinlang.org logo
#arrow-contributors
Title
# arrow-contributors
y

Youssef Shoaib [MOD]

03/22/2023, 7:38 PM
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

simon.vergauwen

03/23/2023, 8:24 AM
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

Youssef Shoaib [MOD]

03/23/2023, 2:03 PM
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

simon.vergauwen

03/26/2023, 5:59 PM
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

Youssef Shoaib [MOD]

03/26/2023, 9:32 PM
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

simon.vergauwen

04/02/2023, 5:19 PM
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

Youssef Shoaib [MOD]

04/02/2023, 5:25 PM
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

simon.vergauwen

04/02/2023, 5:26 PM
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

Youssef Shoaib [MOD]

04/02/2023, 5:26 PM
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

simon.vergauwen

04/02/2023, 5:27 PM
Well, it's not a 100% fix but you'd have to write some pretty obscure code 😄
y

Youssef Shoaib [MOD]

04/02/2023, 5:27 PM
Yes true, because you could do:
Copy code
fun myStupidFun(raise: Raise<Blah>) = with(raise) { ... }
s

simon.vergauwen

04/02/2023, 5:27 PM
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

Youssef Shoaib [MOD]

04/02/2023, 5:28 PM
Oh I misread sorry since you had mentioned contexts so my mind went to
context(Raise) fun runCatching
s

simon.vergauwen

04/02/2023, 5:29 PM
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

Youssef Shoaib [MOD]

04/02/2023, 5:29 PM
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

simon.vergauwen

04/02/2023, 5:30 PM
Yes, 100% agree. It's extremely unlikely, but things sometimes get crazy in the real world 😂
26 Views