Skip to content

[5.3] Allow initializing a wrapped property with a nonmutating setter #31460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions lib/SILGen/SILGenLValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1302,18 +1302,28 @@ namespace {
{
}

bool canRewriteSetAsPropertyWrapperInit() const {
bool canRewriteSetAsPropertyWrapperInit(SILGenFunction &SGF) const {
if (auto *VD = dyn_cast<VarDecl>(Storage)) {
// If this is not a wrapper property that can be initialized from
// a value of the wrapped type, we can't perform the initialization.
auto wrapperInfo = VD->getPropertyWrapperBackingPropertyInfo();
if (!wrapperInfo.initializeFromOriginal)
return false;

bool isAssignmentToSelfParamInInit =
IsOnSelfParameter &&
isa<ConstructorDecl>(SGF.FunctionDC->getAsDecl());

// 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.
if (!VD->isSetterMutating() &&

// 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()
Expand Down Expand Up @@ -1396,7 +1406,7 @@ namespace {
assert(getAccessorDecl()->isSetter());
SILDeclRef setter = Accessor;

if (IsOnSelfParameter && canRewriteSetAsPropertyWrapperInit() &&
if (IsOnSelfParameter && canRewriteSetAsPropertyWrapperInit(SGF) &&
!Storage->isStatic() &&
isBackingVarVisible(cast<VarDecl>(Storage),
SGF.FunctionDC)) {
Expand Down Expand Up @@ -1477,6 +1487,16 @@ namespace {
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)) {
capturedBase = SGF.B.createTrivialLoadOr(
loc, capturedBase, LoadOwnershipQualifier::Take);
}

PartialApplyInst *setterPAI =
SGF.B.createPartialApply(loc, setterFRef,
Substitutions, { capturedBase },
Expand Down Expand Up @@ -1515,6 +1535,7 @@ namespace {
SGF.B.createAssignByWrapper(loc, Mval.forward(SGF), proj.forward(SGF),
initFn.getValue(), setterFn.getValue(),
AssignOwnershipQualifier::Unknown);

return;
}

Expand Down Expand Up @@ -4300,7 +4321,7 @@ static bool trySetterPeephole(SILGenFunction &SGF, SILLocation loc,
}

auto &setterComponent = static_cast<GetterSetterComponent&>(component);
if (setterComponent.canRewriteSetAsPropertyWrapperInit())
if (setterComponent.canRewriteSetAsPropertyWrapperInit(SGF))
return false;
setterComponent.emitAssignWithSetter(SGF, loc, std::move(dest),
std::move(src));
Expand Down
15 changes: 15 additions & 0 deletions lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "swift/AST/Expr.h"
#include "swift/AST/Stmt.h"
#include "swift/ClangImporter/ClangModule.h"
#include "swift/SIL/SILValue.h"
#include "swift/SIL/InstructionUtils.h"
#include "swift/SIL/SILArgument.h"
#include "swift/SIL/SILBuilder.h"
Expand Down Expand Up @@ -1802,6 +1803,20 @@ 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;
Expand Down
45 changes: 39 additions & 6 deletions lib/Sema/CSApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6925,9 +6925,37 @@ 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;
// 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
auto *CD = dyn_cast<ConstructorDecl>(UseDC);
if (!CD)
return false;
// This is not an assignment on self
if (!baseExpr->isSelfExprOf(CD))
return false;

return true;
}

/// Adjust the given type to become the self type when referring to
/// the given member.
static Type adjustSelfTypeForMember(Type baseTy, ValueDecl *member,
static Type adjustSelfTypeForMember(Expr *baseExpr,
Type baseTy, ValueDecl *member,
AccessSemantics semantics,
DeclContext *UseDC) {
auto baseObjectTy = baseTy->getWithoutSpecifierType();
Expand All @@ -6951,10 +6979,15 @@ static Type adjustSelfTypeForMember(Type baseTy, ValueDecl *member,
bool isSettableFromHere =
SD->isSettable(UseDC) && SD->isSetterAccessibleFrom(UseDC);

// If neither the property's getter nor its setter are mutating, the base
// can be an rvalue.
if (!SD->isGetterMutating()
&& (!isSettableFromHere || !SD->isSetterMutating()))
// 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() &&
(!isSettableFromHere || !SD->isSetterMutating()) &&
!isNonMutatingSetterPWAssignInsideInit(baseExpr, member, UseDC))
return baseObjectTy;

// If we're calling an accessor, keep the base as an inout type, because the
Expand Down Expand Up @@ -6984,7 +7017,7 @@ ExprRewriter::coerceObjectArgumentToType(Expr *expr,
Type baseTy, ValueDecl *member,
AccessSemantics semantics,
ConstraintLocatorBuilder locator) {
Type toType = adjustSelfTypeForMember(baseTy, member, semantics, dc);
Type toType = adjustSelfTypeForMember(expr, baseTy, member, semantics, dc);

// If our expression already has the right type, we're done.
Type fromType = cs.getType(expr);
Expand Down
32 changes: 32 additions & 0 deletions test/SILGen/property_wrappers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,38 @@ struct ObservedObject<ObjectType : AnyObject > {
}
}



// rdar://problem/60600911
// Ensure assign_by_wrapper is emitted for initialization
// of a property wrapper with a nonmutating set. Even though such setters
// take `self` by-value.
@propertyWrapper
struct NonMutatingSetterWrapper<Value> {
var value: Value
init(wrappedValue: Value) {
value = wrappedValue
}
var wrappedValue: Value {
get { value }
nonmutating set {
print(" .. nonmutatingSet \(newValue)")
}
}
}

struct NonMutatingWrapperTestStruct {
// CHECK-LABEL: sil hidden [ossa] @$s17property_wrappers28NonMutatingWrapperTestStructV3valACSi_tcfC : $@convention(method) (Int, @thin NonMutatingWrapperTestStruct.Type) -> NonMutatingWrapperTestStruct {
// CHECK: %[[LOAD:[0-9]+]] = load [trivial] %[[SRC:[0-9]+]] : $*NonMutatingWrapperTestStruct
// CHECK-NEXT: %[[SET_PA:[0-9]+]] = partial_apply [callee_guaranteed] %[[PW_SETTER:[0-9]+]](%[[LOAD]]) : $@convention(method) (Int, NonMutatingWrapperTestStruct) -> ()
// CHECK-NEXT: assign_by_wrapper %[[SETVAL:[0-9]+]] : $Int to %[[ADDR:[0-9]+]] : $*NonMutatingSetterWrapper<Int>, init %[[INIT_PA:[0-9]+]] : $@callee_guaranteed (Int) -> NonMutatingSetterWrapper<Int>, set %[[SET_PA]] : $@callee_guaranteed (Int) -> ()
@NonMutatingSetterWrapper var SomeProp: Int
init(val: Int) {
SomeProp = val
}
}


// SR-12443: Crash on property with wrapper override that adds observer.
@propertyWrapper
struct BasicIntWrapper {
Expand Down
30 changes: 30 additions & 0 deletions test/SILOptimizer/di_property_wrappers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,7 @@ public final class Synchronized<Value> {
}
}


struct SR_12341 {
@Wrapper var wrapped: Int = 10
var str: String
Expand Down Expand Up @@ -533,6 +534,34 @@ func testSR_12341() {
_ = SR_12341(condition: true)
}

@propertyWrapper
struct NonMutatingSetterWrapper<Value> {
var value: Value
init(wrappedValue: Value) {
value = wrappedValue
}
var wrappedValue: Value {
get { value }
nonmutating set {
print(" .. nonmutatingSet \(newValue)")
}
}
}

struct NonMutatingWrapperTestStruct {
@NonMutatingSetterWrapper var SomeProp: Int
init(val: Int) {
SomeProp = val
}
}

func testNonMutatingSetterStruct() {
// CHECK: ## NonMutatingSetterWrapper
print("\n## NonMutatingSetterWrapper")
let A = NonMutatingWrapperTestStruct(val: 11)
// CHECK-NEXT: .. nonmutatingSet 11
}

testIntStruct()
testIntClass()
testRefStruct()
Expand All @@ -543,3 +572,4 @@ testDefaultNilOptIntStruct()
testComposed()
testWrapperInitWithDefaultArg()
testSR_12341()
testNonMutatingSetterStruct()
21 changes: 7 additions & 14 deletions test/SILOptimizer/di_property_wrappers_errors.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,23 @@ struct IntStructWithClassWrapper {
@ClassWrapper var wrapped: Int

init() {
wrapped = 42 // expected-error{{'self' used before all stored properties are initialized}}
// expected-note@-1{{'self.wrapped' not initialized}}
} // expected-error{{return from initializer without initializing all stored properties}}
// expected-note@-1{{'self.wrapped' not initialized}}
wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
}

init(conditional b: Bool) {
if b {
self._wrapped = ClassWrapper(wrappedValue: 32)
} else {
wrapped = 42 // expected-error{{'self' used before all stored properties are initialized}}
// expected-note@-1{{'self.wrapped' not initialized}}
wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
}
} // expected-error{{return from initializer without initializing all stored properties}}
// expected-note@-1{{'self.wrapped' not initialized}}
}

init(dynamic b: Bool) {
if b {
wrapped = 42 // expected-error{{'self' used before all stored properties are initialized}}
// expected-note@-1{{'self.wrapped' not initialized}}
wrapped = 42 // expected-error{{variable 'self.wrapped' used before being initialized}}
}
wrapped = 27 // expected-error{{'self' used before all stored properties are initialized}}
// expected-note@-1{{'self.wrapped' not initialized}}
} // expected-error{{return from initializer without initializing all stored properties}}
// expected-note@-1{{'self.wrapped' not initialized}}
wrapped = 27 // expected-error{{variable 'self.wrapped' used before being initialized}}
}
}

// SR_11477
Expand Down