Skip to content
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

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

Open
lilyball mannequin opened this issue Oct 25, 2018 · 14 comments
Open

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

lilyball mannequin opened this issue Oct 25, 2018 · 14 comments
Assignees

Comments

@lilyball
Copy link
Mannequin

lilyball mannequin commented Oct 25, 2018

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
Copy link
Mannequin Author

lilyball mannequin commented Oct 25, 2018

CC @millenomi

@millenomi
Copy link
Contributor

@swift-ci create

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

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
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

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
Copy link
Mannequin Author

lilyball mannequin commented Oct 28, 2018

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
Copy link
Mannequin Author

lilyball mannequin commented Oct 28, 2018

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
Copy link
Mannequin Author

lilyball mannequin commented Oct 28, 2018

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
Copy link
Mannequin Author

lilyball mannequin commented Oct 29, 2018

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
Copy link

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
Copy link
Mannequin Author

lilyball mannequin commented Feb 19, 2019

@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
Copy link

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

@millenomi
Copy link
Contributor

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

@tonyarnold
Copy link

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

@millenomi
Copy link
Contributor

I absolutely understand your frustration.

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

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from swiftlang/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants