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.

Sign up for GitHub

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

Jump to bottom

Fix traced CCE when nested different types. #3337

Merged
merged 4 commits into from
Jan 23, 2024
Merged

Fix traced CCE when nested different types. #3337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25b8a1b
Fix traced CCE when nested different types.
kyay10 Dec 23, 2023
54bfd71
Merge branch 'main' into trace-cce
serras Jan 22, 2024
5fe16bb
Update Fold.kt to match changes from #3349
kyay10 Jan 23, 2024
12d2cee
Update api dump
kyay10 Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions arrow-libs/core/arrow-core/api/arrow-core.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3639,6 +3639,7 @@ public final class arrow/core/raise/RaiseKt {
public static final fun toValidated (Lkotlin/jvm/functions/Function1;)Larrow/core/Validated;
public static final fun toValidated (Lkotlin/jvm/functions/Function2;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
public static final fun traced (Larrow/core/raise/Raise;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function2;)Ljava/lang/Object;
public static final fun withCause (Ljava/util/concurrent/CancellationException;Ljava/util/concurrent/CancellationException;)Ljava/util/concurrent/CancellationException;
public static final fun withError (Larrow/core/raise/Raise;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;)Ljava/lang/Object;
public static final fun zipOrAccumulate (Larrow/core/raise/Raise;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function9;)Ljava/lang/Object;
public static final fun zipOrAccumulate (Larrow/core/raise/Raise;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function8;)Ljava/lang/Object;
Expand Down
22 changes: 17 additions & 5 deletions arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Fold.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,17 +224,29 @@ public inline fun <Error, A> Raise<Error>.traced(
@BuilderInference block: Raise<Error>.() -> A,
trace: (trace: Trace, error: Error) -> Unit
): A {
val isOuterTraced = this is DefaultRaise && isTraced
val nested: DefaultRaise = if (isOuterTraced) this as DefaultRaise else DefaultRaise(true)
val nested = DefaultRaise(true)
return try {
block.invoke(nested)
block(nested).also { nested.complete() }
} catch (e: CancellationException) {
nested.complete()
nomisRev marked this conversation as resolved.
Show resolved Hide resolved
val r: Error = e.raisedOrRethrow(nested)
trace(Trace(e), r)
if (isOuterTraced) throw e else raise(r)
// If our outer Raise happens to be traced
// Then we want the stack trace to match the inner one
try {
raise(r)
} catch (rethrown: CancellationException) {
throw rethrown.withCause(e)
nomisRev marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} catch (rethrown: CancellationException) {
throw rethrown.withCause(e)
}
} catch (rethrown: RaiseCancellationException) {
throw RaiseCancellationException(rethrown.raised, rethrown.raise, e.cause ?: rethrown.cause)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do this because of #3235 unless we @PublishedApi RaiseCancellationException.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been contemplating doing this.. but I've been a bit absent last 2 months due to some personal things keeping me busy.

My rationale for making it public is because I've had a scenario or two where I wanted to differentiate between RaiseCancellationException, and (Job) CancellationException. It's in really low-level edge case scenarios though, but it would allow figuring out if the CancellationException comes from Arrow, from KotlinX Coroutines, or from somewhere else.

Perhaps it warrants @DelicateApi or something, like KotlinX does to force @OptIn so it requires "flagging" low-level code.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think giving users that option is a good idea, especially because we already have tests confirming that try-catch can recover from raise. We'd have to make both Raise exceptions public thus. It's another question if we should allow inspecting the error, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I would say don't expose it if we don't have a need for it.

I got a small WIP, that relies on some new functionality and unofficial support of Kotlin 😅
It adds a public sealed class, with expect/actual implementation to disable the stack traces.

I'm going to raise it as a PR targeting this one, so we can discuss further based on a code example ☺️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now be replaced by the sealed public error for Raise, and then this PR is ready to be merged IMO 🥳 🙌

nomisRev marked this conversation as resolved.
Show resolved Hide resolved
}
}

@PublishedApi
internal fun CancellationException.withCause(cause: CancellationException): CancellationException = when (this) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make this part of the API? It feels that it can be directly placed in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It encapsulates doing a check with the RaiseCancellationException, which is private. I can instead make it @PublishedAPI internal

is RaiseCancellationException -> RaiseCancellationException(raised, raise, cause.cause ?: cause)
else -> this
}

/** Returns the raised value, rethrows the CancellationException if not our scope */
@PublishedApi
@Suppress("UNCHECKED_CAST")
Expand Down Expand Up @@ -262,7 +274,7 @@ internal class DefaultRaise(@PublishedApi internal val isTraced: Boolean) : Rais
private class RaiseCancellationExceptionNoTrace(val raised: Any?, val raise: Raise<Any?>) :
CancellationExceptionNoTrace()

private class RaiseCancellationException(val raised: Any?, val raise: Raise<Any?>) : CancellationException()
private class RaiseCancellationException(val raised: Any?, val raise: Raise<Any?>, override val cause: Throwable? = null) : CancellationException()

internal expect open class CancellationExceptionNoTrace() : CancellationException

Expand Down
6 changes: 3 additions & 3 deletions arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Trace.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public value class Trace(private val exception: CancellationException) {
* Note, the first line in the stacktrace will be the `RaiseCancellationException`.
* The users call to `raise` can found in the_second line of the stacktrace.
*/
public fun stackTraceToString(): String = exception.stackTraceToString()
public fun stackTraceToString(): String = (exception.cause ?: exception).stackTraceToString()

/**
* Prints the stacktrace.
Expand All @@ -29,7 +29,7 @@ public value class Trace(private val exception: CancellationException) {
* The users call to `raise` can found in the_second line of the stacktrace.
*/
public fun printStackTrace(): Unit =
exception.printStackTrace()
(exception.cause ?: exception).printStackTrace()

/**
* Returns the suppressed exceptions that occurred during cancellation of the surrounding coroutines,
Expand All @@ -39,5 +39,5 @@ public value class Trace(private val exception: CancellationException) {
* if the finalizer then results in a `Throwable` it will be added as a `suppressedException` to the [CancellationException].
*/
public fun suppressedExceptions(): List<Throwable> =
exception.suppressedExceptions
exception.cause?.suppressedExceptions.orEmpty() + exception.suppressedExceptions
}
10 changes: 10 additions & 0 deletions arrow-libs/core/arrow-core/src/commonTest/kotlin/arrow/core/raise/TraceSpec.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,14 @@ class TraceSpec : StringSpec({
}
}
}

"nested tracing - different types" {
either {
traced<Any?, _>({
traced<String, _> ({
raise(Unit)
}) { _, _ -> unreachable() }
}) { _, unit -> unit shouldBe Unit }
}
}
})