Oh. I was thinking in the rev 2 revision of the proposal, you'd have didSet(oldValue) { _ = oldValue } to mean 'access the getter without using it'. (I presume the second source compatibility example in the proposal is outdated under the revision, as that would trigger warnings still as it is using oldValue implicitly?)
Yeah, the second example would need to be updated so it mentions oldValue in a parameter list.
Actually we can just remove it because as long as you have declared oldValue in the parameter list, it doesn’t matter if you use it or not as the getter will always be called.
Just to add one more thing here in response to the inconsistency of requiring explictness for didSet compared to the other implicit examples in the language (like error for catch or newValue for set), my position is that they are different. This didSet proposal exists in the first place because it would mean a change in behaviour, calling or not calling the getter depending on the presence of the newValue in the code block; the revised proposal is essentially an argument about whether the difference in behaviour should be more clearly signalled by having to use different syntax for each case.
error and newValue are different in that there is no change in behaviour backing the syntactic choices.
I would personally deprecate implicit error in catch at the tip of the hat but with no active harm, it's hard to justify. It would purely be syntactic preference. In contrast, the oldValue warnings have motivation.
I support the original proposal, but the above is why I also support the revision. It is true the warnings are not essential and could be introduced later, so maybe that's an argument just to keep everyone happy for the time being and only introduce the first version of the proposal.
(A nudge theory would be that if you just improved autocomplete along with implementing the original version of SE-0268, people would gradually migrate to using the explicit form more frequently of their own accord, so in a few years the deprecation warnings could be added without as much pushback.)
3 Likes
I think we should do
-
didSet { }for non-loading -
didSet(variable) { }for loading withvariable, and -
didSet(_) { }for loading without variable
It would be quite consistent with the current argument model (that _ is still loaded).
I'm quite neutral on the deprecation otherwise.
1 Like
I am strongly -1 on deprecating implicit oldValue.
I am strongly +1 on the rest of the proposal.
The stated reason for deprecating the implicit oldValue is "In order to avoid confusion as to when the getter is called or not, the implicit oldValue is now deprecated."
And a little more flavor from the last post of the original review on the topic:
"During the review, some concern was expressed around how the mere use of oldValue in the body of the code would change the behavior."
I don't see a great source of confusion. If I ask for the old value, it gets the oldValue. If I don't ask for the old value, it doesn't get the old value.
I don't think it is a surprise that when you use the implicit oldValue in the body of a didSet method that you are getting the old value. That is literally what you just asked for.
The existence of the variable oldValue is implicit but its use is always explicit in your code.
I also don't see how forcing folks to explicitly declare the variable clears up the supposed confusion. I've already asked for the old value once in my code by typing oldValue in the body of didSet. If I was confused when I typed it once, why I am less confused when I type it twice? (Or don't even type it, just click "Fix" because the tools told me to?)
One benefit I can think of is that you can see if you are accessing the old value by looking at the didSet declaration and not scanning the code itself.
But, if that kind of visibility is important to a developer, the explicit version of the syntax already exists. It doesn't need to be forced on all developers.
From the stats @beccadax cited earlier in the thread, in the source compatibility suite all uses of old value in didSet methods use the implicit oldValue.
There doesn't seem to be widespread confusion that when using the implicit oldValue you get the old value. I'd say, if anything, it seems that using the implicit oldValue is widely preferred and causes no confusion as to what it does.
I realize that with this proposal, didSet methods that don't access the old value will not fetch the old value, and so there is a performance implication if you add access to the old value. (Just as there is a potential performance implication when you add a line of code that newly accesses any property/variable.)
I think the performance implication should be mentioned in the documentation in The Swift Programming Language section on didSet. But I don't think the compiler needs to remind developers of the implication repeatedly.
For me, the amount of potential confusion among developers due to this change seems pretty low. Since the explicit syntax is almost never used, that seems to indicate that the implicit oldValue is preferred and that developers clearly understand what it does. That using implicit oldValue gets the old value seems almost self-evident.
I think deprecating syntax that is widely used and clearly preferred should meet a very high bar. I think the performance implications should be noted in documentation but I don't think the bar is high enough to deprecate the implicit oldValue.
13 Likes
I never use the "magical" values, and always specify them when I need them - so afaik, the proposal has absolutely no negative consequences for me, but the positive effect that other codebases would adopt my preference ;-)
Even if this is a breaking change, I really hope that we didn't reach a state where compatibility trumbs everything else, and we end up with something that favors the past over the future.
Actually, I'd be even fine with something that is more radical: I wonder why property delegates haven't been mentioned yet (I searched for the term ;-). Wrapping property access looks like a rather obvious use case for this concept, and it would be nice if piling up features would finally pay off by allowing at least minor simplifications.
2 Likes
I just jumped right in and suggest a syntax, but totally forgot to ask — does this second review cover the syntax, or only the deprecation? Since it’s not obvious in the proposal what didSet(_) would do, if it’s valid even.
Since @John_McCall’s comment seems to lean toward the latter.
didSet(_) does not compile anyway (since you need to pass an identifier). I’m not sure if it’s worth supporting though, since you can just write didSet(oldValue) and not use it in the body.
For people who are concerned about writing oldValue explicitly - during the pitch phase, I searched the entirely of GitHub (and the projects I work on) and found around 30k uses of oldValue in didSet vs. 400k uses of didSet without oldValue. Obviously, it’s not a perfect search but it seems to me that most people don’t even use the oldValue and the ones who do generally use it to compare change in value (which is something that could be addressed in the future through a didChange observer perhaps).
I don’t think this change will be too disruptive, because the warning will have a fix-it and the migrator can automatically apply it to all instances. We can also improve code completion to suggest oldValue so it doesn’t take any extra effort to write the explicit oldValue in parenthesis.
Of course, if people feel it’s better to delay this decision and have a separate pitch for all implicit variables, then I think that’s fine as well. But I think in practice, the impact seems to be much less than what’s being claimed in this thread.
It seems to me that whether we should deprecate it or not really depends on how much people value implicit variables. I would be interested in hearing what people are using the implicit oldValue for specifically (apart from something like oldValue != myProperty).
4 Likes
Another common use of oldValue is for state transitions — moving from the old state to the new state.
Which is why it feels right to consider Swift's use of implicit variables as a whole in a separate pitch instead of piecemeal. Personally, I don't think the syntax change is necessary. I don't think it accomplishes the goal to "avoid confusion", rather, I think it would generate more confusion as to why it's now required. @James_Dempsey said it well:
What was confusing was why the getter was called when the oldValue was not used inside of didSet which was determined to be a bug. That confusion has been cleared up by fixing the bug. There's no other confusion to "avoid".
8 Likes
Exactly this. The only confusion arises from the fact that in the current implementation the getter is called regardless of whether you use it or not - which is fixed by the change in the proposal.
The proposed new behavior (where oldValue is accessed if you call it and isn't if you don't) is exactly what most people would expect - and what many already assumed it was doing before this proposal brought the issue to light.
That's why the attempt to tack a warning about implicit oldValue declaration onto this proposal seems less about solving an ambiguity introduced by this change, and more about inserting the thin end of the wedge for removing implicit variable declarations altogether - as reinforced by the fact that almost everyone pushing for this warning openly admits they'd also like to see implicit error removed from do/catch at some point in future.
10 Likes
I think the arguments of the implicit oldValue and similar can only be made by experienced swift users. I remember the days when I started learning the language and how I was confused where these implicit values came from all of a sudden.
In general I‘m in favor of the whole proposal as I bumped too many times into the issue with the getter being called when I don’t expect it. These problems also exist in key-paths which is really painful and I hope this proposal is the first step in fixing these inconveniences.
2 Likes
I would actually suggest that if there is a part of the proposal as it stands that is confusing, it's the behavior that declaring (but not using) the oldValue causes it to be accessed.
It would make sense to me that
didSet(foo) {
// no access of foo
}
Should raise a warning, since it's unclear if this was a deliberate attempt to invoke the getter or just that the foo declaration was accidentally left after the reference was removed.
Instead, the only way to deliberately trigger an access to oldValue when you aren't actually using the value should be to write
didSet {
_ = oldValue
}
or
didSet(foo) {
_ = foo
}
Since this is exactly what you would write to invoke a getter in any other place in your code (I've written this plenty of times to instantiate the lazy view getter on a UIViewController for example).
8 Likes
FWIW, I agree with this assessment.
1 Like
This is why I want to push for didSet(_) { } as it would be inlined with parameter in func declaration.
It would be a little inconsistent with the rest of the accessors though.
It’s just simpler to explain to anyone (including beginners) that using oldValue calls the getter and not using it doesn’t OR declaring oldValue in didSet parameter list calls the getter and not declaring it doesn’t (and if you do use oldValue without declaring it, you get a warning and perhaps an error in a later version of Swift when things have settled down).
1 Like
True that, but as a whole, the accessor is the odds-one-out. Since it's the only one that doesn't allow for wildcard argument name (among func, subscript, even variable binding _ = ...). It is fine so far as there is little-to-no reason to request for the implicit value without using it, but this proposal does change the situation, even if for didSet only.
I’ve been writing Swift since a long time and I agree that there were a lot of things that confused me initially, such as these magical parameters. We should remember that most people who use Swift don’t actually participate in proposal reviews like this one, which more often than not get dominated by opinions of experienced developers (which is not necessarily a bad thing, but it’s easy to forget how things were when you were learning the language for the first time). I think people new to Swift might actually welcome the change (here’s a S/O question by someone being confused about the implicit oldValue).
I hope that the Core Team takes this into consideration during their decision making process and not let the opinions of just experienced developers dictate whether the implicit parameter be deprecated or not, as certainly it would help in making the behaviour less confusing for people who are new to Swift.
3 Likes
I’m in favor of the proposal. I am in favor of deprecating implicit use of oldValue and would like to see the same treatment applied to other places like newValue and error. But, I do not think that the behavior change needs to include the deprecation of OldValue.