After upgrading to Xcode 12, we just ran into an unexpected consequence of SE-0268's new "simple" _modify semantics. In short, code that previously ran just fine will now fail at runtime with a simultaneous access error.
The issue lies in interactions with optional chaining and an assignment expression that references the property. In particular, with the non-simple version, the synthesized _modify coroutine yields a reference to a stack-allocated copy of the value, and therefore does not hold memory access on the property during the yield. But in the new version, it holds [modify] [dynamic] access during the yield. Without optional chaining this is generally fine, as the right-hand side of the assignment is evaluated before the _modify is invoked, but with optional chaining it has to defer evaluating the right-hand side until it's verified that the property's value is nonnull.
A reproduction case looks something like
class One {
var two: Two? = Two() {
didSet {}
}
struct Two {
var x: Int = 42
}
func run() {
two?.x = (two?.x ?? 0) + 1
}
}
One().run()
This will crash under Swift 5.3, but run just fine if I use didSet(oldValue) instead.
You can also reproduce this by using a mutating method that takes a closure that reads the property again. This seems perhaps less likely to me than the optional chaining issue, but is still not unreasonable:
class One {
var two: Two = Two() {
didSet {}
}
struct Two {
var x: Int = 42
mutating func mutate(_ f: () -> Int) {
x = f()
}
}
func run() {
two.mutate { two.x + 1 }
}
}
One().run()
I'm not sure what (if anything) should be done about this, I assume Swift 5.2 would have crashed on this without didSet at all (though I don't have it accessible at the moment to test), but this is a runtime incompatibility that was not called out in the SE-0268 proposal.
The proposal does call out the possibility of a breakage if you're relying on the previous behaviour, but you're right, it doesn't explicitly talk about exclusivity violation, my apologies! I will amend the proposal to mention it.
It seems to me that didSet(oldValue) should trigger an access conflict too, at least based on this post:
but I am not qualified enough to provide an accurate answer on this topic. cc @Andrew_Trick
I assume Swift 5.2 would have crashed on this without didSet at all (though I don't have it accessible at the moment to test)
Yes, it will trigger an access conflict, if there isn’t a didSet.
I think unconditionally causing a conflict is the way to go.
It also really bugs me that this is a runtime violation. I wish this could be a static violation instead, at least when there’s no library evolution concerns.
Is it just me or does introducing a runtime crasher for code that used to work seem really bad? Surely this is much worse than a source compatibility break. Is there really no way to detect this issue at compile time? At first glance it doesn’t seem much different from other statically-checked exclusivity violations.
It does feel bad, but I think consistency is better, because today it crashes without a didSet, and it crashes in Swift 5.3 with a didSet that doesn’t reference oldValue. So this same crash would have occurred in Swift 5.2 if you simply removed the didSet.
Unfortunately, even making it crash always doesn’t completely eliminate the inconsistency, because a computed property won’t crash in this case. And a stored property won’t crash on assignment if you don’t invoke optional chaining.
This really does seem like something that should be detected statically, but I don’t see how we can do that across module boundaries, as a computed property might be replaced behind the scenes with stored property, thus introducing a runtime crash where previously there was no static violation.
We could consider introducing a warning in cases where a property is referenced from within a call to _modify as that can be statically determined, and the warning can say something along the lines of how this might be a runtime exclusivity violation. And maybe make this an error if the property is defined within the same module and is known to be a violation.
When you say this can’t be determined statically across module boundaries, do you mean even when the modules are being compiled together from source (as in SwiftPM for example)? Or is this just a limitation when linking against a binary module such as Foundation?
Could someone please explain in simple terms why this is an exclusivity violation that will crash a program? I.e. for someone who is not totally sure what a “modify coroutine” is or how it gets involved in some basic Swift code involving didSet and (optional) assignment.
When ? is used on the left-hand side of an assignment, it includes the entire rest of the assignment within its "scope". Since the type of ?'s operand is a value type, we have to access that value to check whether it's present before we can evaluate the rest of the statement. If the value is present, we'll have to access it again to do the assignment. In principle we could do this as two separate accesses: we could copy the value out, modify that copy, then write the whole value back. That's how this works for fully computed variables, and it's how it used to work for variables like the one in this example. It allows the rest of the statement to read the value between the accesses — or even modify it, although those modifications would be clobbered during the final write back. But SE-0268 changes the rules when a variable just has a didSet so that they're more like what we do with a fully stored variable, and it says we should instead do a single access that starts when ? checks whether the value is present and doesn't end until the assignment is complete. That's somewhat more efficient and predictable, but it doesn't allow this kind of intervening access during the assignment.
This particular example can be rewritten to two?.x += 1, since two?.x can only be nil if two is, which we've already checked isn't the case.
Is that desirable? This sounds like the very code that Exclusivity Rules is trying to prevent. I feel like it's more inlined to treat copy-mutate-writeback as a big mutating access (even though it's actually a few getter/setter calls). Maybe we can share the access between the copy and the original, or something... .
That the optional assignment is one big mutating access, instead of read-access when checking for nil, then promoted to write-access at during the writeback, sounds like a separated issue.
Well, I don’t think it was desireable, which is part of why I encouraged this change in Evolution. But here I’m just stating the reality of it as plainly as I can.
