[Property wrapers] Fix l-value computation in buildStorageReference

Prior to this commit, buildStorageReference used to take an 'isLValue' argument
that specified whether the reference should be an l-value or not.

This provided sufficient information in case of a stored property
(self.member). However, when accessing a wrapped property
(self._member.wrappedValue), this doesn't have enough information to decide
whether intermediate components of the expression should be l-values or not.
Prior to this commit, this was extrapolated from the 'isLValue' argument,
leading to compiler crashes or unnecessary intermediate l-values when using
non-standard mutability with property wrappers.

To fix this, this commit replaces the 'isLValue' argument with two arguments:

  isUsedForGetAccess: Specifies that the referenced storage is meant to be
                      accessed with a getter
  isUsedForSetAccess: Specifies that the referenced storage is meant to be
                      accessed with a setter

and updates buildStorageReference to correctly figure out l-value-ness of
self and intermediate components from these arguments.

Author: roop

lib/Sema/TypeCheckStorage.cpp

@@ -355,8 +355,13 @@ IsSetterMutatingRequest::evaluate(Evaluator &evaluator,
   // or not based on the composition of the wrappers.
   if (auto var = dyn_cast<VarDecl>(storage)) {
     if (auto mut = var->getPropertyWrapperMutability()) {
-      return mut->Setter == PropertyWrapperMutability::Mutating
-        && result;
+      bool isMutating = mut->Setter == PropertyWrapperMutability::Mutating;
+      if (var->getParsedAccessor(AccessorKind::DidSet)) {
+        // If there's a didSet, we call the getter for the 'oldValue', and so
+        // should consider the getter's mutatingness as well
+        isMutating |= (mut->Getter == PropertyWrapperMutability::Mutating);
+      }
+      return isMutating && result;
     }
   }
 
@@ -601,14 +606,18 @@ getEnclosingSelfPropertyWrapperAccess(VarDecl *property, bool forProjected) {
   return result;
 }
 
-/// Build an l-value for the storage of a declaration. Returns nullptr if there
+/// Build a reference to the storage of a declaration. Returns nullptr if there
 /// was an error. This should only occur if an invalid declaration was type
 /// checked; another diagnostic should have been emitted already.
+
 static Expr *buildStorageReference(AccessorDecl *accessor,
                                    AbstractStorageDecl *storage,
                                    TargetImpl target,
-                                   bool isLValue,
+                                   bool isUsedForGetAccess,
+                                   bool isUsedForSetAccess,
                                    ASTContext &ctx) {
+  // Whether the last component of the expression should be an l-value
+  bool isLValue = isUsedForSetAccess;
   // Local function to "finish" the expression, creating a member reference
   // to the given sequence of underlying variables.
   Optional<EnclosingSelfPropertyWrapperAccess> enclosingSelfAccess;
@@ -762,38 +771,27 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
     return finish(storageDRE);
   }
 
-  bool isMemberLValue = isLValue;
-  auto propertyWrapperMutability =
-      [&](Decl *decl) -> Optional<std::pair<bool, bool>> {
-    auto var = dyn_cast<VarDecl>(decl);
-    if (!var)
-      return None;
-    auto mut = var->getPropertyWrapperMutability();
-    if (!mut)
-      return None;
-    return std::make_pair(mut->Getter == PropertyWrapperMutability::Mutating,
-                          mut->Setter == PropertyWrapperMutability::Mutating);
-  };
+  // Build self
 
-  // If we're accessing a property wrapper, determine if the
-  // intermediate access requires an lvalue.
-  if (auto mut = propertyWrapperMutability(accessor->getStorage())) {
-    isMemberLValue = mut->first;
-    if (isLValue)
-      isMemberLValue |= mut->second;
+  bool isGetterMutating = storage->isGetterMutating();
+  bool isSetterMutating = storage->isSetterMutating();
+  if (target == TargetImpl::Wrapper || target == TargetImpl::WrapperStorage) {
+    auto var = cast<VarDecl>(accessor->getStorage());
+    if (auto mutability = var->getPropertyWrapperMutability()) {
+      // We consider the storage's mutability too because the wrapped property
+      // might be part of a class, in case of which nothing is mutating.
+      isGetterMutating = (mutability->Getter == PropertyWrapperMutability::Mutating)
+        ? (storage->isGetterMutating() || storage->isSetterMutating())
+        : storage->isGetterMutating();
+      isSetterMutating = (mutability->Setter == PropertyWrapperMutability::Mutating)
+        ? (storage->isGetterMutating() || storage->isSetterMutating())
+        : storage->isGetterMutating();
+    }
   }
 
-  bool isSelfLValue = storage->isGetterMutating();
-  if (isMemberLValue)
-    isSelfLValue |= storage->isSetterMutating();
-
-  // If we're accessing a property wrapper, determine if
-  // the self requires an lvalue.
-  if (auto mut = propertyWrapperMutability(storage)) {
-    isSelfLValue = mut->first;
-    if (isMemberLValue)
-      isSelfLValue |= mut->second;
-  }
+  // If the accessor is mutating, then self should be referred as an l-value
+  bool isSelfLValue = (isGetterMutating && isUsedForGetAccess) ||
+                      (isSetterMutating && isUsedForSetAccess);
 
   Expr *selfDRE =
     buildSelfReference(selfDecl, selfAccessKind, isSelfLValue,
@@ -806,6 +804,27 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
     selfDRE = new (ctx) DerivedToBaseExpr(selfDRE, selfTypeForAccess);
   }
 
+  // Build self.member or equivalent
+
+  bool isMemberLValue = isLValue;
+  if (target == TargetImpl::Wrapper || target == TargetImpl::WrapperStorage) {
+    // If the outermost property wrapper's getter / setter is mutating,
+    // then the reference to the backing storage should be an l-value.
+    auto var = cast<VarDecl>(accessor->getStorage());
+    auto varMember = (target == TargetImpl::WrapperStorage)
+      ? &PropertyWrapperTypeInfo::projectedValueVar
+      : &PropertyWrapperTypeInfo::valueVar;
+    auto wrapper = (target == TargetImpl::WrapperStorage)
+      ? var->getOriginalWrappedProperty(
+          PropertyWrapperSynthesizedPropertyKind::StorageWrapper)
+          ->getAttachedPropertyWrapperTypeInfo(0)
+      : var->getAttachedPropertyWrapperTypeInfo(0);
+    bool isWrapperGetterMutating = (wrapper.*varMember)->isGetterMutating();
+    bool isWrapperSetterMutating = (wrapper.*varMember)->isSetterMutating();
+    isMemberLValue = (isWrapperGetterMutating && isUsedForGetAccess) ||
+                     (isWrapperSetterMutating && isUsedForSetAccess);
+  }
+
   Expr *lookupExpr;
   ConcreteDeclRef memberRef(storage, subs);
   auto type = storage->getValueInterfaceType().subst(subs);
@@ -883,6 +902,8 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
     lookupExpr->setType(type);
   }
 
+  // Build self.member.wrappedValue if applicable
+
   return finish(lookupExpr);
 }
 
@@ -893,7 +914,9 @@ createPropertyLoadOrCallSuperclassGetter(AccessorDecl *accessor,
                                          AbstractStorageDecl *storage,
                                          TargetImpl target,
                                          ASTContext &ctx) {
-  return buildStorageReference(accessor, storage, target, /*isLValue=*/false,
+  return buildStorageReference(accessor, storage, target,
+                               /*isUsedForGetAccess=*/true,
+                               /*isUsedForSetAccess=*/false,
                                ctx);
 }
 
@@ -1045,7 +1068,9 @@ void createPropertyStoreOrCallSuperclassSetter(AccessorDecl *accessor,
     return;
 
   Expr *dest = buildStorageReference(accessor, storage, target,
-                                     /*isLValue=*/true, ctx);
+                                     /*isUsedForGetAccess=*/false,
+                                     /*isUsedForSetAccess=*/true,
+                                     ctx);
 
   // Error recovery.
   if (dest == nullptr)
@@ -1495,7 +1520,10 @@ synthesizeObservedSetterBody(AccessorDecl *Set, TargetImpl target,
   VarDecl *OldValue = nullptr;
   if (VD->getParsedAccessor(AccessorKind::DidSet)) {
     Expr *OldValueExpr
-      = buildStorageReference(Set, VD, target, /*isLValue=*/true, Ctx);
+      = buildStorageReference(Set, VD, target,
+                              /*isUsedForGetAccess=*/true,
+                              /*isUsedForSetAccess=*/true,
+                              Ctx);
 
     // Error recovery.
     if (OldValueExpr == nullptr) {
@@ -1626,10 +1654,13 @@ synthesizeCoroutineAccessorBody(AccessorDecl *accessor, ASTContext &ctx) {
   SourceLoc loc = storage->getLoc();
   SmallVector<ASTNode, 1> body;
 
-  bool isLValue = accessor->getAccessorKind() == AccessorKind::Modify;
+  bool isModify = accessor->getAccessorKind() == AccessorKind::Modify;
 
   // Build a reference to the storage.
-  Expr *ref = buildStorageReference(accessor, storage, target, isLValue, ctx);
+  Expr *ref = buildStorageReference(accessor, storage, target,
+                /*isUsedForGetAccess=*/true,
+                /*isUsedForSetAccess=*/isModify,
+                ctx);
   if (ref != nullptr) {
     // Wrap it with an `&` marker if this is a modify.
     ref = maybeWrapInOutExpr(ref, ctx);

test/SILGen/property_wrappers.swift

@@ -363,6 +363,20 @@ struct MutatingGet<T> {
   }
 }
 
+@propertyWrapper
+struct MutatingGetNonMutatingSet<T> {
+  private var fixed: T
+
+  var wrappedValue: T {
+    mutating get { fixed }
+    nonmutating set { }
+  }
+
+  init(wrappedValue initialValue: T) {
+    fixed = initialValue
+  }
+}
+
 struct ObservingTest {
 	// ObservingTest.text.setter
 	// CHECK-LABEL: sil hidden [ossa] @$s17property_wrappers13ObservingTestV4textSSvs : $@convention(method) (@owned String, @guaranteed ObservingTest) -> ()
@@ -382,6 +396,26 @@ struct ObservingTest {
   @MutatingGet var integer2: Int = 17 {
     willSet { }
   }
+
+  @MutatingGetNonMutatingSet var text3: String = "" {
+    didSet { }
+  }
+
+  @MutatingGetNonMutatingSet var integer3: Int = 17 {
+    willSet { }
+  }
+}
+
+struct NonObservingTest {
+  @NonMutatingSet var text: String = ""
+  @MutatingGet var text2: String = ""
+  @MutatingGetNonMutatingSet var text3: String = ""
+}
+
+class NonObservingClassTest {
+  @NonMutatingSet var text: String = ""
+  @MutatingGet var text2: String = ""
+  @MutatingGetNonMutatingSet var text3: String = ""
 }
 
 // Tuple initial values.