[SE-0268] [Typechecker] Refine didSet semantics

[theblixguy]

At the moment, we load and pass the oldValue to didSet when it's called, even if we do not reference the oldValue inside the body of the didSet. This means we end up calling the getter to do redundant work and prevent certain code from working (such as a Delayed/LateInitialized property wrapper).

This PR introduces two changes:

1. If the didSet does not reference the oldValue in its body, then the call to fetch the oldValue will be skipped. We refer to this as a "simple" didSet.
2. If we have a "simple" didSet and no willSet, then we could allow for modifications to happen in-place.

Resolves SR-11297, SR-11280 and SR-5982
Resolves rdar://problem/54133764
Resolves rdar://problem/29733553
Resolves FB6975769

---

Swift Evolution thread: https://forums.swift.org/t/skipping-call-to-getter-when-oldvalue-is-not-used-in-didset/27858

[theblixguy]

cc @jckarter @rjmccall @DougGregor can you please review? I am not sure if this requires an evolution proposal or not, but I am happy to create a proposal if needed! Thank you.

[jrose-apple]

This is a semantics-breaking change, and a really subtle one when it comes to lazy. I don't think we should take it without a language break (and a proposal).

[CodaFi]

I'm also concerned with the impact this will have on the quality of debug info. If we disappear this parameter at Sema-time we won't be able to recover it later, even if we wanted to.

[theblixguy]

Sure, I can create a proposal and pitch it, but need I confirmation from Core Team before I do it. We could also version-gate it so it only works for Swift 5.2/6/whatever or above.

[slavapestov]

I would definitely bring this up in the forums. The core team might decide the breakage is acceptable. After all, a getter should ideally not have side effects or depend on evaluation order.

With lazy, you can argue that not forcing the value to be computed before calling the setter is a feature. And yes, it might change observed behavior, but I would expect this to happen very rarely if at all in real code.

[theblixguy]

I have created a pitch thread here with a draft proposal: https://forums.swift.org/t/skipping-call-to-getter-when-oldvalue-is-not-used-in-didset/27858

John says it does need to go through evolution but will likely be accepted, so I'll run it through the process.

[rjmccall]

I wouldn't go so far as "will likely be accepted"; I'm just saying it seems viable.

[theblixguy]

Just rebased on top of master because lots of conflicts started to creep in.

EDIT: Oh well, all the requestification that's going on keeps introducing new conflicts 😅

[roop]

@theblixguy Should we be checking for property wrappers / @objc, and avoid using synthesizeModifyCoroutineBodyWithSimpleDidSet() in those cases?

[theblixguy]

Yeah, I think we'd need to suppress it for @objc properties. I am not sure about property wrappers though.

[theblixguy]

@rjmccall Do you mind taking a look at the _modify and ReadWriteImplKind related changes?

[slavapestov]

How about this -- the oldValue parameter is always present, but it has an Optional type. References to oldValue inside the body are implicitly unwrapped.

A post-pass (or request) that runs after body type checking looks for references to oldValue in the type checked body. If there are none, set a flag (or the result of the request) on the AccessorDecl noting that.

Then when you synthesize the body of the setter (where didSet gets called), pass in .none instead of loading the old value if that flag in the accessor is set (if computation of this flag is a request, this is where it would be kicked off. Synthesizing the body of the setter is done sufficiently "late" (in SILGen) that you can kick off/depend on the type checking to have completed on the didSet body.

What do you think?

[rjmccall]

Hmm. I want to lay out the longer-term vision here, and then we can decide what's reasonable to do in the short term for this PR.

Right now, I think didSet and willSet suffer from being treated too much like sugar for a setter, when actually they're quite interestingly different from just defining a setter (or at least didSet is). SE-0268 moves us pretty decidedly in this direction. Currently the observers are always internal to a particular implementation file. It would be nice to get away from that and make them first-class accessors with the same visibility as the storage declaration they're attached to. That would let us expand the access to (non-opaque) observed storage in SILGen, like we do for every other kind of storage implementation, instead of just calling a setter that's synthesized to use the observers. That, in turn, would let us both be more precise about accesses (e.g. accessing the base storage as a read-write access when we have a non-simple didSet and copying oldValue during that instead of performing two separate accesses), remove some special-case logic from the synthesis of observed accessors, and generally improve intra-module optimization. And this should all be possible because we currently never expose observed properties across ABI boundaries as anything except opaque.

[rjmccall]

Slava's optional-parameter idea is interesting, but while it avoids the circularity problem, it also introduces a lot of special cases and might be hard to root out in the long term. And the circularity problem seems directly solvable: as Slava says, we need to be figuring this out as part of computing the interface type of the observer. Normally, that would cause really bad circularity problems, but there should never be any direct uses of the observer that would need its interface type.

[theblixguy]

Hmm, okay. The interface type for didSet is computed when visitEmittedAccessors is called, which ends up calling visitFuncDecl which kicks off InterfaceTypeRequest. So, I think we either need to

(1) delay InterfaceTypeRequest computation
(2) kick off DidSetContainsOldValueReferenceRequest before InterfaceTypeRequest, updating the didSet's parameter list directly in DidSetContainsOldValueReferenceRequest if there's no use of implicit oldValue. Then, when the interface type is computed, it would have the right type and we won't have to re-compute it.

I tested out (2) (see defeca6) and it seems to work fine. What do you think of this approach?

[theblixguy]

Sorry for the ping, but it’s been a while... do you have any thoughts on above? I would like to get this wrapped up and merged soon.

[rjmccall]

(2) seems reasonable to me, but I'd like Slava's opinion.

[slavapestov]

(2) sounds fine. Can you fix the merge conflicts and we'll look over this PR one more time?

[theblixguy]

@slavapestov Done!

[DougGregor]

This is very nice! I have one small bundle of changes requested, but otherwise I'd this is ready to go in. Adding new requests to the evaluator happens all the time, so once you've got this tweaked and rebased let's test & merge before someone else gets something in that conflicts.

[theblixguy]

@swift-ci please test

[theblixguy]

Hmm, interesting SILVerifier crash:

19:54:49 SIL verification failed: function_ref inside fragile function cannot reference a private or hidden symbol: (SingleFunction && RefF->isExternalDeclaration()) || RefF->hasValidLinkageForFragileRef()
19:54:49 Verifying instruction:
19:54:49 ->   // function_ref Progress.cancellationHandler.didset
19:54:49   %6 = function_ref @$s10Foundation8ProgressC19cancellationHandleryycSgvW : $@convention(method) (@guaranteed Progress) -> () // user: %7
19:54:49      %7 = apply %6(%0) : $@convention(method) (@guaranteed Progress) -> ()
19:54:49 In function:
19:54:49 // Progress.cancellationHandler.modify
19:54:49 sil [transparent] [serialized] [ossa] @$s10Foundation8ProgressC19cancellationHandleryycSgvM : $@yield_once @convention(method) (@guaranteed Progress) -> @yields @inout Optional<@callee_guaranteed () -> ()> {
19:54:49 // %0                                             // users: %7, %2, %1
19:54:49 bb0(%0 : @guaranteed $Progress):
19:54:49   debug_value %0 : $Progress, let, name "self", argno 1 // id: %1
19:54:49   %2 = ref_element_addr %0 : $Progress, #Progress.cancellationHandler // user: %3
19:54:49   %3 = begin_access [modify] [dynamic] %2 : $*Optional<@callee_guaranteed () -> ()> // users: %5, %10, %4
19:54:49   yield %3 : $*Optional<@callee_guaranteed () -> ()>, resume bb1, unwind bb2 // id: %4
19:54:49 
19:54:49 bb1:                                              // Preds: bb0
19:54:49   end_access %3 : $*Optional<@callee_guaranteed () -> ()> // id: %5
19:54:49   // function_ref Progress.cancellationHandler.didset
19:54:49   %6 = function_ref @$s10Foundation8ProgressC19cancellationHandleryycSgvW : $@convention(method) (@guaranteed Progress) -> () // user: %7
19:54:49   %7 = apply %6(%0) : $@convention(method) (@guaranteed Progress) -> ()
19:54:49   %8 = tuple ()                                   // user: %9
19:54:49   return %8 : $()                                 // id: %9
19:54:49 
19:54:49 bb2:                                              // Preds: bb0
19:54:49   end_access %3 : $*Optional<@callee_guaranteed () -> ()> // id: %10
19:54:49   unwind                                          // id: %11
19:54:49 } // end sil function '$s10Foundation8ProgressC19cancellationHandleryycSgvM'

[theblixguy]

Reproducer:

open class Foo {
  open var bar: Int = 1 {
    didSet {}
  }
}

Same with public. Removing public/open from either the class or property fixes the SIL verification error.

[theblixguy]

Hmm, seems like in above code the didSet observer ends up with private linkage and so we can't use it from the _modify accessor. @slavapestov do you think we need to change it?

[DougGregor]

I suspect that _modify shouldn't be transparent when there's a didSet?

[slavapestov]

Changing the modify to not be transparent in this case makes sense, yeah. Alternatively, you could have modify call the setter instead of accessing the stored property directly, but I guess that's not as efficient.

[theblixguy]

You're absolutely right, there's even a comment in IsAccessorTransparentRequest that talks about this (in context of setters, but applies here as well). Thank you! There were a few other failing tests due to SIL output changes, I have fixed them so let's see if they pass now:

@swift-ci please test

[theblixguy]

There's a few other failing tests because we have a lot of tests that use didSet without oldValue (so SIL output has some changes for example) and there was one crasher somewhere in SILGen - I am gonna take a look at it now. I need to run tests again because I can't access the old logs.

In the meantime, I made changes to two tests but I am not too sure if its correct. Can someone take a look?

[theblixguy]

@swift-ci please test

[theblixguy]

@swift-ci please test

[theblixguy]

@swift-ci please test

[theblixguy]

@swift-ci please test

[theblixguy]

@swift-ci please test source compatibility