This is a follow-up to the previous review discussion. The review was discussed with the core team, and the team believes the proposal should be accepted with a specific amendment to address concerns in the prior review: that in cases where an implicit oldValue parameter is used, causing the value to be fetched, a warning should be generated asking the user to add the parameter explicitly.
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?
Note this warning can be silenced in a way that is compatible with older compilers (important for people who want to build without warnings with both compilers during a transition between the two).
Implicit use of oldValue appears to be extremely common in the wild. I may have messed up the analysis somehow, but I searched the source compatibility suite, and it appears to contain:
1,051 instances of didSet
186 uses of oldValue (this will include other variables that coincidentally have that name)
Literally zero uses of didSet with an explicit name
Are we prepared to warn on nearly every use of oldValue in existing Swift code?
My personal opinion is that this warning will be unnecessarily disruptive to common usage and the proposal is just fine without the warning.
I am opposed to removing (or deprecating) the implicit oldValue. This feature, alongside the implicit newValue in setters, is one of the many nice things that make Swift pleasant and convenient to use.
I would view it as a net negative to have to write “didSet(oldValue) { … }”, just as I would view it as a negative to have to write “set(newValue) { … }”. It is by far preferable to write “set { … }” and “didSet { … }”.
Notably, if it were required to manually specify the name, then I would probably call it “x” most of the time, and I expect many others would choose similarly short names. These short names would reduce (but not eliminate) the inconvenience of having to provide a name manually, but at the cost of clarity and consistency.
I do not think that simplifying the compiler implementation is worth the certain reduction in convenience, and the probable reduction in clarity and consistency of code, that this would bring.
I have to agree with Brent -- I have 165 uses of oldValue in my project, and I don't look forward to having to add explicit declarations for them. Xcode will make the process easier, but I don't really want the declarations there in the first place. I find it intuitive that if you use oldValue you will access the property, and I don't really see how adding the declaration serves to clarify that.
I still think this is a great proposal even with the change, but I definitely prefer the previous revision.
I don't think deprecating implicit oldValue is a good move.
For one thing, it's very disruptive because it forces the declaration of the parameter where almost everyone was relying on the implicit name.
For another, it somewhat forces you to keep in sync the didSet declaration with its body if you want to keep things efficient. If you add oldValue in the body you need to add it in the declaration; if you remove it from the body you should remove it from the declaration too (unless you're relying on the semantics of calling the getter, which pretty much no one cares about). It's not like you're expressing a commitment in the API with this parameter, so why not let the compiler maintain this boilerplate for you?
It does not help with preserving source compatibility since cases where implicit oldValue is unused but you were relying on the getter being called are not going to get any warning. Maybe the idea is for the warning elsewhere to teach people oldValue has the side effect of calling the getter so they think to add it at places they need the getter to be called, but is the lesson worth all this disruption?
And it's only a warning: the compiler still has to support implicit oldValue and is not saving any CPU cycle.
In the end, I don't see any reason why the implicit declaration for oldValue should be deprecated.
The issue about calling the getter is the entire rationale for this proposal; if the argument is that pretty much no one cares about it, then no part of this proposal should be adopted. I think the premise here is that people do care about it, and that requiring people who use the old value in the body to update the declaration reflects that.
I don’t buy this argument. Instead of requiring you to write any particular variable name n times, this proposal would require you to write the name n + 1 times. But the additional one time that you would need to write the name is so inconvenient that you would choose a shorter name, even though you simultaneously strongly favor the clarity of a longer name?
I would argue, on the contrary, that both convenience and clarity argue in favor of the new proposal: writing x twice is still fewer keypresses than writing oldValue even once, and explicitly declaring a variable name (even if it is one letter long) is surely more clear than using a magical name declared nowhere (even if perfectly named).
To my mind there is one major reason to argue against deprecating implicit oldValue—which is that it is a source-breaking deprecation. That it can be automatically migrated in all cases, and that the new syntax to which it is migrated is backwards compatible in all cases, are major mitigating factors. But nonetheless the degree to which it is source-breaking still weighs on the proposal. That much, I think, is unarguable.
Perhaps I expressed myself unclearly. What I meant is that pretty much no one cares about the semantics of calling the getter (which in most cases should have no side effect). Avoiding calling the getter when you can is very desirable for performance reasons though.
I am strongly against deprecating the implicit oldValue. I understand the rationale behind the amendment — it changes the behavior/semantics — but when a user reaches for the oldValue they're, in a way, opting for a different behavior anyways. So why is this a concern? Isn't it as it should be?
I think @Nevin's arguments against the change are absolutely worth taking into account and I echo this:
I think this response is disingenuous because it leaves out the fact that we all use code completion. Unless opening a parenthesis after didSet automatically suggested the spelling "oldValue" (which isn't a behavior Xcode exhibits anywhere) you leave it to the developer to type the full name out, which will lead to some misspellings or undesirable shortenings. On the flip side, with implicit oldValue, typing o then Enter to select the code completion for oldValue is just as many keystrokes as typing x twice (once for declaration, once for use.) Swift shouldn't be, and has never been, about counting keystrokes, though. So the entire argument really should be moved past. Clarity over brevity.
oldValue is no less magical than the implicit error inside of catch, or $0 inside of a closure, or newValue inside of set. This isn't magical in Swift, it's commonplace.
We do know of some counter-examples. There's an API in Cocoa (I don't remember what) which checks that you don't read it before writing it; if you override it with a didSet, Swift will currently call the getter just to provide the oldValue parameter, and that call will violate the precondition. That's not just theoretical, it's an actual user-reported bug which will be fixed by this semantics change.
But the performance story certainly seems somewhat compelling to me on its own. We want performance to be more predictable in general.
To be clear, neither of those requires deprecating the implicit oldValue parameter.
The current behaviour of calling the getter every time is unintuitive, I hadn't really thought about it happening at all before the first round of the proposal came up, so I think a change is warranted.
I regexed my current open project and I'd be facing about 80 fixits when this change was introduced, but realistically it would only take five minutes to go through and modernise them to the explicit form. (I've never had much luck getting the migrator wizard to work.) The real pain point will be all the didSet statements I will write in the future. Migration warnings are fine IMO but you need the tooling support to be in lock-step.
As @clayellis mentioned, autocomplete needs to step up here. The status quo is:
The fact this doesn't already have didSet (oldValue) in it is probably a primary factor as to why the implicit syntax form is so prevalent today. We collectively forget about using the explicit form because we aren't ever prompted to do it. The proposal text doesn't mention autocomplete at all — and while it may be obvious — it would be nice to get a firm assurance that it will be in place.
Swift rarely favours brevity over correctness and I personally would not want the fears of a minor 'source break' to stand in the way, particularly in this case where the change to modernise is so simple mechanically and effectively involves no decision making at all on the part of the programmer.
thanks, fixed