[SR-6066] observe NSKeyValueObservedChange oldValue and newValue double optionals incorrectly nil when observing an optional property

[bobergj]

Previous ID	SR-6066
Radar	rdar://problem/39297127
Original Reporter	@bobergj
Type	Bug
Status	Reopened
Resolution

Attachment: Download

Environment

Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)

Additional Detail from JIRA

Votes	1
Component/s	Foundation
Labels	Bug
Assignee	None
Priority	Medium

md5: 71e8a3bdae50c0f3c476808e7f3096c9

Issue Description:

Test case:

import Foundation

print("Check what the print output of a double optional is..")
let optionalString: Optional<String> = nil
let optionalOptionalString: Optional<Optional<String>> = optionalString
print(optionalOptionalString)
print("OK, now to the actual test")


class A : NSObject {
    @objc dynamic var anOptionalString: String? = nil
}

func testObserve() {
    
    let a = A()
    let keyPath = \A.anOptionalString
    a.observe(keyPath, options: [.old, .new]) { (objectChanged, observedChange) in
        print("oldValue: \(observedChange.oldValue)")
        print("newValue: \(observedChange.newValue)")

    }
    
    a.anOptionalString = nil
    a.anOptionalString = "abc"
    a.anOptionalString = "def"
}


testObserve()

Output:

Check what the print output of a double optional is..
Optional(nil)
OK, now to the actual test
oldValue: nil
newValue: nil
oldValue: nil
newValue: Optional(Optional("abc"))
oldValue: Optional(Optional("abc"))
newValue: Optional(Optional("def"))

Expected output:

Check what the print output of a double optional is..
Optional(nil)
OK, now to the actual test
oldValue: Optional(nil)
newValue: Optional(nil)
oldValue: Optional(nil)
newValue: Optional(Optional("abc"))
oldValue: Optional(Optional("abc"))
newValue: Optional(Optional("def"))

In https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/NSObject.swift

public struct NSKeyValueObservedChange<Value> {
  ///newValue and oldValue will only be non-nil if .new/.old is passed to   `observe()`. In general, get the most up to date value by accessing it directly on the observed object instead.
  public let newValue: Value?
  public let oldValue: Value?
  ...
}

Since we are passing the options:

[.old, .new]

above, the expectation is that newValue and oldValue aren't nil.

I got bit by this when using observe in a generic context and using a guard statement to bind newValue and oldValue in the beginning of the observe callback closure.

[belkadan]

cc @jckarter

[jckarter]

This would be @Catfish-Man's wheelhouse.

[tkrajacic]

Any progress on this?

[belkadan]

Not sure if there's a Radar specifically for this issue.

@swift-ci create

[tkrajacic]

This is still broken in Xcode 10 beta and current Swift 4.2 snapshot.

This is a big nuisance since it prevents us from observing optionals. 🙁

extension NSObjectProtocol where Self: NSObject {
    func observe<Value>(_ keyPath: ReferenceWritableKeyPath<Self, Value>, onChange: @escaping (Value) -> ()) -> Disposable {
        let observation = observe(keyPath, options: [.initial, .new]) { _, change in
            // TODO: change.newValue should never be `nil`, but when observing an optional property that's set to `nil`, then change.newValue is `nil` instead of `Optional(nil)`. This is the bug report for this: https://bugs.swift.org/browse/SR-6066
            guard let newValue = change.newValue else { return }
            onChange(newValue)
        }
        return Disposable { observation.invalidate() }
    }
    
    func bind<Value, Target>(_ sourceKeyPath: ReferenceWritableKeyPath<Self, Value>, to target: Target, at targetKeyPath: ReferenceWritableKeyPath<Target, Value>) -> Disposable {
        return observe(sourceKeyPath) { target[keyPath: targetKeyPath] = $0 }
    }
}

final class Disposable {
    let dispose: () -> ()
    init(_ dispose: @escaping () -> ()) {
        self.dispose = dispose
    }
    deinit {
        dispose()
    }
}

Is there a workaround @belkadan?

[belkadan]

For newValue you can always just ask the object for the current value, so it's not that big of a deal. Also, you could just trust the KVO system when using the .new or .old options, and assume you're never going to have nil for the new or old value except on the initial observation.

        let observation = observe(keyPath, options: [.initial, .new]) { _, change in
            // TODO: change.newValue should never be `nil`, but when observing an optional property that's set to `nil`, then change.newValue is `nil` instead of `Optional(nil)`. Flatten one level of optional to deal with this.
            let newValue = change.newValue ?? nil
            onChange(newValue)
        }

[belkadan]

(I was going to say "don't ask me; I don't work on KVO", but I guess I do have a track record of finding workarounds…)

[tkrajacic]

Well, that doesn't work. (Value of optional type 'Value?' must be unwrapped to a value of type 'Value')

I DO want to observe optional properties, so they WILL be nil. And then the newValue MUST be `Optional(nil)`

[tkrajacic]

   (I was going to say "don't ask me; I don't work on KVO", but I guess I do have a track record of finding workarounds…)

@belkadan I am sorry to bother you, but you do have a track record of being super helpful in that way. I'll write some more bug reports in return… Promise! 😉

[belkadan]

You've been good about filing bug reports, so thank you.

You should be able to play terrible casting tricks here to get what you need:

let newValue = change.newValue ?? (nil as Any? as! Value)

[tkrajacic]

I almost peed myself a little when I saw your casting workaround… This works indeed!

And you saved the day YET AGAIN! ❤️

Thank you sooo much! I tried many forms of casting but somehow always missed it 🙂

[swift-ci]

Comment by Afonso Graça (JIRA)

Unfortunately this workaround doesn't work for Swift 4.1 🙁 any other voodoo that might do the trick @belkadan?

[swift-ci]

Comment by Zhu Shengqi (JIRA)

@belkadan This bug still exists on Xcode 10.2 with Swift 5 (app store version). Although we can still get the keyPath's value by accessing the keyPath again (which incurs additional performance cost), it annoys me that I have to capture the original object in the closure (which add the unnecessary weak reference).

By the way, the bug was reported in 2017 (and is level medium). Now it's 2019, but the ticket still has no assignee. So I assume the bug won't be fixed in the near future (or just so hard to fix)? No blaming, just want to know if we could have a fix for this issue in Swift 5.2 or 6.

[swift-ci]

Comment by Zhu Shengqi (JIRA)

@belkadan I just created a pull request trying to fix this issue swiftlang/swift#24356 . Hope this would fix the case.

[swift-ci]

Comment by Zhu Shengqi (JIRA)

@bobergj afonso (JIRA User) @belkadan The fix for this issue has been merged into Master branch. See swiftlang/swift#24356 . I'm not sure whether this fix will go into swift 5.1.