Fix swift KVO when keyPath having a optional value

[zetasq]

Fix a long standing issue https://bugs.swift.org/browse/SR-6066.
When keyPath has an optional value, we provide an overloaded method on _KeyValueCodingAndObserving to handle the special case.

Resolves SR-6066.

[zetasq]

@lilyball Do you have time checking this fix? You have fixed several Swift KVO issues before, I think you can help me with this : )

[lilyball]

It's not clear to me how this is supposed to do anything at all. All uses of Value in the overload are replaced with Value?, meaning if Value == T then this overload should behave identically to the original method where Value == T?.

There's also no attempt here to distinguish "value present but nil" from "value not present" (e.g. the difference between the the oldValue property for an .initial versus an subsequent mutation). For properties with optional values, if the value is present but nil the Obj-C KVO change will provide an NSNull value in the change dictionary, but if the value is missing then the change dictionary won't have the key.

Of course, when handling that, there's the unfortunate edge case of something like @property (nullable) NSNull *foo; because the KVO dictionary has no way to distinguish between "property is nil" versus "property contains NSNull". That said, this would be a pretty useless property so I'm not terribly worried.

[lilyball]

For comparison's sake here's the implementation in PMKVObserver. This handles NSNull as best as it can, and also has a workaround for dealing with RawRepresentable values. PMKVObserver does the casting when the old and new properties are accessed, so it has to deal with potentially-optional values always, but an equivalent Swift KVO implementation could just do special logic in an overload like the one this PR is trying to add.

[zetasq]

You are right. I just updated the commit and add the explicit NSNull type checking logic. Maybe I should add test cases to cover the fix?

[lilyball]

This doesn't look like it should be a doc comment

[lilyball]

Unless Value == NSNull.self, the expression .some(unwrappedValue as? Value) will already evaluate to .some(nil) for the case where unwrappedValue is NSNull.

In the case where Value == NSNull.self there's no way to distinguish between the property actually containing NSNull or containing nil so either behavior works.

Given that, we can just remove this if and the special handling of NSNull entirely and just use return .some(unwrappedValue as? Value). Though we should probably keep the comment to explain why we're even unwrapping this at all instead of just using return unwrappedValue as? Value?.

[zetasq]

I just updated the comment format and unwrapping logic according to review.

[lilyball]

The logic looks fine to me, but this still needs tests.

[zetasq]

I just added corresponding tests and verified in my local environment.

By the way, is there any documentation aboutCHECK-LABEL: and CHECK-NEXT: stuff? I cannot find it in https://github.com/apple/swift/blob/master/docs/Testing.md , so it took me a while to understand the existing test code in KVOKeyPaths.swift.

[millenomi]

Anything that adds API surface needs to go through additional internal review.

[millenomi]

(Also I don’t think we can take it as-is without availability.)

[zetasq]

I'm uncertain which Swift version should the availability requires (e.g. 5.1, 5.2 or 6). So the version will be discussed after the PR passes the internal review, right?

[millenomi]

@zetasq please add the following availability to the new method in your patch:

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)

[zetasq]

After I added the availability annotation, there was an error when I run the tests saying 'observe(_:options:changeHandler:)' is only available on OS X 9999 or newer. How can I make availability check in test file KVOKeyPaths.swift?

This also reminds me that if people are using a new SDK shipped with this new overloaded method and observe a keyPath with optional value, but with their deployment target set to iOS 11, will they also come across the error, or just resolve to the old observe(_:options:changeHandler:) method without any warning or error? If the behavior is the former (I think so), how can people force use the old observe(_:options:changeHandler:) method in their code?

[zetasq]

I find that it's really hard (maybe impossible) to explicitly use the old observe method with an optional value in this patch. The other option is adding two additional methods (one for non-optional and one for optional) with different names instead.

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
public func observeKeyPath<Value>(_ keyPath: KeyPath<Self, Value>,
 options: NSKeyValueObservingOptions = [], 
changeHandler: @escaping (Self, NSKeyValueObservedChange<Value>) -> Void)

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
public func observeKeyPath<Value>(_ keyPath: KeyPath<Self, Value?>,
 options: NSKeyValueObservingOptions = [], 
changeHandler: @escaping (Self, NSKeyValueObservedChange<Value?>) -> Void)

But this seems needing more discussion and review I think.

[millenomi]

After I added the availability annotation, there was an error when I run the tests saying 'observe(_:options:changeHandler:)' is only available on OS X 9999 or newer. How can I make availability check in test file KVOKeyPaths.swift?

You need to use if #available(…), just like regular client code does.

I find that it's really hard (maybe impossible) to explicitly use the old observe method with an optional value in this patch. The other option is adding two additional methods (one for non-optional and one for optional) with different names instead.

If the old method has incorrect behavior wrt optionals, isn't this a desirable outcome?

[Moximillian]

@millenomi do I understand correctly that

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)

essentially disables this PR for undetermined time, and Apple will later decide a version to include this change, if any.

So then it would be logical to do the same @available for the tests in this PR as well.

From my viewpoint this PR is a fix to a bug in observe (i.e. correctly handling optional observation), it's not a new feature as such. Ideally one should be able to use one function syntax for both non-optional and optional cases, and overloads would take care of doing the right thing.

[zetasq]

You need to use if #available(…), just like regular client code does.
This would work for normal methods, but it seems trickier for overloaded methods.

After I add availability annotation to the new overloaded method, the other observations (old testing code) in KVOKeyPaths.swift would cause compiler to emit error like error: 'observe(_:options:changeHandler:)' is only available on OS X 9999 or newer.

So I assume this would happen if we put this in a new iOS SDK. The compiler would give similar error (like error: 'observe(_:options:changeHandler:)' is only available on iOS 13 or newer if people observe a keyPath with optional value (the code compiles without error with older SDK), and they cannot explicitly reference the old observe method to make it compile.

The scenario is like this

// Assume the user is using the new Xcode with the new overloaded method in the SDK
if #available(iOS 13, *) {
    // The user can safely use the old or the new `observe` method depending the keyPath type.
    // The overloading mechanism automatically handles this.
} else {
    // What should the user do? The new overloaded `observe` method cannot be used.
    // But if the old `observe` method cannot be **explicitly** referenced, compiler would still emit the same availability error.
}

[Moximillian]

I guess it’s possible that availability doesn’t work the way we’d like it to work when doing overloads like this PR.

In that case we can’t use availability, but instead could use

#if false
...code here...
#endif

Which is effectively what the availability is doing anyway in this PR

[lilyball]

But we need availability anyway, because if you write code against this new overload, and then deploy to an OS that doesn't provide it in the stdlib, that wouldn't work.

[Moximillian]

@lilyball I understand, but in this overload case, the availability isn't working right:
BEFORE:

// Library
func test<T>(value: T) {
  print(value)
}

// Client app
let value: String? = "hello"
test(value: value)

AFTER:

// Library
func test<T>(value: T) {
  print(value)
}
@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
func test<T>(value: T?) {
  print("Overload \(String(describing: value))")
}

// Client app
let value: String? = "hello"
test(value: value)     // all versions will produce error here, even when client code isn't changed.
// error: ...: error: 'test(value:)' is only available on OS X 9999 or newer

[zetasq]

I'm not worried that the existing code would not compile with the new SDK, as long as we could use availability checking to fix it. But when the availability checking is applied, I find that there is another problem we need to solve:

// Assume the user is using the new version of Xcode and have the overloaded method in the SDK

// We want to observe a keyPath with an optional value
if #available(iOS 13, *) {
    // The user can safely use the old or the new `observe` method depending the keyPath type.
    // The overloading mechanism automatically handles this.
} else {
    // What should be put here? 
    // The new overloaded `observe` method will still be automatically referenced when we provide a keyPath with an optional value.
    // If we cannot **explicitly** reference the old `observe` method, compiler would still emit the same availability error.
}

So we need to reference the old observe method explicitly when writing code for compatibility. Any ideas? Maybe I'm in the wrong direction to solve this issue?

[Moximillian]

@zetasq I think client code using new compiler, but old SDK should not need any changes. And this availability error affects all code. This needs to be worked in the library side, not by asking everyone add ”if #available” in client code.

[Moximillian]

@zetasq I think client code using new compiler, but old SDK should not need any changes. And this availability error affects all code. This needs to be worked in the library side, not by asking everyone add ”if #available” in client code.

So one way to workaround this is adding overload to old API, although that might be also undesirable.

AFTER:

// Library
func test<T>(value: T) {
  print(value)
}

@available(...something here about versions less than 9999....)
func test<T>(value: T?) {
  test(value: value as Any)
}

@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
func test<T>(value: T?) {
  print("Overload \(String(describing: value))")
}

// Client app
let value: String? = "hello"
test(value: value)

[zetasq]

@zetasq I think client code using new compiler, but old SDK should not need any changes.

We don't change the old SDK. The overloading method will only exist in the new SDK.

This needs to be worked in the library side, not by asking everyone add ”if #available” in client code.

If availability checking works (we have a problem in the else clause now), this won't break the old code with the old SDK. It only affects the old code with the new SDK, which requires availability checking. This seems fine to me.

[zetasq]

So one way to workaround this is adding overload to old API.

We cannot add any code to the shipped SDK. I'm not sure what you means.

[millenomi]

@zetasq You raise a very correct (and obvious once explained; whoops) point re: the else branch. This needs me to think about this and circle back.

[lilyball]

Can we reimplement this as a dynamic type-check inside the existing method?

[Moximillian]

@lilyball dynamic type-check is possible at least by using mirror or with ExpressibleByNilLiteral. It would be nice to use single existing function rather than introduce an overload.

func test<T>(value: T) {
  if T.self is ExpressibleByNilLiteral.Type {
    print("Optional: \(String(describing: value))")
  } else {
    print("Regular: \(value)")
  }
}

// TEST

let regularValue: String = "hi"
let optionalValue: String? = "hello"

test(value: regularValue) // Regular: hi
test(value: optionalValue) // Optional: Optional("hello")

[zetasq]

Dynamic type-checking is a viable option. But to identify an optional type in the runtime, I have to introduce a private protocol _OptionalForKVO to type-erase the generic Optional type. Anyway, the result seems good, the tests passed, and we don't need to check for availability. Any comments on this new implementation?

[millenomi]

I can take these with no internal review. Thanks :)

[millenomi]

@swift-ci please smoke test and merge

[millenomi]

@swift-ci please smoke test and merge

[zetasq]

01:00:20 Running Swift tests for: check-swift-validation-macosx-x86_64
01:00:20 /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 1, aborting
01:00:20 /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 1, aborting
01:00:21 Build step 'Execute shell' marked build as failure

I cannot see the reason for the failed tests. Maybe I should rebase my branch onto the latest master branch?

[zetasq]

@millenomi Can you help to see why the smoke test fails? The stdlib related tests succeed on my laptop (OSX environment).

[zetasq]

@millenomi I just resolved the merge conflicts. Can you help to rerun the smoke test?

[millenomi]

@swift-ci please smoke test

[zetasq]

Will this fix come into Swift 5.1 branch? So I can add version check for my own app code : )

[zetasq]

@millenomi Could this pull request be merged now? Or there's something I can do to help?

[zetasq]

@jrose-apple Can you help to check if this pull request can be merged? It seems to be over a month since the CI tests passed.

[millenomi]

@swift-ci please smoke test and merge