-
Notifications
You must be signed in to change notification settings - Fork 454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix bind logic for Option to raise the transformed value. #2948
Conversation
Current version just returns the transform, however None is the error situation and should trigger a raise. This changes the API.
@@ -281,9 +281,9 @@ public interface Raise<in R> { | |||
* <!--- TEST lines.isEmpty() --> | |||
*/ | |||
@RaiseDSL | |||
public fun <A> Option<A>.bind(transform: Raise<R>.(None) -> A): A = | |||
public fun <A> Option<A>.bind(transform: Raise<R>.() -> R): A = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @freynder,
We spoke and Slack and I am afraid I was too fast to respond 🙈 My apologies.
Looking at this signature, with this implementation I am actually wondering if it makes sense at all. So first of all thank you to raise this PR, and thoughts on the signature 🙏
I am going to rubber duck my thoughts, and ask some feedback from some fellow contributors at the same time.
The lambda Raise<R>.() -> A
allows to bind, or unwrap, Option
and in the case None
is encountered it will invoke the lambda. The lambda allows to raise a value of R
into the outer interface Raise<R>
, or fallback to a value of A.
- The
Raise<R>
lambda receiver is redundant because we're already inside the context ofinterface Raise<R>
, so even without the lambda receiver we can callraise(r)
. - Without
Raise<R>
, we have() -> A
which matches the already existingOption
API ofgetOrElse
. - Since this function is defined inside of an
interface
, it cannot beinline
so it's inferior to the already existinggetOrElse
which is inline and thus allows forsuspend
to pass through.
So this API can be completely replaced by getOrElse
and simplifies the examples of the KDoc above.
suspend fun test() {
val empty: Option<Int> = None
either {
- val x: Int = empty.bind { _: None -> 1 }
+ val x: Int = empty.getOrElse { 1 }
- val y: Int = empty.bind { _: None -> raise("Something bad happened: Boom!") }
+ val y: Int = empty.getOrElse { raise("Something bad happened: Boom!") }
- val z: Int = empty.recover { _: None ->
+ val z: Int = empty.getOrElse {
delay(10)
1
- }.bind { raise("Something bad happened: Boom!") }
+ }
x + y + z
} shouldBe Either.Left("Something bad happened: Boom!")
}
As the example shows we can now call suspend fun delay
directly into the getOrElse
lambda, and we can also still call raise
& co from inside the getOrElse
lambda.
The same applies for Result#bind
. Since none of this code has yet been released, we can still remove these APIs in favour of the simpler inline
methods on the original APIs. Fixing this issue also requires some other small changes in Builders.kt.
If you're interested @freynder I'd be happy to guide you through all the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @nomisRev,
No problem, I hadn't looked at this from an API perspective, just wanted to make it consistent with the Either.bind() and others, they also do raise on error.
I fully agree with your assessment though, it would make the API much simpler. My only concern is that the bind() functions provided a consistent way for all monads to raise (and thus short-circuit) the program. getOrElse makes the raise call optional. I don't have enough knowledge currently to determine if that's important though.
Thank you for your great feedback, very much appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Looking further at the code, I also noticed the same reasoning related to your point "1." can be applied to Raise.catch:
@RaiseDSL
public inline fun <R, A> Raise<R>.catch(
@BuilderInference action: Raise<R>.() -> A,
@BuilderInference catch: Raise<R>.(Throwable) -> A,
): A {
contract {
callsInPlace(action, EXACTLY_ONCE)
callsInPlace(catch, AT_MOST_ONCE)
}
return fold({ action(this) }, { this@catch.catch(it) }, { this@catch.raise(it) }, { it })
}
Here the catch function receiver also seems redundant as it does not need the Raise receiver since it can access it from the this@catch Raise scope.
A counter argument to this may be that the caller may want to use a predefined lambda variable as a handler for the catch function with receiver to receive the context. Not sure how common that would be though, personally I would prefer to be explicit.
Probably also same remark for recover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @freynder,
This is great feedback and I was actually also looking at this. There is multiple benefits to this actually, and we should review the current signatures in arrow.core.raise.*
to see how we can simplify them.
Looking for example at recover
:
@RaiseDSL
- public inline fun <R, E, A> Raise<R>.recover(
+ public inline fun <E, A> recover(
@BuilderInference action: Raise<E>.() -> A,
- @BuilderInference recover: Raise<R>.(E) -> A,
+ recover: (E) -> A,
): A {
contract {
callsInPlace(action, EXACTLY_ONCE)
callsInPlace(recover, AT_MOST_ONCE)
}
return fold<E, A, A>({ action(this) }, { throw it }, { recover(it) }, { it })
}
Besides still supporting the current use-cases it also makes this function more general purpose, because you can now use it to execute Raise<E>
based programs and resolve the error completely without having to stick to Raise<E>
.
So thanks again for raising this issue, and PR. I think it will result in another significant improvement, and simplification of this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nomisRev,
Wow, that indeed removes the Raise<R>
receiver completely. Loving how elegant this is implemented with the fold function actually creating an isolated raise context and using the general callbacks to transform / act on the possible outcomes.
Looking at it from the above perspective, recover and catch are basically predefined behaviors for fold. That makes the mental model much easier for me.
- fold: run action, general callbacks for exception, recover, transformation
- fold: run action, rethrow exception, general callbacks for recover, transformation
- recover: run action, rethrow exception, recover from raise, identity transformation
- catch: run action, catch exception, re-raise raise, identity transformation
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @freynder,
That is absolutely correct. I might steal that phrasing, and those bullet points for the new documentation 😁
I am going to close this PR, since it's preceded by #2954.
Thank you again for triggering this simplification, and discussion! Glad it helped you get a better mental model of this encoding as well.
Current version just returns the transform, however None is the error situation and should trigger a raise. This changes the API.