Skip to content

Commit 9d72a2b

Browse files
jrose-applejrose-apple
authored
Merge pull request #15280 from hamishknight/didset-recursion
[Sema] Only directly access members within didSet if accessed on 'self'
2 parents 9091c25 + 89c9e03 commit 9d72a2b

File tree

7 files changed

+397
-64
lines changed

7 files changed

+397
-64
lines changed

‎include/swift/AST/Decl.h

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2350,7 +2350,13 @@ class ValueDecl : public Decl {
23502350
/// Determines the kind of access that should be performed by a
23512351
/// DeclRefExpr or MemberRefExpr use of this value in the specified
23522352
/// context.
2353-
AccessSemantics getAccessSemanticsFromContext(const DeclContext *DC) const;
2353+
///
2354+
/// \param DC The declaration context.
2355+
///
2356+
/// \param isAccessOnSelf Whether this is a member access on the implicit
2357+
/// 'self' declaration of the declaration context.
2358+
AccessSemantics getAccessSemanticsFromContext(const DeclContext *DC,
2359+
bool isAccessOnSelf) const;
23542360

23552361
/// Print a reference to the given declaration.
23562362
std::string printRef() const;

‎lib/AST/Decl.cpp

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1296,20 +1296,34 @@ static bool isPolymorphic(const AbstractStorageDecl *storage) {
12961296
/// Determines the access semantics to use in a DeclRefExpr or
12971297
/// MemberRefExpr use of this value in the specified context.
12981298
AccessSemantics
1299-
ValueDecl::getAccessSemanticsFromContext(const DeclContext *UseDC) const {
1299+
ValueDecl::getAccessSemanticsFromContext(const DeclContext *UseDC,
1300+
bool isAccessOnSelf) const {
13001301
// If we're inside a @_transparent function, use the most conservative
13011302
// access pattern, since we may be inlined from a different resilience
13021303
// domain.
13031304
ResilienceExpansion expansion = UseDC->getResilienceExpansion();
13041305

13051306
if (auto *var = dyn_cast<AbstractStorageDecl>(this)) {
1306-
// Observing member are accessed directly from within their didSet/willSet
1307-
// specifiers. This prevents assignments from becoming infinite loops.
1308-
if (auto *UseFD = dyn_cast<AccessorDecl>(UseDC))
1309-
if (var->hasStorage() && var->hasAccessorFunctions() &&
1310-
UseFD->getStorage() == var)
1311-
return AccessSemantics::DirectToStorage;
1312-
1307+
auto isMember = var->getDeclContext()->isTypeContext();
1308+
if (isAccessOnSelf)
1309+
assert(isMember && "Access on self, but var isn't a member");
1310+
1311+
// Within a variable's own didSet/willSet specifier, access its storage
1312+
// directly if either:
1313+
// 1) It's a 'plain variable' (i.e a variable that's not a member).
1314+
// 2) It's an access to the member on the implicit 'self' declaration.
1315+
// If it's a member access on some other base, we want to call the setter
1316+
// as we might be accessing the member on a *different* instance.
1317+
// 3) We're not in Swift 5 mode (or higher), as in earlier versions of Swift
1318+
// we always performed direct accesses.
1319+
// This prevents assignments from becoming infinite loops in most cases.
1320+
if (!isMember || isAccessOnSelf ||
1321+
!UseDC->getASTContext().isSwiftVersionAtLeast(5))
1322+
if (auto *UseFD = dyn_cast<AccessorDecl>(UseDC))
1323+
if (var->hasStorage() && var->hasAccessorFunctions() &&
1324+
UseFD->getStorage() == var)
1325+
return AccessSemantics::DirectToStorage;
1326+
13131327
// "StoredWithTrivialAccessors" are generally always accessed indirectly,
13141328
// but if we know that the trivial accessor will always produce the same
13151329
// thing as the getter/setter (i.e., it can't be overridden), then just do a

‎lib/Sema/CSApply.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,8 +247,14 @@ getImplicitMemberReferenceAccessSemantics(Expr *base, VarDecl *member,
247247
}
248248
}
249249

250+
// Check whether this is a member access on 'self'.
251+
bool isAccessOnSelf = false;
252+
if (auto *baseDRE = dyn_cast<DeclRefExpr>(base->getValueProvidingExpr()))
253+
if (auto *baseVar = dyn_cast<VarDecl>(baseDRE->getDecl()))
254+
isAccessOnSelf = baseVar->isSelfParameter();
255+
250256
// If the value is always directly accessed from this context, do it.
251-
return member->getAccessSemanticsFromContext(DC);
257+
return member->getAccessSemanticsFromContext(DC, isAccessOnSelf);
252258
}
253259

254260
void ConstraintSystem::propagateLValueAccessKind(Expr *E, AccessKind accessKind,

‎lib/Sema/TypeCheckExpr.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,8 @@ bool TypeChecker::requireArrayLiteralIntrinsics(SourceLoc loc) {
516516
Expr *TypeChecker::buildCheckedRefExpr(VarDecl *value, DeclContext *UseDC,
517517
DeclNameLoc loc, bool Implicit) {
518518
auto type = getUnopenedTypeOfReference(value, Type(), UseDC);
519-
AccessSemantics semantics = value->getAccessSemanticsFromContext(UseDC);
519+
auto semantics = value->getAccessSemanticsFromContext(UseDC,
520+
/*isAccessOnSelf*/false);
520521
return new (Context) DeclRefExpr(value, loc, Implicit, semantics, type);
521522
}
522523

@@ -526,7 +527,8 @@ Expr *TypeChecker::buildRefExpr(ArrayRef<ValueDecl *> Decls,
526527
assert(!Decls.empty() && "Must have at least one declaration");
527528

528529
if (Decls.size() == 1 && !isa<ProtocolDecl>(Decls[0]->getDeclContext())) {
529-
AccessSemantics semantics = Decls[0]->getAccessSemanticsFromContext(UseDC);
530+
auto semantics = Decls[0]->getAccessSemanticsFromContext(UseDC,
531+
/*isAccessOnSelf*/false);
530532
return new (Context) DeclRefExpr(Decls[0], NameLoc, Implicit, semantics);
531533
}
532534

‎test/SILGen/properties.swift

Lines changed: 100 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -495,10 +495,26 @@ protocol ForceAccessors {
495495
}
496496

497497
struct DidSetWillSetTests: ForceAccessors {
498+
// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV{{[_0-9a-zA-Z]*}}fC
499+
init(x : Int) {
500+
// Accesses to didset/willset variables are direct in init methods and dtors.
501+
a = x
502+
a = x
503+
504+
// CHECK: bb0(%0 : $Int, %1 : $@thin DidSetWillSetTests.Type):
505+
// CHECK: [[SELF:%.*]] = mark_uninitialized [rootself]
506+
// CHECK: [[PB_SELF:%.*]] = project_box [[SELF]]
507+
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
508+
// CHECK: [[P1:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
509+
// CHECK-NEXT: assign %0 to [[P1]]
510+
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
511+
// CHECK: [[P2:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
512+
// CHECK-NEXT: assign %0 to [[P2]]
513+
}
514+
498515
var a: Int {
516+
// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vw
499517
willSet(newA) {
500-
// CHECK-LABEL: // {{.*}}.DidSetWillSetTests.a.willset
501-
// CHECK-NEXT: sil hidden @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vw
502518
// CHECK: bb0(%0 : $Int, %1 : $*DidSetWillSetTests):
503519
// CHECK-NEXT: debug_value %0
504520
// CHECK-NEXT: debug_value_addr %1 : $*DidSetWillSetTests
@@ -517,11 +533,23 @@ struct DidSetWillSetTests: ForceAccessors {
517533
// CHECK-NEXT: // function_ref properties.takeInt(Swift.Int) -> ()
518534
// CHECK-NEXT: [[TAKEINTFN:%.*]] = function_ref @$S10properties7takeInt{{[_0-9a-zA-Z]*}}F
519535
// CHECK-NEXT: apply [[TAKEINTFN]](%0) : $@convention(thin) (Int) -> ()
536+
537+
a = zero // reassign, but don't infinite loop.
538+
539+
// CHECK-NEXT: // function_ref properties.zero.unsafeMutableAddressor : Swift.Int
540+
// CHECK-NEXT: [[ZEROFN:%.*]] = function_ref @$S10properties4zero{{[_0-9a-zA-Z]*}}vau
541+
// CHECK-NEXT: [[ZERORAW:%.*]] = apply [[ZEROFN]]() : $@convention(thin) () -> Builtin.RawPointer
542+
// CHECK-NEXT: [[ZEROADDR:%.*]] = pointer_to_address [[ZERORAW]] : $Builtin.RawPointer to [strict] $*Int
543+
// CHECK-NEXT: [[READ:%.*]] = begin_access [read] [dynamic] [[ZEROADDR]] : $*Int
544+
// CHECK-NEXT: [[ZERO:%.*]] = load [trivial] [[READ]]
545+
// CHECK-NEXT: end_access [[READ]] : $*Int
546+
// CHECK-NEXT: [[WRITE:%.*]] = begin_access [modify] [unknown] %1
547+
// CHECK-NEXT: [[AADDR:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
548+
// CHECK-NEXT: assign [[ZERO]] to [[AADDR]]
520549
}
521550

551+
// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vW
522552
didSet {
523-
// CHECK-LABEL: // {{.*}}.DidSetWillSetTests.a.didset
524-
// CHECK-NEXT: sil hidden @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vW
525553
// CHECK: bb0(%0 : $Int, %1 : $*DidSetWillSetTests):
526554
// CHECK-NEXT: debug
527555
// CHECK-NEXT: debug_value_addr %1 : $*DidSetWillSetTests
@@ -536,7 +564,7 @@ struct DidSetWillSetTests: ForceAccessors {
536564
// CHECK-NEXT: [[TAKEINTFN:%.*]] = function_ref @$S10properties7takeInt{{[_0-9a-zA-Z]*}}F
537565
// CHECK-NEXT: apply [[TAKEINTFN]]([[A]]) : $@convention(thin) (Int) -> ()
538566

539-
a = zero // reassign, but don't infinite loop.
567+
(self).a = zero // reassign, but don't infinite loop.
540568

541569
// CHECK-NEXT: // function_ref properties.zero.unsafeMutableAddressor : Swift.Int
542570
// CHECK-NEXT: [[ZEROFN:%.*]] = function_ref @$S10properties4zero{{[_0-9a-zA-Z]*}}vau
@@ -551,23 +579,16 @@ struct DidSetWillSetTests: ForceAccessors {
551579
}
552580
}
553581

554-
init(x : Int) {
555-
// Accesses to didset/willset variables are direct in init methods and dtors.
556-
a = x
557-
a = x
558-
}
582+
// This is the synthesized getter and setter for the willset/didset variable.
559583

560-
// These are the synthesized getter and setter for the willset/didset variable.
561-
562-
// CHECK-LABEL: // {{.*}}.DidSetWillSetTests.a.getter
563-
// CHECK-NEXT: sil hidden [transparent] @$S10properties010DidSetWillC5TestsV1aSivg
584+
// CHECK-LABEL: sil hidden [transparent] @$S10properties010DidSetWillC5TestsV1aSivg
564585
// CHECK: bb0(%0 : $DidSetWillSetTests):
565586
// CHECK-NEXT: debug_value %0
566587
// CHECK-NEXT: %2 = struct_extract %0 : $DidSetWillSetTests, #DidSetWillSetTests.a
567588
// CHECK-NEXT: return %2 : $Int{{.*}} // id: %3
568589

569-
// CHECK-LABEL: // {{.*}}.DidSetWillSetTests.a.setter
570-
// CHECK-NEXT: sil hidden @$S10properties010DidSetWillC5TestsV1aSivs
590+
591+
// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV1aSivs
571592
// CHECK: bb0(%0 : $Int, %1 : $*DidSetWillSetTests):
572593
// CHECK-NEXT: debug_value %0
573594
// CHECK-NEXT: debug_value_addr %1
@@ -591,25 +612,47 @@ struct DidSetWillSetTests: ForceAccessors {
591612
// CHECK-NEXT: // function_ref {{.*}}.DidSetWillSetTests.a.didset : Swift.Int
592613
// CHECK-NEXT: [[DIDSETFN:%.*]] = function_ref @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vW : $@convention(method) (Int, @inout DidSetWillSetTests) -> ()
593614
// CHECK-NEXT: apply [[DIDSETFN]]([[OLDVAL]], [[WRITE]]) : $@convention(method) (Int, @inout DidSetWillSetTests) -> ()
594-
595-
// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV{{[_0-9a-zA-Z]*}}fC
596-
// CHECK: bb0(%0 : $Int, %1 : $@thin DidSetWillSetTests.Type):
597-
// CHECK: [[SELF:%.*]] = mark_uninitialized [rootself]
598-
// CHECK: [[PB_SELF:%.*]] = project_box [[SELF]]
599-
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
600-
// CHECK: [[P1:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
601-
// CHECK-NEXT: assign %0 to [[P1]]
602-
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
603-
// CHECK: [[P2:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
604-
// CHECK-NEXT: assign %0 to [[P2]]
605615
}
606616

607617

608618
// Test global observing properties.
609619

610620
var global_observing_property : Int = zero {
621+
// The variable is initialized with "zero".
622+
// CHECK-LABEL: sil private @globalinit_{{.*}}_func1 : $@convention(c) () -> () {
623+
// CHECK: bb0:
624+
// CHECK-NEXT: alloc_global @$S10properties25global_observing_propertySiv
625+
// CHECK-NEXT: %1 = global_addr @$S10properties25global_observing_propertySivp : $*Int
626+
// CHECK: properties.zero.unsafeMutableAddressor
627+
// CHECK: return
628+
629+
// CHECK-LABEL: sil hidden @$S10properties25global_observing_property{{[_0-9a-zA-Z]*}}vW
611630
didSet {
631+
// The didSet implementation needs to call takeInt.
612632
takeInt(global_observing_property)
633+
634+
// CHECK: function_ref properties.takeInt
635+
// CHECK-NEXT: function_ref @$S10properties7takeInt{{[_0-9a-zA-Z]*}}F
636+
637+
// Setting the variable from within its own didSet doesn't recursively call didSet.
638+
global_observing_property = zero
639+
640+
// CHECK: // function_ref properties.global_observing_property.unsafeMutableAddressor : Swift.Int
641+
// CHECK-NEXT: [[ADDRESSOR:%.*]] = function_ref @$S10properties25global_observing_propertySivau : $@convention(thin) () -> Builtin.RawPointer
642+
// CHECK-NEXT: [[ADDRESS:%.*]] = apply [[ADDRESSOR]]() : $@convention(thin) () -> Builtin.RawPointer
643+
// CHECK-NEXT: [[POINTER:%.*]] = pointer_to_address [[ADDRESS]] : $Builtin.RawPointer to [strict] $*Int
644+
// CHECK-NEXT: // function_ref properties.zero.unsafeMutableAddressor : Swift.Int
645+
// CHECK-NEXT: [[ZEROFN:%.*]] = function_ref @$S10properties4zero{{[_0-9a-zA-Z]*}}vau
646+
// CHECK-NEXT: [[ZERORAW:%.*]] = apply [[ZEROFN]]() : $@convention(thin) () -> Builtin.RawPointer
647+
// CHECK-NEXT: [[ZEROADDR:%.*]] = pointer_to_address [[ZERORAW]] : $Builtin.RawPointer to [strict] $*Int
648+
// CHECK-NEXT: [[READ:%.*]] = begin_access [read] [dynamic] [[ZEROADDR]] : $*Int
649+
// CHECK-NEXT: [[ZERO:%.*]] = load [trivial] [[READ]]
650+
// CHECK-NEXT: end_access [[READ]] : $*Int
651+
// CHECK-NEXT: [[WRITE:%.*]] = begin_access [modify] [dynamic] [[POINTER]] : $*Int
652+
// CHECK-NEXT: assign [[ZERO]] to [[WRITE]] : $*Int
653+
// CHECK-NEXT: end_access [[WRITE]] : $*Int
654+
// CHECK-NOT: function_ref @$S10properties25global_observing_property{{[_0-9a-zA-Z]*}}vW
655+
// CHECK: end sil function
613656
}
614657
}
615658

@@ -618,21 +661,7 @@ func force_global_observing_property_setter() {
618661
global_observing_property = x
619662
}
620663

621-
// The property is initialized with "zero".
622-
// CHECK-LABEL: sil private @globalinit_{{.*}}_func1 : $@convention(c) () -> () {
623-
// CHECK: bb0:
624-
// CHECK-NEXT: alloc_global @$S10properties25global_observing_propertySiv
625-
// CHECK-NEXT: %1 = global_addr @$S10properties25global_observing_propertySivp : $*Int
626-
// CHECK: properties.zero.unsafeMutableAddressor
627-
// CHECK: return
628-
629-
// The didSet implementation needs to call takeInt.
630-
631-
// CHECK-LABEL: sil hidden @$S10properties25global_observing_property{{[_0-9a-zA-Z]*}}vW
632-
// CHECK: function_ref properties.takeInt
633-
// CHECK-NEXT: function_ref @$S10properties7takeInt{{[_0-9a-zA-Z]*}}F
634-
635-
// The setter needs to call didSet implementation.
664+
// global_observing_property's setter needs to call didSet.
636665

637666
// CHECK-LABEL: sil hidden @$S10properties25global_observing_property{{[_0-9a-zA-Z]*}}vs
638667
// CHECK: function_ref properties.global_observing_property.unsafeMutableAddressor
@@ -643,25 +672,44 @@ func force_global_observing_property_setter() {
643672

644673
// Test local observing properties.
645674

675+
// CHECK-LABEL: sil hidden @$S10properties24local_observing_property{{[_0-9a-zA-Z]*}}SiF
646676
func local_observing_property(_ arg: Int) {
647677
var localproperty: Int = arg {
648678
didSet {
649679
takeInt(localproperty)
680+
localproperty = zero
650681
}
651682
}
652-
683+
653684
takeInt(localproperty)
654685
localproperty = arg
655-
}
656-
657-
// This is the local_observing_property function itself. First alloc and
658-
// initialize the property to the argument value.
659686

660-
// CHECK-LABEL: sil hidden @{{.*}}local_observing_property
661-
// CHECK: bb0([[ARG:%[0-9]+]] : $Int)
662-
// CHECK: [[BOX:%[0-9]+]] = alloc_box ${ var Int }
663-
// CHECK: [[PB:%.*]] = project_box [[BOX]]
664-
// CHECK: store [[ARG]] to [trivial] [[PB]]
687+
// Alloc and initialize the property to the argument value.
688+
// CHECK: bb0([[ARG:%[0-9]+]] : $Int)
689+
// CHECK: [[BOX:%[0-9]+]] = alloc_box ${ var Int }
690+
// CHECK: [[PB:%.*]] = project_box [[BOX]]
691+
// CHECK: store [[ARG]] to [trivial] [[PB]]
692+
}
693+
694+
// didSet of localproperty (above)
695+
// Ensure that setting the variable from within its own didSet doesn't recursively call didSet.
696+
697+
// CHECK-LABEL: sil private @$S10properties24local_observing_property{{[_0-9a-zA-Z]*}}SiF13localproperty{{[_0-9a-zA-Z]*}}SivW
698+
// CHECK: bb0(%0 : $Int, %1 : ${ var Int })
699+
// CHECK: [[POINTER:%.*]] = project_box %1 : ${ var Int }, 0
700+
// CHECK: // function_ref properties.zero.unsafeMutableAddressor : Swift.Int
701+
// CHECK-NEXT: [[ZEROFN:%.*]] = function_ref @$S10properties4zero{{[_0-9a-zA-Z]*}}vau
702+
// CHECK-NEXT: [[ZERORAW:%.*]] = apply [[ZEROFN]]() : $@convention(thin) () -> Builtin.RawPointer
703+
// CHECK-NEXT: [[ZEROADDR:%.*]] = pointer_to_address [[ZERORAW]] : $Builtin.RawPointer to [strict] $*Int
704+
// CHECK-NEXT: [[READ:%.*]] = begin_access [read] [dynamic] [[ZEROADDR]] : $*Int
705+
// CHECK-NEXT: [[ZERO:%.*]] = load [trivial] [[READ]]
706+
// CHECK-NEXT: end_access [[READ]] : $*Int
707+
708+
// CHECK-NEXT: [[WRITE:%.*]] = begin_access [modify] [unknown] [[POINTER]] : $*Int
709+
// CHECK-NEXT: assign [[ZERO]] to [[WRITE]] : $*Int
710+
// CHECK-NEXT: end_access [[WRITE]] : $*Int
711+
// CHECK-NOT: function_ref @$S10properties24local_observing_property{{[_0-9a-zA-Z]*}}SiF13localproperty{{[_0-9a-zA-Z]*}}SivW
712+
// CHECK: end sil function
665713

666714
func local_generic_observing_property<T>(_ arg: Int, _: T) {
667715
var localproperty1: Int = arg {

0 commit comments

Comments
 (0)