diff --git a/Package.swift b/Package.swift index 37e0222f6..02627050f 100644 --- a/Package.swift +++ b/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( @@ -76,6 +80,7 @@ let package = Package( dependencies: [ "Workflow", .product(name: "CustomDump", package: "swift-custom-dump"), + .product(name: "IssueReporting", package: "xctest-dynamic-overlay"), ], path: "WorkflowTesting/Sources", linkerSettings: [.linkedFramework("XCTest")] @@ -125,7 +130,10 @@ let package = Package( .target( name: "WorkflowReactiveSwift", - dependencies: ["ReactiveSwift", "Workflow"], + dependencies: [ + "Workflow", + .product(name: "ReactiveSwift", package: "ReactiveSwift"), + ], path: "WorkflowReactiveSwift/Sources" ), .target( @@ -139,7 +147,10 @@ let package = Package( .target( name: "WorkflowRxSwift", - dependencies: ["RxSwift", "Workflow"], + dependencies: [ + "Workflow", + .product(name: "RxSwift", package: "RxSwift"), + ], path: "WorkflowRxSwift/Sources" ), .target( diff --git a/Workflow/Sources/Debugging.swift b/Workflow/Sources/Debugging.swift index 956e63137..f8159a42e 100644 --- a/Workflow/Sources/Debugging.swift +++ b/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 diff --git a/Workflow/Sources/SubtreeManager.swift b/Workflow/Sources/SubtreeManager.swift index cf262e8c3..2dfee9f28 100644 --- a/Workflow/Sources/SubtreeManager.swift +++ b/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`. diff --git a/Workflow/Sources/WorkflowNode.swift b/Workflow/Sources/WorkflowNode.swift index d4ba6a691..ea934de02 100644 --- a/Workflow/Sources/WorkflowNode.swift +++ b/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 diff --git a/Workflow/Tests/ApplyContextTests.swift b/Workflow/Tests/ApplyContextTests.swift index b873f86bb..19a5a943f 100644 --- a/Workflow/Tests/ApplyContextTests.swift +++ b/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? let onApply = { (context: ApplyContext) 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) } } diff --git a/Workflow/Tests/SubtreeManagerTests.swift b/Workflow/Tests/SubtreeManagerTests.swift index 4c97d00c7..9d0687d1a 100644 --- a/Workflow/Tests/SubtreeManagerTests.swift +++ b/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! - _ = 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()