[SR-9081] NSKeyValueObservingCustomization fundamentally broken, should be removed

[lilyball]

Previous ID	SR-9081
Radar	rdar://problem/45567572
Original Reporter	@lilyball
Type	Bug

Environment

Tested in Apple Swift version 4.2 (swiftlang-1000.11.37.1 clang-1000.11.45.1)
Confirmed with visual inspection of latest Swift master as of yesterday (14d4235b571d7a4e2ae51854f0d665f96959d182)

Additional Detail from JIRA

Votes	0
Component/s	Foundation
Labels	Bug
Assignee	@millenomi
Priority	Medium

md5: c351b51b1cd966d932da6c16f7b7d333

relates to:

• SR-9077 NSKeyValueObservingCustomization is broken with multiple threads

Issue Description:

The NSKeyValueObservingCustomization protocol is fundamentally broken and I believe it cannot be fixed. We should remove it.

The breakage is the fact that implementing it relies on being able to do equality testing on key paths, but equality testing on key paths doesn't work in the face of subclassing. By that I mean given the following code

class Foo: NSObject {
    @objc dynamic var name: String?
}
class Bar: Foo {}

The expression \Foo.name == \Bar.name returns false even though Bar is just inheriting its property from Foo. This means that an implementation of NSKeyValueObservingCustomization cannot possibly work properly on anything besides a non-final class. Even if keypath construction in this instance were changed such that \Bar.name returns the same thing as \Foo.name, the same cannot be done for the more complicated case

class Foo: NSObject {
    @objc dynamic var name: String? { get { ... } }
}
class Bar: NSObject {
    override var name: String? {
        get { ... }
        set { ... }
    }
}

Even though this code doesn't change the type of the property, it does change the type of the KeyPath and thus \Bar.name == \Foo.name cannot be true.

Here's some sample code that demonstrates this issue using NSKeyValueObservingCustomization:

import Foundation

class Foo: NSObject, NSKeyValueObservingCustomization {
    @objc dynamic var length: Int {
        return end - start
    }

    @objc dynamic var start: Int
    @objc dynamic var end: Int

    init(_ range: Range<Int>) {
        start = range.lowerBound
        end = range.upperBound
    }

    static func keyPathsAffectingValue(for key: AnyKeyPath) -> Set<AnyKeyPath> {
        return key == \Foo.length ? [\Foo.start, \Foo.end] : []
    }

    static func automaticallyNotifiesObservers(for key: AnyKeyPath) -> Bool {
        return true
    }
}

class Bar: Foo {}
class Baz: Foo {}

func testFooLength<T: Foo>(_ foo: T) {
    let token = foo.observe(\.length, options: [.old, .new]) { (foo, change) in
        print("length: \(change.oldValue!) -> \(change.newValue!)")
    }
    withExtendedLifetime(token) {
        foo.end = 20
    }
}

print("Testing observation on root type Foo")
testFooLength(Foo(0..<10)) // this works
print("Testing observation on subclass type Bar")
testFooLength(Bar(0..<10)) // this does not work
print("Testing observation on subclass type Baz treated statically as root type Foo")
testFooLength(Baz(0..<10) as Foo) // this works

The output of this code is

Testing observation on root type Foo
length: 10 -> 20
Testing observation on subclass type Bar
Testing observation on subclass type Baz treated statically as root type Foo
length: 10 -> 20

Notice how the functionality of the observation depends on whether the observing code is statically treating the object as a Foo or as a subclass thereof.

As near as I can tell, the only way to fix this without removing the protocol is to redefine AnyKeyPath.== to do a comparison on the _kvcKeyPathString (assuming the key paths even have them), but this is not an acceptable fix as it breaks the notion of keypath equality for all other uses.

Given how fundamentally broken this protocol is, I believe we should actually take the step of marking it unavailable (rather than merely deprecated).

[lilyball]

CC @millenomi

[millenomi]

@swift-ci create

[lilyball]

I just had a thought for a potential partial solution. We could introduce a new AnyKeyPath subclass (one which always returns nil from the append methods) that is explicitly created with an Obj-C key path and implements equality such that it just compares Obj-C key paths. This way we can ditch the table entirely and provide these new key paths to the protocol methods and equality will work. This isn’t a great solution as appending won’t work, and as the new key path can’t be substituted for the original despite comparing as equal. So I’d still recommend removing the protocol entirely and encouraging people to just implement the original Obj-C one using #keyParh.

[lilyball]

Also I said this in SR-9077 but the protocol as it exists is also broken if someone uses the Obj-C observation method instead of the Swift KeyPath one as the global table will never be populated to begin with this way.

[lilyball]

Another way in which this protocol is broken: if a superclass overrides automaticallyNotifiesObservers(forKey:) in order to change the default value to false instead of true, this will prevent the NSKeyValueObservingCustomization method from ever firing (as the superclass won't ever invoke NSObject's implementation).

[lilyball]

Sample code that demonstrates the issue with NSKeyValueObservingCustomization methods never firing if the KVO observation was done with the Obj-C method instead of the KeyPath variant:

import Foundation

class Foo: NSObject, NSKeyValueObservingCustomization {
    @objc dynamic var name: String?

    static var invokedAutomaticallyNotifiesObservers: Bool = false

    class func keyPathsAffectingValue(for key: AnyKeyPath) -> Set<AnyKeyPath> {
        return []
    }

    class func automaticallyNotifiesObservers(for key: AnyKeyPath) -> Bool {
        invokedAutomaticallyNotifiesObservers = true
        return true
    }
}

class Bar: NSObject {
    override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey: Any]?, context: UnsafeMutableRawPointer?) {
        print("KVO: \(object!).\(keyPath!): \(change!)")
    }
}

let foo = Foo()
let bar = Bar()
foo.addObserver(bar, forKeyPath: #keyPath(Foo.name), context: nil)
foo.name = "wat"
assert(Foo.invokedAutomaticallyNotifiesObservers, "NSKeyValueObservingCustomization methods did not fire")

[lilyball]

Oh boy, an even easier way to break this protocol, without invoking any Obj-C other other weirdness: actually use key paths instead of just keys. If you use a full key path, the individual components of the path don't get mapped, only the full path does. So creating an observation using an actual path will not invoke the NSKeyValueObservingCustomization methods.

Sample code:

import Foundation

class Foo: NSObject, NSKeyValueObservingCustomization {
    @objc dynamic var name: String?

    static func automaticallyNotifiesObservers(for key: AnyKeyPath) -> Bool {
        print(#function, key)
        return true
    }

    static func keyPathsAffectingValue(for key: AnyKeyPath) -> Set<AnyKeyPath> {
        print(#function, key)
        return []
    }
}

class Bar: NSObject, NSKeyValueObservingCustomization {
    @objc dynamic var foo: Foo = Foo()

    static func automaticallyNotifiesObservers(for key: AnyKeyPath) -> Bool {
        print(#function, key)
        return true
    }

    static func keyPathsAffectingValue(for key: AnyKeyPath) -> Set<AnyKeyPath> {
        print(#function, key)
        return []
    }
}

let bar = Bar()
let token = bar.observe(\.foo.name) { (object, change) in
    print("KVO:", object, change)
}
bar.foo.name = "wat"
token.invalidate()
print("done")

This code prints

KVO: <unnamed.Bar: 0x7fef18c0b870> NSKeyValueObservedChange<Optional<String>>(kind: __C.NSKeyValueChange, newValue: Optional(nil), oldValue: Optional(nil), indexes: nil, isPrior: false)
done

Note how neither Foo nor Bar have their NSKeyValueObservingCustomization methods invoked, even though I'm doing nothing weirder than using a path with more than one component.

[lilyball]

Yet another thought: Even with the hacky workaround of using a special ObjCKeyPath subclass that just does Obj-C keypath comparisons, we can't even install our swizzles until someone invokes NSObject.observe(_:options:changeHandler:) (as Swift does not provide access to Obj-C's +load) so no matter what, if someone uses exclusively the Obj-C observation methods, our NSKeyValueObservingCustomization protocol won't get invoked.

[tonyarnold]

If there's no way to salvage the appending story (calling back to super to get the dependent key paths that they parent class (or even the frameworks!) would like to add, then yeah - I have to say that this API isn't really usable as-is.

I've used KVO to append a new dependent key to a property provided by an AppKit class, such as the following property on an NSDocument subclass in my app:

override var isDocumentEdited: Bool {
    // Ensure that if the user hasn't yet saved the file to disk, we don't prompt to save any edits
    return self.fileURL != nil
        && super.isDocumentEdited
}

override class func keyPathsForValuesAffectingValue(forKey key: String) -> Set<String> {
    switch key {
    case #keyPath(isDocumentEdited):
        let keyPaths = super.keyPathsForValuesAffectingValue(forKey: key)
        return keyPaths.union([#keyPath(fileURL)])

    default:
        return super.keyPathsForValuesAffectingValue(forKey: key)
    }
}

As far as I can tell, there's no way to achieve this using this AnyKeyPath API, is there?

[lilyball]

@tonyarnold Appending to the superclass's list isn't a problem, the superclass will have declared conformance to this protocol so you can just call super (though adding conformance to an open class is therefore an API-incompatible change as subclasses will have to adjust). The problem with the subclassing scenario is the fact that the KeyPath values will literally be different depending on whether the observer constructed the keypath against the superclass or subclass type, and therefore comparing the key is going to break in this scenario (either the superclass implementation or the subclass implementation will fail to compile). Plus all the other myriad ways this API is broken as documented above.

[tonyarnold]

Thanks for explaining in more detail, I appreciate it - I was off base with my understanding of the issue.

[millenomi]

I do not have a comment at this time but I wanted to note I’m reading the discussion here.

[tonyarnold]

@millenomi thanks - sorry if my frustrated jokes are coming off the wrong way.

[millenomi]

I absolutely understand your frustration.

I’ll have more news when I have a good course of action and a decent release vehicle.