Avoid KVC Crashes in setValue on let Properties at Compile Time (and KVO Problems by Forgetting @objc or dynamic)

I have the following crashing code:

@objcMembers class C: NSObject {
    dynamic let s: String? = ""

    func kvcWrite(newS: String) {
        let k = #keyPath(s)
        setValue(newS, forKeyPath: k)
        print(s ?? "nil")
    }
}

let c = C()
c.kvcWrite(newS: "crash")

The problem is that setValue tries to write on a let property which crashes at runtime.

We currently migrate all our models from ObjC to Codable Swift classes, where we ran into this caveat. Unfortunately the #keyPath method does not check if the value is both readable and writable (which is kind of legitimate, as reading the value with KVC would work correctly for let properties).

Now I ask myself if I can somehow prevent this problem at compile time?

1 Like

Shouldn’t “s” be a “var" if you are going to modify it? Let variables can only be initialized, not modified.

Jonathan

Exactly. It‘s a bug I introduced and I like to avoid slipping through the compiler.

I believe this is an example that highlights the benefits of Swift's type system over the much weaker Objective-C one. Using Swift 4 key paths does give an error:

import Foundation
@objcMembers class C: NSObject {
    dynamic let s: String? = ""

    func kvcWrite(newS: String) {
        let k = \C.s
        self[keyPath: k] = newS
        print(s ?? "nil")
    }
}

let c = C()
c.kvcWrite(newS: "crash")
kvc.swift:7:26: error: cannot assign to immutable expression of type 'String?'
        self[keyPath: k] = newS
        ~~~~~~~~~~~~~~~~ ^

I imagine your example is much reduced from your app, so I can't tell if it would be possible to replace the Objective-C #keyPaths with the Swift ones in the real code.

1 Like

Thank you @huon, that's good advice. For my real application that means I still have to migrate to Swift all the code that calls setValue or that passes String keypaths as well.

Actually, I think the situation is actually the reverse of what you both said. It's correct for @fabb's #keyPath version to compile but crash, and incorrect for @huon's keypath version to produce a compile time error.

The reason is that property s is dynamic, which means it might be overridden in a subclass by a property that has both a getter and a setter. kvcWrite should crash at runtime only if there is no setter in the actual class of the receiver.

No I don‘t think so.

For one, I never said that crashing #keyPath would be an incorrect behavior. It‘s a caveat, but I also think it‘s correct behavior.

For two, I do think the current KeyPath behavior is the correct behavior and works as expected in regards to OO polymorphism rules. Getting a KeyPath on a subtype where the property is writable in the subtype, and readonly on the parent class, a WritableKeyPath should be returned. But getting it on an object with the static type of the parent class, even if it‘s currently an object of the subclass at runtime, should return a readonly KeyPath, as the compiler cannot guarantee writability. The new KeyPath type was introduced to provide more type safety when working with keypaths, and guaranteeing writability at compile time is part of that type safety.

3 Likes

Hmmm, I have a different problem now, and that's a missing dynamic keyword breaking KVO. Consider this:

@objc class C: NSObject {
	@objc /*dynamic*/ var s: String? = ""
	
	override init() {
		super.init()
		addObserver(self, forKeyPath: #keyPath(s), options: [], context: nil)
	}
	
	override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) {
		print(keyPath ?? "")
	}
}

let c = C()
c.s = "new1"
c.s = "new2"

Without dynamic on the property s, the observeValue method will never be called. Even the new KeyPaths can't save me from that. Any ideas apart from writing unit tests?

You need dynamic so that the Swift compiler always uses dynamic objc dispatch, and triggers KVO "magic". @objc itself is not enough: it does allow Objective-C dispatch but does not enforce it.

Now the compiler doesn't know that you intend to use KVO on your property, so it won't warn you if you forget dynamic.

So tests (written in Swift) are indeed a good idea.

Actually there is a newer KVO observing method in Swift that utilizes KeyPath:

@objc class C: NSObject {
	@objc dynamic var s: String = ""
	var observer: NSKeyValueObservation? = nil
	override init() {
		super.init()
		observer = observe(\.s, options: [.new], changeHandler: { (o, change) in
			print("new value: \(change.newValue)")
		})
	}
}


let c = C()
c.s = "new"

But still, the same problem exists, when dynamic is missing, KVO won't fire. It would be helpful if KeyPath would somehow convey whether it is dynamic for all its path components.

The way KVO works requires dynamic.

You can ignore when people and documentation tell you so, but there's no point complaining, because there is no bug that needs any fix. Just add dynamic, as documented, and enjoy KVO, as promised.

Now what about learning why dynamic is required, so that you feel more comfortable with the feature?

KVO requires dynamic because it is firmly rooted in the Objective-C runtime, not the Swift runtime. Both languages don't use the same technique at all to implement code like c.s = "foo" (unless you instruct Swift to be Objective-C compatible with dynamic). If you are interested and want to learn more about how KVO works, try the excellent https://www.mikeash.com/pyblog/friday-qa-2009-01-23.html.

It's totally ok that dynamic is needed, and I understand why it's needed for KVO. Here is another very good article explaining @objc and dynamic: https://swiftunboxed.com/interop/objc-dynamic/

I just hoped there was a typesafe way to deal with it, but as it seems there is not. I just prefer compile errors over unit tests ;-)

2 Likes

I’d also like to see this become a compiler error, like we have for selectors.

1 Like

But... you're right indeed! This might even deserve an issue in https://bugs.swift.org?

Hm, since KeyPaths currently are usable for non-dynamic keypaths as well, a compile error cannot just be added without destroying these other use cases.

Maybe a type safe way would be if the \ syntax would return a different subclass if all path elements are dynamic, something like DynamicKeyPath (and the same for a writable keypath, double inheritance issues let aside for now). Then the observe method could be changed to only take DynamicKeyPaths, and the compiler would do the rest.

Does this sound sensible?

I chatted briefly to @Joe_Groff, and it sounds like that's unfortunately a fairly fundamental objection to this particular technique for achieving this. If we are going to track this in the type system, it probably can't be through extra subclasses, because it will be hard to get that to behave uniformly/as people expect. However it would be great to have an issue for the diagnostic @Jon_Shier suggests for "obviously incorrect" things like observe(\.s, ... where s isn't dynamic.

FWIW, I really dislike "just read the documentation" as an excuse for footguns in code. Documentation should be the last resort for describing "how to hold this API right": it's so much better when the language, compiler and even the API itself is designed to make the correct thing also the easy thing (and, at the moment, it's very easy to forget about dynamic). When there's holes, we should be critically thinking about ways to patch them.

It might be that there's no good way that doesn't have unacceptable sacrifices (e.g. maybe the API gets too hard to use, or becomes too slow, or would require introducing multiple inheritance into the language), in which case documentation is what's left, but there's often some way to improve it. One example is adding more diagnostics, as suggested here.

Btw. when forgetting both dynamic and @objc, the call to observe even crashes. So currently dynamic requires @objc as well, but when that‘s no more necessary at one point in the future, and observe still relies on both dynamic and @objc, we have a problem if we fix the problem by just checking the whole keypath for dynamic.

class C: NSObject {
	var s: String = ""
	var observer: NSKeyValueObservation? = nil
	override init() {
		super.init()
		observer = observe(\.s, options: [.new], changeHandler: { (o, change) in
			print("new value: \(change.newValue)")
		}) // Crash
	}
}


let c = C()
c.s = "new"

That's why the rest of my message, that you did not quote, goes on with the beginning of an explanation. Please don't associate my name with RTFM. And I really dislike partial quotes, which plague these forums in a painful fashion.

Documentation should be the last resort for describing "how to hold this API right": it's so much better when the language, compiler and even the API itself is designed to make the correct thing also the easy thing (and, at the moment, it's very easy to forget about dynamic). When there's holes, we should be critically thinking about ways to patch them.

Sure. Keep on reading the thread: we all agree.

Sorry; my point was a bit broader.

It's really great that you gave an explanation! That's definitely an important part of bridging the knowledge gap for complicated features. However, the rest of your message implies that the person to which you're responding doesn't know how KVO works and should learn (by linking to the exact same documentation that they do) and, most importantly, once that happens, they would never accidentally forget to write dynamic. Additionally, explanations like this are documentation in a different form—with the same major downside: any knowledge is not compiler enforced—and even if the person in this case didn't know how it worked, that only solves the problem for one person.

The key to my point is that humans are fallible: I deeply understand how multiplication and addition work, and yet I've accidentally expanded x - (y - z) to x - y - z annoyingly often. Having the rule explained to me again wouldn't fix anything, but having someone, or something, flagging the mistake when I make it would help a lot.

And, the thing that really prompted my response was saying that was no point in complaining. In direct contrast to what you said, there is a point to complaining that doing something in one place (adding a call to observe, in this case) requires remembering to make changes in other places (adding a dynamic): half the point of Swift being so much more static than Objective-C is how much more helpful the computer can be in finding when humans violate rules, rather than just relying on humans to read and remember them all. Complaints—such as filing JIRAs, or radars, or even forum posts, as here—help the Swift team/community understand common pitfalls, even if a complaint is driven by a misunderstanding (which it isn't, in this particular case). If we see patterns or notice easy-to-plug holes causing these pitfalls, we can design language/compiler/API features to address them.

I'm very glad that you agree with the later suggestion for adding diagnostics. :slight_smile:

So any ideas how the compiler could add diagnostics, or fix the issue by other means? Currently we have 2 non-practical suggestions in this thread:

  1. Make KeyPath only usable for dynamic keypaths - would break a lot of current use cases, as KeyPath is helpful in other contexts for non-dynamic keypaths as well

  2. Add new subtypes to KeyPath and WritableKeyPath that guarantee that all path elements are dynamic (and maybe @objc too) - problematic as we need to add 2 subtypes which is overcomplicating the existing KeyPath class hierarchy

Any other ideas?

Terms of Service

Privacy Policy

Cookie Policy