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

An implicitly-declared variable is not an implicit behavior. That didSet { } always calls the getter today is both implicit and unexpected. No one is contesting this proposed change. However, referencing oldValue in the body is explicit. I fail to understand why anyone should be surprised that the getter is invoked.

6 Likes

I agree with those who have argued that deprecating oldValue when it is not explicitly declared is inconsistent and unnecessary.

Using oldValue obviously requires a get. Not using it obviously doesn't require a get - except that there is a bug in current versions of the compiler.

Jeremy

6 Likes

We keep talking about how didSet(oldValue) makes performance implications clear or explicit but I fail to see how that's the case. In my opinion, only documentation can make it clear what the implications are.

My thoughts exactly

I thought I would survey my own code for didset blocks. Among dozens of didSet blocks I found only a single use of oldValue. It was an implicit use. I did find one class with a dozen or so didSet blocks like the following:

didSet(newValue) { // code that didn't reference newValue or oldValue }

FWIW, I inherited that code but I certainly copied and pasted some of the existing didSet blocks to make new ones. I never noticed that the parameter name was reversed from what it should be.

What I think will happen if this change is implemented is that there will be lots of code that ends up with the oldValue parameter but doesn't use it. Unless there's a lint rule for this that gets widely used the fetching/non-fetching behavior will simply be ignored. Swift doesn't normally warn about unused parameters and there's not normally any negative side effect of not using parameters. didSet apparently will be a special case that violates this rule. I would frankly assume that unused parameters are optimized away, and I would be wrong in this case. I understand that side effects of getters need to be taken into account and I guess that's why the fetching behavior would have to be done.

And yes, I removed all my unused newValue parameters in order that my code doesn't fetch the oldValue unnecessarily in the future.

I'm in favor of leaving implicit oldValue the way it is.

1 Like

I think someone suggested further up the thread that we could add another warning for unused oldValue which could be useful in the scenario you described.

1 Like

Well, the inconsistency is in changing the semantics then. There’s no proposal to get rid of the implicitly declared parameters in willSet and set because whether or not you use them has no performance or behavior implications.

If consistency were actually that crucial, shouldn’t we reject the whole proposal?

Can you elaborate on what you mean by this? I’m not sure what is inconsistent about the semantics.

See this comment. It is not feasible to give willSet without newValue the same treatment as didSet without oldValue. So why should the syntax rules be “consistent”?

I think it would be more surprising to a user why they can implicitly call newValue but not oldValue. To me that would feel inconsistent. There is already a semantic difference between the two in that one is before setting and one is after setting, so I don’t see a benefit to changing the syntax. On the other hand there is a visual and behavioral consistency in keeping the way you access the property observers identically shaped.

For me, the consistency of both having “observer” semantics is why they should both have the same syntax, while whether or not the oldValue is fetched feels like an implementation detail.

1 Like

46⁄142 or roughly one third of the posts in this thread (#97 onward) were made after the review period was scheduled to have closed (December 9). Is this thread even supposed to still be open?

@Ben_Cohen

1 Like

Those are not at all comparable. When a type has an instance property which is not mentioned at all in a method body, no one expects the getter for that property to be invoked when the method is called. The behavior of didSet violates this expectation. The main thrust of this proposal is to fix this unexpected inconsistency. The performance aspects are important, but not the only (or main?) motivation.

For willSet, an attempt to optimize would lead to an internal inconsistency in behavior, but in either case, the behavior of willSet does not violate any expectations today.

2 Likes

Review threads are typically left open until the announcement thread is created so there’s a place to continue the discussion. A few times crucial feedback has come after the review technically ended.

Precisely. You have detailed why willSet and didSet are not comparable. You have given an excellent description of how it’s not just about performance but also about semantic differences, and how the implicit variable causes confusion in didSet but not willSet. Hence my point that the “consistency” argument here against changing syntax doesn’t hold water.

Fine. But expectations are important, too. There's no expectation that a method body with zero references to a property should read the property.

On the other hand, willSet is presaging a change to the property. When value semantics are in play, this is typically done as read-modify-write(back), and only optimizations alter that flow (in-place modification). So there's no behavioral expectation being violated when the optimization can't be done.

2 Likes

I'm +1 on the proposal. I'm somewhat ambivalent on the deprecation of implicit oldValue. I would have no issue moving to something more explicit (especially as it has a behavioural impact), but I feel that if that change was to be made it would also need to be made to both set and willSet for consistency (and should ideally be made at the same time). That said, sticking with the implicit oldValue will certainly save a lot of time for developers and confusion for those looking at older sample code and tutorials online.

One thing I haven't seen covered (though I may have just missed it) is regarding the naming of the argument in the the explicit didSet(oldValue) scenario. It's a minor detail, but would this imply that the oldValue variable would be able to be called anything, as with any function? Or would it still be required to be oldValue?

It already can be named anything, and this won't change. You can even name it newValue if you want to confuse everyone (someone did this earlier in the thread, most likely by mistake).

Yes, you can call it anything you want (but the fix-it attached to the deprecation warning is going to use oldValue since that's what the implicit variable is called).

*If the core team decides to deprecate the implicit oldValue :grimacing: *ducks for cover*

1 Like

Review Conclusion

Thanks to everyone for the feedback. Given the arguments against the deprecation warning, the core team has decided to accept the original proposal, of altering the fetch semantics without deprecating the implicit oldValue.

Thanks to everyone for the thoughtful discussion of the merits.

Ben

17 Likes