Skip to content

Commit 49e70ed

Browse files
tkremenektkremenek
authored
Merge pull request #30994 from theblixguy/fix/SR-12178_5.2
[5.2] [Typechecker] Fix _modify for wrapped properties with observers
2 parents eb30e62 + 30ad8d0 commit 49e70ed

File tree

6 files changed

+138
-18
lines changed

6 files changed

+138
-18
lines changed

‎include/swift/AST/Decl.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4707,6 +4707,13 @@ class AbstractStorageDecl : public ValueDecl {
47074707
/// Does this storage require a 'modify' accessor in its opaque-accessors set?
47084708
bool requiresOpaqueModifyCoroutine() const;
47094709

4710+
/// Does this storage have any explicit observers (willSet or didSet) attached
4711+
/// to it?
4712+
bool hasObservers() const {
4713+
return getParsedAccessor(AccessorKind::WillSet) ||
4714+
getParsedAccessor(AccessorKind::DidSet);
4715+
}
4716+
47104717
SourceRange getBracesRange() const {
47114718
if (auto info = Accessors.getPointer())
47124719
return info->getBracesRange();

‎lib/Sema/TypeCheckDeclOverride.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1568,8 +1568,7 @@ static bool checkSingleOverride(ValueDecl *override, ValueDecl *base) {
15681568
// Make sure that the overriding property doesn't have storage.
15691569
if ((overrideASD->hasStorage() ||
15701570
overrideASD->getAttrs().hasAttribute<LazyAttr>()) &&
1571-
!(overrideASD->getParsedAccessor(AccessorKind::WillSet) ||
1572-
overrideASD->getParsedAccessor(AccessorKind::DidSet))) {
1571+
!overrideASD->hasObservers()) {
15731572
bool downgradeToWarning = false;
15741573
if (!ctx.isSwiftVersionAtLeast(5) &&
15751574
overrideASD->getAttrs().hasAttribute<LazyAttr>()) {

‎lib/Sema/TypeCheckStorage.cpp‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -757,6 +757,8 @@ static Expr *buildStorageReference(AccessorDecl *accessor,
757757
bool isMemberLValue = isLValue;
758758
auto propertyWrapperMutability =
759759
[&](Decl *decl) -> Optional<std::pair<bool, bool>> {
760+
if (target != TargetImpl::Wrapper && target != TargetImpl::WrapperStorage)
761+
return None;
760762
auto var = dyn_cast<VarDecl>(decl);
761763
if (!var)
762764
return None;
@@ -1531,8 +1533,7 @@ synthesizeSetterBody(AccessorDecl *setter, ASTContext &ctx) {
15311533
}
15321534

15331535
if (var->hasAttachedPropertyWrapper()) {
1534-
if (var->getParsedAccessor(AccessorKind::WillSet) ||
1535-
var->getParsedAccessor(AccessorKind::DidSet)) {
1536+
if (var->hasObservers()) {
15361537
return synthesizeObservedSetterBody(setter, TargetImpl::Wrapper, ctx);
15371538
}
15381539

@@ -1580,20 +1581,24 @@ synthesizeCoroutineAccessorBody(AccessorDecl *accessor, ASTContext &ctx) {
15801581
assert(accessor->isCoroutine());
15811582

15821583
auto storage = accessor->getStorage();
1584+
auto storageReadWriteImpl = storage->getReadWriteImpl();
15831585
auto target = (accessor->hasForcedStaticDispatch()
15841586
? TargetImpl::Ordinary
15851587
: TargetImpl::Implementation);
15861588

15871589
// If this is a variable with an attached property wrapper, then
15881590
// the accessors need to yield the wrappedValue or projectedValue.
1589-
if (auto var = dyn_cast<VarDecl>(storage)) {
1590-
if (var->hasAttachedPropertyWrapper()) {
1591-
target = TargetImpl::Wrapper;
1592-
}
1591+
if (accessor->getAccessorKind() == AccessorKind::Read ||
1592+
storageReadWriteImpl == ReadWriteImplKind::Modify) {
1593+
if (auto var = dyn_cast<VarDecl>(storage)) {
1594+
if (var->hasAttachedPropertyWrapper()) {
1595+
target = TargetImpl::Wrapper;
1596+
}
15931597

1594-
if (var->getOriginalWrappedProperty(
1595-
PropertyWrapperSynthesizedPropertyKind::StorageWrapper)) {
1596-
target = TargetImpl::WrapperStorage;
1598+
if (var->getOriginalWrappedProperty(
1599+
PropertyWrapperSynthesizedPropertyKind::StorageWrapper)) {
1600+
target = TargetImpl::WrapperStorage;
1601+
}
15971602
}
15981603
}
15991604

@@ -2043,8 +2048,7 @@ IsAccessorTransparentRequest::evaluate(Evaluator &evaluator,
20432048
// FIXME: This should be folded into the WriteImplKind below.
20442049
if (auto var = dyn_cast<VarDecl>(storage)) {
20452050
if (var->hasAttachedPropertyWrapper()) {
2046-
if (var->getParsedAccessor(AccessorKind::DidSet) ||
2047-
var->getParsedAccessor(AccessorKind::WillSet))
2051+
if (var->hasObservers())
20482052
return false;
20492053

20502054
break;
@@ -2581,11 +2585,17 @@ static void finishPropertyWrapperImplInfo(VarDecl *var,
25812585
}
25822586
}
25832587

2584-
if (wrapperSetterIsUsable)
2588+
if (!wrapperSetterIsUsable) {
2589+
info = StorageImplInfo::getImmutableComputed();
2590+
return;
2591+
}
2592+
2593+
if (var->hasObservers()) {
2594+
info = StorageImplInfo::getMutableComputed();
2595+
} else {
25852596
info = StorageImplInfo(ReadImplKind::Get, WriteImplKind::Set,
25862597
ReadWriteImplKind::Modify);
2587-
else
2588-
info = StorageImplInfo::getImmutableComputed();
2598+
}
25892599
}
25902600

25912601
static void finishNSManagedImplInfo(VarDecl *var,

‎test/SILGen/property_wrapper_observers.swift‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// RUN: %target-swift-emit-silgen %s | %FileCheck %s
2+
3+
// 1. Make sure the wrapped property setter calls the observers
4+
// 2. Make sure the synthesized _modify coroutine calls the wrapped property setter
5+
6+
@propertyWrapper
7+
struct Foo {
8+
private var _storage: [Int] = []
9+
10+
init(wrappedValue value: [Int]) {
11+
self._storage = value
12+
}
13+
14+
var wrappedValue: [Int] {
15+
get { _storage }
16+
set { _storage = newValue }
17+
}
18+
}
19+
20+
class Bar {
21+
@Foo var someArray = [1, 2, 3] {
22+
willSet {}
23+
didSet {}
24+
}
25+
}
26+
27+
// Bar.someArray.setter
28+
29+
// CHECK-LABEL: sil hidden [ossa] @$s26property_wrapper_observers3BarC9someArraySaySiGvs : $@convention(method) (@owned Array<Int>, @guaranteed Bar) -> () {
30+
// CHECK: bb0([[VALUE:%.*]] : @owned $Array<Int>, [[BAR:%.*]] : @guaranteed $Bar):
31+
32+
// CHECK: [[WILLSET:%.*]] = function_ref @$s26property_wrapper_observers3BarC9someArraySaySiGvw : $@convention(method) (@guaranteed Array<Int>, @guaranteed Bar) -> ()
33+
// CHECK-NEXT: [[RESULT_WS:%.*]] = apply [[WILLSET]](%{{[0-9]+}}, [[BAR]]) : $@convention(method) (@guaranteed Array<Int>, @guaranteed Bar) -> ()
34+
35+
// CHECK: [[WRAPPED_VALUE_SETTER:%.*]] = function_ref @$s26property_wrapper_observers3FooV12wrappedValueSaySiGvs : $@convention(method) (@owned Array<Int>, @inout Foo) -> ()
36+
// CHECK-NEXT: [[RESULT_WVS:%.*]] = apply [[WRAPPED_VALUE_SETTER]](%{{[0-9]+}}, %{{[0-9]+}}) : $@convention(method) (@owned Array<Int>, @inout Foo) -> ()
37+
38+
// CHECK: [[DIDSET:%.*]] = function_ref @$s26property_wrapper_observers3BarC9someArraySaySiGvW : $@convention(method) (@guaranteed Array<Int>, @guaranteed Bar) -> ()
39+
// CHECK-NEXT: [[RESULT_DS:%.*]] = apply [[DIDSET]](%{{[0-9]+}}, [[BAR]]) : $@convention(method) (@guaranteed Array<Int>, @guaranteed Bar) -> ()
40+
// CHECK: }
41+
42+
// Bar.someArray.modify
43+
44+
// CHECK-LABEL: sil hidden [ossa] @$s26property_wrapper_observers3BarC9someArraySaySiGvM : $@yield_once @convention(method) (@guaranteed Bar) -> @yields @inout Array<Int> {
45+
// CHECK: bb0([[BAR:%.*]] : @guaranteed $Bar):
46+
// CHECK-NEXT: debug_value [[BAR]] : $Bar, let, name "self", argno 1
47+
// CHECK-NEXT: [[ALLOC_STACK:%.*]] = alloc_stack $Array<Int>
48+
// CHECK-NEXT: // function_ref Bar.someArray.getter
49+
// CHECK-NEXT: [[GETTER:%.*]] = function_ref @$s26property_wrapper_observers3BarC9someArraySaySiGvg : $@convention(method) (@guaranteed Bar) -> @owned Array<Int>
50+
// CHECK-NEXT: [[RESULT:%.*]] = apply [[GETTER]]([[BAR]]) : $@convention(method) (@guaranteed Bar) -> @owned Array<Int>
51+
// CHECK-NEXT: store [[RESULT]] to [init] [[ALLOC_STACK]] : $*Array<Int>
52+
// CHECK-NEXT: yield [[ALLOC_STACK]] : $*Array<Int>, resume bb1, unwind bb2
53+
54+
// CHECK: bb1:
55+
// CHECK-NEXT: [[VALUE:%.*]] = load [take] [[ALLOC_STACK]] : $*Array<Int>
56+
// CHECK-NEXT: // function_ref Bar.someArray.setter
57+
// CHECK-NEXT: [[SETTER:%.*]] = function_ref @$s26property_wrapper_observers3BarC9someArraySaySiGvs : $@convention(method) (@owned Array<Int>, @guaranteed Bar) -> ()
58+
// CHECK-NEXT: [[RESULT:%.*]] = apply [[SETTER]]([[VALUE]], [[BAR]]) : $@convention(method) (@owned Array<Int>, @guaranteed Bar) -> ()
59+
// CHECK-NEXT: dealloc_stack [[ALLOC_STACK]] : $*Array<Int>
60+
// CHECK-NEXT: [[TUPLE:%.*]] = tuple ()
61+
// CHECK-NEXT: return [[TUPLE]] : $()
62+
63+
// CHECK: bb2:
64+
// CHECK-NEXT: [[NEWVALUE:%.*]] = load [copy] [[ALLOC_STACK]] : $*Array<Int>
65+
// CHECK-NEXT: // function_ref Bar.someArray.setter
66+
// CHECK-NEXT: [[SETTER:%.*]] = function_ref @$s26property_wrapper_observers3BarC9someArraySaySiGvs : $@convention(method) (@owned Array<Int>, @guaranteed Bar) -> ()
67+
// CHECK-NEXT: [[RESULT:%.*]] = apply [[SETTER]]([[NEWVALUE]], [[BAR]]) : $@convention(method) (@owned Array<Int>, @guaranteed Bar) -> ()
68+
// CHECK-NEXT: destroy_addr [[ALLOC_STACK]] : $*Array<Int>
69+
// CHECK-NEXT: dealloc_stack [[ALLOC_STACK]] : $*Array<Int>
70+
// CHECK-NEXT: unwind
71+
// CHECK-END: }

‎test/Sema/property_wrapper_property_with_observer.swift‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// RUN: %target-run-simple-swift
2+
3+
// REQUIRES: executable_test
4+
5+
@propertyWrapper
6+
struct Foo {
7+
private var _storage: [Int] = []
8+
9+
init(wrappedValue value: [Int]) {
10+
self._storage = value
11+
}
12+
13+
var wrappedValue: [Int] {
14+
get { _storage }
15+
set { _storage = newValue }
16+
}
17+
}
18+
19+
class Bar {
20+
@Foo var someArray = [1, 2, 3] {
21+
willSet {
22+
print(newValue)
23+
}
24+
25+
didSet {
26+
print(oldValue)
27+
}
28+
}
29+
}
30+
31+
let bar = Bar()
32+
// CHECK: [4, 2, 3]
33+
// CHECK: [1, 2, 3]
34+
bar.someArray[0] = 4

‎tools/SourceKit/lib/SwiftLang/SwiftEditor.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,8 +1190,7 @@ static bool inferIsSettableSyntactically(const AbstractStorageDecl *D) {
11901190
}
11911191
if (D->hasParsedAccessors()) {
11921192
return D->getParsedAccessor(AccessorKind::Set) != nullptr ||
1193-
D->getParsedAccessor(AccessorKind::WillSet) != nullptr ||
1194-
D->getParsedAccessor(AccessorKind::DidSet) != nullptr;
1193+
D->hasObservers();
11951194
} else {
11961195
return true;
11971196
}

0 commit comments

Comments
 (0)