I think this is my first ever post in this community so excuse me if my tone or message is inappropriate or not in-depth enough.
I have to say that I read this proposal and I immediately thought 'why isn't this happening already?' Meaning, I'm a huge fan of all the low-level stuff Apple has been doing over the years, like LLVM and the continuous improvements the Compiler team does to keep our code as fast and lean as possible. And given how crazy Apple is about maximum performance and efficiency, I would've thought that Swift being in version 5.1 already, this would be a thing, though I understand why it isn't.
As such, huge +1 on my end. My only question would be how does this complicate future development of getters and setters, since by my experience, making things 'smarter' often makes thing harder to work with down the line.
Absolutely a +1 from me, this seems like it could yield very significant improvements to some code bases and feels like the behavior we should have had all along.
I think it's a good change, and thought it would be especially nice in the context of lazy var (where the computation of the "default" value can be expensive).
But then I wanted to check... and the compiler does not even allow observers for lazy properties!
I created a discussion thread about it. I have a WIP PR to allow observers on lazy properties, just needs a little push. The behaviour should be the same, except that the oldValue in the didSet would just be the initial value if the property hasn’t been forced yet.
Of course, this proposal will prevent that from happening if you don’t refer to the oldValue, which is nice.
I have a PR opened on the swift evolution repo that adds a few clarifications to the proposal text. The implementation exempts a didSet(oldValue) { ... } from the new behaviour since the oldValue is explicitly specified. Now, the code for this was sitting in my branch (locally) for a while and since it wasn't mentioned in the proposal text before the review kicked off, it likely needs additional approval from the core team whether they want to consider it or not.
If not, then the alternative it is to do _ = oldValue in the body, which isn't great but in real life almost no one would need to do it (unless they were doing something very special).
Anyway, fundamentally there's only two ways we can implement this proposal - we can either require an explicitoldValue parameter to call the getter or we can infer it from the presence of an explicit oldValue in the body (or, in the parameter list). Either way, we will have some breakage. The former will definitely cause more breakage as well as a loss of usability (among other things) and it's already been discussed in the pitch thread, hence the current design.
Now, what breakage is acceptable and how much is up to the core team and depending on their decision, the implementation can be very easily changed to do the former instead of the latter.
It bears clarifying that didSet(foo) { ... } and didSet(_) { ... } also preserve the old behavior.
No, there are three ways. We can use a new spelling for the proposed new behavior. didSet() { ... } was discussed as an option during the pitch, I believe. We could also consider a more obvious spelling such as nongetting didSet { ... } in the same vein as nonmutating set { ... }.
Sure, although you can't do didSet(_) today since you need to specify an identifier. However, the restriction can be lifted if needed.
It's still source breaking though (and has the same drawbacks as solution #1). Again, it can certainly be done depending on whether the breakage is acceptable. Although I think a change like that is beyond the scope of this proposal and it would be best to pitch that separately.
Disagree. It would just be a different surface syntax for the proposed behavior and needs to be reckoned with if a source-breaking alternative is to be chosen over it.
Okay, I may have misunderstood. I was thinking it would be source breaking if we made it an error to ignore oldValue without didSet(), although a warning + fix-it will be fine.
Syntax changes is not in scope as part of this proposal - but it doesn't mean we can't consider it for this proposal to be accepted/implemented. It think it will create an inconsistency with other accessors though and become another thing you would have to explain to a newcomer.
I’m not sure what you mean by this. To be clear, your proposal changes the meaning of existing syntax. I am saying that there is an alternative solution which does not make any change to existing syntax. That such an alternative exists is not to be taken lightly.
I’m not interested in debating the relative merits of these designs here. I’m simply pointing out that it is not true that there are only two fundamental ways of approaching this problem and that all ways are source-breaking. There are three ways and one of them is not source-breaking: this should be mentioned in the proposal and it should be considered carefully. The “Alternatives Considered” section is not an accurate reflection of the alternatives considered during the pitch phase.
Sorry, I mean it's not what I (the proposal author) is proposing, but I agree it's something that could be considered, if the core team thinks the other solutions are not acceptable.
Thanks, I will amend the above mentioned PR to include it as well.
Review manager note: I have merged @suyashsrijan's update to the proposal, adding some clarifications from this thread about class behavior, the workaround to preserve source compatibility, and some alternatives considered. You can see the diff here.
I wonder if we should always perform modifications in-place, and then just make the use of oldValue affect whether a copy must be made first. This would have identical behavior to the current proposal for copy-on-write types, but it would eliminate the assignment back to the storage at the end of the operation, and it would cause the exclusivity duration of the access to extend over the entire operation until (but not including) the call to didSet, which might be a more intuitive rule.
Programmers could still force didSet to purely be sugar for a setter by defining an empty willSet.
My interpretation of the proposal is that it changes the exclusivity rule when oldValue is not used (technically it's source breaking, but in a way that simplifies the rules).
I'm strongly in favor of this proposal along with John's suggestion of modifying in-place unconditionally. It's very confusing for Swift to exhibit different behavior due to a seemingly unrelated change. The following should be an exclusivity violation regardless of whether bar has a didSet observer, and regardless of whether the observer references oldValue. Currently, the compiler fails to enforce this.
public class C {
var bar: Int = 0
{ didSet { _ = oldValue } } // nothing on this line should affect exclusivity enforcement.
}
func testBar(_ x: inout Int, _ c: C) {
x = 1
c.bar = 2 // simultaneous access to "bar"
}
public func test(c: C) {
testBar(&c.bar, c) // "bar" originally accessed here
}
To clarify, is this related to how the _modify is synthesised? Currently we have to check for the lack of a willSet and presence of a simple didSet, so does it mean the new rule is checking whether we use oldValue or not (rather than checking willSet and didSet)?
Yes, when you synthesize _modify, you should synthesize it as:
_modify {
#if hasDidSet && didSetHasArgument
// If we have a didSet with an argument, we have to remember the old value.
let oldValue = storage
#endif
// Perform the modification:
#if hasWillSet
// With willSet, we have to copy into a temporary and modify that.
var temp = storage
yield &temp
// Pass the new value of the temporary to willSet.
willSet(temp)
// Write back to the storage.
storage = temp
#else
// Otherwise, we can directly modify the storage.
yield &storage
#endif
// Call didSet if present, passing the saved value if necessary.
#if hasDidSet
#if didSetHasArgument
didSet(oldValue)
#else
didSet()
#endif
#endif
}
Ideally there would be a slight further refinement where, when we copy the old value, we actually do that as part of the same formal access to the storage as the yield; but we can take care of that during implementation review, and worst case I think we can do it later as a refinement without putting anybody out. (It would only semantically matter when adding observers to an inherited property.)
Hmm, this looks close to how the implementation currently does it i.e. if we have (1) no willSet and (2) didSet without oldValue in body then we synthesise _modify differently so it yields the storage and calls didSet(), otherwise we just synthesise it normally.
(Also, what if didSet has no argument but the oldValue is used in the body - we will be emitting a warning for that, but does that prevent the synthesis of _modify like above until the fix-it is applied?)
I consider "oldValue is used in the body" to be a sub-case of "didSet has an argument" — as long as that's permitted, the argument is implicit.
If we want the optimal code-generation where we load the old value as part of the same access, we'll have to make the "stored/inherited with observers" case first-class in ReadWriteImplKind and teach SILGen how to emit it directly.