Merge pull request #22297 from millenomi/revert-fix_kvo

Simulator build fix — (temporarily) revert "Merge pull request #20103 from lilyball/fix_kvo"

Author: Joe Shajrawi

stdlib/public/Darwin/Foundation/NSObject.swift

@@ -52,79 +52,55 @@ fileprivate extension NSObject {
 // must coexist, so it was renamed. The old name must not be used in the
 // new runtime.
 @objc private class __KVOKeyPathBridgeMachinery : NSObject {
-    @nonobjc static let swizzler: () = {
+    @nonobjc static var keyPathTable: [String : AnyKeyPath] = {
         /*
          Move all our methods into place. We want the following:
          __KVOKeyPathBridgeMachinery's automaticallyNotifiesObserversForKey:, and keyPathsForValuesAffectingValueForKey: methods replaces NSObject's versions of them
          NSObject's automaticallyNotifiesObserversForKey:, and keyPathsForValuesAffectingValueForKey: methods replace NSObject's __old_unswizzled_* methods
          NSObject's _old_unswizzled_* methods replace __KVOKeyPathBridgeMachinery's methods, and are never invoked
          */
-        threeWaySwizzle(#selector(NSObject.keyPathsForValuesAffectingValue(forKey:)), with: #selector(NSObject.__old_unswizzled_keyPathsForValuesAffectingValue(forKey:)))
-        threeWaySwizzle(#selector(NSObject.automaticallyNotifiesObservers(forKey:)), with: #selector(NSObject.__old_unswizzled_automaticallyNotifiesObservers(forKey:)))
-    }()
-    
-    /// Performs a 3-way swizzle between `NSObject` and `__KVOKeyPathBridgeMachinery`.
-    ///
-    /// The end result of this swizzle is the following:
-    /// * `NSObject.selector` contains the IMP from `__KVOKeyPathBridgeMachinery.selector`
-    /// * `NSObject.unswizzledSelector` contains the IMP from the original `NSObject.selector`.
-    /// * __KVOKeyPathBridgeMachinery.selector` contains the (useless) IMP from `NSObject.unswizzledSelector`.
-    ///
-    /// This swizzle is done in a manner that modifies `NSObject.selector` last, in order to ensure thread safety
-    /// (by the time `NSObject.selector` is swizzled, `NSObject.unswizzledSelector` will contain the original IMP)
-    @nonobjc private static func threeWaySwizzle(_ selector: Selector, with unswizzledSelector: Selector) {
         let rootClass: AnyClass = NSObject.self
         let bridgeClass: AnyClass = __KVOKeyPathBridgeMachinery.self
 
-        // Swap bridge.selector <-> NSObject.unswizzledSelector
-        let unswizzledMethod = class_getClassMethod(rootClass, unswizzledSelector)!
-        let bridgeMethod = class_getClassMethod(bridgeClass, selector)!
-        method_exchangeImplementations(unswizzledMethod, bridgeMethod)
+        let dependentSel = #selector(NSObject.keyPathsForValuesAffectingValue(forKey:))
+        let rootDependentImpl = class_getClassMethod(rootClass, dependentSel)!
+        let bridgeDependentImpl = class_getClassMethod(bridgeClass, dependentSel)!
+        method_exchangeImplementations(rootDependentImpl, bridgeDependentImpl) // NSObject <-> Us
 
-        // Swap NSObject.selector <-> NSObject.unswizzledSelector
-        // NSObject.unswizzledSelector at this point contains the bridge IMP
-        let rootMethod = class_getClassMethod(rootClass, selector)!
-        method_exchangeImplementations(rootMethod, unswizzledMethod)
-    }
-
-    private class BridgeKey : NSObject, NSCopying {
-        let value: String
-
-        init(_ value: String) {
-            self.value = value
-        }
-
-        func copy(with zone: NSZone? = nil) -> Any {
-            return self
-        }
-
-        override func isEqual(_ object: Any?) -> Bool {
-            return value == (object as? BridgeKey)?.value
-        }
-
-        override var hash: Int {
-            var hasher = Hasher()
-            hasher.combine(ObjectIdentifier(BridgeKey.self))
-            hasher.combine(value)
-            return hasher.finalize()
-        }
-    }
-
-    /// Temporarily maps a `String` to an `AnyKeyPath` that can be retrieved with `_bridgeKeyPath(_:)`.
-    ///
-    /// This uses a per-thread storage so key paths on other threads don't interfere.
-    @nonobjc fileprivate static func _withBridgeableKeyPath(from keyPathString: String, to keyPath: AnyKeyPath, block: () -> Void) {
-        _ = __KVOKeyPathBridgeMachinery.swizzler
-        let key = BridgeKey(keyPathString)
-        let oldValue = Thread.current.threadDictionary[key]
-        Thread.current.threadDictionary[key] = keyPath
-        defer { Thread.current.threadDictionary[key] = oldValue }
-        block()
+        let originalDependentImpl = class_getClassMethod(bridgeClass, dependentSel)! //we swizzled it onto this class, so this is actually NSObject's old implementation
+        let originalDependentSel = #selector(NSObject.__old_unswizzled_keyPathsForValuesAffectingValue(forKey:))
+        let dummyDependentImpl = class_getClassMethod(rootClass, originalDependentSel)!
+        method_exchangeImplementations(originalDependentImpl, dummyDependentImpl) // NSObject's original version <-> NSObject's __old_unswizzled_ version
+        
+        let autoSel = #selector(NSObject.automaticallyNotifiesObservers(forKey:))
+        let rootAutoImpl = class_getClassMethod(rootClass, autoSel)!
+        let bridgeAutoImpl = class_getClassMethod(bridgeClass, autoSel)!
+        method_exchangeImplementations(rootAutoImpl, bridgeAutoImpl) // NSObject <-> Us
+        
+        let originalAutoImpl = class_getClassMethod(bridgeClass, autoSel)! //we swizzled it onto this class, so this is actually NSObject's old implementation
+        let originalAutoSel = #selector(NSObject.__old_unswizzled_automaticallyNotifiesObservers(forKey:))
+        let dummyAutoImpl = class_getClassMethod(rootClass, originalAutoSel)!
+        method_exchangeImplementations(originalAutoImpl, dummyAutoImpl) // NSObject's original version <-> NSObject's __old_unswizzled_ version
+        
+        return [:]
+    }()
+    
+    @nonobjc static var keyPathTableLock = NSLock()
+    
+    @nonobjc fileprivate static func _bridgeKeyPath(_ keyPath: __owned AnyKeyPath) -> String {
+        guard let keyPathString = keyPath._kvcKeyPathString else { fatalError("Could not extract a String from KeyPath \(keyPath)") }
+        __KVOKeyPathBridgeMachinery.keyPathTableLock.lock()
+        defer { __KVOKeyPathBridgeMachinery.keyPathTableLock.unlock() }
+        __KVOKeyPathBridgeMachinery.keyPathTable[keyPathString] = keyPath
+        return keyPathString
     }
 
     @nonobjc fileprivate static func _bridgeKeyPath(_ keyPath:String?) -> AnyKeyPath? {
         guard let keyPath = keyPath else { return nil }
-        return Thread.current.threadDictionary[BridgeKey(keyPath)] as? AnyKeyPath
+        __KVOKeyPathBridgeMachinery.keyPathTableLock.lock()
+        defer { __KVOKeyPathBridgeMachinery.keyPathTableLock.unlock() }
+        let path = __KVOKeyPathBridgeMachinery.keyPathTable[keyPath]
+        return path
     }
 
     @objc override class func automaticallyNotifiesObservers(forKey key: String) -> Bool {
@@ -151,85 +127,65 @@ fileprivate extension NSObject {
 }
 
 func _bridgeKeyPathToString(_ keyPath:AnyKeyPath) -> String {
-    guard let keyPathString = keyPath._kvcKeyPathString else { fatalError("Could not extract a String from KeyPath \(keyPath)") }
-    return keyPathString
+    return __KVOKeyPathBridgeMachinery._bridgeKeyPath(keyPath)
+}
+
+func _bridgeStringToKeyPath(_ keyPath:String) -> AnyKeyPath? {
+    return __KVOKeyPathBridgeMachinery._bridgeKeyPath(keyPath)
 }
 
 // NOTE: older overlays called this NSKeyValueObservation. We now use
 // that name in the source code, but add an underscore to the runtime
 // name to avoid conflicts when both are loaded into the same process.
 @objc(_NSKeyValueObservation)
 public class NSKeyValueObservation : NSObject {
-    // We use a private helper class as the actual observer. This lets us attach the helper as an associated object
-    // to the object we're observing, thus ensuring the helper will still be alive whenever a KVO change notification
-    // is broadcast, even on a background thread.
-    //
-    // For the associated object, we use the Helper instance itself as its own key. This guarantees key uniqueness.
-    private class Helper : NSObject {
-        @nonobjc weak var object : NSObject?
-        @nonobjc let path: String
-        @nonobjc let callback : (NSObject, NSKeyValueObservedChange<Any>) -> Void
-        
-        // workaround for <rdar://problem/31640524> Erroneous (?) error when using bridging in the Foundation overlay
-        // specifically, overriding observeValue(forKeyPath:of:change:context:) complains that it's not Obj-C-compatible
-        @nonobjc static let swizzler: () = {
-            let bridgeClass: AnyClass = Helper.self
-            let observeSel = #selector(NSObject.observeValue(forKeyPath:of:change:context:))
-            let swapSel = #selector(Helper._swizzle_me_observeValue(forKeyPath:of:change:context:))
-            let swapObserveMethod = class_getInstanceMethod(bridgeClass, swapSel)!
-            class_addMethod(bridgeClass, observeSel, method_getImplementation(swapObserveMethod), method_getTypeEncoding(swapObserveMethod))
-        }()
-        
-        @nonobjc init(object: NSObject, keyPath: AnyKeyPath, options: NSKeyValueObservingOptions, callback: @escaping (NSObject, NSKeyValueObservedChange<Any>) -> Void) {
-            _ = Helper.swizzler
-            let path = _bridgeKeyPathToString(keyPath)
-            self.object = object
-            self.path = path
-            self.callback = callback
-            super.init()
-            objc_setAssociatedObject(object, associationKey(), self, .OBJC_ASSOCIATION_RETAIN)
-            __KVOKeyPathBridgeMachinery._withBridgeableKeyPath(from: path, to: keyPath) {
-                object.addObserver(self, forKeyPath: path, options: options, context: nil)
-            }
-        }
-        
-        @nonobjc func invalidate() {
-            guard let object = self.object else { return }
-            object.removeObserver(self, forKeyPath: path, context: nil)
-            objc_setAssociatedObject(object, associationKey(), nil, .OBJC_ASSOCIATION_ASSIGN)
-            self.object = nil
-        }
-        
-        @nonobjc private func associationKey() -> UnsafeRawPointer {
-            return UnsafeRawPointer(Unmanaged.passUnretained(self).toOpaque())
-        }
-        
-        @objc private func _swizzle_me_observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSString : Any]?, context: UnsafeMutableRawPointer?) {
-            guard let object = object as? NSObject, object === self.object, let change = change else { return }
-            let rawKind:UInt = change[NSKeyValueChangeKey.kindKey.rawValue as NSString] as! UInt
-            let kind = NSKeyValueChange(rawValue: rawKind)!
-            let notification = NSKeyValueObservedChange(kind: kind,
-                                                        newValue: change[NSKeyValueChangeKey.newKey.rawValue as NSString],
-                                                        oldValue: change[NSKeyValueChangeKey.oldKey.rawValue as NSString],
-                                                        indexes: change[NSKeyValueChangeKey.indexesKey.rawValue as NSString] as! IndexSet?,
-                                                        isPrior: change[NSKeyValueChangeKey.notificationIsPriorKey.rawValue as NSString] as? Bool ?? false)
-            callback(object, notification)
-        }
-    }
 
-    @nonobjc private let helper: Helper
+    @nonobjc weak var object : NSObject?
+    @nonobjc let callback : (NSObject, NSKeyValueObservedChange<Any>) -> Void
+    @nonobjc let path : String
+    
+    //workaround for <rdar://problem/31640524> Erroneous (?) error when using bridging in the Foundation overlay
+    @nonobjc static var swizzler : NSKeyValueObservation? = {
+        let bridgeClass: AnyClass = NSKeyValueObservation.self
+        let observeSel = #selector(NSObject.observeValue(forKeyPath:of:change:context:))
+        let swapSel = #selector(NSKeyValueObservation._swizzle_me_observeValue(forKeyPath:of:change:context:))
+        let rootObserveImpl = class_getInstanceMethod(bridgeClass, observeSel)!
+        let swapObserveImpl = class_getInstanceMethod(bridgeClass, swapSel)!
+        method_exchangeImplementations(rootObserveImpl, swapObserveImpl)
+        return nil
+    }()
 
-    fileprivate init(object: NSObject, keyPath: AnyKeyPath, options: NSKeyValueObservingOptions, callback: @escaping (NSObject, NSKeyValueObservedChange<Any>) -> Void) {
-        helper = Helper(object: object, keyPath: keyPath, options: options, callback: callback)
+    fileprivate init(object: NSObject, keyPath: AnyKeyPath, callback: @escaping (NSObject, NSKeyValueObservedChange<Any>) -> Void) {
+        path = _bridgeKeyPathToString(keyPath)
+        let _ = NSKeyValueObservation.swizzler
+        self.object = object
+        self.callback = callback
+    }
+    
+    fileprivate func start(_ options: NSKeyValueObservingOptions) {
+        object?.addObserver(self, forKeyPath: path, options: options, context: nil)
     }
 
     ///invalidate() will be called automatically when an NSKeyValueObservation is deinited
     @objc public func invalidate() {
-        helper.invalidate()
+        object?.removeObserver(self, forKeyPath: path, context: nil)
+        object = nil
+    }
+    
+    @objc func _swizzle_me_observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSString : Any]?, context: UnsafeMutableRawPointer?) {
+        guard let ourObject = self.object, object as? NSObject == ourObject, let change = change else { return }
+        let rawKind:UInt = change[NSKeyValueChangeKey.kindKey.rawValue as NSString] as! UInt
+        let kind = NSKeyValueChange(rawValue: rawKind)!
+        let notification = NSKeyValueObservedChange(kind: kind,
+                                                    newValue: change[NSKeyValueChangeKey.newKey.rawValue as NSString],
+                                                    oldValue: change[NSKeyValueChangeKey.oldKey.rawValue as NSString],
+                                                    indexes: change[NSKeyValueChangeKey.indexesKey.rawValue as NSString] as! IndexSet?,
+                                                    isPrior: change[NSKeyValueChangeKey.notificationIsPriorKey.rawValue as NSString] as? Bool ?? false)
+        callback(ourObject, notification)
     }
 
     deinit {
-        invalidate()
+        object?.removeObserver(self, forKeyPath: path, context: nil)
     }
 }
 
@@ -241,14 +197,16 @@ extension _KeyValueCodingAndObserving {
             options: NSKeyValueObservingOptions = [],
             changeHandler: @escaping (Self, NSKeyValueObservedChange<Value>) -> Void)
         -> NSKeyValueObservation {
-        return NSKeyValueObservation(object: self as! NSObject, keyPath: keyPath, options: options) { (obj, change) in
+        let result = NSKeyValueObservation(object: self as! NSObject, keyPath: keyPath) { (obj, change) in
             let notification = NSKeyValueObservedChange(kind: change.kind,
                                                         newValue: change.newValue as? Value,
                                                         oldValue: change.oldValue as? Value,
                                                         indexes: change.indexes,
                                                         isPrior: change.isPrior)
             changeHandler(obj as! Self, notification)
         }
+        result.start(options)
+        return result
     }
 
     public func willChangeValue<Value>(for keyPath: __owned KeyPath<Self, Value>) {

stdlib/public/Darwin/Foundation/NSSet.swift

@@ -17,7 +17,7 @@ extension Set {
   ///
   /// The provided `NSSet` will be copied to ensure that the copy can
   /// not be mutated by other code.
-  fileprivate init(_cocoaSet: __shared NSSet) {
+  fileprivate init(_cocoaSet: __shared AnyObject) {
     assert(_isBridgedVerbatimToObjectiveC(Element.self),
       "Set can be backed by NSSet _variantStorage only when the member type can be bridged verbatim to Objective-C")
     // FIXME: We would like to call CFSetCreateCopy() to avoid doing an

stdlib/public/Darwin/Foundation/NSSortDescriptor.swift

@@ -11,22 +11,18 @@
 //===----------------------------------------------------------------------===//
 
 @_exported import Foundation // Clang module
-import ObjectiveC
 
 extension NSSortDescriptor {
     public convenience init<Root, Value>(keyPath: KeyPath<Root, Value>, ascending: Bool) {
         self.init(key: _bridgeKeyPathToString(keyPath), ascending: ascending)
-        objc_setAssociatedObject(self, UnsafeRawPointer(&associationKey), keyPath, .OBJC_ASSOCIATION_RETAIN)
     }
 
     public convenience init<Root, Value>(keyPath: KeyPath<Root, Value>, ascending: Bool, comparator cmptr: @escaping Foundation.Comparator) {
         self.init(key: _bridgeKeyPathToString(keyPath), ascending: ascending, comparator: cmptr)
-        objc_setAssociatedObject(self, UnsafeRawPointer(&associationKey), keyPath, .OBJC_ASSOCIATION_RETAIN)
     }
 
     public var keyPath: AnyKeyPath? {
-        return objc_getAssociatedObject(self, UnsafeRawPointer(&associationKey)) as? AnyKeyPath
+        guard let key = self.key else { return nil }
+        return _bridgeStringToKeyPath(key)
     }
 }
-
-private var associationKey: ()?

test/stdlib/KVOKeyPaths.swift

@@ -108,68 +108,7 @@ print("target removed")
 // CHECK-NEXT: swiftValue 42, objcValue three
 // CHECK-NEXT: swiftValue 42, objcValue four
 // CHECK-NEXT: swiftValue 13, objcValue four
-// The next 2 logs are actually a bug and shouldn't happen
 // CHECK-NEXT: swiftValue 13, objcValue four
 // CHECK-NEXT: swiftValue 13, objcValue four
 // CHECK-NEXT: target removed
 
-//===----------------------------------------------------------------------===//
-// Test NSKeyValueObservingCustomization issue with observing from the callbacks
-//===----------------------------------------------------------------------===//
-
-class Target2 : NSObject, NSKeyValueObservingCustomization {
-    @objc dynamic var name: String?
-
-    class Dummy : NSObject {
-        @objc dynamic var name: String?
-    }
-
-    // In both of the callbacks, observe another property with the same key path.
-    // We do it in both because we're not sure which callback is invoked first.
-    // This ensures that using KVO with key paths from one callback doesn't interfere
-    // with the ability to look up the key path using the other.
-    static func keyPathsAffectingValue(for key: AnyKeyPath) -> Set<AnyKeyPath> {
-        print("keyPathsAffectingValue: key == \\.name:", key == \Target2.name)
-        _ = Dummy().observe(\.name) { (_, _) in }
-        return []
-    }
-
-    static func automaticallyNotifiesObservers(for key: AnyKeyPath) -> Bool {
-        print("automaticallyNotifiesObservers: key == \\.name:", key == \Target2.name)
-        _ = Dummy().observe(\.name) { (_, _) in }
-        return true
-    }
-}
-
-print("registering observer for Target2")
-withExtendedLifetime(Target2()) { (target) in
-    _ = target.observe(\.name) { (_, _) in }
-}
-print("observer removed")
-
-// CHECK-LABEL: registering observer for Target2
-// CHECK-DAG: keyPathsAffectingValue: key == \.name: true
-// CHECK-DAG: automaticallyNotifiesObservers: key == \.name: true
-// CHECK-NEXT: observer removed
-
-//===----------------------------------------------------------------------===//
-// Test NSSortDescriptor keyPath support
-//===----------------------------------------------------------------------===//
-
-// This one doesn't really match the context of "KVO KeyPaths" but it's close enough
-
-class Sortable1 : NSObject {
-    @objc var name: String?
-}
-
-class Sortable2 : NSObject {
-    @objc var name: String?
-}
-
-print("creating NSSortDescriptor")
-let descriptor = NSSortDescriptor(keyPath: \Sortable1.name, ascending: true)
-_ = NSSortDescriptor(keyPath: \Sortable2.name, ascending: true)
-print("keyPath == \\Sortable1.name:", descriptor.keyPath == \Sortable1.name)
-
-// CHECK-LABEL: creating NSSortDescriptor
-// CHECK-NEXT: keyPath == \Sortable1.name: true