SE-0268 (Review #2) — Review didSet Semantics

Evolution Proposal Reviews
81

But don’t worry, most set blocks shouldn’t be more than a few lines and would be defined very close to the member declaration, just like most didSet blocks!

82

Isn't that a False Dilemma? There are numerous ways you can distinguish the semantics, the most aggressive of which is requiring new syntax.

IMO the "right" way to notify users for something this in the weeds that will affect so few people is with documentation. The problem is, the most popular tooling (Xcode) for Swift has no accessible documentation for either didSet or oldValue. That's the problem I think needs to be addressed.

Keep in mind, the goal is "to avoid confusion as to when the getter is called or not". To me, having a parameter does not, on it's own, indicate that kind of a semantic difference. If that's true of most users they will still have to read the documentation in order to know about the difference. Yes, requiring the named parameter will make it obvious when that's specifically what's being looked for, but how often will that happen?

It's a corner case to use oldValue (6% according to @suyashsrijan: SE-0268 (Review #2) — Review didSet Semantics - #49 by suyashsrijan), IME it's a corner case for oldValue to be used for anything other than a check for didChange, and it's a corner case of that where this semantic difference would cause a problem. So, this deprecation would make 6% of didSet usage more verbose, to indicate a difference that will only matter to...I'll say 0.1%, (though it's probably even smaller). And even in that case they will still have to manually find the documentation for these semantics.

6 Likes
83

I don't understand why we need to change code to distinguish between these behaviors. If didSet uses oldValue, the getter is invoked. If not, it's not. That should be simple enough to learn and/or document. Especially if we consider the current behavior to be a bug.

11 Likes
84

I think Swift aims to favour clarity over brevity, and being more explicit in case of oldValue specifically doesn't seem to have any harm.

In the pitch thread, it was also mentioned that this should've been the case since day 1. However, we do have an opportunity to fix that right now if we want to. I would be interested in knowing if people are willing to change their opinion if we also update code completion as a follow-up improvement. I think that once that is fixed then people will feel a bit different about the whole thing because you won't have to write it out manually each time (if you even run into that situation, which is not that common, at least according to my research). Of course, people who think there is a lot of value of having implicit oldValue are not going to be persuaded by changes to tooling.

85

This sounds to me like an excellent rationale for a linter setting or style guide rule requiring you to explicitly declare oldValue. But I don’t think it reaches the level of necessity to warrant a warning.

3 Likes
86

I'm +1 for the bug fix, but -1 for the deprecation of the implicit oldValue. Here's how I'm looking at it:

  • Not using the implicit oldValue and not declaring any explicit arguments implies (correctly, now) that the getter will not be called
  • Using oldValue or another argument in the body of didSet makes it pretty clear that the getter is being called (whether or not the argument was explicitly declared)
  • Declaring an explicit argument but not using it, when done intentionally and looked at after reading this thread, makes sense to have the getter be called, but does not make sense without the context of this thread. If I were to read this in anyone's code (including my own), I would have thought it slipped their mind and was a mistake.
  • Being forced to ensure that an argument is explicitly declared if I am to use it but not declared if I am not using it (so that the getter is not called) seems like a recipe for disaster, at least for me personally.

The gist of this is that if the implicit oldValue is not deprecated, developers have a choice on whether or not they'd like to use an explicit didSet(oldValue), while if it is deprecated we are forcing every developer to do something that a large amount of people clearly don't want to do.

10 Likes
87

That’s an interesting observation. One instance would be an empty didSet, but I suppose this would be pretty rare in practice (and really something that a power user would use for something very specific), but still, seeing an empty didSet would definitely look like a mistake if someone didn’t know that it was calling the getter. I think the difference would be in the explicit-ness of that (i.e didSet(oldValue) {} vs didSet {} - which one makes it explicit that the getter is called?).

88

Would you mind clarifying what you mean by "changing syntax" here?

It seems to me that this discussion has come down to deciding between two alternatives:

  1. Invoking the getter depends on the presence of the "didSet(xxx)" syntax, where the parentheses are the important part and the name inside them is immaterial.

  2. Invoking the getter depends on the presence of the "xxx" variable in the body of the accessor, where the occurrence of the name is the important part, and it is immaterial what determines the name (implicit or explicit declaration).

To me, these are both "syntax", and the alternatives are between changing what the relevant syntax is (#1), and changing what the existing syntax indicates (#2). Both of those could plausibly be described as "changing syntax".

FWIW, my vote is currently for option #2, because I haven't yet seen a compelling reason to adopt #1.

edited to fix typo "setter" for "getter"

89

If the idea is to start the process of de-sugaring accessors (please don't :innocent:), IMO that should be a separate pitch that includes all of them. It's a significant change in direction that shouldn't be evaluated as a knock-on feature of another pitch. Regardless of the original intention, there are now several years of trends and norms to be taken into consideration that affect that decision.

As mentioned previously, the tooling around this needs improvement regardless of this pitch. If we're taking such improvements into account, I think it makes more sense for this be evaluated after the new tooling is in place, not before.

5 Likes
90

I mean, the tooling changes I suggested are something that should’ve been done a long time ago and it’s incremental work that I think should be done regardless of whether we deprecate the implicit oldValue or not. Here’s one such recent change related to didSet: [CodeCompletion] Suggest the property name in its didSet clause by rintaro · Pull Request #28319 · apple/swift · GitHub

The way I think it would work is that when you start to write didSet, code completion is going to offer two suggestions: one with a parameter name and one without.

91

Yasss that's been bugging me the last few weeks!

Yes! this should be done (for all accessors) regardless of this pitch. It wouldn't surprise me if some users don't even know you can have a custom name for oldValue.

The completion should also add the (always required) new code block. Right now it just completes the name.

What I would really love is to be able to start writing didSet after a property and have code completion give the option to add the whole expression!

2 Likes
92

Assuming after all this discussion the Core Team still decides to go ahead with the syntax change, I'd like to suggest making the warning more symmetrical:

didSet {
   log(oldValue) // warning: oldValue not declared, consider adding declaration
}

So it makes you add the declaration. And then later you remove oldValue from the body so you get:

didSet(oldValue) { // warning: oldValue is never used, consider removing it
   log("boo!")
}

This is similar to the var / let fix-it that depend on whether you mutate a local variable; it goes in both directions.

If you really need to trigger the getter but not use the value, this is how you can avoid the warning:

didSet(oldValue) {
   _ = oldValue // triggering the getter, no warning
}

This could be an alternative fix-it when oldValue is declared but unused.

I'm still of the opinion that implicit oldValue should not be deprecated. But I think the approach above is better than what is currently in the proposal. In particular you'll get reminded when the declaration becomes unnecessary after you remove oldValue from the body. It could also apply nicely everywhere else we have implicit declarations of this kind, if it ever comes to that.

9 Likes
93

Yeah, that sounds like a reasonable tweak to how the new warnings work.

94

I love the proposal, but I’m not a fan of didSet(oldValue) nor deprecating didSet raw when it uses oldValue.

The former seems unnecessarily wordy and raises questions for me (can I use a different identifier than oldValue?) and screws up the symmetry with willSet’s newValue.

As near as I can tell the only reason to have the didSet(oldValue) form is to tell the programmer “Hey you’ll be calling the oldValue here that might be slow,” except it’ll be no slower than it was before this change was made, and we’d have to live with this syntax forever.

Also oldValue just seems like a bizarre thing to call out. I can’t think of other places in Swift where you have to mention some variable in order to tell the compiler you understand and consent to it being slightly slower.

Note also that the only case in which this proposal might cause some source breakage (someone was crazily relying on fetching oldVaue and some side effect of it) is NOT addressed at all by the user seeing didSet(oldValue) — that just tells them it definitely WILL be fetched. So we’re asking the user to learn something new (eg, didSet(oldValue)) in order to work around a possible but very unlikely source-breaking change, but if the user has to learn something new to work around this change, why not have the new thing be “hey didSet has different semantics” instead of “hey didSet has a new spelling for your case?”

The same thinking applies to why we shouldn’t warn if the user doesn’t use didSet(oldValue) but accesses oldValue — I think the former is wordy and silly and ugly and useless (the latter based on my possibly-flawed understanding of its creation) so there’s no need to deprecate raw didSet in the “use oldValue” case. Just skip the adding former.

-Wil Shipley
Chief Monster

1 Like
95

didSet(oldValue) is not new - you can write it today (and yes, you can swap oldValue with any other identifier).

The problem with didSet is unique, so while it does screw the symmetry, it’s also (probably) not a good idea to change the rest because of it (as they don’t have the same problem).

I think it boils down to how the semantics is communicated to anyone reading the code. With didSet(oldValue), it’s clear that the oldValue is fetched and with didSet it’s clear that the oldValue is not fetched. You don’t have to read a special section of the Swift documentation to realize that. A lot of people were already unaware that the oldValue was always being fetched, even when unused. So, being explicit can help make that thought go away and reduce the chances of confusion.

Some people prefer the presence of oldValue in the body to be the thing that communicates the semantics. I don’t know what the right way is, but I also don’t see any harm in being explicit. Anyway, this has been discussed in-depth in this thread, so it really depends on whether you find value in the explicitness (while keeping it mind that the semantics are changing) or whether you find it “noise” and would rather keep using the implicit oldValue.

I think we’ve all expressed our opinions and it’s up to the Core Team now to decide whether to deprecate or not.

1 Like
96

Btw, I’m still confused about how willSet evades this treatment.

It doesn’t have any benefit for set operation, but it’d have some optimization benefit for modify operation, no?

Esp. if we do partial-modification to the variable

If we have willSet with newValue then it would force the operation to hold both versions of variable, preventing in-place partial modification.

var observed: ... {
  willSet {
    // `observed` & `newValue` must be separated instances
    // Can’t mutate in-place
    print(observed, newValue)
  }
}

So even if we mutate a very small chunk of observed it is forced to copy an entirely new value into newValue.

// This cause CoW to `observed`,
// even if we only modify `partial`
a.observed.partial = ...

Previous discussions seem to apply to assignment, but I don’t see how it applies to partial modification.

Or am I missing something :thinking:.

97

There are two ways to give willSet semantics for a non-instantaneous modification (e.g. calling a mutating method on the property):

  • You can do what Swift does today, which is to force the modification to happen to a temporary copy and then call willSet immediately before writing back.
  • You can call willSet before the modification begins.

The second makes more sense to me abstractly, but it's totally incompatible with willSet getting passed the new value, and it's a radically different point in the execution of the program than the first (in our example of a mutating method, either before the call or after it). It would be unacceptable for the timing of the call to willSet to shift this much based only on whether newValue was used in the body. So we're locked into the first option.

There's really no way to optimize willSet given those semantics; it has to make a copy.

5 Likes
98

I'm in favour of this change and deprecating implicit oldValue.

I think this meets the bar needed for a "source-breaking" change (it isn't really source breaking IMO, it just produces a warning for the now-deprecated thing). C++ does similar things from time to time - one of the projects I'm working on now is getting warnings saying 'register' storage class specifier is deprecated and incompatible with C++17 from a 3rd-party dependency (even though we're compiling in C++11 mode).

Just like Clang still compiles that code with register in it, we will still compile code with implicit oldValue in compatibility mode, so I don't think this is such a big deal or anything developers aren't used to handling. That's not to say we should make a habit of it.

2 Likes
99

I am in favor of the behavioral change, but I am not in favor of deprecating implicit oldValue. I have not heard an argument for why it is actively harmful, and I don't see the value in adding warnings for code that is not at all incorrect.

2 Likes
100

If it's deprecated, it will be removed in a future version. It's not source breaking currently, (though to some warning == error), but it will be in future. My guess is that would happen with Swift 6.0, but that's likely yet to be determined.

My understanding is C++''s version usage is pretty fragmented and high adoption of the latest version takes several years. IME Swift version adoption is incomparably faster :slight_smile: