[Typechecker] Fix _modify for wrapped properties with observers

[theblixguy]

Since we implemented _modify synthesis for property wrappers, it seems like we go through the _modify even when the wrapped property has explicit observers attached to it, which means the observer is never called. For example:

@propertyWrapper 
struct Foo {
  private var _storage: [Int] = []

  init(wrappedValue value: [Int]) {
    self._storage = value
  }

  var wrappedValue: [Int] {
    get { _storage }
    set { _storage = newValue }
  }
}

class Bar {
  @Foo var someArray = [1, 2, 3] {
    willSet { print(newValue) }
  }
}

let bar = Bar()
bar.someArray[0] = 4 // This doesn't trigger willSet anymore

This used to work fine on Swift 5.1, but not anymore.

So, if the wrapped property has any observers, then fix _modify to call setter.

Resolves SR-12178, SR-12089
Resolves rdar://problem/59496047

[slavapestov]

_modify is still going to be used if the property is used as a witness for a protocol requirement. Maybe the problem can be fixed another way, by fixing _modify synthesis to do the right thing?

Also, I would prefer if this had a SILGen test as well, demonstrating that the observers are getting called.

[theblixguy]

by fixing _modify synthesis to do the right thing?

You mean calling the observers (like the setter does)?

Also, I would prefer if this had a SILGen test as well, demonstrating that the observers are getting called.

Sure, I will add one!

[slavapestov]

You mean calling the observers (like the setter does)?

Yeah, exactly. Can you also try to write a test case that demonstrates the breakage with a protocol conformance?

[theblixguy]

Yeah, exactly.

Great. I need to add that code anyway for SE-0268 to work. I have one question though - how will this work if we have a willSet? i.e. we'll need the newValue but it won’t exist here I think. In case of a setter, it receives the newValue as argument, but I’m not sure if it’s possible to do:

willSet(newValue) // not sure
yield &wrappedValue
didSet(oldValue) // possible

Can you also try to write a test case that demonstrates the breakage with a protocol conformance?

When would this breakage occur exactly? (since you can't explicitly add _modify to a @Foo var bar : T nor to the protocol property requirement. Do we always choose to use _modify?). Did we had this breakage before when we weren't synthesising _modify?

[theblixguy]

var tmp = wrappedValue
yield &tmp
call setter(tmp)

This could work I suppose... but if we are gonna delegate to the setter then might as well use it in the first place? Maybe not because of the protocol requirement issue you mentioned before.

[slavapestov]

The modify coroutine in this case should just be the standard one we synthesize for computed properties:

var tmp = getter()
yield &tmp
setter(tmp)

The synthesized setter already calls willSet/didSet in the right order.

I suspect what's happening is we're synthesizing the modify coroutine as if this was a plain stored property, ie

var tmp = storage
yield &tmp
storage = tmp // does not call setter

[theblixguy]

Hmm, yeah. Looking at the SIL, it seems like we're basically doing:

var tmp = wrappedValue.getter()
yield &tmp
wrappedValue.setter(tmp)

I was thinking that not changing the TargetImpl if we have observers should restore the default behavior of the synthesizeCoroutineAccessorBody.

[theblixguy]

Well, I thought it would work in theory, but I just tried and it doesn’t work as one of the member ref expressions in the chain to the wrappedValue end up with an LValue type which SILGen doesn’t like. I’m not sure if I’m doing something wrong because I’m aware that lvalue computation is sort of broken (but should be fixed if we merge PR 29069).

[slavapestov]

Hi @theblixguy, were you able to find any more time to look into this?

[theblixguy]

@slavapestov Sorry I got caught up with work so didn't have time to investigate beyond my last update above, but I'll take another look shortly! I'll also try including commits from #29069 to see if that fixes the lvalue computation issue I mentioned above. It seems like the AST just has the yield statement which gets decomposed into the getter-yield-setter pattern above in SILGen so I need to take a look there as well, but I suspect the fix lies in Sema only.

[DougGregor]

@swift-ci please smoke test

[DougGregor]

@theblixguy Are you planning to look into this, or will you need some help?

[theblixguy]

@DougGregor Yep, I had other PRs in the queue I wanted to finish off but now that's done, I'll can now look into what Slava suggested tomorrow. I tried a couple of things to get it to emit a modify like we do for a normal observed property but ran into problems (as mentioned above). I am hoping that #29069 could help here, if not then I'll need to figure out another way. If you have any suggestions then please let me know!

Also, in the meantime, if you want to get this fix in, feel free to (test failures looks unrelated).

[theblixguy]

Okay, I pulled in the changes from #29069 and I don't get the LValue crash I was getting before, which is great, however now I get an infinite loop at runtime. I think this is expected because what I tried to do was to keep the ReadWriteImplKind as Modify, but disable the code in synthesizeCoroutineAccessorBody that changes target to TargetImpl::Wrapper or TargetImpl::WrapperStorage. This isn't right because then the _modify for the wrapped property will just yield itself which in SIL end up as a call to _modify and thus an infinite loop.

From discussions above, it seems like the _modify should be like this for a unobserved wrapped property:

var tmp = wrappedValue.getter()
yield &tmp
wrappedValue.setter(tmp)

and for observed wrapped properties:

var tmp = getter()
yield &tmp
setter(tmp)

but to switch between these forms I need to change the StorageImplInfo in finishPropertyWrapperImplInfo and not change the target in synthesizeCoroutineAccessorBody, which is what I have done in this PR and that works. Apart from that, I can't think of anything else that influences the change in form above.

If the current approach in this PR is not correct, then I think I am kinda stuck (at least because I've been thinking the solution to this bug is in Sema and not SILGen, etc).

[slavapestov]

This is the "universal" form of a modify coroutine that should work with any implementation:

var tmp = getter()
yield &tmp
setter(tmp)

I need to re-familiarize myself with the code before giving you further feedback though. I'll pull in your change and play around with it and see if I can get it working.

[theblixguy]

Thank you! So I suppose there are two parts to this - what the _modify looks like in AST (there’s just a yield statement in the body) and what it looks like in SIL (getter-yield-setter). I admit I haven’t spent time looking at the SIL implementation, so I haven’t got the full understanding of how exactly it works.

At the moment I was trying to discover whether I have got the first part right (StorageImplInfo and TargetInfo). But maybe I was looking at the wrong thing...

[slavapestov]

I think the second part should work even with a property wrapper property, as long as the first part works. I'll let you know what I find.

[slavapestov]

I got your test case working by making the following changes on master (without this PR applied):

• If a wrapped property has observers, use ReadWriteImpl::MaterializeToTemporary
• In synthesizeCoroutineAccessorBody(), don't set the target to TargetImpl::Wrapped or TargetImpl::WrappedStorage if it has observers
• In synthesizeCoroutineAccessorBody(), don't call synthesizeModifyCoroutineBodyWithSimpleDidSet() if the property is wrapped

However, now I suspect this will break SE-0268 semantics since we'll still call the getter even if the observer doesn't need it.

[slavapestov]

Also, unrelated: it feels to me that synthesizeModifyCoroutineBodyWithSimpleDidSet() is redundant. The default coroutine should already do the right thing, with calling willSet and didSet, etc. Is it just an optimization at this point?

[slavapestov]

I cleaned up my version of the fix slightly and this is what I have now. It's almost identical to your change, but a little bit simpler perhaps. Do you want to see if it passes tests?

diff --git a/lib/Sema/TypeCheckStorage.cpp b/lib/Sema/TypeCheckStorage.cpp
index ea9487f27e7..556c7843613 100644
--- a/lib/Sema/TypeCheckStorage.cpp
+++ b/lib/Sema/TypeCheckStorage.cpp
@@ -1668,16 +1668,18 @@ synthesizeCoroutineAccessorBody(AccessorDecl *accessor, ASTContext &ctx) {
                    ? TargetImpl::Ordinary
                    : TargetImpl::Implementation);
 
-  // If this is a variable with an attached property wrapper, then
-  // the accessors need to yield the wrappedValue or projectedValue.
-  if (auto var = dyn_cast<VarDecl>(storage)) {
-    if (var->hasAttachedPropertyWrapper()) {
-      target = TargetImpl::Wrapper;
-    }
+  if (storageReadWriteImpl == ReadWriteImplKind::Modify) {
+    // If this is a variable with an attached property wrapper, then
+    // the accessors need to yield the wrappedValue or projectedValue.
+    if (auto var = dyn_cast<VarDecl>(storage)) {
+      if (var->hasAttachedPropertyWrapper()) {
+        target = TargetImpl::Wrapper;
+      }
 
-    if (var->getOriginalWrappedProperty(
-            PropertyWrapperSynthesizedPropertyKind::StorageWrapper)) {
-      target = TargetImpl::WrapperStorage;
+      if (var->getOriginalWrappedProperty(
+              PropertyWrapperSynthesizedPropertyKind::StorageWrapper)) {
+        target = TargetImpl::WrapperStorage;
+      }
     }
   }
 
@@ -1685,7 +1687,6 @@ synthesizeCoroutineAccessorBody(AccessorDecl *accessor, ASTContext &ctx) {
   SmallVector<ASTNode, 1> body;
 
   bool isLValue = accessor->getAccessorKind() == AccessorKind::Modify;
-
   if (isLValue &&
       (storageReadWriteImpl == ReadWriteImplKind::StoredWithSimpleDidSet ||
        storageReadWriteImpl == ReadWriteImplKind::InheritedWithSimpleDidSet)) {
@@ -2683,9 +2684,17 @@ static void finishPropertyWrapperImplInfo(VarDecl *var,
     }
   }
 
-  if (wrapperSetterIsUsable)
+  if (wrapperSetterIsUsable) {
+    bool hasObservers = var->getParsedAccessor(AccessorKind::WillSet) ||
+                        var->getParsedAccessor(AccessorKind::DidSet);
+    if (hasObservers) {
+      info = StorageImplInfo::getMutableComputed();
+      return;
+    }
+
     info = StorageImplInfo(ReadImplKind::Get, WriteImplKind::Set,
-                           ReadWriteImplKind::Modify);
+                           ReadWriteImplKind::MaterializeToTemporary);
+  }
   else
     info = StorageImplInfo::getImmutableComputed();
 }

[theblixguy]

However, now I suspect this will break SE-0268 semantics since we'll still call the getter even if the observer doesn't need it.

Hmm it would be a bit unfortunate if we need to suppress it for property wrappers.

Also, unrelated: it feels to me that synthesizeModifyCoroutineBodyWithSimpleDidSet() is redundant. The default coroutine should already do the right thing, with calling willSet and didSet, etc. Is it just an optimization at this point?

I guess so, let me check and I will create a PR to remove it if its indeed redundant!

I cleaned up my version of the fix slightly and this is what I have now. It's almost identical to your change, but a little bit simpler perhaps. Do you want to see if it passes tests?

Thanks! Two questions:

1. storageReadWriteImpl == ReadWriteImplKind::Modify - should the _read coroutine use the TargetImpl::Wrapper/TargetImpl::WrapperStorage as well? This test seems to rely on it.
2. It seems like we're effectively using getMutableComputed() in both scenarios? (Sorry, maybe I misread the diff)

[slavapestov]

Hmm it would be a bit unfortunate if we need to suppress it for property wrappers.

I think I was wrong about this part. As long as the setter's body is synthesized correctly we should be fine.

storageReadWriteImpl == ReadWriteImplKind::Modify - should the _read coroutine use the TargetImpl::Wrapper/TargetImpl::WrapperStorage as well? This test makes sure it does.

Yeah, I suggest changing my code so that the read coroutine is not affected.

It seems like we're effectively using getMutableComputed() in both scenarios?

Not quite. getMutableComputed() returns a StorageInfo with ReadWriteImpl::MaterializeToTemporary, whereas ordinary wrapped properties use ReadWriteImpl::Modify.

[slavapestov]

The difference is that an inout access of a ::MaterializeToTemporary is SILGen'd to a call of the getter followed by a call of the setter. A ::Modify SILGen's as a call to the modify coroutine. This means that for a ::MaterializeToTemporary, modify is only used as a protocol witness, and a valid implementation of modify is to yield the property itself. With a ::Modify, you can't do that; the modify coroutine then calls itself, causing an infinite loop.

[theblixguy]

Thanks so much for the explanation! So I have done some cleanups and I also added a hasObservers() convenience method as I found few instances of code that performs the same computation.

[theblixguy]

Btw it seems like the _modify does call the getter so I think what you suspected is true, but I suppose it's fine because we're not calling it to fetch the oldValue for the didSet, but to fetch the value so _modify can yield it (although I am worried it breaks the "modifications in-place" bit because if the didSet has no oldValue then the _modify should be calling didSet directly, instead of calling the setter)

[slavapestov]

This looks good. Do you mind adding a SILGen test as well?

[slavapestov]

You're right, a custom modify implementation can be more efficient here because it can yield the underlying storage and then call the observer. We can consider optimizing it later.

[theblixguy]

@swift-ci please smoke test

[theblixguy]

Should I cherry-pick this to 5.2 branch as well?

[DougGregor]

Please do cherry-pick to 5.2

[slavapestov]

Thanks!