Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 15 additions & 4 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -61,21 +61,26 @@ 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(
name: "WorkflowTesting",
dependencies: [
"Workflow",
.product(name: "CustomDump", package: "swift-custom-dump"),
.product(name: "IssueReporting", package: "xctest-dynamic-overlay"),
],
path: "WorkflowTesting/Sources",
linkerSettings: [.linkedFramework("XCTest")]
Expand Down Expand Up @@ -125,7 +130,10 @@ let package = Package(

.target(
name: "WorkflowReactiveSwift",
dependencies: ["ReactiveSwift", "Workflow"],
dependencies: [
"Workflow",
.product(name: "ReactiveSwift", package: "ReactiveSwift"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

standardized some of these so now all the 'package-level' dependencies are just strings, and the external ones are .product()

],
path: "WorkflowReactiveSwift/Sources"
),
.target(
Expand All @@ -139,7 +147,10 @@ let package = Package(

.target(
name: "WorkflowRxSwift",
dependencies: ["RxSwift", "Workflow"],
dependencies: [
"Workflow",
.product(name: "RxSwift", package: "RxSwift"),
],
path: "WorkflowRxSwift/Sources"
),
.target(
Expand Down
31 changes: 31 additions & 0 deletions Workflow/Sources/Debugging.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
14 changes: 14 additions & 0 deletions Workflow/Sources/SubtreeManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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`.
Expand Down
16 changes: 15 additions & 1 deletion Workflow/Sources/WorkflowNode.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
21 changes: 12 additions & 9 deletions Workflow/Tests/ApplyContextTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

@jamieQ jamieQ Sep 23, 2025

Choose a reason for hiding this comment

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

converted this to XCTest b/c when testing against the oldest version of the IssueReporting dependency we support (1.2.2), for some reason the test seemed to behave differently and didn't appear to actually 'escape' the context. i assume that means there's a bug somewhere, but did not investigate.

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
}

Expand All @@ -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)
}
}

Expand Down
21 changes: 13 additions & 8 deletions Workflow/Tests/SubtreeManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
* limitations under the License.
*/

import IssueReporting
import ReactiveSwift
import XCTest

@testable import Workflow

final class SubtreeManagerTests: XCTestCase {
Expand Down Expand Up @@ -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()

Expand Down
Loading