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

hamishknight · 2018-03-15T20:35:29Z

Currently we always directly access a member when mutating it in its own didSet/willSet observer (in order to prevent infinite recursion). However we do this even when accessing the same member on a different instance.

This PR changes the behaviour such that only member accesses on the implicit 'self' declaration are accessed directly within that member's own willSet/didSet.

Note that this change has the potential to cause recursion in cases where it didn't previously, for example this will now infinitely recurse:

struct S {
  var i = 0 {
    didSet {
      var s = self
      s.i = 5
    }
  }
}

var s = S()
s.i = 2

I don't know how serious this impact is though.

Resolves SR-419.

slavapestov · 2018-03-15T20:45:18Z

lib/AST/Decl.cpp

+        // *different* instance.
+        base = base->getValueProvidingExpr();
+        if (auto baseDRE = dyn_cast<DeclRefExpr>(base))
+          if (baseDRE->getDecl() == UseFD->getImplicitSelfDecl())


This probably won't work if the base expression references a 'self' decl in a capture list and not the actual 'self'.

@slavapestov Ah, good point! Looking into it, we actually already don't perform direct accesses to members accessed in closures within their own observers (as we only look at AccessorDecl decl contexts), e.g:

struct S { var i = 0 { didSet { { self.i = 1 }() } } } var s = S() s.i = 9 // already infinite recursion

This PR won't change that behaviour. Would it be worth looking into allowing direct access in this case in this PR, or should this be done in a separate PR?

I think the current behavior is correct, just like in initializers we go through the property instead of doing direct access when within a closure.

slavapestov · 2018-03-15T20:45:36Z

include/swift/AST/Decl.h

+  /// \param base If querying for a MemberRefExpr, this is its base expression.
+  ///             Otherwise, \c nullptr.
+  AccessSemantics getAccessSemanticsFromContext(const DeclContext *DC,
+                                                Expr *base) const;


Could this just be a boolean parameter instead of an Expr? Have the caller decide if its the magic 'self' decl or not.

@slavapestov We could do, though I can't immediately think of a good name for such a parameter; calling it something like isAccessOnSelf makes sense for members, but not for variables in general. Any ideas?

isSelfPropertyAccess maybe?

@slavapestov But then we'd have to pass true for variables which seems a bit confusing, as we want direct accesses on both variables and members on self.

@slavapestov We could always just make it an enum, but I don't know if that's a bit excessive for this one situation

You could make it a default argument with a default value of 'true'

@slavapestov Good idea, though to avoid confusion for future readers/when debugging, I think the parameter should be named something like isVariableOrSelfPropertyAccess. If that's good with you, I'll go ahead and make that change.

I honestly like it better the way Hamish has it originally; we don't have to move the checks up to the caller and we don't end up with a parameter that jams two conditions into one. If not that, however, I think I'd prefer isAccessOnSelf, but you only check it if the VarDecl is a property.

@jrose-apple Ah, good idea to check whether the var is a property within getAccessSemanticsFromContext, that makes it much nicer.

Honestly I'm not too attached to passing the base expression vs. a bool flag; as far as I can tell, member accesses should call through getImplicitMemberReferenceAccessSemantics anyway, so I don't think it's a problem having the 'self' base checking logic there.

Therefore I'll go ahead and make this change, @slavapestov could you please take a look when you get a chance?

Thank you both for your feedback!

slavapestov · 2018-03-15T21:12:17Z

@jrose-apple You should take a look at this.

jrose-apple

I didn't look too closely at the test cases but the code change looks good to me (however the quibbling over tracking the base comes out). Thanks, Hamish!

jrose-apple · 2018-03-16T17:11:22Z

Is this something we should limit to Swift 5 mode?

Currently we always directly access a member when mutating it in its own didSet/willSet observer (in order to prevent infinite recursion). However we also do this even when accessing the same member on a *different* instance. This commit changes the behaviour such that only member accesses on the implicit 'self' declaration are accessed directly within that member's own willSet/didSet. Note that this change has the potential to cause recursion in cases where it didn't previously, for example this will now infinitely recurse: struct S { var i = 0 { didSet { var s = self s.i = 5 } } } var s = S() s.i = 2 I don't know how serious this impact is though. Resolves SR-419.

Instead of passing the base expression of a member access to `getAccessSemanticsFromContext`, we now just pass a bool flag for whether this is a member access on the implicit 'self' declaration.

hamishknight · 2018-03-25T17:50:03Z

Is this something we should limit to Swift 5 mode?

@jrose-apple I didn't have any strong opinions on that at the time; but now that I think about, given how subtly source breaking it could be, I think it's probably best to limit to Swift 5 mode.

It would be great if we could diagnose infinite recursion of property observers (via their setters) in order to alleviate the subtlety of this change – @CodaFi do you have any plans for making #11869 work with mutually recursive functions? I'm thinking it could probably be temporarily hacked in the mean time by using the AST (if we have access to it) in order to treat a call to a property's setter within one of its own observers as a recursive call.

CodaFi · 2018-03-26T01:59:46Z

I have some plans to extend it to diagnose patterns like the above. When this is merged, could you file an SR with the code sample and tag me in it?

jrose-apple

Thanks, Hamish! Mostly looks good to me, only minor remaining comments. @slavapestov, anything else you want to weigh in on for this new version?

jrose-apple · 2018-03-26T17:07:38Z

lib/Sema/CSApply.cpp

+  if (auto *baseDRE = dyn_cast<DeclRefExpr>(base->getValueProvidingExpr()))
+    if (auto *AFD = dyn_cast<AbstractFunctionDecl>(DC))
+      if (auto *selfDecl = AFD->getImplicitSelfDecl())
+        if (baseDRE->getDecl() == selfDecl)


Please test against VarDecl::isSelfParameter instead. That handles the use of self in non-function contexts like initial value expressions.

jrose-apple · 2018-03-26T17:08:22Z

test/SILGen/properties.swift

@@ -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.


Why change the test?

@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)?

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.

jrose-apple · 2018-03-26T17:09:13Z

test/SILGen/properties_swift4.swift

@@ -0,0 +1,126 @@
+
+// RUN: %target-swift-frontend -swift-version 4 -module-name properties -Xllvm -sil-full-demangle -parse-as-library -emit-silgen -disable-objc-attr-requires-foundation-module %s | %FileCheck %s


You don't use @objc in this test, so you don't need -disable-objc-attr-requires-foundation-module.

Use VarDecl::isSelfParameter to check whether the base is 'self'.

jrose-apple · 2018-03-26T18:05:24Z

@swift-ci Please test

jrose-apple · 2018-03-26T18:05:45Z

Oh, one more request, I guess: can you add this to CHANGELOG.md? It seems like an important enough change to mention there.

swift-ci · 2018-03-26T18:05:52Z

Build failed
Swift Test OS X Platform
Git Sha - 4b0cbc0

swift-ci · 2018-03-26T18:07:05Z

Build failed
Swift Test Linux Platform
Git Sha - 4b0cbc0

hamishknight · 2018-03-26T18:07:43Z

@jrose-apple Sure thing, should I do that as a follow-up PR?

jrose-apple · 2018-03-26T18:11:48Z

Either way. I did start the tests but they can be interrupted and restarted without difficulty.

hamishknight · 2018-03-26T21:06:00Z

@jrose-apple I've edited the changelog locally, though given the tests have now finished, I think I'll do it as a follow-up PR if that's good with you.

slavapestov · 2018-03-26T22:07:48Z

LGTM

hamishknight · 2018-03-26T22:50:48Z

Thank you both for your reviews! I have opened a follow-up PR for the changelog here: #15526

In swiftlang#15280 we changed the behaviour of setting a property within its own `didSet` or `willSet` observer, such that we only perform direct accesses on `self` in Swift 5 mode. This commit adds this to the changelog and, in addition, does some gardening for the SR links.

slavapestov reviewed Mar 15, 2018

View reviewed changes

jrose-apple reviewed Mar 16, 2018

View reviewed changes

hamishknight force-pushed the didset-recursion branch from 2a2a986 to 8c2aa92 Compare March 17, 2018 13:34

hamishknight added 3 commits March 24, 2018 21:13

[Sema] Revise swiftlang#15280 in response to feedback

5f43627

Instead of passing the base expression of a member access to `getAccessSemanticsFromContext`, we now just pass a bool flag for whether this is a member access on the implicit 'self' declaration.

[Sema] Limit swiftlang#15280 to Swift 5 mode

4b0cbc0

hamishknight force-pushed the didset-recursion branch from 8c2aa92 to 4b0cbc0 Compare March 25, 2018 16:58

jrose-apple reviewed Mar 26, 2018

View reviewed changes

[Sema] Revise swiftlang#15280 in response to feedback (2)

89c9e03

Use VarDecl::isSelfParameter to check whether the base is 'self'.

jrose-apple merged commit 9d72a2b into swiftlang:master Mar 26, 2018

hamishknight deleted the didset-recursion branch March 26, 2018 22:36

hamishknight mentioned this pull request Mar 26, 2018

Add didSet/willSet recursion change to the changelog #15526

Merged

hamishknight mentioned this pull request Mar 15, 2018

[SR-419] Property observers not triggered on other instances of same class #43036

Closed

		@@ -0,0 +1,126 @@

		// RUN: %target-swift-frontend -swift-version 4 -module-name properties -Xllvm -sil-full-demangle -parse-as-library -emit-silgen -disable-objc-attr-requires-foundation-module %s \| %FileCheck %s

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

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

Uh oh!

Conversation

hamishknight commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Mar 15, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Mar 16, 2018

Uh oh!

hamishknight commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Mar 26, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamishknight Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Mar 26, 2018

Uh oh!

jrose-apple commented Mar 26, 2018

Uh oh!

swift-ci commented Mar 26, 2018

Uh oh!

swift-ci commented Mar 26, 2018

Uh oh!

hamishknight commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Mar 26, 2018

Uh oh!

hamishknight commented Mar 26, 2018

Uh oh!

slavapestov commented Mar 26, 2018

Uh oh!

hamishknight commented Mar 26, 2018

Uh oh!

Uh oh!

hamishknight commented Mar 25, 2018 •

edited

Loading

hamishknight Mar 26, 2018 •

edited

Loading

hamishknight commented Mar 26, 2018 •

edited

Loading