Pitch: didSet Semantics

[suyashsrijan]

Thanks! I put the deprecation in "Future Directions" as I feel it should be a separate pitch where we can discuss newValue, $0, error and other implicit arguments as part of the change.

[wadetregaskis]

@daveanderson

I agree with this ^ statement that the use of oldValue without explicitly writing didSet(oldValue) should be deprecated. IMO it is overly "magic" for the getter to be called iff oldValue is referenced (and not calling the getter if it is not).

I don’t find the ‘magic’ here inscrutable at all, because in my mind it’s just the compiler optimising away unused code. The compiler does that all the time, and as along as there are no functional side-effects, it very much should. In this case the only functional side-effect of this optimisation appears to be that reasonable code will no longer crash. I’d accept that side-effect.

Additionally, if I specify ‘oldValue’ explicitly and that introduces a crash even if I don’t actually use it in the method body, I’d consider that a weird compiler bug (which might not be fair principally, but is certainly fair in layman’s terms - “WTF? Why are you crashing just because I chose to name the parameter?!?”).

Furthermore, if explicitly specifying the parameter is to become required, then it should be so for the whole family of property observer methods, for consistency. Otherwise this would be leaking what’s essentially just a compiler implementation detail into the language syntax.

Thus I suggest that the question of implicit vs explicit parameters is actually best considered orthogonally - i.e. in a different pitch and justified only on its own merits.

[John_McCall]

Swift cannot in general eliminate unused calls to getters because such calls might have side effects. To put it another way, whether setting a property invokes its getter is part of the property's semantics, so this proposal is necessarily about changing those semantics and cannot be thought of purely as an optimization.

To me, didSet requiring a read of the old value even if you do nothing to suggest you want the old value is a violation of the Principle of Least Surprise. I think the best design here is:

• didSet() { ... } (i.e. explicitly taking no parameter) should definitely suppress the read of the old value.
• didSet(v) { ... } (i.e. explicitly taking a parameter) should force the read of the old value, but there should be a warning if v is unused.
• didSet { ... } (i.e. implicitly taking an oldValue parameter) should force the read of the old value only if the implicit oldValue parameter is used.

Of these, the third seems like the only controversial point.

[xwu (Xiaodi Wu)]

@John_McCall

I think the best design here is:

• didSet() { ... } (i.e. explicitly taking no parameter) should definitely suppress the read of the old value.
• didSet(v) { ... } (i.e. explicitly taking a parameter) should force the read of the old value, but there should be a warning if v is unused.
• didSet { ... } (i.e. implicitly taking an oldValue parameter) should force the read of the old value only if the implicit oldValue parameter is used.

Of these, the third seems like the only controversial point.

The controversy being that a change in the semantics of the operation based on what’s in the implementation body is difficult to understand and work with.

I think the previously proposed change is, although source-breaking, the cleanest final design:

• didSet(x) forces a read (and by extension didSet(_) would likewise)
• didSet suppresses the read, and it is an error to use oldValue

I think I’d find it surprising (or, even if not surprised, too subtle) that a new syntax didSet() wouldn’t match the behavior of didSet.

[John_McCall]

Maybe, but that’s premised on abandoning oldValue entirely, which hurts usability somewhat as well as being (as you say) source-breaking.

[suyashsrijan]

I like the first option i.e. didSet() { ... } to prevent the oldValue from being loaded and accessible inside the body.

So:

1. didSet() { ... } -> the oldValue is not loaded and not accessible
2. didSet { ... } -> the oldValue is loaded and is accessible
3. didSet { ... } where oldValue is not referenced -> warning with fix-it to add ()

We can avoid the source breakage with this approach and most people will have to make no changes to their existing code (i.e. they won't have to change their didSet { // use oldValue } to didSet(oldValue) { // use oldValue }, which I personally think would be bad for usability.

[GetSwifty]

@suyashsrijan

didSet { ... } where oldValue is not referenced -> warning with fix-it to add ()

This would mean the implicit-no-parameter declaration would have different behavior than the explicit-no-parameter declaration.

My summary:

• There are corner cases where didSet accessing oldValue can cause problems.
• If the oldValue is never used in didSet, it's unnecessary to access oldValue.
• didSet can be changed so it doesn't unnecessarily access oldValue
• That change should have little to no negative effects on existing code and will fix bugs/problems that people are or are not aware exist.

Outside of frustration with magic parameters, (which IMO is off-topic), the primary pushback seems to be concern about implicit/magic behavior.

If anything I would say this change is removing magic. There's currently no user-visibility to indicate that oldValue is accessed by didSet when not used. Bugs caused by this are unintuitive and the explanation is weedy.

As this is how I would expect it to work and barely more than an implementation detail, I support just making the change.

Allowing an empty parameter list when people want to be more explicit makes sense since it matches the way functions are declared. However if there is a push to re-examine the ergonomics around Computed Properties and Property Observers, it might be good to wait till that discussion occurs.

Changing any other behavior or requiring users to request this behavior explicitly seems unnecessary and over-designed.

[griotspeak (TJ Usiyan)]

What about making

didSet { ... } (i.e. implicitly taking an oldValue parameter)

continue to work and trigger a warning?

[suyashsrijan]

Thanks, to be honest, I am happy with not changing the syntax for didSet (which is why I have put this change in Future Directions). However, I thought if people (including the Core Team) want the proposal to include a change to the syntax for didSet then that could be one possible solution (that also does not lead to source breakage). However, if there's concerns then I am happy to opt for a different solution.

Anyway, most people in the thread agree that we shouldn't require users to explicitly declare oldValue or add (). I think forcing rest of the users to make changes to their existing code is unreasonable, especially given this is a special case. One of the great things about the proposed change is that it would bring the benefits of the change to everyone without having them to tweak their code.

As I wrote in the proposal originally, changes to implicit parameters deserve a separate pitch, which would include other implicit parameters as well. This change is only about semantics and I think it shouldn't really require changes to the syntax.

[suyashsrijan]

What about making
didSet { ... } (i.e. implicitly taking an oldValue parameter)
continue to work and trigger a warning?

How would you silence the warning?

[griotspeak (TJ Usiyan)]

Currently "change the code" is the only way but I am personally in favor of (the thing that the core team has explicitly made statements against) being able to pass flags to suppress individual warnings.

[suyashsrijan]

A lot of people use didSet without using the oldValue, eg:

var arrayOfModels: [Model] {
  didSet {
    tableView.reloadData()
  }
}

I don't think we can expect users to change their code here.

[griotspeak (TJ Usiyan)]

to clarify, I am talking about cases where you use the implicit parameter oldValue. I don't have a strong feeling about the case you have there

[John_McCall]

That's essentially the "deprecation" path — it's forcing people to either stop using oldValue or declare it explicitly. So opinions on it are going to split based on whether you consider the implicit parameter to have positive value or not.

I personally think that we probably should've required the parameter to be explicit from the beginning, but that it's not a serious enough problem now to justify regressing usability. I do think it's important that didSet { ... } without any explicit parameter clause and without any use of the parameter stop performing an implicit read of the property; most of the rest of the design I'm less certain about. But I'm just speaking for myself, not the entire core team.

[xwu (Xiaodi Wu)]

@John_McCall

I do think it's important that didSet { ... } without any explicit parameter clause and without any use of the parameter stop performing an implicit read of the property; most of the rest of the design I'm less certain about.

Let's reason it through from that starting point. If we agree on this point, then there are two and only two options for didSet with implicit oldValue:

A. It works as it does currently (an implicit read); or
B. it is deprecated/removed.

With option B, you have a usability regression, since it is source breaking and requires the user now to write something to trigger an explicit read.

With option A, you have the following problem, which does not exist today. Given the following:

didSet /* 1 */ {
  /* stuff */
  frobnicate(oldValue) /* 2 */
}

Essentially, what happens at (1) would depend on what's written at (2).

It would be another thing if the implicit read happened at (2); then you could say: of course, a read happens when you're accessing the result of that read. But that's logically impossible; the read must be performed before executing the didSet implementation since, well, the value would have been set by the time we get to (2).

So, the semantic meaning of didSet would depend on what happens inside its implementation. This in and of itself is a usability regression from the status quo.

@John_McCall

So opinions on it are going to split based on whether you consider the implicit parameter to have positive value or not.

There's more to it than that; one's judgment of the current value of the implicit parameter is only a small (and likely irrelevant) portion of whether choosing option B is preferable over option A.

Rather, we would have a usability regression from the status quo regardless of which option we choose, and the judgment is whether we prefer explicit-with-usability-regression or implicit-with-different-usability-regression.

[John_McCall]

That's an interesting perspective. These are clearly different senses of usability, however: one is "there is a new syntactic burden to make this value available", and the other is "an important aspect of the behavior of this property cannot be known without examining the entire didSet body".

[GetSwifty]

@xwu

So, the semantic meaning of didSet would depend on what happens inside its implementation. This in and of itself is a usability regression from the status quo.

For it to be a usability regression, it would have to negatively effect users. I'm unaware of any negative consequences caused be removing an invisible get when it's not used.

Whether it happens at "1" or "2" matters from an implementation point of view, but from a user perspective in >99% of cases it won't make a difference. It's a corner case to even create this problem, and generally only by advanced users. Putting half a paragraph in the documentation should answer any questions for the few having issues. This change should fix the vast majority of those problems anyway, so that number should be very small.

Unless there are harmful effects I'm unaware of, this fixes problems, and creates no new problems. That's a usability win

[xwu (Xiaodi Wu)]

@GetSwifty

Whether it happens at "1" or "2" matters from an implementation point of view, but from a user perspective in >99% of cases it won't make a difference.

I think you’re missing the point. I am not arguing against removing the implicit read; in fact, I am quite for it.

I am concerned about an implicit read happening where a user may not expect it. The issue isn’t whether “it” happens at 1 or 2, but whether an implicit read happens at all. This cannot be known if we maintain source compatibility without a reader examining the entire implementation body.

Where it matters is when a read has side effects; since side effects are not annotated in Swift, an implicit invocation is doubly invisible, and if knowing that it occurs requires examining the entire implementation body (rather than ‘just’ knowing that it’s part of the semantics of didSet), then it’s triply invisible. Unexpected behavior hidden beneath three layers of invisibility is a usability concern.

So now you’ve been made aware of the harmful behavior at issue.

[xwu (Xiaodi Wu)]

@John_McCall

That's an interesting perspective. These are clearly different senses of usability, however: one is "there is a new syntactic burden to make this value available", and the other is "an important aspect of the behavior of this property cannot be known without examining the entire didSet body".

Right, different senses, but two sides of the same coin. To make the important aspect of behavior known upfront, one must have some visible syntax; to avoid this burden of new syntax, one must forgo upfront knowledge of the important aspect of behavior. Put in this way, I think Swift has traditionally chosen one alternative over the other.

[GetSwifty]

Ok, I guess in that case I agree with you. It would be nice if the get wasn't ever implicit. Is there a way to accomplish that without source-breaking changes?