Skip to content

[Sema] Only directly access members within didSet if accessed on 'self' #15280

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.

Sign up for GitHub

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 4 commits into from
Mar 26, 2018
Merged

[Sema] Only directly access members within didSet if accessed on 'self' #15280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a78e15
[Sema] Only directly access members within didSet if accessed on 'self'
hamishknight Mar 13, 2018
5f43627
[Sema] Revise #15280 in response to feedback
hamishknight Mar 17, 2018
4b0cbc0
[Sema] Limit #15280 to Swift 5 mode
hamishknight Mar 25, 2018
89c9e03
[Sema] Revise #15280 in response to feedback (2)
hamishknight Mar 26, 2018
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
8 changes: 7 additions & 1 deletion include/swift/AST/Decl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2351,7 +2351,13 @@ class ValueDecl : public Decl {
/// Determines the kind of access that should be performed by a
/// DeclRefExpr or MemberRefExpr use of this value in the specified
/// context.
AccessSemantics getAccessSemanticsFromContext(const DeclContext *DC) const;
///
/// \param DC The declaration context.
///
/// \param isAccessOnSelf Whether this is a member access on the implicit
/// 'self' declaration of the declaration context.
AccessSemantics getAccessSemanticsFromContext(const DeclContext *DC,
bool isAccessOnSelf) const;

/// Print a reference to the given declaration.
std::string printRef() const;
Expand Down
30 changes: 22 additions & 8 deletions lib/AST/Decl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,20 +1296,34 @@ static bool isPolymorphic(const AbstractStorageDecl *storage) {
/// Determines the access semantics to use in a DeclRefExpr or
/// MemberRefExpr use of this value in the specified context.
AccessSemantics
ValueDecl::getAccessSemanticsFromContext(const DeclContext *UseDC) const {
ValueDecl::getAccessSemanticsFromContext(const DeclContext *UseDC,
bool isAccessOnSelf) const {
// If we're inside a @_transparent function, use the most conservative
// access pattern, since we may be inlined from a different resilience
// domain.
ResilienceExpansion expansion = UseDC->getResilienceExpansion();

if (auto *var = dyn_cast<AbstractStorageDecl>(this)) {
// Observing member are accessed directly from within their didSet/willSet
// specifiers. This prevents assignments from becoming infinite loops.
if (auto *UseFD = dyn_cast<AccessorDecl>(UseDC))
if (var->hasStorage() && var->hasAccessorFunctions() &&
UseFD->getStorage() == var)
return AccessSemantics::DirectToStorage;

auto isMember = var->getDeclContext()->isTypeContext();
if (isAccessOnSelf)
assert(isMember && "Access on self, but var isn't a member");

// Within a variable's own didSet/willSet specifier, access its storage
// directly if either:
// 1) It's a 'plain variable' (i.e a variable that's not a member).
// 2) It's an access to the member on the implicit 'self' declaration.
// If it's a member access on some other base, we want to call the setter
// as we might be accessing the member on a *different* instance.
// 3) We're not in Swift 5 mode (or higher), as in earlier versions of Swift
// we always performed direct accesses.
// This prevents assignments from becoming infinite loops in most cases.
if (!isMember || isAccessOnSelf ||
!UseDC->getASTContext().isSwiftVersionAtLeast(5))
if (auto *UseFD = dyn_cast<AccessorDecl>(UseDC))
if (var->hasStorage() && var->hasAccessorFunctions() &&
UseFD->getStorage() == var)
return AccessSemantics::DirectToStorage;

// "StoredWithTrivialAccessors" are generally always accessed indirectly,
// but if we know that the trivial accessor will always produce the same
// thing as the getter/setter (i.e., it can't be overridden), then just do a
Expand Down
8 changes: 7 additions & 1 deletion lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,14 @@ getImplicitMemberReferenceAccessSemantics(Expr *base, VarDecl *member,
}
}

// Check whether this is a member access on 'self'.
bool isAccessOnSelf = false;
if (auto *baseDRE = dyn_cast<DeclRefExpr>(base->getValueProvidingExpr()))
if (auto *baseVar = dyn_cast<VarDecl>(baseDRE->getDecl()))
isAccessOnSelf = baseVar->isSelfParameter();

// If the value is always directly accessed from this context, do it.
return member->getAccessSemanticsFromContext(DC);
return member->getAccessSemanticsFromContext(DC, isAccessOnSelf);
}

void ConstraintSystem::propagateLValueAccessKind(Expr *E, AccessKind accessKind,
Expand Down
6 changes: 4 additions & 2 deletions lib/Sema/TypeCheckExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,8 @@ bool TypeChecker::requireArrayLiteralIntrinsics(SourceLoc loc) {
Expr *TypeChecker::buildCheckedRefExpr(VarDecl *value, DeclContext *UseDC,
DeclNameLoc loc, bool Implicit) {
auto type = getUnopenedTypeOfReference(value, Type(), UseDC);
AccessSemantics semantics = value->getAccessSemanticsFromContext(UseDC);
auto semantics = value->getAccessSemanticsFromContext(UseDC,
/*isAccessOnSelf*/false);
return new (Context) DeclRefExpr(value, loc, Implicit, semantics, type);
}

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

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

Expand Down
152 changes: 100 additions & 52 deletions test/SILGen/properties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,10 +495,26 @@ protocol ForceAccessors {
}

struct DidSetWillSetTests: ForceAccessors {
// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV{{[_0-9a-zA-Z]*}}fC
init(x : Int) {
// Accesses to didset/willset variables are direct in init methods and dtors.
a = x
a = x

// CHECK: bb0(%0 : $Int, %1 : $@thin DidSetWillSetTests.Type):
// CHECK: [[SELF:%.*]] = mark_uninitialized [rootself]
// CHECK: [[PB_SELF:%.*]] = project_box [[SELF]]
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
// CHECK: [[P1:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
// CHECK-NEXT: assign %0 to [[P1]]
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
// CHECK: [[P2:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
// CHECK-NEXT: assign %0 to [[P2]]
}

var a: Int {
// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vw
willSet(newA) {
// CHECK-LABEL: // {{.*}}.DidSetWillSetTests.a.willset
// CHECK-NEXT: sil hidden @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vw
// CHECK: bb0(%0 : $Int, %1 : $*DidSetWillSetTests):
// CHECK-NEXT: debug_value %0
// CHECK-NEXT: debug_value_addr %1 : $*DidSetWillSetTests
Expand All @@ -517,11 +533,23 @@ struct DidSetWillSetTests: ForceAccessors {
// CHECK-NEXT: // function_ref properties.takeInt(Swift.Int) -> ()
// CHECK-NEXT: [[TAKEINTFN:%.*]] = function_ref @$S10properties7takeInt{{[_0-9a-zA-Z]*}}F
// CHECK-NEXT: apply [[TAKEINTFN]](%0) : $@convention(thin) (Int) -> ()

a = zero // reassign, but don't infinite loop.

// CHECK-NEXT: // function_ref properties.zero.unsafeMutableAddressor : Swift.Int
// CHECK-NEXT: [[ZEROFN:%.*]] = function_ref @$S10properties4zero{{[_0-9a-zA-Z]*}}vau
// CHECK-NEXT: [[ZERORAW:%.*]] = apply [[ZEROFN]]() : $@convention(thin) () -> Builtin.RawPointer
// CHECK-NEXT: [[ZEROADDR:%.*]] = pointer_to_address [[ZERORAW]] : $Builtin.RawPointer to [strict] $*Int
// CHECK-NEXT: [[READ:%.*]] = begin_access [read] [dynamic] [[ZEROADDR]] : $*Int
// CHECK-NEXT: [[ZERO:%.*]] = load [trivial] [[READ]]
// CHECK-NEXT: end_access [[READ]] : $*Int
// CHECK-NEXT: [[WRITE:%.*]] = begin_access [modify] [unknown] %1
// CHECK-NEXT: [[AADDR:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
// CHECK-NEXT: assign [[ZERO]] to [[AADDR]]
}

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

a = zero // reassign, but don't infinite loop.
(self).a = zero // reassign, but don't infinite loop.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change the test?

Copy link
Contributor Author

@hamishknight hamishknight Mar 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrose-apple To ensure we also look through paren exprs to the self base. We still check for a direct access when doing a = zero in willSet, should I swap them (i.e a = zero in didSet, (self).a = zero in willSet)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I usually just prefer that new things be added rather than changing old things, but I forgot that the original form was tested above. It's fine to leave it, thanks.


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

init(x : Int) {
// Accesses to didset/willset variables are direct in init methods and dtors.
a = x
a = x
}
// This is the synthesized getter and setter for the willset/didset variable.

// These are the synthesized getter and setter for the willset/didset variable.

// CHECK-LABEL: // {{.*}}.DidSetWillSetTests.a.getter
// CHECK-NEXT: sil hidden [transparent] @$S10properties010DidSetWillC5TestsV1aSivg
// CHECK-LABEL: sil hidden [transparent] @$S10properties010DidSetWillC5TestsV1aSivg
// CHECK: bb0(%0 : $DidSetWillSetTests):
// CHECK-NEXT: debug_value %0
// CHECK-NEXT: %2 = struct_extract %0 : $DidSetWillSetTests, #DidSetWillSetTests.a
// CHECK-NEXT: return %2 : $Int{{.*}} // id: %3

// CHECK-LABEL: // {{.*}}.DidSetWillSetTests.a.setter
// CHECK-NEXT: sil hidden @$S10properties010DidSetWillC5TestsV1aSivs

// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV1aSivs
// CHECK: bb0(%0 : $Int, %1 : $*DidSetWillSetTests):
// CHECK-NEXT: debug_value %0
// CHECK-NEXT: debug_value_addr %1
Expand All @@ -591,25 +612,47 @@ struct DidSetWillSetTests: ForceAccessors {
// CHECK-NEXT: // function_ref {{.*}}.DidSetWillSetTests.a.didset : Swift.Int
// CHECK-NEXT: [[DIDSETFN:%.*]] = function_ref @$S10properties010DidSetWillC5TestsV1a{{[_0-9a-zA-Z]*}}vW : $@convention(method) (Int, @inout DidSetWillSetTests) -> ()
// CHECK-NEXT: apply [[DIDSETFN]]([[OLDVAL]], [[WRITE]]) : $@convention(method) (Int, @inout DidSetWillSetTests) -> ()

// CHECK-LABEL: sil hidden @$S10properties010DidSetWillC5TestsV{{[_0-9a-zA-Z]*}}fC
// CHECK: bb0(%0 : $Int, %1 : $@thin DidSetWillSetTests.Type):
// CHECK: [[SELF:%.*]] = mark_uninitialized [rootself]
// CHECK: [[PB_SELF:%.*]] = project_box [[SELF]]
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
// CHECK: [[P1:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
// CHECK-NEXT: assign %0 to [[P1]]
// CHECK: [[WRITE:%.*]] = begin_access [modify] [unknown] [[PB_SELF]]
// CHECK: [[P2:%.*]] = struct_element_addr [[WRITE]] : $*DidSetWillSetTests, #DidSetWillSetTests.a
// CHECK-NEXT: assign %0 to [[P2]]
}


// Test global observing properties.

var global_observing_property : Int = zero {
// The variable is initialized with "zero".
// CHECK-LABEL: sil private @globalinit_{{.*}}_func1 : $@convention(c) () -> () {
// CHECK: bb0:
// CHECK-NEXT: alloc_global @$S10properties25global_observing_propertySiv
// CHECK-NEXT: %1 = global_addr @$S10properties25global_observing_propertySivp : $*Int
// CHECK: properties.zero.unsafeMutableAddressor
// CHECK: return

// CHECK-LABEL: sil hidden @$S10properties25global_observing_property{{[_0-9a-zA-Z]*}}vW
didSet {
// The didSet implementation needs to call takeInt.
takeInt(global_observing_property)

// CHECK: function_ref properties.takeInt
// CHECK-NEXT: function_ref @$S10properties7takeInt{{[_0-9a-zA-Z]*}}F

// Setting the variable from within its own didSet doesn't recursively call didSet.
global_observing_property = zero

// CHECK: // function_ref properties.global_observing_property.unsafeMutableAddressor : Swift.Int
// CHECK-NEXT: [[ADDRESSOR:%.*]] = function_ref @$S10properties25global_observing_propertySivau : $@convention(thin) () -> Builtin.RawPointer
// CHECK-NEXT: [[ADDRESS:%.*]] = apply [[ADDRESSOR]]() : $@convention(thin) () -> Builtin.RawPointer
// CHECK-NEXT: [[POINTER:%.*]] = pointer_to_address [[ADDRESS]] : $Builtin.RawPointer to [strict] $*Int
// CHECK-NEXT: // function_ref properties.zero.unsafeMutableAddressor : Swift.Int
// CHECK-NEXT: [[ZEROFN:%.*]] = function_ref @$S10properties4zero{{[_0-9a-zA-Z]*}}vau
// CHECK-NEXT: [[ZERORAW:%.*]] = apply [[ZEROFN]]() : $@convention(thin) () -> Builtin.RawPointer
// CHECK-NEXT: [[ZEROADDR:%.*]] = pointer_to_address [[ZERORAW]] : $Builtin.RawPointer to [strict] $*Int
// CHECK-NEXT: [[READ:%.*]] = begin_access [read] [dynamic] [[ZEROADDR]] : $*Int
// CHECK-NEXT: [[ZERO:%.*]] = load [trivial] [[READ]]
// CHECK-NEXT: end_access [[READ]] : $*Int
// CHECK-NEXT: [[WRITE:%.*]] = begin_access [modify] [dynamic] [[POINTER]] : $*Int
// CHECK-NEXT: assign [[ZERO]] to [[WRITE]] : $*Int
// CHECK-NEXT: end_access [[WRITE]] : $*Int
// CHECK-NOT: function_ref @$S10properties25global_observing_property{{[_0-9a-zA-Z]*}}vW
// CHECK: end sil function
}
}

Expand All @@ -618,21 +661,7 @@ func force_global_observing_property_setter() {
global_observing_property = x
}

// The property is initialized with "zero".
// CHECK-LABEL: sil private @globalinit_{{.*}}_func1 : $@convention(c) () -> () {
// CHECK: bb0:
// CHECK-NEXT: alloc_global @$S10properties25global_observing_propertySiv
// CHECK-NEXT: %1 = global_addr @$S10properties25global_observing_propertySivp : $*Int
// CHECK: properties.zero.unsafeMutableAddressor
// CHECK: return

// The didSet implementation needs to call takeInt.

// CHECK-LABEL: sil hidden @$S10properties25global_observing_property{{[_0-9a-zA-Z]*}}vW
// CHECK: function_ref properties.takeInt
// CHECK-NEXT: function_ref @$S10properties7takeInt{{[_0-9a-zA-Z]*}}F

// The setter needs to call didSet implementation.
// global_observing_property's setter needs to call didSet.

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

// Test local observing properties.

// CHECK-LABEL: sil hidden @$S10properties24local_observing_property{{[_0-9a-zA-Z]*}}SiF
func local_observing_property(_ arg: Int) {
var localproperty: Int = arg {
didSet {
takeInt(localproperty)
localproperty = zero
}
}

takeInt(localproperty)
localproperty = arg
}

// This is the local_observing_property function itself. First alloc and
// initialize the property to the argument value.

// CHECK-LABEL: sil hidden @{{.*}}local_observing_property
// CHECK: bb0([[ARG:%[0-9]+]] : $Int)
// CHECK: [[BOX:%[0-9]+]] = alloc_box ${ var Int }
// CHECK: [[PB:%.*]] = project_box [[BOX]]
// CHECK: store [[ARG]] to [trivial] [[PB]]
// Alloc and initialize the property to the argument value.
// CHECK: bb0([[ARG:%[0-9]+]] : $Int)
// CHECK: [[BOX:%[0-9]+]] = alloc_box ${ var Int }
// CHECK: [[PB:%.*]] = project_box [[BOX]]
// CHECK: store [[ARG]] to [trivial] [[PB]]
}

// didSet of localproperty (above)
// Ensure that setting the variable from within its own didSet doesn't recursively call didSet.

// CHECK-LABEL: sil private @$S10properties24local_observing_property{{[_0-9a-zA-Z]*}}SiF13localproperty{{[_0-9a-zA-Z]*}}SivW
// CHECK: bb0(%0 : $Int, %1 : ${ var Int })
// CHECK: [[POINTER:%.*]] = project_box %1 : ${ var Int }, 0
// CHECK: // function_ref properties.zero.unsafeMutableAddressor : Swift.Int
// CHECK-NEXT: [[ZEROFN:%.*]] = function_ref @$S10properties4zero{{[_0-9a-zA-Z]*}}vau
// CHECK-NEXT: [[ZERORAW:%.*]] = apply [[ZEROFN]]() : $@convention(thin) () -> Builtin.RawPointer
// CHECK-NEXT: [[ZEROADDR:%.*]] = pointer_to_address [[ZERORAW]] : $Builtin.RawPointer to [strict] $*Int
// CHECK-NEXT: [[READ:%.*]] = begin_access [read] [dynamic] [[ZEROADDR]] : $*Int
// CHECK-NEXT: [[ZERO:%.*]] = load [trivial] [[READ]]
// CHECK-NEXT: end_access [[READ]] : $*Int

// CHECK-NEXT: [[WRITE:%.*]] = begin_access [modify] [unknown] [[POINTER]] : $*Int
// CHECK-NEXT: assign [[ZERO]] to [[WRITE]] : $*Int
// CHECK-NEXT: end_access [[WRITE]] : $*Int
// CHECK-NOT: function_ref @$S10properties24local_observing_property{{[_0-9a-zA-Z]*}}SiF13localproperty{{[_0-9a-zA-Z]*}}SivW
// CHECK: end sil function

func local_generic_observing_property<T>(_ arg: Int, _: T) {
var localproperty1: Int = arg {
Expand Down
Loading