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

Evolution Proposal Reviews
101

You will still be able to change the language version back to Swift 5 and build without errors

(if it turns into an error in Swift 6 or whatever the next language version is)

102

That's small comfort to those who think the current syntax is perfectly fine, and who will want to use new language features.

I really don't understand why the two changes need to be coupled, when there is clearly no opposition to the first, and much to the second.

9 Likes
103

Although the name in the explicit syntax could be anything, so didSet(foo) or didSet(ov) are also valid. So someone still might need to read the documentation for didSet to figure out what is going on. One reason we tend to use oldValue in these conversations is because that is the name of the implicitly named value.

You could make the case that the existing widespread use of the implicit value and its automatic and fairly clear name makes for more consistent and readable code across all Swift code.

(And by widespread, I mean ‘in didSet implementations where the old value is used, it seems the vast majority of developers seem to use the implicit oldValue to access the old value.)

I really do hope that the implicit oldValue is not deprecated. I don’t think it’s required for the overall didSet change.

I don’t think writers or readers of didSet blocks will misunderstand that when they access oldValue implicitly they are fetching the old value.

And finally I think deprecating usage which is clearly preferred (based on previously mentioned code base audits) deserves its own discussion, with its purpose clearly stated (e.g. ‘Deprecate implicit ‘oldValue’ in didSet’), as opposed to an add-on to a proposal review that - reading its title, does not seem to have any syntax implications at all.

1 Like
104

To be a wee bit elitist, perhaps, I would say that catering to people who would misunderstand should not be an objective for Swift Evolution.

105

Readable or not isn't really the biggest concern here, IMO.

The proposal does an excellent job of illustrating how the need to access oldValue can lead to performance traps. In that case, we had a bit of a language bug in that the mere presence of a didSet watcher caused oldValue to be accessed, even if you didn't use it. It seems that everybody is in favour of fixing that.

So given that we acknowledge the potential performance hit, do we not think it should maybe be a bit less easy to fall back in the hole? Personally, I found this proposal to cause me to rethink my entire usage of oldValue; sometimes the work I was saving by if property != oldValue wasn't worth the cost of actually copying that old value and preventing in-place mutations.

Also - we (or the core team, at least) get to decide when/if it ever becomes a hard error. Maybe it will just be a warning with Swift 6 and an error in version 7.

1 Like
106

How is it less easy when there's going to be a fix-it to add (oldValue) for you? On one hand, people are arguing that it will be trivial to shut up the warning, and then here you're arguing that the warning is an impediment to make it harder to accidentally use oldValue. And what is there about (oldValue) tells anyone they may be falling into a performance trap?

No one accidentally uses oldValue. And, as I wrote above, I don't think we should cater to people who don't understand that accessing the old value will call the getter for that property.

I still don't see the active harm by continuing to allow implicit usage of oldValue.

That is no comfort for those who wish to build with "treat warnings as errors", but also find this change completely needless.

5 Likes
107

But it is. Nearly everyone agrees on the first part of the proposal (not calling the getter if the old value is not used/indicated in some way.

However, the second part of what we're discussing is readability — whether or not it's clear that the getter is being called in different scenarios.

As I've said before, I personally think that we shouldn't deprecate the implicit oldValue, because I think it's better how it is, but I think the important thing is that developers be able to choose how they want to style their code (I, for example, think that didSet(oldValue) is extra information and unneeded).

2 Likes
108

I am not sure how the explicit change to have to declare a name for the old value is a good indicator of performance implications. If we think implicit naming in general is bad, let’s discuss that as a separate proposal.

8 Likes
109

Am I missing something or is this only a meaningful performance hit for specific edge cases, most of which can be easily avoided?

e.g rather than changing an array, map it.

// From the proposal
mutating func update() {
    for index in 0..<items.count {
      items[index] = index + 1
    }
  }

// v.s. an alternative that will only trigger `didSet` once
mutating func update() {
    items = items.enumerated.map { $0.offset + 1 }
}

Not only is that less code, but it would be my preference working with didSet regardless of the current proposal due to avoiding the extra noise of didSet being called so many times.

110

It's not perfect, but then nothing is - it isn't a fundamentally awful thing in the language, or we'd just remove it outright. The point is that it's a bit more explicit, while the idea of having it always floating around at your fingertips that you get with it being implicit, also gives you the feeling that it's zero-cost.

That should be a small intersection. Most people who build with "treat warnings as errors" are cautious and would love something that might better help spot bugs in their codebase.

111

I may not be everyone, but my instinct is that the only people that would even be likely to think about performance aspects are the same people that would understand how to use tooling and profiling to figure out where a problem would be. I don't think that this change would notify a beginner about any performance implications because I don't think that they are thinking about them. I also don't believe that a small percentage of a chance of telling somewhere there are performance implications are worth the churn in the code base or loss of (what I find to be) a useful syntax.

Specifically to the text of the updated proposal:

In order to avoid confusion as to when the getter is called or not, the implicit oldValue is now deprecated

To me the only logical thing that makes sense is that if you see oldValue in the body, it is called. I don't think the value being implicit or explicit has no bearing on how most people would consider the performance aspects.

Edit for clarity:
On first sight without seeing this thread, here's what I would expect to be the behavior:

didSet {
  // no performance impact
}

didSet(oldValue) {
 // no performance impact
}

didSet {
  _ = oldValue // has a performance impact and side effects
}

didSet {
    guard oldValue.doesSomeThing else { return } // performance impact
}
4 Likes
112

To me, passing a parameter also gives me the feeling of zero-cost in Swift.

That's a much better summation of what I've been trying to say :slight_smile:. If the performance implications are big enough to make it clear users, (I don't think it is), shouldn't it be actually explicit?

var myVar: String {
    didSet {
        let oldValue = makeCopyOfOldValue()
        if myVar != oldValue { doAThing() }
    }
}
2 Likes
113

It's really about the semantics, and not just about the performance implications. It's about how we communicate the (new) semantics to a person reading the code. Basically, we have two choices:

didSet(oldValue) {} // fetched
didSet {} // not fetched

vs.

didSet { 
  // fetched or not fetched depends on use of implicit 'oldValue'
}

Some people prefer the former, because the latter is too "magical" and already confuses people today. It also has been argued in the past (by people including at least one Core Team member) that we shouldn't be adding sugar to accessors. We have an opportunity now to fix it, at least for one accessor that is problematic today. Whether we do it for the rest is a separate matter IMO because they don't have the same problem as didSet.

This will fetch the oldValue, because you're explicitly requesting it (this is equivalent to just didSet {} today). If I wrote didSet(oldValue) and it didn't fetch the oldValue, I would find that surprising.

3 Likes
114

IMO, I think the old behavior should just be called a bug, a somewhat nasty one at that. As anyone taking advantage of that access behavior was probably doing something way too clever for their own good. My initial support for removing the implicit was because of my general objection to most of the implicit variables in Swift.

2 Likes
115

To be clear, I understand that. What I was trying to say was that without the context of this forum thread that would be my expectation because it isn't used. Ultimately if the goal is to provide context for if it is called, I'm arguing that it does not.

To me, this proposal is about this:

didSet {
// there used to be a bug here
// but not anymore
}
116

Currently my mental model for didSet is:

didSet {}
Code like this is generated: () -> Void

didSet { oldValue }
Code like this is generated: (Value) -> Void
1 Like
117

There is an item missing in your second choice. If implicit oldValue remains, there is still nothing stopping developers from explicitly declaring the name of the old value to be used as they have always been able to.

didSet(oldValue) {} // fetched is part of both alternatives.

If a developer or organization or open source project decides they want to make exclusive use of the explicit version part of their coding standard or maybe code linting, they can do so. And folks/orgs/projects that feel that accessing oldValue in the body of didSet is clear enough can continue to use it.

118

I don't have anything to add but another vote that the behavior change is great (and what is logically expected), but the deprecation is unnecessary (and logically unexpected). If we wish reevaluate implicit parameters, it should be done separately and holistically.

10 Likes
119

I think it's basically just binary behaviour:

didSet(oldValue) {} // *always* fetched
didSet {} // *never* fetched

vs.

didSet(oldValue) {} // *always* fetched
didSet {} // may or may not be fetched

You can argue that the former set of rules is a bit simpler to explain to someone when talking about didSet and also easier to remember.

120

I don't see that as any clearer or easier to explain than this:

// *always* fetched
didSet { oldValue }

// *never* fetched (used to fetch, but that was a bug)
didSet {}

The bonus to this model is that there is also no source compatibility issue.

2 Likes