[refactor]: improve diagnostics for escaped context instances (#380)

### Issue

the `{Render|Apply}Context` types should not escape from their
respective methods, as this can allow library internals to 'leak out'
and cause various issues[^1]. primarily this can result in lifetime
discrepancies or surprising relationships in the object graph.
historically we've enforced this invariant 'lazily' by setting a bit on
the instances and checking that they are still 'valid' during access
through their public API. however, it recently occurred to me that we
could probably be a bit more proactive about this, and perform a unique
reference check immediately after returning from the client callouts.

### Description

- adds logic to detect escaping context instances (debug mode only)
- adds the 'IssueReporting' dependency to Workflow so we can better test
the logic in our unit tests
- minor incidental refactoring of the Package file
- updates unit tests appropriately

[^1]: this is really more of an issue for RenderContext, as it doesn't
yet zero-out its internal storage when invalidated. since ApplyContext
is newer, we didn't make that mistake twice.

Author: jamieQ

Package.swift

@@ -61,14 +61,18 @@ let package = Package(
         .package(url: "https://github.com/pointfreeco/swift-macro-testing", from: "0.4.0"),
         .package(url: "https://github.com/pointfreeco/swift-perception", "1.5.0" ..< "3.0.0"),
         .package(url: "https://github.com/pointfreeco/swift-custom-dump", from: "1.3.3"),
-        .package(url: "https://github.com/pointfreeco/xctest-dynamic-overlay", from: "1.0.0"),
+        // 'xctest-dynamic-overlay' is actually for "IssueReporting"
+        .package(url: "https://github.com/pointfreeco/xctest-dynamic-overlay", from: "1.2.2"),
     ],
     targets: [
         // MARK: Workflow
 
         .target(
             name: "Workflow",
-            dependencies: ["ReactiveSwift"],
+            dependencies: [
+                .product(name: "IssueReporting", package: "xctest-dynamic-overlay"),
+                .product(name: "ReactiveSwift", package: "ReactiveSwift"),
+            ],
             path: "Workflow/Sources"
         ),
         .target(
@@ -126,7 +130,10 @@ let package = Package(
 
         .target(
             name: "WorkflowReactiveSwift",
-            dependencies: ["ReactiveSwift", "Workflow"],
+            dependencies: [
+                "Workflow",
+                .product(name: "ReactiveSwift", package: "ReactiveSwift"),
+            ],
             path: "WorkflowReactiveSwift/Sources"
         ),
         .target(
@@ -140,7 +147,10 @@ let package = Package(
 
         .target(
             name: "WorkflowRxSwift",
-            dependencies: ["RxSwift", "Workflow"],
+            dependencies: [
+                "Workflow",
+                .product(name: "RxSwift", package: "RxSwift"),
+            ],
             path: "WorkflowRxSwift/Sources"
         ),
         .target(

Workflow/Sources/Debugging.swift

@@ -14,6 +14,10 @@
  * limitations under the License.
  */
 
+#if DEBUG
+import IssueReporting
+#endif
+
 public struct WorkflowUpdateDebugInfo: Codable, Equatable {
     public var workflowType: String
     public var kind: Kind
@@ -168,3 +172,30 @@ extension WorkflowUpdateDebugInfo {
         )
     }()
 }
+
+// MARK: - Runtime Debugging Utilities
+
+#if DEBUG
+/// Debug facility that checks if an instance of a reference type may have 'escaped' from a function.
+/// - Parameters:
+///   - object: The instance to test.
+///   - message: The message to log if not uniquely referenced. The ObjectIdentifier of the instance will be supplied as an argument.
+///
+/// If the instance is **not** known to be uniquely referenced it will:
+///   - Trigger a test failure if running in a testing context.
+///   - Cause an assertion failure otherwise.
+func diagnoseEscapedReference(
+    to object: consuming some AnyObject,
+    _ message: (ObjectIdentifier) -> String
+) {
+    var maybeUniqueReference = consume object
+    if !isKnownUniquelyReferenced(&maybeUniqueReference) {
+        if let _ = TestContext.current {
+            reportIssue(message(ObjectIdentifier(maybeUniqueReference)))
+        } else {
+            assertionFailure(message(ObjectIdentifier(maybeUniqueReference)))
+        }
+    }
+    _ = consume maybeUniqueReference
+}
+#endif

Workflow/Sources/SubtreeManager.swift

@@ -16,6 +16,10 @@
 
 import Dispatch
 
+#if DEBUG
+import IssueReporting
+#endif
+
 extension WorkflowNode {
     /// Manages the subtree of a workflow. Specifically, this type encapsulates the logic required to update and manage
     /// the lifecycle of nested workflows across multiple render passes.
@@ -76,6 +80,16 @@ extension WorkflowNode {
 
             wrapped.invalidate()
 
+            #if DEBUG
+            // Try to ensure it didn't escape from the invocation.
+            diagnoseEscapedReference(to: consume wrapped) { objectID in
+                "Detected escape of a RenderContext<\(WorkflowType.self)> instance from a `render()` method. A RenderContext is only valid to use within render(); allowing it to escape may lead to incorrect behavior. Search for the address of '\(objectID)' in the memory graph debugger to determine why it may have escaped."
+            }
+            #else
+            // For binding lifetime consistency
+            _ = consume wrapped
+            #endif
+
             /// After the render is complete, assign children using *only the children that were used during the render
             /// pass.* This means that any pre-existing children that were *not* used during the render pass are removed
             /// as a result of this call to `render`.

Workflow/Sources/WorkflowNode.swift

@@ -249,7 +249,6 @@ extension WorkflowNode {
         do {
             // FIXME: can we avoid instantiating a class here somehow?
             let context = ConcreteApplyContext(storage: workflow)
-            defer { context.invalidate() }
             let wrappedContext = ApplyContext.make(implementation: context)
 
             let renderOnlyIfStateChanged = hostContext.runtimeConfig.renderOnlyIfStateChanged
@@ -302,6 +301,21 @@ extension WorkflowNode {
             } else {
                 result = performSimpleActionApplication(markStateAsChanged: true)
             }
+
+            // N.B. This logic would perhaps be nicer to put in a defer
+            // statement, but that seems to mess with the precise control
+            // we need over binding lifetimes to perform these checks.
+            _ = consume wrappedContext
+            context.invalidate()
+            #if DEBUG
+            // Try to ensure it didn't escape from the invocation.
+            diagnoseEscapedReference(to: consume context) { objectID in
+                "Detected escaped reference to an ApplyContext<\(A.WorkflowType.self)> when applying action '\(action)'. An ApplyContext is only valid for the duration of its associated action application, and should not escape. Search for the address of '\(objectID)' in the memory graph debugger to investigate."
+            }
+            #else
+            // For binding lifetime consistency
+            _ = consume context
+            #endif
         }
 
         return result

Workflow/Tests/ApplyContextTests.swift

@@ -14,18 +14,18 @@
  * limitations under the License.
  */
 
-import Testing
+import IssueReporting
+import XCTest
 
 @testable import Workflow
 
 @MainActor
-struct ApplyContextTests {
-    @Test
-    func concreteApplyContextInvalidatedAfterUse() async throws {
+final class ApplyContextTests: XCTestCase {
+    func testConcreteApplyContextInvalidatedAfterUse() async throws {
         var escapedContext: ApplyContext<EscapingContextWorkflow>?
         let onApply = { (context: ApplyContext<EscapingContextWorkflow>) in
-            #expect(context[workflowValue: \.property] == 42)
-            #expect(context.concreteStorage != nil)
+            XCTAssert(context[workflowValue: \.property] == 42)
+            XCTAssert(context.concreteStorage != nil)
             escapedContext = context
         }
 
@@ -38,10 +38,13 @@ struct ApplyContextTests {
         let emitEvent = node.render()
         node.enableEvents()
 
-        emitEvent()
+        // We're intentionally escaping the context
+        withExpectedIssue {
+            emitEvent()
+        }
 
-        #expect(escapedContext != nil)
-        #expect(escapedContext?.concreteStorage == nil)
+        XCTAssert(escapedContext != nil)
+        XCTAssert(escapedContext?.concreteStorage == nil)
     }
 }
 

Workflow/Tests/SubtreeManagerTests.swift

@@ -14,8 +14,10 @@
  * limitations under the License.
  */
 
+import IssueReporting
 import ReactiveSwift
 import XCTest
+
 @testable import Workflow
 
 final class SubtreeManagerTests: XCTestCase {
@@ -126,14 +128,17 @@ final class SubtreeManagerTests: XCTestCase {
 
         var escapingContext: RenderContext<ParentWorkflow>!
 
-        _ = manager.render { context -> TestViewModel in
-            XCTAssertTrue(context.isValid)
-            escapingContext = context
-            return context.render(
-                workflow: TestWorkflow(),
-                key: "",
-                outputMap: { _ in AnyWorkflowAction.noAction }
-            )
+        // We expect the context escape check to detect this
+        withExpectedIssue {
+            _ = manager.render { context -> TestViewModel in
+                XCTAssertTrue(context.isValid)
+                escapingContext = context
+                return context.render(
+                    workflow: TestWorkflow(),
+                    key: "",
+                    outputMap: { _ in AnyWorkflowAction.noAction }
+                )
+            }
         }
         manager.enableEvents()