Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager (via email or direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
This proposal seems like something that should have been all along, and anyone relying on this access-but-not-used behavior was probably doing something extremely too clever for their own good.
Please keep comments focused on the review rather than related topics not proposed. Such related topics should have new threads created in discussion or pitch, referencing this review as what prompted them.
Is the problem being addressed significant enough to warrant a change to Swift?
Yes. It avoids potential/existing bugs and improves performance.
Does this proposal fit well with the feel and direction of Swift?
Yes
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Read through and participated some in the original discussion.
I like this proposal. My only question is, what are our recommendations to users who do depend on the current behavior (I’m sure there are some)? What changes can they make that we will guarantee will create an oldValue even if they don’t seem to need it? Whatever they are, we should document them.
This would be a much larger change than it sounds like; the assumption that all properties are readable is baked into the language pretty deeply. It would definitely need to be its own proposal. (Which I assume is at least part of why Ben asked for it to be discussed elsewhere.)
Please add a sentence (or paragraph) on how this affects willSet/didSet for overrides, as opposed to stored properties. (In particular, I think the corresponding optimization for willSet becomes more interesting.)
I do still think the "explicitly specify an argument" approach is a better way to go (didSet(oldValue)). Having the body of your didSet change the CoW behavior for those using your setter seems subtle. Then again, I suppose closure captures are the same, and other than implicit self access we don't require those to be explicit. (Though it's been brought up before.)
I tend to agree. I have always found the ability to reference newValue/oldValue without them seemingly being defined as something that runs counter to Swift. I feel like they should have been something more akin to $oldValue/$newValue, since $ appear in other magic places where synthesis happens.
Can we really change that at this point though? I don't feel like not specifying newValue/oldValue explicitly is actively harmful enough to warrant requiring them
Probably. didSet(_) should probably work too. But wherever is supposed to work, we should explicitly call it out in the proposal, change log, and release notes, and we should make sure the implementation tests that it works.
The didSet observer is synthesised without the oldValue if you do not reference the oldValue inside the body of the observer.
For example:
var foo1: Int = 1 {
didSet { print("bar" } // We synthesise a didSet() and the getter
// is not called to fetch the `oldValue`
}
var foo2: Int = 2 {
didSet { print(oldValue) } // We synthesise a didSet(oldValue) and
// the getter is called to fetch it.
}
This is clarified in the proposal, but if you find it confusing then I can amend it to make it a little more clear. If anyone is relying on the current behaviour, they should hold a reference to the oldValue inside the body of the didSet to ensure the getter is called.
IMO, a fix-it of "hold a reference to the oldValue inside the body" is subtle and confusing as the canonical semantics-preserving solution. I think the proposal should explicitly call out the didSet(oldValue) solution as the preferred one and mention that simply referencing oldValue in the body is an alternate solution.
Sure, I can update the proposal text to note what the solutions are if one wants to preserve the old behaviour.
EDIT: To clarify, the current implementation does support the first workaround (so didSet(oldValue) { ... } will call the getter even if the oldValue is not used).
This is a really nice proposal: it's small in scope, easy to understand, and addresses a number of clear issues in one go as well as providing a nice optimisation opportunity. I'm very much in favour, this is great work.
Yes. Clearing up these semantics would be welcome.
+0.5. This proposal addresses a number of important issues which appear in real-world code, and the tradeoffs seem comparatively minor.
I am a little concerned that it introduces a small source break which doesn't lend itself well to automatic migration or a warning & fix-it. The proposal calls the current behavior "buggy" when justifying breaking code which relied on it, and I don't entirely agree with that assessment. That said, I'm fairly confident this breakage won't appear very often in practice so I remain in favor as long as the behavior change is communicated well. It would be nice if we could scan GitHub repos, etc. to quantify the impact of the break, but I don't think that's feasible for this change.
Yes, I found the examples in the motivation section of the proposal illustrated nicely how the current behavior can be surprising and harmful.
I followed some of the pitch discussion and read the final proposal.
If that's the only effect I guess that's sufficient! I just don't want people reading the proposal to forget that it applies to override willSet/didSet too.