Namely, if a property wrapper has a mutating getter but a non-mutating setter, and the didSet body refers to oldValue, the synthesized setter has to be mutating as well, since it calls the getter to load oldValue. However in the current implementation, if the didSet body does not refer to oldValue, the setter is made non-mutating.
The determination of whether the didSet body refers to oldValue previously relied on parse-time name lookup. This means that it was possible to inspect the non-type checked AST of the didSet body to find (most?) references to oldValue. We would like to eliminate parse-time name lookup to simplify the compiler implementation. While I did not anticipate this to have user-visible impact, it appears that it does, because of SE-0268.
Specifically, here is the revealed problem:
Suppose we are type checking the body of didSet, and it refers to self.
In a didSet body, self is inout if the setter for the underlying storage is mutating
In order to determine whether the setter for the underlying storage is mutating, we must first determine if the body of the didSet references oldValue.
To determine whether didSet refers to oldValue, we now need to type check it first, since we cannot rely on parse-time name lookup.
We now have a cycle, and the type checker produces a "circular reference" diagnostic and gives up.
There are several solutions here:
Give up on removing parse-time name lookup. I don't think this is desirable or needed.
Relax the check for the didSet body referencing oldValue by looking for a syntactic reference to the oldValue identifier, instead of the actual oldValue parameter. This will incorrectly flag code like the following, for example, as loading oldValue when it does not:
var foo: Int = 0 {
didSet {
let oldValue = 3
print(oldValue)
}
}
Relax the check for the mutability of the setter to always make it mutating if the property wrapper has a mutable getter, instead of making it non-mutating if the body does not reference oldValue.
Ban references to self in the didSet body. I suspect this will break a lot of existing code (in fact an existing project in the source compatibility suite revealed this issue in the first place, and obviously I'd like to avoid breaking anything in the source compatibility suite).
I think 3) is the most reasonable solution. I believe property wrappers with a mutating getter and a non-mutating setter should be rare.
However, in this rare case, this becomes a source and binary compatibility break (but not as bad as 4). What are everyone's thoughts on this topic?
I agree that (3) is the way to go. I originally thought that the getter mutability should only be taken into account when there’s actually a didSet, but then adding one would be a breaking change, so I like your rule.
I also think there are approximately zero properties with a mutating getter and non-mutating setter, property wrapper or otherwise, so it almost doesn’t matter what we pick. (3) just seems easiest to explain.
I agree with Jordan, (3) seems most reasonable to me as well. A quick GitHub search reveals no properties in such a configuration (except tests in the test suite and its forks), so I would expect breakage to be rare or zero.
Also, I apologise for not documenting this in the proposal!
It's also worth noting here that 3) brings us back to the status quo before SE-0268; Swift 5.3 changed the ABI in this corner case, my proposed tweak would change it back to the 5.2 ABI.
Unfortunately that is still the case today. If the property wrapper is mutating get/nonmutating set, then just having a willSet gives you a mutating getter and a non-mutating setter. A didSet alone or a didSet/willSet gives you a mutating getter and a mutating setter.
What I'm changing is that even if the didSet does not reference oldValue, you still get a mutating setter, just like you did in Swift 5.2.
Not just properties, but property wrappers, that are also used with an observing accessor. Quite a strange combination. I hope so anyway
I mean, we could change it so the mutating-ness of the setter is always based on both accessors in the property wrapper, just to support this, but admittedly there might be a slightly larger 0 number of instances where people are doing that. ;-)
If that's the direction you're thinking of going in, I would take it all the way and just ban the mutating getter/non-mutating setter combo altogether. Can you think of any use cases for this?
Edit: Sema doesn't even check for this at a fine enough granularity. For example, we reject this with a misleading diagnostic:
struct S {
var foo: Int {
mutating get { 3 }
nonmutating set {}
}
}
func foo(s: S) {
s.foo = 123
}
w.swift:9:5: error: cannot use mutating getter on immutable value: 's' is a 'let' constant
s.foo = 123
~ ^
Fair enough. If it’s really not source-breaking, it would mean we’d never have to think about it again…but if someone actually has one somewhere, your version’s obviously safer.
As a passing thought:
Apparently even this is not a valid mutating getter/non-mutating setter use:
class Box<T> {
var t: T
init(_ x: T) {
t = x
}
}
struct Foo {
lazy var b: Box<Int> = Box(0)
var test: Int {
mutating get {
return b.t
}
nonmutating set {
b.t = newValue // Error: Mark accessor 'mutating' to make 'self' mutable
}
}
}
I cannot think of any other semantic meaning for a mutating getter/non-mutating setter.
