Discuss: Use coroutines instead of exceptions for binding about kotlin-result

[michaelbull]

Thanks for opening this discussion. This is very interesting and to be honest I agree with a lot of your points, e.g. avoiding the use of exceptions.

With regards to the Suspension functions can be called only within coroutine body, could you paste an example of code you are talking about that fails to work with our current solution but would be solved with your proposal? I'm struggling to wrap my head around the description provided.

On the topic of the exception stacktrace, the CancellationException is an internal implementation detail of kotlin coroutines that indicates a coroutine was successfully cancelled in a safe manner, it's used a lot by the library itself so I don't think it being expensive would be problematic if they are using it so frequently internally.

Looking at the implementations in your PR is also confusing me, the distinction between the coroutines vs kotlinx.coroutines is not clear to me. The main difference seems to be that one uses withContext, and nothing else. It is also unclear why one of the scopes needs annotating with @RestrictsSuspension and one doesn't - would this change what code consumers can write within the scope?

Looking at the two beneath they both seem to have the same constraints on their bind function so I also don't understand why one is named "Suspendable" and one isn't. What does the "Suspendable" refer to here if the bind is marked as suspend fun in both instances?

public sealed interface ResultSuspendableBindingScope<E> {
    public suspend fun <V> Result<V, E>.bind(): V
}

@RestrictsSuspension
public sealed interface ResultBindingScope<E> {
    public suspend fun <V> Result<V, E>.bind(): V
}

Another small thing is that I'm pretty sure the code beneath would be incorrect - we would have to catch the potential exception and wrap it in an Err. Maybe I'm wrong here though.

    override fun resumeWith(result: kotlin.Result<V>) {
        finalResult = Ok(result.getOrThrow())
    }

---

Overall the 'pure coroutines' implementation looks cleanest to me, but I don't know what the tradeoff is. I'd be very keen to understand which one you think is better and why, which one improves the developer experience for consumers, and what the tradeoffs between all three of the solutions (existing and the two proposed ones) are.

[dronda-t]

With regards to the Suspension functions can be called only within coroutine body, could you paste an example of code you are talking about that fails to work with our current solution but would be solved with your proposal? I'm struggling to wrap my head around the description provided.

Apologies for the confusion, this is a limitation of using coroutines. The current exception-based solution handles this case, but if using coroutines it would not allow you to bind unless you are within a suspend function.

On the topic of the exception stacktrace, the CancellationException is an internal implementation detail of kotlin coroutines that indicates a coroutine was successfully cancelled in a safe manner, it's used a lot by the library itself so I don't think it being expensive would be problematic if they are using it so frequently internally.

Here I was not referring to CancellationExceptions. I was referring to the fact that if you throw an exception within a coroutine and view the stacktrace, you get extra lines which indicate where the continuation code happened. Not a big deal, but could be slightly more confusion for people trying to analyze the stack trace.

Looking at the implementations in your PR is also confusing me, the distinction between the coroutines vs kotlinx.coroutines is not clear to me. The main difference seems to be that one uses withContext, and nothing else. It is also unclear why one of the scopes needs annotating with @RestrictsSuspension and one doesn't - would this change what code consumers can write within the scope?

The distinction is similar to that of a sequence builder and a flow builder. sequence { } flow { }. In a sequence you cannot call suspend functions that weren't defined to be used by the sequencebuilder.

For example the following is does not compile:

suspend fun test() = withContext(Dispatchers.IO) {
    println("Hello")
}

sequence {
     test()
     yield("Hey")
}

However, in a flow it will compile:

suspend fun test() = withContext(Dispatchers.IO) {
    println("Hello")
}

flow {
     test()
     emit("Hey")
}

This is because flows can handle both coroutine contexts for kotlinx.coroutines and coroutine context from the flow builder.

The same is true between the pure coroutines binding and the kotlinx coroutines binding.

Looking at the two beneath they both seem to have the same constraints on their bind function so I also don't understand why one is named "Suspendable" and one isn't. What does the "Suspendable" refer to here if the bind is marked as suspend fun in both instances?

You're right, the naming here wasn't well thought through as they both "suspend". Perhaps ResultKotlinxCoroutineBindingScope would've been a better name.

Another small thing is that I'm pretty sure the code beneath would be incorrect - we would have to catch the potential exception and wrap it in an Err. Maybe I'm wrong here though.

Why do you want to catch the exception? We are not doing a runCatching here. If someone throws inside a binding I would expect that to get thrown up the stack, not returned as an Error. If you caught, then you would have to somehow join a throwable and the custom Error type together, which would mean we would have to return an Error type of Any.

---

Re: restrictions on where you can call bind, I learned recently that you cannot "bind" in rust when you're in a separate closure. For example `map` in an iterator. That would be closer to how this POC implementation works compared to the current one.

---

I want to make it clear that I'm not confident that this is the right solution for this library. Overall, I think that using pure coroutines feels more correct to me than using an exception, but there's still some research to be done around performance. Most of my points around performance are inferences, but I don't really have any data to prove that this solution is any better from that perspective. Plus the downsides of not being able to call this from a non-suspending function would be pretty disruptive to users of this library.

Feel free to close this issue if you want, I think it could be an interesting solution, but I think there's still more research to be done and I don't really have the time to pursue it at the moment.