Skip to content

Commit 80a9df2

Browse files
hborlahborla
authored
Merge pull request swiftlang#35218 from hborla/assign-by-wrapper-crash
[Property Wrappers] Fix a few corner cases where property wrappers with nonmutating setters fail in DI
2 parents c72a454 + 3640de3 commit 80a9df2

File tree

5 files changed

+99
-84
lines changed

5 files changed

+99
-84
lines changed

‎lib/SILGen/SILGenLValue.cpp‎

Lines changed: 12 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1325,23 +1325,6 @@ namespace {
13251325
wrapperInfo.wrappedValuePlaceholder->getOriginalWrappedValue())
13261326
return false;
13271327

1328-
// If we have a nonmutating setter on a value type, the call
1329-
// captures all of 'self' and we cannot rewrite an assignment
1330-
// into an initialization.
1331-
1332-
// Unless this is an assignment to a self parameter inside a
1333-
// constructor, in which case we would like to still emit a
1334-
// assign_by_wrapper because the setter will be deleted by lowering
1335-
// anyway.
1336-
if (!isAssignmentToSelfParamInInit &&
1337-
!VD->isSetterMutating() &&
1338-
VD->getDeclContext()->getSelfNominalTypeDecl() &&
1339-
VD->isInstanceMember() &&
1340-
!VD->getDeclContext()->getDeclaredInterfaceType()
1341-
->hasReferenceSemantics()) {
1342-
return false;
1343-
}
1344-
13451328
// If this property wrapper uses autoclosure in it's initializer,
13461329
// the argument types of the setter and initializer shall be
13471330
// different, so we don't rewrite an assignment into an
@@ -1490,7 +1473,9 @@ namespace {
14901473
}
14911474

14921475
CanSILFunctionType setterTy = setterFRef->getType().castTo<SILFunctionType>();
1493-
SILFunctionConventions setterConv(setterTy, SGF.SGM.M);
1476+
auto substSetterTy = setterTy->substGenericArgs(SGF.SGM.M, Substitutions,
1477+
SGF.getTypeExpansionContext());
1478+
SILFunctionConventions setterConv(substSetterTy, SGF.SGM.M);
14941479

14951480
// Emit captures for the setter
14961481
SmallVector<SILValue, 4> capturedArgs;
@@ -1509,21 +1494,19 @@ namespace {
15091494

15101495
if (setterConv.getSILArgumentConvention(argIdx).isInoutConvention()) {
15111496
capturedBase = base.getValue();
1497+
} else if (base.getType().isAddress() &&
1498+
base.getType().getObjectType() ==
1499+
setterConv.getSILArgumentType(argIdx,
1500+
SGF.getTypeExpansionContext())) {
1501+
// If the base is a reference and the setter expects a value, emit a
1502+
// load. This pattern is emitted for property wrappers with a
1503+
// nonmutating setter, for example.
1504+
capturedBase = SGF.B.createTrivialLoadOr(
1505+
loc, base.getValue(), LoadOwnershipQualifier::Copy);
15121506
} else {
15131507
capturedBase = base.copy(SGF, loc).forward(SGF);
15141508
}
15151509

1516-
// If the base is a reference and the setter expects a value, emit a
1517-
// load. This pattern is emitted for property wrappers with a
1518-
// nonmutating setter, for example.
1519-
if (base.getType().isAddress() &&
1520-
base.getType().getObjectType() ==
1521-
setterConv.getSILArgumentType(argIdx,
1522-
SGF.getTypeExpansionContext())) {
1523-
capturedBase = SGF.B.createTrivialLoadOr(
1524-
loc, capturedBase, LoadOwnershipQualifier::Take);
1525-
}
1526-
15271510
capturedArgs.push_back(capturedBase);
15281511
}
15291512

@@ -1538,8 +1521,6 @@ namespace {
15381521
assert(value.isRValue());
15391522
ManagedValue Mval = std::move(value).asKnownRValue(SGF).
15401523
getAsSingleValue(SGF, loc);
1541-
auto substSetterTy = setterTy->substGenericArgs(SGF.SGM.M, Substitutions,
1542-
SGF.getTypeExpansionContext());
15431524
auto param = substSetterTy->getParameters()[0];
15441525
SILType loweredSubstArgType = Mval.getType();
15451526
if (param.isIndirectInOut()) {

‎lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -625,6 +625,22 @@ bool LifetimeChecker::shouldEmitError(const SILInstruction *Inst) {
625625
}))
626626
return false;
627627

628+
// Ignore loads used only by an assign_by_wrapper setter. This
629+
// is safe to ignore because assign_by_wrapper will only be
630+
// re-written to use the setter if the value is fully initialized.
631+
if (auto *load = dyn_cast<SingleValueInstruction>(Inst)) {
632+
if (auto Op = load->getSingleUse()) {
633+
if (auto PAI = dyn_cast<PartialApplyInst>(Op->getUser())) {
634+
if (std::find_if(PAI->use_begin(), PAI->use_end(),
635+
[](auto PAIUse) {
636+
return isa<AssignByWrapperInst>(PAIUse->getUser());
637+
}) != PAI->use_end()) {
638+
return false;
639+
}
640+
}
641+
}
642+
}
643+
628644
EmittedErrorLocs.push_back(InstLoc);
629645
return true;
630646
}
@@ -1842,20 +1858,6 @@ void LifetimeChecker::handleLoadUseFailure(const DIMemoryUse &Use,
18421858
TheMemory.isAnyInitSelf() && !TheMemory.isClassInitSelf()) {
18431859
if (!shouldEmitError(Inst)) return;
18441860

1845-
// Ignore loads used only for a set-by-value (nonmutating) setter
1846-
// since it will be deleted by lowering anyway.
1847-
auto load = cast<SingleValueInstruction>(Inst);
1848-
if (auto Op = load->getSingleUse()) {
1849-
if (auto PAI = dyn_cast<PartialApplyInst>(Op->getUser())) {
1850-
if (std::find_if(PAI->use_begin(), PAI->use_end(),
1851-
[](auto PAIUse) {
1852-
return isa<AssignByWrapperInst>(PAIUse->getUser());
1853-
}) != PAI->use_end()) {
1854-
return;
1855-
}
1856-
}
1857-
}
1858-
18591861
diagnose(Module, Inst->getLoc(), diag::use_of_self_before_fully_init);
18601862
noteUninitializedMembers(Use);
18611863
return;

‎lib/Sema/CSApply.cpp‎

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6828,26 +6828,22 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
68286828
llvm_unreachable("Unhandled coercion");
68296829
}
68306830

6831-
/// Detect if the expression is an assignment to a `self` wrapped property that
6832-
/// has a nonmutating setter, inside a constructor.
6833-
///
6834-
/// We use this to decide when to produce an inout_expr instead of a load_expr
6835-
/// for the sake of emitting a reference required by the assign_by_wrapper
6836-
/// instruction.
6837-
static bool isNonMutatingSetterPWAssignInsideInit(Expr *baseExpr,
6838-
ValueDecl *member,
6839-
DeclContext *UseDC) {
6840-
// Setter is mutating
6841-
if (cast<AbstractStorageDecl>(member)->isSetterMutating())
6842-
return false;
6831+
/// Detect whether an assignment to \c baseExpr.member in the given
6832+
/// decl context can potentially be initialization of a property wrapper.
6833+
static bool isPotentialPropertyWrapperInit(Expr *baseExpr,
6834+
ValueDecl *member,
6835+
DeclContext *UseDC) {
68436836
// Member is not a wrapped property
68446837
auto *VD = dyn_cast<VarDecl>(member);
68456838
if (!(VD && VD->hasAttachedPropertyWrapper()))
68466839
return false;
6847-
// This is not an expression inside a constructor
6840+
6841+
// Assignment to a wrapped property can only be re-written to
6842+
// initialization in an init.
68486843
auto *CD = dyn_cast<ConstructorDecl>(UseDC);
68496844
if (!CD)
68506845
return false;
6846+
68516847
// This is not an assignment on self
68526848
if (!baseExpr->isSelfExprOf(CD))
68536849
return false;
@@ -6887,15 +6883,14 @@ static Type adjustSelfTypeForMember(Expr *baseExpr,
68876883
bool isSettableFromHere =
68886884
SD->isSettable(UseDC) && SD->isSetterAccessibleFrom(UseDC);
68896885

6890-
// If neither the property's getter nor its setter are mutating, and
6891-
// this is not a nonmutating property wrapper setter,
6892-
// the base can be an rvalue.
6893-
// With the exception of assignments to a wrapped property inside a
6894-
// constructor, where we need to produce a reference to be used on
6895-
// the assign_by_wrapper instruction.
6896-
if (!SD->isGetterMutating() &&
6886+
// If neither the property's getter nor its setter are mutating,
6887+
// the base can be an rvalue unless the assignment is potentially
6888+
// initializing a property wrapper. If the assignment can be re-
6889+
// written to property wrapper initialization, the base type should
6890+
// be an lvalue.
6891+
if (!SD->isGetterMutating() &&
68976892
(!isSettableFromHere || !SD->isSetterMutating()) &&
6898-
!isNonMutatingSetterPWAssignInsideInit(baseExpr, member, UseDC))
6893+
!isPotentialPropertyWrapperInit(baseExpr, member, UseDC))
68996894
return baseObjectTy;
69006895

69016896
if (isa<SubscriptDecl>(member))

‎test/SILOptimizer/di_property_wrappers.swift‎

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -537,30 +537,66 @@ func testSR_12341() {
537537

538538
@propertyWrapper
539539
struct NonMutatingSetterWrapper<Value> {
540-
var value: Value
541-
init(wrappedValue: Value) {
542-
value = wrappedValue
543-
}
544-
var wrappedValue: Value {
545-
get { value }
546-
nonmutating set {
547-
print(" .. nonmutatingSet \(newValue)")
548-
}
540+
var value: Value
541+
init(wrappedValue: Value) {
542+
print(" .. init \(wrappedValue)")
543+
value = wrappedValue
544+
}
545+
var wrappedValue: Value {
546+
get { value }
547+
nonmutating set {
548+
print(" .. nonmutatingSet \(newValue)")
549549
}
550+
}
550551
}
551552

552553
struct NonMutatingWrapperTestStruct {
553-
@NonMutatingSetterWrapper var SomeProp: Int
554-
init(val: Int) {
555-
SomeProp = val
556-
}
554+
@NonMutatingSetterWrapper var SomeProp: Int
555+
init(val: Int) {
556+
SomeProp = val
557+
SomeProp = val + 1
558+
}
557559
}
558560

559561
func testNonMutatingSetterStruct() {
560562
// CHECK: ## NonMutatingSetterWrapper
561563
print("\n## NonMutatingSetterWrapper")
562564
let A = NonMutatingWrapperTestStruct(val: 11)
563-
// CHECK-NEXT: .. nonmutatingSet 11
565+
// CHECK-NEXT: .. init 11
566+
// CHECK-NEXT: .. nonmutatingSet 12
567+
}
568+
569+
@propertyWrapper
570+
final class ClassWrapper<T> {
571+
private var _wrappedValue: T
572+
var wrappedValue: T {
573+
get { _wrappedValue }
574+
set {
575+
print(" .. set \(newValue)")
576+
_wrappedValue = newValue
577+
}
578+
}
579+
580+
init(wrappedValue: T) {
581+
print(" .. init \(wrappedValue)")
582+
self._wrappedValue = wrappedValue
583+
}
584+
}
585+
586+
struct StructWithClassWrapper<T> {
587+
@ClassWrapper private var _storage: T?
588+
589+
init(value: T?) {
590+
_storage = value
591+
}
592+
}
593+
594+
func testStructWithClassWrapper() {
595+
// CHECK: ## StructWithClassWrapper
596+
print("\n## StructWithClassWrapper")
597+
let _ = StructWithClassWrapper<Int>(value: 10)
598+
// CHECK-NEXT: .. init nil
599+
// CHECK-NEXT: .. set Optional(10)
564600
}
565601

566602
testIntStruct()
@@ -574,3 +610,4 @@ testComposed()
574610
testWrapperInitWithDefaultArg()
575611
testSR_12341()
576612
testNonMutatingSetterStruct()
613+
testStructWithClassWrapper()

‎test/SILOptimizer/di_property_wrappers_errors.swift‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,22 @@ struct IntStructWithClassWrapper {
2323
@ClassWrapper var wrapped: Int
2424

2525
init() {
26-
wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
26+
wrapped = 42
2727
}
2828

2929
init(conditional b: Bool) {
3030
if b {
3131
self._wrapped = ClassWrapper(wrappedValue: 32)
3232
} else {
33-
wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
33+
wrapped = 42
3434
}
3535
}
3636

3737
init(dynamic b: Bool) {
3838
if b {
39-
wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
39+
wrapped = 42
4040
}
41-
wrapped = 27 // expected-error{{variable 'self.wrapped' used before being initialized}}
41+
wrapped = 27
4242
}
4343
}
4444

0 commit comments

Comments
 (0)