Skip to content
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

Raise DSL does not recover when using an interface with Raise<E> #3126

Closed
cldfzn opened this issue Sep 6, 2023 · 8 comments
Closed

Raise DSL does not recover when using an interface with Raise<E> #3126

cldfzn opened this issue Sep 6, 2023 · 8 comments

Comments

@cldfzn
Copy link

cldfzn commented Sep 6, 2023

I have a code base that is using interfaces as contexts (waiting for proper support from tooling, etc.. before using context receivers). When attempting to enhance a trait to run with the Raise DSL, recovery no longer works as expected.

suspend fun <ENV> ENV.create1(workspace: Workspace, model: Model): Model where
ENV : HasRepository,
ENV : HasIndex,
ENV : Raise<DomainError> {
    val created = withRepository(Repository.context) { create()(workspace, model) }
    recover({ withIndexFails(Index.context) { update(workspace, created) } }) { _: DomainError -> } // DomainError propagates from here when using the withIndexFails function
    return created
}

suspend fun <ENV> ENV.create2(workspace: Workspace, model: Model): Model where
ENV : HasRepository,
ENV : HasIndex,
ENV : Raise<DomainError> {
    val created = withRepository(Repository.context) { create()(workspace, model) }
    withIndexFails(Index.context) {
        recover({
            Either.catch { update(workspace, created) }.mapLeft { indexContext.exceptionHandler(it) }.bind()
        }) { _: DomainError -> } // Recovers from the error when I let the exception bubble up to here
    }
    return created
}

suspend fun <ENV> ENV.create3(workspace: Workspace, model: Model): Model where
ENV : HasRepository,
ENV : HasIndex,
ENV : Raise<DomainError> {
    val created = withRepository(Repository.context) { create()(workspace, model) }
    recover({ withIndex(Index.context) { update(workspace, created) }.bind() }) { _: DomainError -> } // Again succeeds when the bind happens here
    return created
}

suspend fun <ENV, T : Entity<ID>, ID, VAL, E : BaseError> ENV.withIndexFails(
    context: IndexOperationsContext<T, ID, E>,
    block: suspend IndexOperationsWithError<T, ID, E>.() -> VAL
): VAL where ENV : HasIndex, ENV : HasWorkingDirectory, ENV : Raise<E> =
    with(
        object :
            IndexOperationsWithError<T, ID, E>,
            HasIndexOperations<T, ID, E> by HasIndexOperations(root, context, indexType),
            Raise<E> by this {}
    ) {
        Either.catch { block() }.mapLeft { indexContext.exceptionHandler(it) }.bind()
    }

suspend fun <ENV, T : Entity<ID>, ID, VAL, E : BaseError> ENV.withIndex(
    context: IndexOperationsContext<T, ID, E>,
    block: suspend HasIndexOperationsWithError<T, ID, E>.() -> VAL
): Either<E, VAL> where ENV : HasIndex, ENV : HasWorkingDirectory, ENV : Raise<E> =
    with(
        object :
            HasIndexOperationsWithError<T, ID, E>,
            HasIndexOperations<T, ID, E> by HasIndexOperations(root, context, indexType),
            Raise<E> by this {}
    ) {
        Either.catch { block() }.mapLeft { indexContext.exceptionHandler(it) }
    }

suspend fun <ENV, T, ID, E> ENV.update(workspace: WorkspaceId, model: T): Unit where
ENV : IndexOperationsWithError<T, ID, E> =
    with(indexContext) {
        refreshIndex(index()) // throws exception
    }

Is this expected behavior or am I doing something wrong?

@nomisRev
Copy link
Member

nomisRev commented Sep 7, 2023

Hey @cldfzn,

Thank you for the bug report, and using Arrow ☺️
I am missing some signatures and context to completely understand what is going on here.

context.exceptionHandler is of shape (Throwable) -> E? Because Either.catch will not "catch" E but only Throwable, but since it type checks for you I guess that's not the problem.

recovery no longer works as expected.

Was this working for you in a previous version, if yes. Which version did this work for you, and in which version did you encounter regression?

When using the above, the following code is unable to recover and propagates any raised Error out of the recover.

I'm not sure I understand, can you give an example of usage? The actual and expected result?

@cldfzn
Copy link
Author

cldfzn commented Sep 7, 2023

Updated the above to hopefully illustrate the problem better. I'm not sure what interactions are occurring here, I'm trying to create a minimal test to reproduce but haven't been able to replicate the behavior yet.

@cldfzn
Copy link
Author

cldfzn commented Sep 7, 2023

Ah, I think I see the issue. Since I wrap raise to create my own contextual class, the below referential checks are going to fail in some cases, due to recover (aka fold) creating a new raise and passing that to the block function. Since my ENV context satisfies all of the constraints, it is implicitly used and the Raise from the recover function is not used.

@PublishedApi
@Suppress("UNCHECKED_CAST")
internal fun <R> CancellationException.raisedOrRethrow(raise: DefaultRaise): R =
  when {
    this is RaiseCancellationExceptionNoTrace && this.raise === raise -> raised as R
    this is RaiseCancellationException && this.raise === raise -> raised as R
    else -> throw this
  }

@cldfzn
Copy link
Author

cldfzn commented Sep 8, 2023

This is a standalone test:

import arrow.core.Either
import arrow.core.raise.Raise
import arrow.core.raise.either
import arrow.core.raise.recover
import io.kotest.assertions.arrow.core.shouldBeRight
import io.kotest.core.spec.style.BehaviorSpec

class RaiseSpec :
    BehaviorSpec({
        Given("a function returning the recover value") {
            val result = either<Error, Unit> {
                val env = object : HasInitialWithError<Error> by HasInitialWithError(this, "data") {}
                with(env) {
                    recover({ enhance(context) { update("") } }) { _: Error -> "recovered" }
                }
            }

            Then("the result should be from the recovery function") { result.shouldBeRight("recovered") }
        }
    })

val context: OperationsContext<String, Error> =
    OperationsContext(fn = { raise(SomeError) }, exceptionHandler = { SomeError })

fun interface ExceptionHandler<E> {
    operator fun invoke(throwable: Throwable): E
}

fun <ENV, T, E> ENV.update(model: T): String where ENV : HasOperationsWithError<T, E> =
    with(context) { Either.catch { fn(model) }.mapLeft { exceptionHandler(it) }.bind() }

sealed interface Error

object SomeError : Error

interface HasInitialWithError<E> : Raise<E> {
    val data: String

    companion object {
        operator fun <E> invoke(raise: Raise<E>, data: String): HasInitialWithError<E> =
            object : HasInitialWithError<E>, Raise<E> by raise {
                override val data: String = data
            }
    }
}

interface HasOperations<T, E> {
    val context: OperationsContext<T, E>

    companion object {
        operator fun <T, E> invoke(context: OperationsContext<T, E>): HasOperations<T, E> =
            object : HasOperations<T, E> {
                override val context: OperationsContext<T, E> = context
            }
    }
}

interface HasOperationsWithError<T, E> : HasOperations<T, E>, Raise<E> {
    companion object {
        operator fun <T, E> Raise<E>.invoke(context: OperationsContext<T, E>): HasOperationsWithError<T, E> =
            object : HasOperationsWithError<T, E>, Raise<E> by this {
                override val context: OperationsContext<T, E> = context
            }
    }
}

class OperationsContext<T, E>(
    val fn: HasOperationsWithError<T, E>.(T) -> String,
    val exceptionHandler: ExceptionHandler<E>
)

suspend fun <ENV, T, VAL, E> ENV.enhance(
    context: OperationsContext<T, E>,
    block: suspend HasOperationsWithError<T, E>.() -> VAL
): VAL where ENV : HasInitialWithError<E>, ENV : Raise<E> =
    with(object : HasOperationsWithError<T, E>, HasOperations<T, E> by HasOperations(context), Raise<E> by this {}) {
        block()
    }

@nomisRev
Copy link
Member

Ah, I think I see the issue. Since I wrap raise to create my own contextual class, the below referential checks are going to fail in some cases, due to recover (aka fold) creating a new raise and passing that to the block function. Since my ENV context satisfies all of the constraints, it is implicitly used and the Raise from the recover function is not used.

Thanks for sharing the reproducible test! Your use-case is something I originally wanted to support, and I think I've used in some cases but perhaps I encoded it slightly different. I need to make some time to properly wrap my head around it and look into it.

We originally worked with a Token class for scoping checks, perhaps we have to bring those back. Or can encode it in the original way, without any additional performance hits. I'll report back asap!

@kyay10
Copy link
Collaborator

kyay10 commented Dec 21, 2023

FWIW, #3052 ran into the same issue, and the fix was to define custom recover methods for your custom Raise types. I think that's the simplest solution for now to get your code working.

@cldfzn
Copy link
Author

cldfzn commented Dec 21, 2023

Yeah, I’ve worked around the issue. I’ve begun converting the code base to use context receivers as most tooling blockers have been resolved. Using receivers is just so ergonomic I’m less worried about a solution for this problem for myself. Would be interesting to know if there is a nice solution in the end though.

@kyay10
Copy link
Collaborator

kyay10 commented Feb 6, 2024

As per #3334, this feature isn't possible, even if we overhauled Raise extensively. I did some experiments that I'd be happy to recreate if need be, but my understanding is that the best solution to this is just composition by using context receivers.

@kyay10 kyay10 closed this as completed Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants