From 918c9844fa2fdb8211f10c321740d7f89359e1f0 Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Mon, 11 Jan 2021 13:25:09 +0000 Subject: [PATCH 1/7] Revert "Port ObservableObject implementation with Runtime" This reverts commit bccff3e7c84bc559e1aa0aa9ca878400360d439d. --- Package.swift | 4 +- Sources/OpenCombine/ObservableObject.swift | 81 ---------------------- 2 files changed, 1 insertion(+), 84 deletions(-) diff --git a/Package.swift b/Package.swift index 96f4ea029..99d4616cd 100644 --- a/Package.swift +++ b/Package.swift @@ -23,15 +23,13 @@ let package = Package( .library(name: "OpenCombineDispatch", targets: ["OpenCombineDispatch"]), .library(name: "OpenCombineFoundation", targets: ["OpenCombineFoundation"]), ], - dependencies: [.package(url: "https://github.com/MaxDesiatov/Runtime.git", from: "2.1.2")], targets: [ .target(name: "COpenCombineHelpers"), .target( name: "OpenCombine", dependencies: [ .target(name: "COpenCombineHelpers", - condition: .when(platforms: supportedPlatforms.except([.wasi]))), - .product(name: "Runtime", package: "Runtime", condition: .when(platforms: [.wasi])), + condition: .when(platforms: supportedPlatforms.except([.wasi]))) ], exclude: [ "Publishers/Publishers.Encode.swift.gyb", diff --git a/Sources/OpenCombine/ObservableObject.swift b/Sources/OpenCombine/ObservableObject.swift index 75b903b76..e6195cae0 100644 --- a/Sources/OpenCombine/ObservableObject.swift +++ b/Sources/OpenCombine/ObservableObject.swift @@ -5,10 +5,6 @@ // Created by Sergej Jaskiewicz on 08/09/2019. // -#if canImport(Runtime) -import Runtime -#endif - /// A type of object with a publisher that emits before the object has changed. /// /// By default an `ObservableObject` synthesizes an `objectWillChange` publisher that @@ -46,86 +42,10 @@ public protocol ObservableObject: AnyObject { var objectWillChange: ObjectWillChangePublisher { get } } -#if swift(>=5.1) && canImport(Runtime) -private protocol _ObservableObjectProperty { - var objectWillChange: ObservableObjectPublisher? { get set } -} - -extension _ObservableObjectProperty { - - fileprivate static func installPublisher( - _ publisher: ObservableObjectPublisher, - on publishedStorage: UnsafeMutableRawPointer - ) { - // It is safe to call assumingMemoryBound here because we know for sure - // that the actual type of the pointee is Self. - publishedStorage - .assumingMemoryBound(to: Self.self) - .pointee - .objectWillChange = publisher - } - - fileprivate static func getPublisher( - from publishedStorage: UnsafeMutableRawPointer - ) -> ObservableObjectPublisher? { - // It is safe to call assumingMemoryBound here because we know for sure - // that the actual type of the pointee is Self. - return publishedStorage - .assumingMemoryBound(to: Self.self) - .pointee - .objectWillChange - } -} - -extension Published: _ObservableObjectProperty {} -#endif - extension ObservableObject where ObjectWillChangePublisher == ObservableObjectPublisher { // swiftlint:disable let_var_whitespace #if swift(>=5.1) /// A publisher that emits before the object has changed. - #if canImport(Runtime) - public var objectWillChange: ObservableObjectPublisher { - var installedPublisher: ObservableObjectPublisher? - let info = try! typeInfo(of: Self.self) - for property in info.properties { - let storage = Unmanaged - .passUnretained(self) - .toOpaque() - .advanced(by: property.offset) - - guard let fieldType = property.type as? _ObservableObjectProperty.Type else { - // Visit other fields until we meet a @Published field - continue - } - - // Now we know that the field is @Published. - if let alreadyInstalledPublisher = fieldType.getPublisher(from: storage) { - installedPublisher = alreadyInstalledPublisher - // Don't visit other fields, as all @Published fields - // already have a publisher installed. - break - } - - // Okay, this field doesn't have a publisher installed. - // This means that other fields don't have it either - // (because we install it only once and fields can't be added at runtime). - var lazilyCreatedPublisher: ObjectWillChangePublisher { - if let publisher = installedPublisher { - return publisher - } - let publisher = ObservableObjectPublisher() - installedPublisher = publisher - return publisher - } - - fieldType.installPublisher(lazilyCreatedPublisher, on: storage) - - // Continue visiting other fields. - } - return installedPublisher ?? ObservableObjectPublisher() - } - #else @available(*, unavailable, message: """ The default implementation of objectWillChange is not available yet. \ It's being worked on in \ @@ -134,7 +54,6 @@ extension ObservableObject where ObjectWillChangePublisher == ObservableObjectPu public var objectWillChange: ObservableObjectPublisher { fatalError("unimplemented") } - #endif #else public var objectWillChange: ObservableObjectPublisher { return ObservableObjectPublisher() From c8446b8cfc8ef3b079fcc72a039ba38c20e7ed4b Mon Sep 17 00:00:00 2001 From: Sergej Jaskiewicz Date: Sun, 8 Nov 2020 17:44:33 +0300 Subject: [PATCH 2/7] Fix some lock acquiring in Publishers.FlatMap (#194) --- .../Publishers/Publishers.FlatMap.swift | 33 +++++++++++-------- .../PublisherTests/FlatMapTests.swift | 25 ++++++++++++++ 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/Sources/OpenCombine/Publishers/Publishers.FlatMap.swift b/Sources/OpenCombine/Publishers/Publishers.FlatMap.swift index dcbb6adfe..b1b59c1db 100644 --- a/Sources/OpenCombine/Publishers/Publishers.FlatMap.swift +++ b/Sources/OpenCombine/Publishers/Publishers.FlatMap.swift @@ -157,17 +157,17 @@ extension Publishers { public func receive(subscriber: Downstream) where Child.Output == Downstream.Input, Upstream.Failure == Downstream.Failure { - let inner = Inner(downstream: subscriber, + let outer = Outer(downstream: subscriber, maxPublishers: maxPublishers, map: transform) - subscriber.receive(subscription: inner) - upstream.subscribe(inner) + subscriber.receive(subscription: outer) + upstream.subscribe(outer) } } } extension Publishers.FlatMap { - private final class Inner + private final class Outer : Subscriber, Subscription, CustomStringConvertible, @@ -243,7 +243,7 @@ extension Publishers.FlatMap { subscription.request(maxPublishers) } - fileprivate func receive(_ input: Upstream.Output) -> Subscribers.Demand { + fileprivate func receive(_ input: Input) -> Subscribers.Demand { lock.lock() let cancelledOrCompleted = self.cancelledOrCompleted lock.unlock() @@ -260,9 +260,9 @@ extension Publishers.FlatMap { return .none } - fileprivate func receive(completion: Subscribers.Completion) { - outerSubscription = nil + fileprivate func receive(completion: Subscribers.Completion) { lock.lock() + outerSubscription = nil outerFinished = true switch completion { case .finished: @@ -272,6 +272,8 @@ extension Publishers.FlatMap { let wasAlreadyCancelledOrCompleted = cancelledOrCompleted cancelledOrCompleted = true for (_, subscription) in subscriptions { + // Cancelling subscriptions with the lock acquired. Not good, + // but that's what Combine does. This code path is tested. subscription.cancel() } subscriptions = [:] @@ -354,16 +356,21 @@ extension Publishers.FlatMap { fileprivate func cancel() { lock.lock() + if cancelledOrCompleted { + lock.unlock() + return + } cancelledOrCompleted = true let subscriptions = self.subscriptions self.subscriptions = [:] + let outerSubscription = self.outerSubscription + self.outerSubscription = nil lock.unlock() for (_, subscription) in subscriptions { subscription.cancel() } - // Combine doesn't acquire the lock here. Weird. + // Combine doesn't acquire outerLock here. Weird. outerSubscription?.cancel() - outerSubscription = nil } // MARK: - Reflection @@ -471,9 +478,9 @@ extension Publishers.FlatMap { private func releaseLockThenSendCompletionDownstreamIfNeeded( outerFinished: Bool ) -> Bool { - #if DEBUG +#if DEBUG lock.assertOwner() // Sanity check - #endif +#endif if !cancelledOrCompleted && outerFinished && buffer.isEmpty && subscriptions.count + pendingSubscriptions == 0 { cancelledOrCompleted = true @@ -495,10 +502,10 @@ extension Publishers.FlatMap { CustomReflectable, CustomPlaygroundDisplayConvertible { private let index: SubscriptionIndex - private let inner: Inner + private let inner: Outer fileprivate let combineIdentifier = CombineIdentifier() - fileprivate init(index: SubscriptionIndex, inner: Inner) { + fileprivate init(index: SubscriptionIndex, inner: Outer) { self.index = index self.inner = inner } diff --git a/Tests/OpenCombineTests/PublisherTests/FlatMapTests.swift b/Tests/OpenCombineTests/PublisherTests/FlatMapTests.swift index aae8e06bf..f07dcb896 100644 --- a/Tests/OpenCombineTests/PublisherTests/FlatMapTests.swift +++ b/Tests/OpenCombineTests/PublisherTests/FlatMapTests.swift @@ -643,6 +643,31 @@ final class FlatMapTests: XCTestCase { XCTAssertEqual(childSubscription2.history, [.requested(.unlimited)]) } + func testCrashesWhenUpstreamFailsDuringChildCancellation() { + let helper = OperatorTestHelper( + publisherType: CustomPublisherBase.self, + initialDemand: .unlimited, + receiveValueDemand: .none, + createSut: { $0.flatMap { $0 } } + ) + + let childSubscription = CustomSubscription() + let child = CustomPublisher(subscription: childSubscription) + + var counter = 0 + childSubscription.onCancel = { + if counter >= 5 { return } + counter += 1 + helper.publisher.send(completion: .failure(.oops)) + } + + XCTAssertEqual(helper.publisher.send(child), .none) + + assertCrashes { + helper.publisher.send(completion: .failure(.oops)) + } + } + func testDoesNotCompleteWithBufferedValues() { let upstreamPublisher = PassthroughSubject() From 1b31094b945ecad375dab98d9ff82edb1da27ce8 Mon Sep 17 00:00:00 2001 From: Nomo Nomad <58949287+nomo-nomad@users.noreply.github.com> Date: Fri, 11 Dec 2020 08:41:20 -0500 Subject: [PATCH 3/7] Update README.md (#199) --- README.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index dd9606671..dbcbef919 100644 --- a/README.md +++ b/README.md @@ -24,9 +24,14 @@ dependencies: [ .package(url: "https://github.com/OpenCombine/OpenCombine.git", from: "0.11.0") ], targets: [ - .target(name: "MyAwesomePackage", dependencies: ["OpenCombine", - "OpenCombineDispatch", - "OpenCombineFoundation"]) + .target( + name: "MyAwesomePackage", + dependencies: [ + "OpenCombine", + .product(name: "OpenCombineFoundation", package: "OpenCombine"), + .product(name: "OpenCombineDispatch", package: "OpenCombine") + ] + ), ] ``` From 7d6808721cfcddc296cd14de54f8979929e8ec07 Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Sun, 8 Nov 2020 11:57:34 +0900 Subject: [PATCH 4/7] Implementation for ObservableObject with Mirror --- Sources/OpenCombine/ObservableObject.swift | 57 +++++++--- Sources/OpenCombine/Published.swift | 15 ++- .../ObservableObjectTests.swift | 106 ++++++++++++++++++ 3 files changed, 160 insertions(+), 18 deletions(-) create mode 100644 Tests/OpenCombineTests/ObservableObjectTests.swift diff --git a/Sources/OpenCombine/ObservableObject.swift b/Sources/OpenCombine/ObservableObject.swift index e6195cae0..d8a1d83c1 100644 --- a/Sources/OpenCombine/ObservableObject.swift +++ b/Sources/OpenCombine/ObservableObject.swift @@ -42,26 +42,55 @@ public protocol ObservableObject: AnyObject { var objectWillChange: ObjectWillChangePublisher { get } } -extension ObservableObject where ObjectWillChangePublisher == ObservableObjectPublisher { - // swiftlint:disable let_var_whitespace +private protocol _ObservableObjectProperty { + var objectWillChange: ObservableObjectPublisher? { get nonmutating set } +} + #if swift(>=5.1) +extension Published: _ObservableObjectProperty {} + +extension ObservableObject where ObjectWillChangePublisher == ObservableObjectPublisher { + /// A publisher that emits before the object has changed. - @available(*, unavailable, message: """ - The default implementation of objectWillChange is not available yet. \ - It's being worked on in \ - https://github.com/broadwaylamb/OpenCombine/pull/97 - """) - public var objectWillChange: ObservableObjectPublisher { - fatalError("unimplemented") - } -#else public var objectWillChange: ObservableObjectPublisher { - return ObservableObjectPublisher() + var installedPublisher: ObservableObjectPublisher? + let reflection = Mirror(reflecting: self) + for (_, property) in reflection.children { + guard let property = property as? _ObservableObjectProperty else { + // Visit other fields until we meet a @Published field + continue + } + + // Now we know that the field is @Published. + if let alreadyInstalledPublisher = property.objectWillChange { + installedPublisher = alreadyInstalledPublisher + // Don't visit other fields, as all @Published fields + // already have a publisher installed. + break + } + + // Okay, this field doesn't have a publisher installed. + // This means that other fields don't have it either + // (because we install it only once and fields can't be added at runtime). + var lazilyCreatedPublisher: ObjectWillChangePublisher { + if let publisher = installedPublisher { + return publisher + } + let publisher = ObservableObjectPublisher() + installedPublisher = publisher + return publisher + } + + property.objectWillChange = lazilyCreatedPublisher + + // Continue visiting other fields. + } + return installedPublisher! } -#endif - // swiftlint:enable let_var_whitespace } +#endif + /// A publisher that publishes changes from observable objects. public final class ObservableObjectPublisher: Publisher { diff --git a/Sources/OpenCombine/Published.swift b/Sources/OpenCombine/Published.swift index b2e474eaf..a9300c490 100644 --- a/Sources/OpenCombine/Published.swift +++ b/Sources/OpenCombine/Published.swift @@ -107,8 +107,15 @@ public struct Published { case value(Value) case publisher(Publisher) } + @propertyWrapper + private class Box { + var wrappedValue: Storage + init(wrappedValue: Storage) { + self.wrappedValue = wrappedValue + } + } - private var storage: Storage + @Box private var storage: Storage internal var objectWillChange: ObservableObjectPublisher? { get { @@ -119,7 +126,7 @@ public struct Published { return publisher.subject.objectWillChange } } - set { + nonmutating set { projectedValue.subject.objectWillChange = newValue } } @@ -145,14 +152,14 @@ public struct Published { /// /// - Parameter initialValue: The publisher's initial value. public init(wrappedValue: Value) { - storage = .value(wrappedValue) + _storage = Box(wrappedValue: .value(wrappedValue)) } /// The property for which this instance exposes a publisher. /// /// The `projectedValue` is the property accessed with the `$` operator. public var projectedValue: Publisher { - mutating get { + get { switch storage { case .value(let value): let publisher = Publisher(value) diff --git a/Tests/OpenCombineTests/ObservableObjectTests.swift b/Tests/OpenCombineTests/ObservableObjectTests.swift new file mode 100644 index 000000000..fa3f8a3d2 --- /dev/null +++ b/Tests/OpenCombineTests/ObservableObjectTests.swift @@ -0,0 +1,106 @@ +// +// ObservableObjectTests.swift +// +// +// Created by kateinoigakukun on 2020/12/22. +// + +import XCTest + +#if swift(>=5.1) + +#if OPENCOMBINE_COMPATIBILITY_TEST +import Combine + +@available(macOS 10.15, iOS 13.0, *) +private typealias Published = Combine.Published + +@available(macOS 10.15, iOS 13.0, *) +private typealias ObservableObject = Combine.ObservableObject +#else +import OpenCombine + +private typealias Published = OpenCombine.Published + +private typealias ObservableObject = OpenCombine.ObservableObject +#endif + +@available(macOS 10.15, iOS 13.0, *) +final class ObservableObjectTests: XCTestCase { + + func testBasicBehavior() { + let testObject = TestObject() + var downstreamSubscription1: Subscription? + let tracking1 = TrackingSubscriberBase( + receiveSubscription: { downstreamSubscription1 = $0 } + ) + + testObject.objectWillChange.subscribe(tracking1) + tracking1.assertHistoryEqual([.subscription("ObservableObjectPublisher")]) + downstreamSubscription1?.request(.max(2)) + tracking1.assertHistoryEqual([.subscription("ObservableObjectPublisher")]) + testObject.state1 += 1 + testObject.state1 += 2 + testObject.state1 += 3 + tracking1.assertHistoryEqual([.subscription("ObservableObjectPublisher"), + .signal, + .signal, + .signal]) + testObject.state2 += 1 + tracking1.assertHistoryEqual([.subscription("ObservableObjectPublisher"), + .signal, + .signal, + .signal, + .signal]) + downstreamSubscription1?.request(.max(10)) + tracking1.assertHistoryEqual([.subscription("ObservableObjectPublisher"), + .signal, + .signal, + .signal, + .signal]) + + let tracking2 = TrackingSubscriberBase( + receiveSubscription: { $0.request(.unlimited) } + ) + testObject.objectWillChange.subscribe(tracking2) + tracking2.assertHistoryEqual([.subscription("ObservableObjectPublisher")]) + + testObject.state1 = 42 + tracking1.assertHistoryEqual([.subscription("ObservableObjectPublisher"), + .signal, + .signal, + .signal, + .signal, + .signal]) + tracking2.assertHistoryEqual([.subscription("ObservableObjectPublisher"), + .signal]) + + downstreamSubscription1?.cancel() + testObject.state1 = -1 + + tracking1.assertHistoryEqual([.subscription("ObservableObjectPublisher"), + .signal, + .signal, + .signal, + .signal, + .signal]) + tracking2.assertHistoryEqual([.subscription("ObservableObjectPublisher"), + .value(()), + .value(())]) + } +} + +@available(macOS 10.15, iOS 13.0, *) +private final class TestObject: ObservableObject { + @Published var state1: Int + @Published var state2: Int + var nonPublished: Int + + init(_ initialValue: Int = 0) { + _state1 = Published(initialValue: initialValue) + _state2 = Published(initialValue: initialValue) + nonPublished = initialValue + } +} + +#endif From 326c12f43bbc9572f8e62a6064af454bd682679d Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Tue, 22 Dec 2020 16:40:21 +0900 Subject: [PATCH 5/7] Address swiftlint warning --- Sources/OpenCombine/Published.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/OpenCombine/Published.swift b/Sources/OpenCombine/Published.swift index a9300c490..8b4002282 100644 --- a/Sources/OpenCombine/Published.swift +++ b/Sources/OpenCombine/Published.swift @@ -110,6 +110,7 @@ public struct Published { @propertyWrapper private class Box { var wrappedValue: Storage + init(wrappedValue: Storage) { self.wrappedValue = wrappedValue } From 0bea23e6d6dca2a75a04015badbc0c4e0c8b4715 Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Tue, 22 Dec 2020 22:50:34 +0900 Subject: [PATCH 6/7] Added ObservableObject test suites picked from original PR PR: https://github.com/OpenCombine/OpenCombine/pull/97 --- .../Helpers/AssertCrashes.swift | 9 + .../ObservableObjectTests.swift | 317 ++++++++++++++++++ 2 files changed, 326 insertions(+) diff --git a/Tests/OpenCombineTests/Helpers/AssertCrashes.swift b/Tests/OpenCombineTests/Helpers/AssertCrashes.swift index 7a67be4c8..66d1e6ff3 100644 --- a/Tests/OpenCombineTests/Helpers/AssertCrashes.swift +++ b/Tests/OpenCombineTests/Helpers/AssertCrashes.swift @@ -84,4 +84,13 @@ extension XCTest { } #endif // !Xcode && !os(iOS) && !os(watchOS) && !os(tvOS) && !WASI } + + @available(macOS 10.13, iOS 8.0, *) + func assertCrashesOnDarwin(within body: () -> Void) { +#if canImport(Darwin) && OPENCOMBINE_COMPATIBILITY_TEST + assertCrashes(within: body) +#else + body() +#endif + } } diff --git a/Tests/OpenCombineTests/ObservableObjectTests.swift b/Tests/OpenCombineTests/ObservableObjectTests.swift index fa3f8a3d2..d06ad0c1d 100644 --- a/Tests/OpenCombineTests/ObservableObjectTests.swift +++ b/Tests/OpenCombineTests/ObservableObjectTests.swift @@ -27,6 +27,12 @@ private typealias ObservableObject = OpenCombine.ObservableObject @available(macOS 10.15, iOS 13.0, *) final class ObservableObjectTests: XCTestCase { + var disposeBag = [AnyCancellable]() + + override func tearDown() { + disposeBag = [] + super.tearDown() + } func testBasicBehavior() { let testObject = TestObject() @@ -88,6 +94,317 @@ final class ObservableObjectTests: XCTestCase { .value(()), .value(())]) } + + func testNoFields() { + let observableObject = NoFields() + _ = observableObject.objectWillChange + } + + func testNoPublishedFields() { + let observableObject = NoPublishedFields() + _ = observableObject.objectWillChange + } + + func testPublishedFieldIsConstant() { + let observableObject = PublishedFieldIsConstant() + + let publisher1 = observableObject.objectWillChange + let publisher2 = observableObject.objectWillChange + + XCTAssert(publisher1 === publisher2, + """ + Even if the Published field is a constant, a publisher \ + should be installed there. + """) + } + + func testDerivedClassWithPublishedField() { + let observableObject = ObservedDerivedWithObservedBase() + + var counter = 0 + + observableObject.objectWillChange.sink { + counter += 1 + }.store(in: &disposeBag) + + XCTAssertEqual(observableObject.publishedValue0, 0) + XCTAssertEqual(observableObject.simpleValue, "what") + XCTAssertEqual(observableObject.subclassPublished0, 0) + XCTAssertEqual(observableObject.subclassPublished1, 1) + XCTAssertEqual(observableObject.subclassPublished2, 2) + + observableObject.publishedValue0 += 5 + + XCTAssertEqual(counter, 1) + XCTAssertEqual(observableObject.publishedValue0, 5) + + Published[_enclosingInstance: observableObject, + wrapped: \.simpleValue, + storage: \.publishedValue1] += "???" + + XCTAssertEqual(counter, 2) + XCTAssertEqual(observableObject.simpleValue, "what") + + observableObject.subclassPublished0 += 3 + + XCTAssertEqual(counter, 3) + XCTAssertEqual(observableObject.subclassPublished0, 3) + + observableObject.subclassPublished1 += 3 + + XCTAssertEqual(counter, 4) + XCTAssertEqual(observableObject.subclassPublished1, 4) + + observableObject.subclassPublished2 += 3 + + XCTAssertEqual(counter, 5) + XCTAssertEqual(observableObject.subclassPublished1, 4) + } + + func testObjCClassSubclass() { + let observableObject = ObjCClassSubclass() + let publisher1 = observableObject.objectWillChange + let publisher2 = observableObject.objectWillChange + XCTAssert(publisher1 === publisher2) + } + + func testResilientClassSubclass() { + let observableObject = ResilientClassSubclass() + let publisher1 = observableObject.objectWillChange + let publisher2 = observableObject.objectWillChange + + XCTAssert(publisher1 === publisher2) + } + + func testResilientClassSubclass2() { + let observableObject = ResilientClassSubclass2() + let publisher1 = observableObject.objectWillChange + let publisher2 = observableObject.objectWillChange + + XCTAssert(publisher1 === publisher2) + } + + func testGenericClass() { + let observableObject = GenericClass(123, true) + + var counter = 0 + + observableObject.objectWillChange.sink { counter += 1 }.store(in: &disposeBag) + XCTAssertEqual(counter, 0) + XCTAssertEqual(observableObject.value1, 123) + XCTAssertEqual(observableObject.value2, true) + + observableObject.value1 += 1 + + XCTAssertEqual(counter, 1) + XCTAssertEqual(observableObject.value1, 124) + + observableObject.value2.toggle() + + XCTAssertEqual(counter, 2) + XCTAssertEqual(observableObject.value2, false) + } + + func testGenericSubclassOfResilientClass() { + let observableObject = ResilientClassGenericSubclass("hello", true) + + var counter = 0 + + // A bug in Combine (FB7471594). It should not crash. Why would it crash? + assertCrashesOnDarwin { + observableObject.objectWillChange.sink { counter += 1 }.store(in: &disposeBag) + XCTAssertEqual(counter, 0) + XCTAssertEqual(observableObject.value1, "hello") + XCTAssertEqual(observableObject.value2, true) + + observableObject.value1 += "!" + + XCTAssertEqual(counter, 1) + XCTAssertEqual(observableObject.value1, "hello!") + + observableObject.value2.toggle() + + XCTAssertEqual(counter, 2) + XCTAssertEqual(observableObject.value2, false) + } + } + + func testGenericSubclassOfResilientClass2() { + let observableObject = ResilientClassGenericSubclass2("hello", true) + + var counter = 0 + + // A bug in Combine (FB7471594). It should not crash. Why would it crash? + assertCrashesOnDarwin { + observableObject.objectWillChange.sink { counter += 1 }.store(in: &disposeBag) + XCTAssertEqual(counter, 0) + XCTAssertEqual(observableObject.value1, "hello") + XCTAssertEqual(observableObject.value2, true) + + observableObject.value1 += "!" + + XCTAssertEqual(counter, 1) + XCTAssertEqual(observableObject.value1, "hello!") + + observableObject.value2.toggle() + + XCTAssertEqual(counter, 2) + XCTAssertEqual(observableObject.value2, false) + + observableObject.value3.toggle() + + XCTAssertEqual(counter, 3) + XCTAssertEqual(observableObject.value3, true) + } + } + + func testObservableDerivedWithNonObservableBase() { + let observableObject = ObservedDerivedWithNonObservedBase() + var counter = 0 + observableObject.objectWillChange.sink { counter += 1 }.store(in: &disposeBag) + + XCTAssertEqual(counter, 0) + XCTAssertEqual(observableObject.nonObservedBaseValue0, 10) + XCTAssertEqual(observableObject.nonObservedBaseValue1, .pi) + XCTAssertEqual(observableObject.observedDerivedValue2, + "Asuka is obviously the best girl.") + XCTAssertEqual(observableObject.observedDerivedValue3, 255) + + observableObject.nonObservedBaseValue0 -= 1 + XCTAssertEqual(counter, 1) + XCTAssertEqual(observableObject.nonObservedBaseValue0, 9) + + observableObject.nonObservedBaseValue1 *= 2 + XCTAssertEqual(counter, 2) + XCTAssertEqual(observableObject.nonObservedBaseValue1, 2 * .pi) + + observableObject.observedDerivedValue2 = "Nevermind." + XCTAssertEqual(counter, 3) + XCTAssertEqual(observableObject.observedDerivedValue2, "Nevermind.") + + observableObject.observedDerivedValue3 &+= 1 + XCTAssertEqual(counter, 4) + XCTAssertEqual(observableObject.observedDerivedValue3, 0) + } + + func testNSObjectSubclass() { + let observableObject = NSObjectSubclass() + var counter = 0 + observableObject.objectWillChange.sink { counter += 1 }.store(in: &disposeBag) + + XCTAssertEqual(counter, 0) + XCTAssertEqual(observableObject.value0, 0) + XCTAssertEqual(observableObject.value1, 42) + + observableObject.value0 += 1 + XCTAssertEqual(counter, 1) + XCTAssertEqual(observableObject.value0, 1) + + observableObject.value1 += 1 + XCTAssertEqual(counter, 2) + XCTAssertEqual(observableObject.value1, 43) + } +} + +@available(macOS 10.15, iOS 13.0, *) +private final class NoFields: ObservableObject {} + +@available(macOS 10.15, iOS 13.0, *) +private final class NoPublishedFields: ObservableObject { + var field = NoFields() + var int = 0 +} + +@available(macOS 10.15, iOS 13.0, *) +private final class PublishedFieldIsConstant: ObservableObject { + let publishedValue = Published(initialValue: 42) +} + +@available(macOS 10.15, iOS 13.0, *) +private class ObservedBase: ObservableObject { + @Published var publishedValue0 = 0 + var publishedValue1 = Published(initialValue: "Hello!") + let publishedValue2 = Published(initialValue: 42) + var simpleValue = "what" +} + +@available(macOS 10.15, iOS 13.0, *) +private final class ObservedDerivedWithObservedBase: ObservedBase { + @Published var subclassPublished0 = 0 + @Published var subclassPublished1 = 1 + @Published var subclassPublished2 = 2 +} + +@available(macOS 10.15, iOS 13.0, *) +extension NSNumber: ObservableObject {} + +@available(macOS 10.15, iOS 13.0, *) +private final class ObjCClassSubclass: NSObject, ObservableObject { + @Published var published = 10 +} + +@available(macOS 10.15, iOS 13.0, *) +private class ResilientClassSubclass: JSONDecoder, ObservableObject { + @Published var published0 = 10 + @Published var published1 = "hello!" +} + +@available(macOS 10.15, iOS 13.0, *) +private final class ResilientClassSubclass2: ResilientClassSubclass { + @Published var published3 = true +} + +@available(macOS 10.15, iOS 13.0, *) +extension JSONEncoder: ObservableObject {} + +@available(macOS 10.15, iOS 13.0, *) +private final class GenericClass: ObservableObject { + @Published var value1: Value1 + @Published var value2: Value2 + + init(_ value1: Value1, _ value2: Value2) { + self.value1 = value1 + self.value2 = value2 + } +} + +@available(macOS 10.15, iOS 13.0, *) +private class NonObservedBase { + @Published var nonObservedBaseValue0 = 10 + @Published var nonObservedBaseValue1 = Double.pi +} + +@available(macOS 10.15, iOS 13.0, *) +private class ObservedDerivedWithNonObservedBase: NonObservedBase, ObservableObject { + @Published var observedDerivedValue2 = "Asuka is obviously the best girl." + @Published var observedDerivedValue3: UInt8 = 255 +} + +@available(macOS 10.15, iOS 13.0, *) +private class NSObjectSubclass: NSObject, ObservableObject { + @Published var value0 = 0 + @Published var value1: UInt8 = 42 +} + +@available(macOS 10.15, iOS 13.0, *) +private class ResilientClassGenericSubclass + : JSONDecoder, + ObservableObject +{ + @Published var value1: Value1 + @Published var value2: Value2 + + init(_ value1: Value1, _ value2: Value2) { + self.value1 = value1 + self.value2 = value2 + } +} + +@available(macOS 10.15, iOS 13.0, *) +private final class ResilientClassGenericSubclass2 + : ResilientClassGenericSubclass +{ + @Published var value3 = false } @available(macOS 10.15, iOS 13.0, *) From 63e4acc32fb181abb9c3dcda98fc8d396524ab83 Mon Sep 17 00:00:00 2001 From: Yuta Saito Date: Tue, 22 Dec 2020 22:51:25 +0900 Subject: [PATCH 7/7] Fix class inheritance behavior --- Sources/OpenCombine/ObservableObject.swift | 53 ++++++++++++---------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/Sources/OpenCombine/ObservableObject.swift b/Sources/OpenCombine/ObservableObject.swift index d8a1d83c1..2317bb87a 100644 --- a/Sources/OpenCombine/ObservableObject.swift +++ b/Sources/OpenCombine/ObservableObject.swift @@ -54,38 +54,41 @@ extension ObservableObject where ObjectWillChangePublisher == ObservableObjectPu /// A publisher that emits before the object has changed. public var objectWillChange: ObservableObjectPublisher { var installedPublisher: ObservableObjectPublisher? - let reflection = Mirror(reflecting: self) - for (_, property) in reflection.children { - guard let property = property as? _ObservableObjectProperty else { - // Visit other fields until we meet a @Published field - continue - } + var reflection: Mirror? = Mirror(reflecting: self) + while let aClass = reflection { + for (_, property) in aClass.children { + guard let property = property as? _ObservableObjectProperty else { + // Visit other fields until we meet a @Published field + continue + } - // Now we know that the field is @Published. - if let alreadyInstalledPublisher = property.objectWillChange { - installedPublisher = alreadyInstalledPublisher - // Don't visit other fields, as all @Published fields - // already have a publisher installed. - break - } + // Now we know that the field is @Published. + if let alreadyInstalledPublisher = property.objectWillChange { + installedPublisher = alreadyInstalledPublisher + // Don't visit other fields, as all @Published fields + // already have a publisher installed. + break + } - // Okay, this field doesn't have a publisher installed. - // This means that other fields don't have it either - // (because we install it only once and fields can't be added at runtime). - var lazilyCreatedPublisher: ObjectWillChangePublisher { - if let publisher = installedPublisher { + // Okay, this field doesn't have a publisher installed. + // This means that other fields don't have it either + // (because we install it only once and fields can't be added at runtime). + var lazilyCreatedPublisher: ObjectWillChangePublisher { + if let publisher = installedPublisher { + return publisher + } + let publisher = ObservableObjectPublisher() + installedPublisher = publisher return publisher } - let publisher = ObservableObjectPublisher() - installedPublisher = publisher - return publisher - } - property.objectWillChange = lazilyCreatedPublisher + property.objectWillChange = lazilyCreatedPublisher - // Continue visiting other fields. + // Continue visiting other fields. + } + reflection = aClass.superclassMirror } - return installedPublisher! + return installedPublisher ?? ObservableObjectPublisher() } }