Uploaded image for project: 'Swift'
  1. Swift
  2. SR-9081

NSKeyValueObservingCustomization fundamentally broken, should be removed

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open
    • Priority: Medium
    • Resolution: Unresolved
    • Component/s: Foundation
    • Labels:
      None
    • 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)

      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).

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                millenomi Lily Vulcano
                Reporter:
                Eridius Lily Ballard
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated: