Conversation
Resolution ambiguity in higher order functionsI pulled in If you have a function that takes a block with a raise context, then you call that function in an existing builder like context(_: Raise<E>)
inline fun <E, T> withLoggingRaise(
logger: Logger,
context: String,
operation: context(Raise<E>) () -> T,
) = either {
logger.operationStart(context)
operation()
}.fold(
ifLeft = {
logger.operationFailure(context, it.toString())
raise(it)
},
ifRight = {
logger.operationSuccess(context)
it
},
)
val x = either {
withLoggingRaise(logger, "operation") {
// If you select the non context param version of this for autocomplete and/or
// dont explicitly import it separately, this will be the `Raise<E>.raise()
// variant using `this` from either block instead of context(Raise<E>) from withLoggingRaise
// which will prevent the things in fold from being called
raise("error")
// Doing this or making sure to add the correct import will raise from the `operation` block in withLoggingRaise
com.example.mrf.raise("error")
}
}This wont be an issue if/when context params are fully released and arrow can deprecate/remove the receiver param variant, but I'm not sure what a good solution is in the (probably lengthy) interim UPDATE: UPDATE 2: |
|
It turns out that to prevent ambiguity, it's better to keep a completely separate contextual-first set of functions instead of mixing receivers and context parameters. This is done in the new @mrf7 this is an issue we're working on in the Kotlin team, we're working on new reporting to ensure resolution works as expected. |
Thanks for the reply and for working on this both in arrow and internally at JB. I think a compiler warning to let you know when what you might expect is being called is actually being shadowed is a great solution |
|
|
||
| context(raise: Raise<Error>) @RaiseDSL public inline fun <Error, OtherError, A> withError( | ||
| transform: (OtherError) -> Error, | ||
| @BuilderInference block: Raise<OtherError>.() -> A |
There was a problem hiding this comment.
Should these blocks be context(Raise<OtherError> instead like in the added builders functions to reduce resolution ambiguity?
There was a problem hiding this comment.
Yes, thanks for the catch, everything should be a context parameter.
| } | ||
|
|
||
| @Suppress("NOTHING_TO_INLINE") | ||
| public inline operator fun <A> Value<A>.getValue(thisRef: Nothing?, property: KProperty<*>): A = value |
There was a problem hiding this comment.
This may need to be moved to allow access when RaiseAccumulate isn't available as a dispatch receiver.
There was a problem hiding this comment.
Unfortunately getValue is not supported with context parameters (yet).
There was a problem hiding this comment.
However, I found a way to turn it into a member of Value<A> itself, so we can keep the API.
There was a problem hiding this comment.
It doesn't actually need the dispatch/context, so it can just exist at the top-level
Edit: or yeah it can just be in Value lol!
|
This PR slightly touches |
|
|
||
| public typealias Raise<A> = arrow.core.raise.Raise<A> | ||
| public typealias SingletonRaise<A> = arrow.core.raise.SingletonRaise<A> | ||
| public typealias ResultRaise = arrow.core.raise.ResultRaise |
There was a problem hiding this comment.
Similarly, this should just be Raise<Throwable>
| return with(raise) { this@bind.bind() } | ||
| } | ||
|
|
||
| context(raise: ResultRaise) @RaiseDSL public fun <A> Result<A>.bind(): A { |
There was a problem hiding this comment.
Either use Raise<Throwable> or update the typealias as above
|
I don't want to hijack your PR @serras, but I also really care about this feature. Would you mind if I made some reasonable changes directly to the branch? Especially with |
|
I am fine with comparing approaches. However, I don’t think that removing |
|
Oh fine yeah. If one-to-one copy is the goal, then you've done it! |
|
Just note that I'd like to relook at this and finalise our API for contexts before publishing a version and committing to binary and source compatibility (that's my main concern really. I didn't want a situation where people may have broken builds because we regret the exact signature of the API or something. @serras if you're happy with the PR as-is then we should merge now. Alternativey we can wait for Simon to have a look. Up to you! |
4 participants
This PR supersedes #3606. The difference is that the contextual functions are now directly in
arrow-core, instead of creating a new module. This seems to be safe, since context parameters do not poison the binary.