Merge pull request #1302 from square/ray/throwables

Better crash reporter error grouping from RenderContext assertions

Author: rjrjr

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/RealRenderContext.kt

@@ -72,15 +72,15 @@ internal class RealRenderContext<out PropsT, StateT, OutputT>(
     key: String,
     handler: (ChildOutputT) -> WorkflowAction<PropsT, StateT, OutputT>
   ): ChildRenderingT {
-    checkNotFrozen { "renderChild(${child.identifier})" }
+    checkNotFrozen(child) { "renderChild(${child.identifier})" }
     return renderer.render(child, props, key, handler)
   }
 
   override fun runningSideEffect(
     key: String,
     sideEffect: suspend CoroutineScope.() -> Unit
   ) {
-    checkNotFrozen { "runningSideEffect($key)" }
+    checkNotFrozen(key) { "runningSideEffect($key)" }
     sideEffectRunner.runningSideEffect(key, sideEffect)
   }
 
@@ -90,15 +90,15 @@ internal class RealRenderContext<out PropsT, StateT, OutputT>(
     vararg inputs: Any?,
     calculation: () -> ResultT
   ): ResultT {
-    checkNotFrozen { "remember($key)" }
+    checkNotFrozen(key) { "remember($key)" }
     return rememberStore.remember(key, resultType, inputs = inputs, calculation)
   }
 
   /**
    * 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<out PropsT, StateT, OutputT>(
     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()}"
+    }
 }

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/SubtreeManager.kt

@@ -126,7 +126,7 @@ internal class SubtreeManager<PropsT, StateT, OutputT>(
     // 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\""
         }
       }

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 : Throwable> T.withKey(stackTraceKey: Any): T

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/internal/WorkflowNode.kt

@@ -163,7 +163,7 @@ internal class WorkflowNode<PropsT, StateT, OutputT, RenderingT>(
   ) {
     // 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<PropsT, StateT, OutputT, RenderingT>(
     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\""
       }
     }

workflow-runtime/src/iosMain/kotlin/com/squareup/workflow1/internal/Throwables.ios.kt

@@ -0,0 +1,3 @@
+package com.squareup.workflow1.internal
+
+actual fun <T : Throwable> T.withKey(stackTraceKey: Any): T = this

workflow-runtime/src/jsMain/kotlin/com/squareup/workflow1/internal/Throwables.js.kt

@@ -0,0 +1,3 @@
+package com.squareup.workflow1.internal
+
+actual fun <T : Throwable> T.withKey(stackTraceKey: Any): T = this

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 : Throwable> 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()
+}

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)
+  }
+}