Pitch: didSet Semantics

I think this would be pretty bad UX. One very common use case for didSet is to do:

var value: String {
  didSet {
    if value != oldValue {
      // Do something
    }
  }
}

In all of those use cases you’d then have to write didSet(oldValue) instead, making your code harder to read without any real benefit to you. We’d essentially be penalizing the common case for an improvement to the uncommon case. Feels like the wrong trade-off.

13 Likes

This may be true to some degree, but I think the fixes to the problems that are mentioned overweights the lost convenience. As @Chris_Lattner3 once said in response to some language sugar pitch of mine, it would be bad if we provide sugar for convenience which will prevent us from having other important language features.

This proposal I assume will also fix the key-path bug that I hit so many times already. Every time I hit it, it forces me to uglify the implementation with a closure based workaround because it prevents me from using key-paths since they suffer the same issue and call the getter where I expect them to call the setter only.

I‘m +1 on this change.

2 Likes

I'm +1.

It seem to fix some potential and/or real bugs without otherwise causing problems.

I think it's ok not pass it implicitly, as it's simply not doing something the user isn't asking to happen anyway. This could potentially see a slight performance hit if oldValue is added later, but that seems trivial and not worth worrying about.

It also doesn't seem necessary to require declaring oldValue. It's not source compatible and feels like the opposite direction for what approach should add complexity. The default use case for most users probably will still be to use oldValue, so if usage should be explicit it should be for when users don't use it. e.g:

var foo: String {
    didSet(_) {
        //Explicity saying oldValue isn't wanted, so it won't be passed
    }
}
5 Likes

The concerns in this thread is about whether there will be an additional language mode (-swift-version, or "Swift Language Version" in Xcode) for Swift 5.1 that's different from Swift 5 mode (there will not be) and when the next such language mode will be introduced (unknown, up to the core team). Nothing has changed with the compiler shipping in Xcode 11 betas (Swift 5.1), or whether that compiler supports ABI and module stability (yes). Sorry for any confusion!

4 Likes

I agree with this. I find the current behaviour quite strange (AFAIK nothing else in the language injects named variables in to your scope — except perhaps ‘self’ for instance methods).

It will break source, but at the same time, it should be quite simple to detect this and offer fix-its and implement a compatibility behaviour for older language versions.

2 Likes

This kind of implicit variable binding arises in try/catch too for a magic error variable. I also think it is strange and wouldn't mind it being removed. Frankly, it is so obscure people always write catch let error explicitly anyway.

In case it's not clear what I'm describing, the language currently allows this code:

do {
    try operation()
} catch {
    print(error)
}
6 Likes

That’s a good point, I forgot about that. Still, I consider that more like ‘self’ because no additional code can be executed by binding the thing to a name.

set does inject newValue.

1 Like

My known list of injected variables off the top of my head:

  • self in instance scope
  • self in static/class scope
  • $0, $1 etc. in clousures
  • newValue in willSet
  • oldValue in didSet
  • newValue in set
  • error in catch

I have a feeling there a couple more I can't think of off the top of my head, but that's not too small of a list. as it is.

Given how common it is in Computed Properties and Property Observers, how much (subjectively) cleaner it is to not have a parameter list there, and how long this behavior has been available it seems unnecessary to add such a requirement.

10 Likes

I'm likely not fully following the complexity from the compiler pov, but what you saying here sorta doesn't feel like it lines up in part with the already updated docs:

https://docs.swift.org/swift-book/GuidedTour/Compatibility.html calls out

…the following changes are available only to code that uses Swift 5.1 or later…

(along with other parts there)

So I had read some of those docs thinking there already was a 5.0 vs. 5.1 mode to help catch/flag when using things that do require the newer runtime/etc. Does that mean things that require 5.1 runtime support instead would error if the min os version wasn't high enough (or things weren't gated with availability checks)?

TVL

Proposal Updated

  1. API Resilience section has been updated.
  2. Polished up the overall proposal a bit.

Proposal has also been submitted to Swift Evolution repo: https://github.com/apple/swift-evolution/pull/1068

If you have any suggestions or improvements, please feel free to let me know. Thank you!

2 Likes

This title is too generic for what is actually being proposed. I think you’re previous “didSet semantics” title was in the right direction.

Okay, I will change the title. I agree "didSet Semantics" is good, but I found that it slightly misleading due to the change to _modify, but I guess the proposed changes are linked together so it's probably fine to not make it too generic and keep the "didSet" in the title for clarity.

EDIT: Title change reverted. Thanks!

1 Like

Nice. I’m glad to see a PR up to fix this issue. I’m hoping it can come soon and doesn’t have to wait for a major version. If people were relying on the old behavior, I feel like this is relying on an odd defect in the compiler, more than a feature?

3 Likes

I can appreciate that the suggestions to deprecate the implicit oldValue in didSet were called out as a potential future direction. I think we could probably go back and forth on whether or not that should happen now or later for a while. Thanks @suyashsrijan

1 Like

Self and self are obviously special for myriad reasons. $0 is an otherwise invalid variable name. Personally, I would be fine with all the rest going away.

If there is a semantic distinction that can be made clearer when we get rid of oldValue in didSet, then it seems like the ideal time to make this change would be when we introduce the distinct semantics.

8 Likes

+1

1 Like

Looks really good, +1 for me.

I do have a couple of issues with the wording of the text, though:

It feels weird to read this without some evidence backing it up. I understand why you’d think it’s true, but personally I have my doubts. Maybe just clarify that this is your opinion?

I only understood why it doesn’t provide any real benefit after reading the discussion here. If you’re gonna make this claim I’d add an explanation of why it’s true.

Still, great work :)

3 Likes

Thanks, I have updated the proposal text!

As author of the https://bugs.swift.org/browse/SR-11280 bug where a "Delayed" property wrapper crashes due to the implicit property access to get oldValue I'm a strong +1 on this proposal.

I'd be happier if we said "using oldValue without explicitly declaring it is deprecated" (i.e. you should write didSet(oldValue) , as you've always been able to do, if you want to use it). That way it's obvious from the code whether the getter will be called or not.

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).

Pushing the $0, $1 shorthand for closures to the extreme, one could imagine a proposal that newValue from willSet, oldValue from didSet, newValue in set and error in catch should each be available via a shorthand $0. I would still strongly prefer explicit opt-in for each one of these vs a shorthand $0 value.

1 Like
Terms of Service

Privacy Policy

Cookie Policy