diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/RealRenderContext.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/RealRenderContext.kt index ae1e28556d..23876c855a 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/RealRenderContext.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/RealRenderContext.kt @@ -72,7 +72,7 @@ internal class RealRenderContext( key: String, handler: (ChildOutputT) -> WorkflowAction ): ChildRenderingT { - checkNotFrozen { "renderChild(${child.identifier})" } + checkNotFrozen(child) { "renderChild(${child.identifier})" } return renderer.render(child, props, key, handler) } @@ -80,7 +80,7 @@ internal class RealRenderContext( key: String, sideEffect: suspend CoroutineScope.() -> Unit ) { - checkNotFrozen { "runningSideEffect($key)" } + checkNotFrozen(key) { "runningSideEffect($key)" } sideEffectRunner.runningSideEffect(key, sideEffect) } @@ -90,7 +90,7 @@ internal class RealRenderContext( vararg inputs: Any?, calculation: () -> ResultT ): ResultT { - checkNotFrozen { "remember($key)" } + checkNotFrozen(key) { "remember($key)" } return rememberStore.remember(key, resultType, inputs = inputs, calculation) } @@ -98,7 +98,7 @@ internal class RealRenderContext( * Freezes this context so that any further calls to this context will throw. */ fun freeze() { - checkNotFrozen { "freeze" } + checkNotFrozen("freeze") { "freeze" } frozen = true } @@ -109,7 +109,13 @@ internal class RealRenderContext( frozen = false } - private fun checkNotFrozen(reason: () -> String) = check(!frozen) { - "RenderContext cannot be used after render method returns: ${reason()}" - } + /** + * @param stackTraceKey ensures unique crash reporter error groups. + * + * @see checkWithKey + */ + private inline fun checkNotFrozen(stackTraceKey: Any, lazyMessage: () -> Any) = + checkWithKey(!frozen, stackTraceKey) { + "RenderContext cannot be used after render method returns: ${lazyMessage()}" + } } diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt index 7ec3bd6ecf..a150d59982 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt @@ -126,7 +126,7 @@ internal class SubtreeManager( // Prevent duplicate workflows with the same key. workflowTracer.trace("CheckingUniqueMatches") { children.forEachStaging { - require(!(it.matches(child, key, workflowTracer))) { + requireWithKey(!(it.matches(child, key, workflowTracer)), stackTraceKey = child) { "Expected keys to be unique for ${child.identifier}: key=\"$key\"" } } diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/Throwables.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/Throwables.kt index 208f9b5f41..9dd3b1954a 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/Throwables.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/Throwables.kt @@ -1,8 +1,66 @@ package com.squareup.workflow1.internal -import kotlinx.coroutines.CancellationException +import kotlin.contracts.ExperimentalContracts +import kotlin.contracts.contract -internal tailrec fun Throwable.unwrapCancellationCause(): Throwable? { - if (this !is CancellationException) return this - return cause?.unwrapCancellationCause() +/** + * Like Kotlin's [require], but uses [stackTraceKey] to create a fake top element + * on the stack trace, ensuring that crash reporter's default grouping will create unique + * groups for unique keys. + * + * So far [stackTraceKey] is only effective on JVM, it has no effect in other languages. + * + * @see [withKey] + * + * @throws IllegalArgumentException if the [value] is false. + */ +@OptIn(ExperimentalContracts::class) +internal inline fun requireWithKey( + value: Boolean, + stackTraceKey: Any, + lazyMessage: () -> Any = { "Failed requirement." } +) { + contract { + returns() implies value + } + if (!value) { + val message = lazyMessage() + val exception: Throwable = IllegalArgumentException(message.toString()) + throw exception.withKey(stackTraceKey) + } } + +/** + * Like Kotlin's [check], but uses [stackTraceKey] to create a fake top element + * on the stack trace, ensuring that crash reporter's default grouping will create unique + * groups for unique keys. + * + * So far [stackTraceKey] is only effective on JVM, it has no effect in other languages. + * + * @see [withKey] + * + * @throws IllegalStateException if the [value] is false. + */ +@OptIn(ExperimentalContracts::class) +internal inline fun checkWithKey( + value: Boolean, + stackTraceKey: Any, + lazyMessage: () -> Any = { "Check failed." } +) { + contract { + returns() implies value + } + if (!value) { + val message = lazyMessage() + val exception: Throwable = IllegalStateException(message.toString()) + throw exception.withKey(stackTraceKey) + } +} + +/** + * Uses [stackTraceKey] to create a fake top element on the stack trace, ensuring + * that crash reporter's default grouping will create unique groups for unique keys. + * + * So far only effective on JVM, this is a pass through in other languages. + */ +internal expect fun T.withKey(stackTraceKey: Any): T diff --git a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt index d3a013bafd..f73d2f374b 100644 --- a/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt +++ b/workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt @@ -163,7 +163,7 @@ internal class WorkflowNode( ) { // Prevent duplicate side effects with the same key. sideEffects.forEachStaging { - require(key != it.key) { "Expected side effect keys to be unique: \"$key\"" } + requireWithKey(key != it.key, key) { "Expected side effect keys to be unique: \"$key\"" } } sideEffects.retainOrCreate( @@ -179,7 +179,10 @@ internal class WorkflowNode( calculation: () -> ResultT ): ResultT { remembered.forEachStaging { - require(key != it.key || resultType != it.resultType || !inputs.contentEquals(it.inputs)) { + requireWithKey( + key != it.key || resultType != it.resultType || !inputs.contentEquals(it.inputs), + stackTraceKey = key + ) { "Expected combination of key, inputs and result type to be unique: \"$key\"" } } diff --git a/workflow-runtime/src/iosMain/kotlin/com/squareup/workflow1/internal/Throwables.ios.kt b/workflow-runtime/src/iosMain/kotlin/com/squareup/workflow1/internal/Throwables.ios.kt new file mode 100644 index 0000000000..b991bc135b --- /dev/null +++ b/workflow-runtime/src/iosMain/kotlin/com/squareup/workflow1/internal/Throwables.ios.kt @@ -0,0 +1,3 @@ +package com.squareup.workflow1.internal + +actual fun T.withKey(stackTraceKey: Any): T = this diff --git a/workflow-runtime/src/jsMain/kotlin/com/squareup/workflow1/internal/Throwables.js.kt b/workflow-runtime/src/jsMain/kotlin/com/squareup/workflow1/internal/Throwables.js.kt new file mode 100644 index 0000000000..b991bc135b --- /dev/null +++ b/workflow-runtime/src/jsMain/kotlin/com/squareup/workflow1/internal/Throwables.js.kt @@ -0,0 +1,3 @@ +package com.squareup.workflow1.internal + +actual fun T.withKey(stackTraceKey: Any): T = this diff --git a/workflow-runtime/src/jvmMain/kotlin/com/squareup/workflow1/internal/Throwables.jvm.kt b/workflow-runtime/src/jvmMain/kotlin/com/squareup/workflow1/internal/Throwables.jvm.kt new file mode 100644 index 0000000000..eec7784ff5 --- /dev/null +++ b/workflow-runtime/src/jvmMain/kotlin/com/squareup/workflow1/internal/Throwables.jvm.kt @@ -0,0 +1,13 @@ +package com.squareup.workflow1.internal + +internal actual fun T.withKey(stackTraceKey: Any): T = apply { + val realTop = stackTrace[0] + val fakeTop = StackTraceElement( + // Real class name to ensure that we are still "in project". + realTop.className, + "fakeMethodForCrashGrouping", + /* fileName = */ stackTraceKey.toString(), + /* lineNumber = */ stackTraceKey.hashCode() + ) + stackTrace = stackTrace.toMutableList().apply { add(0, fakeTop) }.toTypedArray() +} diff --git a/workflow-runtime/src/jvmTest/kotlin/com/squareup/workflow1/internal/ThrowablesTest.kt b/workflow-runtime/src/jvmTest/kotlin/com/squareup/workflow1/internal/ThrowablesTest.kt new file mode 100644 index 0000000000..3850f178f7 --- /dev/null +++ b/workflow-runtime/src/jvmTest/kotlin/com/squareup/workflow1/internal/ThrowablesTest.kt @@ -0,0 +1,39 @@ +package com.squareup.workflow1.internal + +import kotlin.test.Test +import kotlin.test.assertEquals + +class ThrowablesTest { + + @Test fun `requireWithKey throws IllegalArgumentException`() { + try { + requireWithKey(false, "requiredKey") { "message" } + } catch (e: IllegalArgumentException) { + assertEquals("message", e.message) + e.assertIsKeyedException("requiredKey") + return + } + } + + @Test fun `checkWithKey throws IllegalStateException`() { + try { + checkWithKey(false, "checkedKey") { "message" } + } catch (e: IllegalStateException) { + assertEquals("message", e.message) + e.assertIsKeyedException("checkedKey") + return + } + } + + @Test fun `Throwable withKey adds frame based on key`() { + RuntimeException("cause").withKey("key").assertIsKeyedException("key") + } + + private fun RuntimeException.assertIsKeyedException(key: String) { + val top = stackTrace[0] + val topPlusOne = stackTrace[1] + assertEquals(topPlusOne.className, top.className, "Uses real class name") + assertEquals(key, top.fileName) + assertEquals(key.hashCode(), top.lineNumber) + } +}