Merge pull request #35354 from hborla/5.4-assign-by-wrapper-crash

[5.4][Property Wrappers] Fix a few corner cases where property wrappers with nonmutating setters fail in DI

Author: hborla

lib/SILGen/SILGenLValue.cpp

@@ -1325,23 +1325,6 @@ namespace {
             wrapperInfo.wrappedValuePlaceholder->getOriginalWrappedValue())
           return false;
 
-        // If we have a nonmutating setter on a value type, the call
-        // captures all of 'self' and we cannot rewrite an assignment
-        // into an initialization.
-
-        // Unless this is an assignment to a self parameter inside a
-        // constructor, in which case we would like to still emit a
-        // assign_by_wrapper because the setter will be deleted by lowering
-        // anyway.
-        if (!isAssignmentToSelfParamInInit &&
-            !VD->isSetterMutating() &&
-            VD->getDeclContext()->getSelfNominalTypeDecl() &&
-            VD->isInstanceMember() &&
-            !VD->getDeclContext()->getDeclaredInterfaceType()
-                ->hasReferenceSemantics()) {
-          return false;
-        }
-
         // If this property wrapper uses autoclosure in it's initializer,
         // the argument types of the setter and initializer shall be
         // different, so we don't rewrite an assignment into an
@@ -1490,7 +1473,9 @@ namespace {
         }
 
         CanSILFunctionType setterTy = setterFRef->getType().castTo<SILFunctionType>();
-        SILFunctionConventions setterConv(setterTy, SGF.SGM.M);
+        auto substSetterTy = setterTy->substGenericArgs(SGF.SGM.M, Substitutions,
+                                                        SGF.getTypeExpansionContext());
+        SILFunctionConventions setterConv(substSetterTy, SGF.SGM.M);
 
         // Emit captures for the setter
         SmallVector<SILValue, 4> capturedArgs;
@@ -1509,21 +1494,19 @@ namespace {
 
           if (setterConv.getSILArgumentConvention(argIdx).isInoutConvention()) {
             capturedBase = base.getValue();
+          } else if (base.getType().isAddress() &&
+                     base.getType().getObjectType() ==
+                     setterConv.getSILArgumentType(argIdx,
+                                                   SGF.getTypeExpansionContext())) {
+            // If the base is a reference and the setter expects a value, emit a
+            // load. This pattern is emitted for property wrappers with a
+            // nonmutating setter, for example.
+            capturedBase = SGF.B.createTrivialLoadOr(
+                loc, base.getValue(), LoadOwnershipQualifier::Copy);
           } else {
             capturedBase = base.copy(SGF, loc).forward(SGF);
           }
 
-          // If the base is a reference and the setter expects a value, emit a
-          // load. This pattern is emitted for property wrappers with a
-          // nonmutating setter, for example.
-          if (base.getType().isAddress() &&
-              base.getType().getObjectType() ==
-                  setterConv.getSILArgumentType(argIdx,
-                                                SGF.getTypeExpansionContext())) {
-            capturedBase = SGF.B.createTrivialLoadOr(
-                loc, capturedBase, LoadOwnershipQualifier::Take);
-          }
-
           capturedArgs.push_back(capturedBase);
         }
 
@@ -1538,8 +1521,6 @@ namespace {
         assert(value.isRValue());
         ManagedValue Mval = std::move(value).asKnownRValue(SGF).
                               getAsSingleValue(SGF, loc);
-        auto substSetterTy = setterTy->substGenericArgs(SGF.SGM.M, Substitutions,
-                                                        SGF.getTypeExpansionContext());
         auto param = substSetterTy->getParameters()[0];
         SILType loweredSubstArgType = Mval.getType();
         if (param.isIndirectInOut()) {

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

@@ -625,6 +625,22 @@ bool LifetimeChecker::shouldEmitError(const SILInstruction *Inst) {
       }))
     return false;
 
+  // Ignore loads used only by an assign_by_wrapper setter. This
+  // is safe to ignore because assign_by_wrapper will only be
+  // re-written to use the setter if the value is fully initialized.
+  if (auto *load = dyn_cast<SingleValueInstruction>(Inst)) {
+    if (auto Op = load->getSingleUse()) {
+      if (auto PAI = dyn_cast<PartialApplyInst>(Op->getUser())) {
+        if (std::find_if(PAI->use_begin(), PAI->use_end(),
+                         [](auto PAIUse) {
+                           return isa<AssignByWrapperInst>(PAIUse->getUser());
+                         }) != PAI->use_end()) {
+          return false;
+        }
+      }
+    }
+  }
+
   EmittedErrorLocs.push_back(InstLoc);
   return true;
 }
@@ -1841,20 +1857,6 @@ void LifetimeChecker::handleLoadUseFailure(const DIMemoryUse &Use,
       TheMemory.isAnyInitSelf() && !TheMemory.isClassInitSelf()) {
     if (!shouldEmitError(Inst)) return;
 
-    // Ignore loads used only for a set-by-value (nonmutating) setter
-    // since it will be deleted by lowering anyway.
-    auto load = cast<SingleValueInstruction>(Inst);
-    if (auto Op = load->getSingleUse()) {
-      if (auto PAI = dyn_cast<PartialApplyInst>(Op->getUser())) {
-        if (std::find_if(PAI->use_begin(), PAI->use_end(),
-                         [](auto PAIUse) {
-                           return isa<AssignByWrapperInst>(PAIUse->getUser());
-                         }) != PAI->use_end()) {
-          return;
-        }
-      }
-    }
-
     diagnose(Module, Inst->getLoc(), diag::use_of_self_before_fully_init);
     noteUninitializedMembers(Use);
     return;

lib/Sema/CSApply.cpp

@@ -6828,26 +6828,22 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
   llvm_unreachable("Unhandled coercion");
 }
 
-/// Detect if the expression is an assignment to a `self` wrapped property that
-/// has a nonmutating setter, inside a constructor.
-///
-/// We use this to decide when to produce an inout_expr instead of a load_expr
-/// for the sake of emitting a reference required by the assign_by_wrapper
-/// instruction.
-static bool isNonMutatingSetterPWAssignInsideInit(Expr *baseExpr,
-                                                  ValueDecl *member,
-                                                  DeclContext *UseDC) {
-  // Setter is mutating
-  if (cast<AbstractStorageDecl>(member)->isSetterMutating())
-    return false;
+/// Detect whether an assignment to \c baseExpr.member in the given
+/// decl context can potentially be initialization of a property wrapper.
+static bool isPotentialPropertyWrapperInit(Expr *baseExpr,
+                                           ValueDecl *member,
+                                           DeclContext *UseDC) {
   // Member is not a wrapped property
   auto *VD = dyn_cast<VarDecl>(member);
   if (!(VD && VD->hasAttachedPropertyWrapper()))
     return false;
-  // This is not an expression inside a constructor
+
+  // Assignment to a wrapped property can only be re-written to
+  // initialization in an init.
   auto *CD = dyn_cast<ConstructorDecl>(UseDC);
   if (!CD)
     return false;
+
   // This is not an assignment on self
   if (!baseExpr->isSelfExprOf(CD))
     return false;
@@ -6887,15 +6883,14 @@ static Type adjustSelfTypeForMember(Expr *baseExpr,
   bool isSettableFromHere =
       SD->isSettable(UseDC) && SD->isSetterAccessibleFrom(UseDC);
 
-  // If neither the property's getter nor its setter are mutating, and
-  // this is not a nonmutating property wrapper setter,
-  // the base can be an rvalue.
-  // With the exception of assignments to a wrapped property inside a
-  // constructor, where we need to produce a reference to be used on
-  // the assign_by_wrapper instruction. 
-  if (!SD->isGetterMutating() && 
+  // If neither the property's getter nor its setter are mutating,
+  // the base can be an rvalue unless the assignment is potentially
+  // initializing a property wrapper. If the assignment can be re-
+  // written to property wrapper initialization, the base type should
+  // be an lvalue.
+  if (!SD->isGetterMutating() &&
       (!isSettableFromHere || !SD->isSetterMutating()) &&
-      !isNonMutatingSetterPWAssignInsideInit(baseExpr, member, UseDC))
+      !isPotentialPropertyWrapperInit(baseExpr, member, UseDC))
     return baseObjectTy;
 
   if (isa<SubscriptDecl>(member))

test/SILOptimizer/di_property_wrappers.swift

@@ -537,30 +537,66 @@ func testSR_12341() {
 
 @propertyWrapper
 struct NonMutatingSetterWrapper<Value> {
-    var value: Value
-    init(wrappedValue: Value) {
-        value = wrappedValue
-    }
-    var wrappedValue: Value {
-        get { value }
-        nonmutating set {
-            print("  .. nonmutatingSet \(newValue)")
-        }
+  var value: Value
+  init(wrappedValue: Value) {
+    print("  .. init \(wrappedValue)")
+    value = wrappedValue
+  }
+  var wrappedValue: Value {
+    get { value }
+    nonmutating set {
+      print("  .. nonmutatingSet \(newValue)")
     }
+  }
 }
 
 struct NonMutatingWrapperTestStruct {
-    @NonMutatingSetterWrapper var SomeProp: Int
-    init(val: Int) {
-        SomeProp = val
-    }
+  @NonMutatingSetterWrapper var SomeProp: Int
+  init(val: Int) {
+    SomeProp = val
+    SomeProp = val + 1
+  }
 }
 
 func testNonMutatingSetterStruct() {
   // CHECK: ## NonMutatingSetterWrapper
   print("\n## NonMutatingSetterWrapper")
   let A = NonMutatingWrapperTestStruct(val: 11)
-  // CHECK-NEXT:  .. nonmutatingSet 11
+  // CHECK-NEXT:  .. init 11
+  // CHECK-NEXT:  .. nonmutatingSet 12
+}
+
+@propertyWrapper
+final class ClassWrapper<T> {
+  private var _wrappedValue: T
+  var wrappedValue: T {
+    get { _wrappedValue }
+    set {
+      print(" .. set \(newValue)")
+      _wrappedValue = newValue
+    }
+  }
+
+  init(wrappedValue: T) {
+    print(" .. init \(wrappedValue)")
+    self._wrappedValue = wrappedValue
+  }
+}
+
+struct StructWithClassWrapper<T> {
+   @ClassWrapper private var _storage: T?
+
+   init(value: T?) {
+      _storage = value
+   }
+}
+
+func testStructWithClassWrapper() {
+  // CHECK: ## StructWithClassWrapper
+  print("\n## StructWithClassWrapper")
+  let _ = StructWithClassWrapper<Int>(value: 10)
+  // CHECK-NEXT:  .. init nil
+  // CHECK-NEXT:  .. set Optional(10)
 }
 
 testIntStruct()
@@ -574,3 +610,4 @@ testComposed()
 testWrapperInitWithDefaultArg()
 testSR_12341()
 testNonMutatingSetterStruct()
+testStructWithClassWrapper()

test/SILOptimizer/di_property_wrappers_errors.swift

@@ -23,22 +23,22 @@ struct IntStructWithClassWrapper {
   @ClassWrapper var wrapped: Int
 
   init() {
-    wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
+    wrapped = 42
   }
 
   init(conditional b: Bool) {
      if b {
        self._wrapped = ClassWrapper(wrappedValue: 32)
      } else {
-       wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
+       wrapped = 42
      }
   }
 
   init(dynamic b: Bool) {
     if b {
-      wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
+      wrapped = 42
     }
-    wrapped = 27 // expected-error{{variable 'self.wrapped' used before being initialized}}
+    wrapped = 27
   }
 }