SE-0268: Refine didSet Semantics

+1, mine is same as quoted.

I like this proposal. My only question is, what are our recommendations to users who do depend on the current behavior (I’m sure there are some)? What changes can they make that we will guarantee will create an oldValue even if they don’t seem to need it? Whatever they are, we should document them.

This would be a much larger change than it sounds like; the assumption that all properties are readable is baked into the language pretty deeply. It would definitely need to be its own proposal. (Which I assume is at least part of why Ben asked for it to be discussed elsewhere.)

2 Likes

Could they not just do:

struct Thing {
  var a: Int {
    didSet(oldValue) {
      
    }
  }
}
1 Like

Please add a sentence (or paragraph) on how this affects willSet/didSet for overrides, as opposed to stored properties. (In particular, I think the corresponding optimization for willSet becomes more interesting.)

I do still think the "explicitly specify an argument" approach is a better way to go (didSet(oldValue)). Having the body of your didSet change the CoW behavior for those using your setter seems subtle. Then again, I suppose closure captures are the same, and other than implicit self access we don't require those to be explicit. (Though it's been brought up before.)

3 Likes

I tend to agree. I have always found the ability to reference newValue/oldValue without them seemingly being defined as something that runs counter to Swift. I feel like they should have been something more akin to $oldValue/$newValue, since $ appear in other magic places where synthesis happens.

Can we really change that at this point though? I don't feel like not specifying newValue/oldValue explicitly is actively harmful enough to warrant requiring them

4 Likes

Probably. didSet(_) should probably work too. But wherever is supposed to work, we should explicitly call it out in the proposal, change log, and release notes, and we should make sure the implementation tests that it works.

7 Likes

The didSet observer is synthesised without the oldValue if you do not reference the oldValue inside the body of the observer.

For example:

var foo1: Int = 1 {
  didSet { print("bar" } // We synthesise a didSet() and the getter
                         // is not called to fetch the `oldValue`
}

var foo2: Int = 2 {
  didSet { print(oldValue) } // We synthesise a didSet(oldValue) and
                             // the getter is called to fetch it.
}

This is clarified in the proposal, but if you find it confusing then I can amend it to make it a little more clear. If anyone is relying on the current behaviour, they should hold a reference to the oldValue inside the body of the didSet to ensure the getter is called.

  • What is your evaluation of the proposal?

Warmly positive

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes, and the proposal explains it very well.

  • Does this proposal fit well with the feel and direction of Swift?

Yes. I appreciate that the proposal is presented as a bugfix.

The comment by @brentdax addresses well the real concern of supporting people who rely on the buggy behavior.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

n/a

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I followed the pitch, and I quite enjoy the simplicity and focus of the proposal.

1 Like
  • What is your evaluation of the proposal?
    +1
  • Is the problem being addressed significant enough to warrant a change to Swift?
    yes
  • Does this proposal fit well with the feel and direction of Swift?
    yes
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    I've followed the pitch

IMO, a fix-it of "hold a reference to the oldValue inside the body" is subtle and confusing as the canonical semantics-preserving solution. I think the proposal should explicitly call out the didSet(oldValue) solution as the preferred one and mention that simply referencing oldValue in the body is an alternate solution.

6 Likes

Sure, I can update the proposal text to note what the solutions are if one wants to preserve the old behaviour.

EDIT: To clarify, the current implementation does support the first workaround (so didSet(oldValue) { ... } will call the getter even if the oldValue is not used).

This is a really nice proposal: it's small in scope, easy to understand, and addresses a number of clear issues in one go as well as providing a nice optimisation opportunity. I'm very much in favour, this is great work.

Yes. Clearing up these semantics would be welcome.

A quick reading of the proposal.

1 Like

+0.5. This proposal addresses a number of important issues which appear in real-world code, and the tradeoffs seem comparatively minor.

I am a little concerned that it introduces a small source break which doesn't lend itself well to automatic migration or a warning & fix-it. The proposal calls the current behavior "buggy" when justifying breaking code which relied on it, and I don't entirely agree with that assessment. That said, I'm fairly confident this breakage won't appear very often in practice so I remain in favor as long as the behavior change is communicated well. It would be nice if we could scan GitHub repos, etc. to quantify the impact of the break, but I don't think that's feasible for this change.

Yes, I found the examples in the motivation section of the proposal illustrated nicely how the current behavior can be surprising and harmful.

I followed some of the pitch discussion and read the final proposal.

Sure, is there anything specific you want me to clarify (apart from the fact that we won't call the superclass getter)?

If that's the only effect I guess that's sufficient! I just don't want people reading the proposal to forget that it applies to override willSet/didSet too.

1 Like

I think this is my first ever post in this community so excuse me if my tone or message is inappropriate or not in-depth enough.

I have to say that I read this proposal and I immediately thought 'why isn't this happening already?' Meaning, I'm a huge fan of all the low-level stuff Apple has been doing over the years, like LLVM and the continuous improvements the Compiler team does to keep our code as fast and lean as possible. And given how crazy Apple is about maximum performance and efficiency, I would've thought that Swift being in version 5.1 already, this would be a thing, though I understand why it isn't.

As such, huge +1 on my end. My only question would be how does this complicate future development of getters and setters, since by my experience, making things 'smarter' often makes thing harder to work with down the line.

4 Likes

Absolutely a +1 from me, this seems like it could yield very significant improvements to some code bases and feels like the behavior we should have had all along.

2 Likes

Extra subtle given that it’s a change that would be introduced for Swift 6 which can alter the behavior of existing code.

@jrose’s point sums up my principle concern with the solution as presented here.

I would echo @brentdax’s point below that we should be explicit about how to trigger the previous behavior:

I think it's a good change, and thought it would be especially nice in the context of lazy var (where the computation of the "default" value can be expensive).
But then I wanted to check... and the compiler does not even allow observers for lazy properties!

Could SE-0268 help lifting this limitation?

I created a discussion thread about it. I have a WIP PR to allow observers on lazy properties, just needs a little push. The behaviour should be the same, except that the oldValue in the didSet would just be the initial value if the property hasn’t been forced yet.

Of course, this proposal will prevent that from happening if you don’t refer to the oldValue, which is nice.

2 Likes
Terms of Service

Privacy Policy

Cookie Policy