From eef10e0b7ab6c4864e978dc74e4db6ba79b1e5ac Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Mon, 7 Apr 2025 13:03:56 -0700 Subject: [PATCH 1/3] [Observation] ensure event triggers on deinitialization passes as if all properties that are being observed have changed (for weak storage) (#79823) * [Observation] ensure event triggers on deinitialziation passes as if all properties that are being observed have changed (for weak storage) * Add missing deinitialize method for synthetically triggering willSet * Correct the weak location for tests * Correct the test to actually test the deinitialization willSet trigger instead of testing weak value deinitialization time * Refine the tests for deinit triggers to more tightly trigger deinitialization and weak references * Correct missing trailing closure on deinit replacement * Ensure all potential ids are triggered at the deinitialization edge trigger --- .../Observation/ObservationRegistrar.swift | 22 ++++++++++--- test/stdlib/Observation/Observable.swift | 31 +++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift index 3c356381bee63..fd61a2d28fc5a 100644 --- a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift +++ b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift @@ -110,9 +110,20 @@ public struct ObservationRegistrar: Sendable { } } - internal mutating func cancelAll() { + internal mutating func deinitialize() -> [@Sendable () -> Void] { + var trackers = [@Sendable () -> Void]() + for (keyPath, ids) in lookups { + for id in ids { + if let tracker = observations[id]?.willSetTracker { + trackers.append({ + tracker(keyPath) + }) + } + } + } observations.removeAll() lookups.removeAll() + return trackers } internal mutating func willSet(keyPath: AnyKeyPath) -> [@Sendable (AnyKeyPath) -> Void] { @@ -157,8 +168,11 @@ public struct ObservationRegistrar: Sendable { state.withCriticalRegion { $0.cancel(id) } } - internal func cancelAll() { - state.withCriticalRegion { $0.cancelAll() } + internal func deinitialize() { + let tracking = state.withCriticalRegion { $0.deinitialize() } + for action in tracking { + action() + } } internal func willSet( @@ -189,7 +203,7 @@ public struct ObservationRegistrar: Sendable { } deinit { - context.cancelAll() + context.deinitialize() } } diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift index 9fa23325805fc..2f05994060f75 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/test/stdlib/Observation/Observable.swift @@ -287,6 +287,21 @@ final class CowTest { var container = CowContainer() } +@Observable +final class DeinitTriggeredObserver { + var property: Int = 3 + let deinitTrigger: () -> Void + + init(_ deinitTrigger: @escaping () -> Void) { + self.deinitTrigger = deinitTrigger + } + + deinit { + deinitTrigger() + } +} + + @main struct Validator { @MainActor @@ -511,6 +526,22 @@ struct Validator { expectEqual(subject.container.id, startId) } + suite.test("weak container observation") { + let changed = CapturedState(state: false) + let deinitialized = CapturedState(state: false) + var test = DeinitTriggeredObserver { + deinitialized.state = true + } + withObservationTracking { [weak test] in + _blackHole(test?.property) + } onChange: { + changed.state = true + } + test = DeinitTriggeredObserver { } + expectEqual(deinitialized.state, true) + expectEqual(changed.state, true) + } + runAllTests() } } From 3b937af56d7cef8d0d5c425dee880acaaea88825 Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Tue, 17 Jun 2025 11:21:12 -0700 Subject: [PATCH 2/3] [Observation] Restrict deinitialization fired events to be limited to one per registrar and send a \Self.self as the keypath to avoid property confusions --- .../Observation/ObservationRegistrar.swift | 26 +++++++++++-------- test/stdlib/Observation/Observable.swift | 8 +++--- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift index fd61a2d28fc5a..b00f5288c34a0 100644 --- a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift +++ b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift @@ -110,20 +110,27 @@ public struct ObservationRegistrar: Sendable { } } - internal mutating func deinitialize() -> [@Sendable () -> Void] { - var trackers = [@Sendable () -> Void]() + internal mutating func deinitialize() -> (@Sendable () -> Void)? { + func extractSelf(_ ty: T.Type) -> AnyKeyPath { + return \T.self + } + + var tracker: (@Sendable () -> Void)? for (keyPath, ids) in lookups { for id in ids { - if let tracker = observations[id]?.willSetTracker { - trackers.append({ - tracker(keyPath) - }) + if let found = observations[id]?.willSetTracker { + // convert the keyPath into its \Self.self version + let selfKp = _openExistential(type(of: keyPath).rootType, do: extractSelf) + tracker = { + found(selfKp) + } + break } } } observations.removeAll() lookups.removeAll() - return trackers + return tracker } internal mutating func willSet(keyPath: AnyKeyPath) -> [@Sendable (AnyKeyPath) -> Void] { @@ -169,10 +176,7 @@ public struct ObservationRegistrar: Sendable { } internal func deinitialize() { - let tracking = state.withCriticalRegion { $0.deinitialize() } - for action in tracking { - action() - } + state.withCriticalRegion { $0.deinitialize() }?() } internal func willSet( diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift index 2f05994060f75..81b3f7bf1cbc6 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/test/stdlib/Observation/Observable.swift @@ -290,6 +290,7 @@ final class CowTest { @Observable final class DeinitTriggeredObserver { var property: Int = 3 + var property2: Int = 4 let deinitTrigger: () -> Void init(_ deinitTrigger: @escaping () -> Void) { @@ -528,17 +529,18 @@ struct Validator { suite.test("weak container observation") { let changed = CapturedState(state: false) - let deinitialized = CapturedState(state: false) + let deinitialized = CapturedState(state: 0) var test = DeinitTriggeredObserver { - deinitialized.state = true + deinitialized.state += 1 } withObservationTracking { [weak test] in _blackHole(test?.property) + _blackHole(test?.property2) } onChange: { changed.state = true } test = DeinitTriggeredObserver { } - expectEqual(deinitialized.state, true) + expectEqual(deinitialized.state, 1) // ensure only one invocation is done per deinitialization expectEqual(changed.state, true) } From 8b97bcfb9b251aefa1131fdb3ef8e40c6a556a3f Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Wed, 2 Jul 2025 15:21:05 -0700 Subject: [PATCH 3/3] Break the outer loop of iteration for deinitialization tracker --- .../Sources/Observation/ObservationRegistrar.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift index b00f5288c34a0..58fd1ff827bf4 100644 --- a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift +++ b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift @@ -116,7 +116,7 @@ public struct ObservationRegistrar: Sendable { } var tracker: (@Sendable () -> Void)? - for (keyPath, ids) in lookups { + lookupIteration: for (keyPath, ids) in lookups { for id in ids { if let found = observations[id]?.willSetTracker { // convert the keyPath into its \Self.self version @@ -124,7 +124,7 @@ public struct ObservationRegistrar: Sendable { tracker = { found(selfKp) } - break + break lookupIteration } } }