From 583612355a10173d317ca7b7c1766727b9b6a9b2 Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Thu, 6 Mar 2025 12:30:52 -0800 Subject: [PATCH 1/7] [Observation] ensure event triggers on deinitialziation passes as if all properties that are being observed have changed (for weak storage) --- .../Observation/ObservationRegistrar.swift | 11 ++++++---- test/stdlib/Observation/Observable.swift | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift index 3c356381bee63..e7d35e23bc727 100644 --- a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift +++ b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift @@ -110,7 +110,7 @@ public struct ObservationRegistrar: Sendable { } } - internal mutating func cancelAll() { + internal mutating func deinitialize() { observations.removeAll() lookups.removeAll() } @@ -157,8 +157,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 +192,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..225548119e4bc 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/test/stdlib/Observation/Observable.swift @@ -511,6 +511,28 @@ struct Validator { expectEqual(subject.container.id, startId) } + suite.test("weak container observation") { + let changed = CapturedState(state: false) + var contents: HasIgnoredProperty? = HasIgnoredProperty() + class Container { + var contents: weak HasIgnoredProperty? + init(contents: HasIgnoredProperty) { + self.contents = contents + } + } + + let test = Container(contents: contents!) + withObservationTracking { + _blackHole(test.contents?.ignored) + _blackHole(test.contents?.field) + } onChange: { + changed.state = true + } + + contents = nil + expectEqual(changed.state, true) + } + runAllTests() } } From 60936b840f0c92ec2ea5d616cf9bf51ec4b1d76d Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Fri, 7 Mar 2025 10:41:27 -0800 Subject: [PATCH 2/7] Add missing deinitialize method for synthetically triggering willSet --- .../Sources/Observation/ObservationRegistrar.swift | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift index e7d35e23bc727..ae17f002bd1bf 100644 --- a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift +++ b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift @@ -110,9 +110,18 @@ public struct ObservationRegistrar: Sendable { } } - internal mutating func deinitialize() { + internal mutating func deinitialize() -> [@Sendable () -> Void] { + var trackers = [@Sendable () -> Void]() + for (keyPath, ids) in lookups { + 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] { From b23bb9f8351fbf23c0a565b92cd6f594c3a63023 Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Thu, 13 Mar 2025 15:42:51 -0700 Subject: [PATCH 3/7] Correct the weak location for tests --- test/stdlib/Observation/Observable.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift index 225548119e4bc..e20b8e8a313dd 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/test/stdlib/Observation/Observable.swift @@ -515,7 +515,7 @@ struct Validator { let changed = CapturedState(state: false) var contents: HasIgnoredProperty? = HasIgnoredProperty() class Container { - var contents: weak HasIgnoredProperty? + weak var contents: HasIgnoredProperty? init(contents: HasIgnoredProperty) { self.contents = contents } From dc99d419e8fe180f0c504efe02b78cdb6f312150 Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Wed, 26 Mar 2025 14:32:12 -0700 Subject: [PATCH 4/7] Correct the test to actually test the deinitialization willSet trigger instead of testing weak value deinitialization time --- test/stdlib/Observation/Observable.swift | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift index e20b8e8a313dd..f950050c3de51 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/test/stdlib/Observation/Observable.swift @@ -287,6 +287,12 @@ final class CowTest { var container = CowContainer() } +@Observable +final class DeinitTriggeredObserver { + var property: Int = 3 +} + + @main struct Validator { @MainActor @@ -513,23 +519,13 @@ struct Validator { suite.test("weak container observation") { let changed = CapturedState(state: false) - var contents: HasIgnoredProperty? = HasIgnoredProperty() - class Container { - weak var contents: HasIgnoredProperty? - init(contents: HasIgnoredProperty) { - self.contents = contents - } - } - - let test = Container(contents: contents!) + var test: DeinitTriggeredObserver? = DeinitTriggeredObserver() withObservationTracking { - _blackHole(test.contents?.ignored) - _blackHole(test.contents?.field) + _blackHole(test?.property) } onChange: { changed.state = true } - - contents = nil + test = nil expectEqual(changed.state, true) } From be1d86bf4b15f41772c603c3ac70c5fdece5b4cb Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Thu, 3 Apr 2025 11:24:21 -0700 Subject: [PATCH 5/7] Refine the tests for deinit triggers to more tightly trigger deinitialization and weak references --- test/stdlib/Observation/Observable.swift | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift index f950050c3de51..328895550b3a6 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/test/stdlib/Observation/Observable.swift @@ -290,6 +290,15 @@ final class CowTest { @Observable final class DeinitTriggeredObserver { var property: Int = 3 + let deinitTrigger: () -> Void + + init(_ deinitTrigger: @escaping () -> Void) { + self.deinitTrigger = deinitTrigger + } + + deinit { + deinitTrigger() + } } @@ -519,13 +528,17 @@ struct Validator { suite.test("weak container observation") { let changed = CapturedState(state: false) - var test: DeinitTriggeredObserver? = DeinitTriggeredObserver() - withObservationTracking { + let deinitialized = CapturedState(state: false) + var test = DeinitTriggeredObserver { + deinitialized.state = true + } + withObservationTracking { [weak test] in _blackHole(test?.property) } onChange: { changed.state = true } - test = nil + test = DeinitTriggeredObserver() + expectEqual(deinitialized.state, true) expectEqual(changed.state, true) } From 4b4f93981db9a495e6660aef7ac430f7788c3e2f Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Thu, 3 Apr 2025 17:17:17 -0700 Subject: [PATCH 6/7] Correct missing trailing closure on deinit replacement --- test/stdlib/Observation/Observable.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/stdlib/Observation/Observable.swift b/test/stdlib/Observation/Observable.swift index 328895550b3a6..2f05994060f75 100644 --- a/test/stdlib/Observation/Observable.swift +++ b/test/stdlib/Observation/Observable.swift @@ -537,7 +537,7 @@ struct Validator { } onChange: { changed.state = true } - test = DeinitTriggeredObserver() + test = DeinitTriggeredObserver { } expectEqual(deinitialized.state, true) expectEqual(changed.state, true) } From d3765202c0c85a77302486351d9f852549348f00 Mon Sep 17 00:00:00 2001 From: Philippe Hausler Date: Fri, 4 Apr 2025 10:12:17 -0700 Subject: [PATCH 7/7] Ensure all potential ids are triggered at the deinitialization edge trigger --- .../Sources/Observation/ObservationRegistrar.swift | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift index ae17f002bd1bf..fd61a2d28fc5a 100644 --- a/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift +++ b/stdlib/public/Observation/Sources/Observation/ObservationRegistrar.swift @@ -113,10 +113,12 @@ public struct ObservationRegistrar: Sendable { internal mutating func deinitialize() -> [@Sendable () -> Void] { var trackers = [@Sendable () -> Void]() for (keyPath, ids) in lookups { - if let tracker = observations[id]?.willSetTracker { - trackers.append({ - tracker(keyPath) - }) + for id in ids { + if let tracker = observations[id]?.willSetTracker { + trackers.append({ + tracker(keyPath) + }) + } } } observations.removeAll()