Skip to content

Fix swift KVO when keyPath having a optional value #24356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 25, 2019
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
36 changes: 34 additions & 2 deletions stdlib/public/Darwin/Foundation/NSObject.swift
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,17 @@ public class NSKeyValueObservation : NSObject {
}
}

// Used for type-erase Optional type
private protocol _OptionalForKVO {
static func _castForKVO(_ value: Any) -> Any?
}

extension Optional: _OptionalForKVO {
static func _castForKVO(_ value: Any) -> Any? {
return value as? Wrapped
}
}

extension _KeyValueCodingAndObserving {

///when the returned NSKeyValueObservation is deinited or invalidated, it will stop observing
Expand All @@ -242,9 +253,30 @@ extension _KeyValueCodingAndObserving {
changeHandler: @escaping (Self, NSKeyValueObservedChange<Value>) -> Void)
-> NSKeyValueObservation {
return NSKeyValueObservation(object: self as! NSObject, keyPath: keyPath, options: options) { (obj, change) in

let converter = { (changeValue: Any?) -> Value? in
if let optionalType = Value.self as? _OptionalForKVO.Type {
// Special logic for keyPath having a optional target value. When the keyPath referencing a nil value, the newValue/oldValue should be in the form .some(nil) instead of .none
// Solve https://bugs.swift.org/browse/SR-6066

// NSNull is used by KVO to signal that the keyPath value is nil.
// If Value == Optional<T>.self, We will get nil instead of .some(nil) when casting Optional(<null>) directly.
// To fix this behavior, we will eliminate NSNull first, then cast the transformed value.

if let unwrapped = changeValue {
// We use _castForKVO to cast first.
// If Value != Optional<NSNull>.self, the NSNull value will be eliminated.
let nullEliminatedValue = optionalType._castForKVO(unwrapped) as Any
let transformedOptional: Any? = nullEliminatedValue
return transformedOptional as? Value
}
}
return changeValue as? Value
}

let notification = NSKeyValueObservedChange(kind: change.kind,
newValue: change.newValue as? Value,
oldValue: change.oldValue as? Value,
newValue: converter(change.newValue),
oldValue: converter(change.oldValue),
indexes: change.indexes,
isPrior: change.isPrior)
changeHandler(obj as! Self, notification)
Expand Down
27 changes: 27 additions & 0 deletions test/stdlib/KVOKeyPaths.swift
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,30 @@ print("keyPath == \\Sortable1.name:", descriptor.keyPath == \Sortable1.name)

// CHECK-51-LABEL: creating NSSortDescriptor
// CHECK-51-NEXT: keyPath == \Sortable1.name: true

//===----------------------------------------------------------------------===//
// Test keyPath with optional value has correct oldValue/newValue behavior
//===----------------------------------------------------------------------===//

class TestClassForOptionalKeyPath : NSObject {

// Should not use NSObject? as object type
@objc dynamic var optionalObject: String?

}

let testObjectForOptionalKeyPath = TestClassForOptionalKeyPath()

print("observe keyPath with optional value")

let optionalKeyPathObserver = testObjectForOptionalKeyPath.observe(\.optionalObject, options: [.initial, .old, .new]) { (_, change) in
Swift.print("oldValue = \(change.oldValue as String??), newValue = \(change.newValue as String??)")
}

testObjectForOptionalKeyPath.optionalObject = nil
testObjectForOptionalKeyPath.optionalObject = "foo"

// CHECK-LABEL: observe keyPath with optional value
// CHECK-NEXT: oldValue = Optional(nil), newValue = Optional(nil)
// CHECK-NEXT: oldValue = Optional(nil), newValue = Optional(nil)
// CHECK-NEXT: oldValue = Optional(nil), newValue = Optional(Optional("foo"))