[Property Wrappers] Fix computation of lvalue-ness in buildStorageReference #29069

roop · 2020-01-08T17:38:13Z

This is a follow-up to PR #27727 by @theblixguy to fix l-value-ness computation when building a reference to the storage of properties when synthesising accessors.

This PR fixes the following crashes:

Crash when using a property wrapper with a mutating get and a non-mutating set (SR-11637)
Crash when using a property wrapper with a mutating get, composed with any outer wrapper

Resolves SR-11637.

Implementation

The buildStorageReference function in TypeCheckStorage.cpp builds references to the storage of a property. The references it builds are used in property accessors synthesised automatically by the compiler.

buildStorageReference needs to create expressions of the following forms:

Without property wrappers:
- Stored: self.member
With property wrappers:
- Wrapped: self._member.wrappedValue
- Wrapped with composition: self._member.wrappedValue.wrappedValue….wrappedValue
- Projected: self._member.projectedValue
- Enclosed instance: Wrapper[_enclosedInstance: self, …]

With this PR, the l-valueness of different parts of these expressions are determined thus:

Whether the last component in the expression is an l-value or not is based on the isLValue argument to buildStorageReference
Whether the self component in the expression (except for the use in the _enclosedInstance: parameter above) is an l-value or not should be determined based on whether the accessor being synthesised is a mutating accessor or not. The isSelfReference local variable is used to store this decision.
All the other (middle) components would be instances of property wrappers. Whether such a component is an l-value or not is determined based on whether the access to the property wrapper instance’s wrappedValue (or projectedValue) requires l-valueness or not. The underlyingVars local variable now holds a second field for each entry to store this decision.

In order to determine (2) and (3) above, buildStorageReference now splits its isLValue argument into two arguments: isUsedForGetAccess and isUsedForSetAccess (commit a6e4b1a).

To determine (3) above, a new PropertyWrapperLValueness struct is computed, which holds information on which property wrappers in the chain need to be l-values to be able to get / set the final wrapped value (commit 66d186e).

Testing

This PR just tests that there’s no crash while emitting SIL when using non-default mutatingness with wrappedValue, projectedVaue and composition.

I’m not sure if it’s worthwhile to check for the correct l-valueness for each component of the expression at AST stage by adding tests to test/decl/var/property_wrappers_synthesis.swift. Please let me know if you think that would be useful.

I also haven’t added tests for the subscript(_enclosedInstance:…) form because:

The mutatingness can affect the built reference only when there’s composition
I assume this form isn’t meant to be used with composition by design because there’s no good way to define the wrappedKeyPath’s type when used with composition of wrappers

@DougGregor @theblixguy: Please review.

Prior to this commit, buildStorageReference used to take an 'isLValue' argument that specified whether the reference should be an l-value or not. This provided sufficient information in case of a stored property (self.member). However, when accessing a wrapped property (self._member.wrappedValue), this doesn't have enough information to decide whether intermediate components of the expression should be l-values or not. Prior to this commit, this was extrapolated from the 'isLValue' argument, leading to compiler crashes or unnecessary intermediate l-values when using non-standard mutability with property wrappers. To fix this, this commit replaces the 'isLValue' argument with two arguments: isUsedForGetAccess: Specifies that the referenced storage is meant to be accessed with a getter isUsedForSetAccess: Specifies that the referenced storage is meant to be accessed with a setter and updates buildStorageReference to correctly figure out l-value-ness of self and intermediate components from these arguments.

This fixes l-value computation in buildStorageReference when composing property wrappers. The l-value-ness required for each instance in a property wrapper chain is computed through a request and returned as an instance of PropertyWrapperLValueness.

roop · 2020-01-22T11:03:21Z

@jckarter: This PR uses an extended form of the algorithm you added in commit fa4dd15 for computing mutability when composing property wrappers. So, if you have the time, can you please review this PR as well?

hborla

Thank you for fixing this! Please let me know if you aren't able to continue working on this, or if you need any help.

hborla · 2020-04-13T15:56:34Z

lib/Sema/TypeCheckStorage.cpp

 static Expr *buildStorageReference(AccessorDecl *accessor,
                                   AbstractStorageDecl *storage,
                                   TargetImpl target,
-                                   bool isLValue,
+                                   bool isUsedForGetAccess,
+                                   bool isUsedForSetAccess,


Does it make sense to turn isUsedForGetAccess and isUsedForSetAccess into one enum parameter? It seems like having false for both arguments isn't valid input.

Having false for both is indeed not valid. I did consider using an enum but wasn't sure which was better, so I went with the simpler case.

hborla · 2020-04-13T16:04:35Z

lib/Sema/TypeCheckStorage.cpp

+          bool isWrapperRefLValue = isLValue;
+          if (i < lastWrapperIdx - 1) {
+            bool isLValueForGet = lvalueness->isLValueForGetAccess[i+1];
+            bool isLValueForSet = lvalueness->isLValueForSetAccess[i+1];


Why are these accessing i + 1 instead of i?

Given Swift code like @A @B @C var member: Int, we're building an expression like self._member.wrappedValue.wrappedValue.wrappedValue.

@A @B @C var member: Int self _member wrappedValue wrappedValue wrappedValue (of type A) (of type B) (of type C) (of type Int) | | | | +----------------------------------------+ +------------+ Based on PropertyWrapperLValueness Based on (lvalueness) isUsedForSetAccess argument | | | | +---------+ +--------------------------------------------+ Tracked by Tracked by isMemberLValue underlyingVars[i].second

Consequently:

isMemberLValue <- lvalueness[0]

underlyingVars[0].second <- lvalueness[1]

underlyingVars[1].second <- lvalueness[2]

underlyingVars[2].second <- isUsedForSetAccess

hborla · 2020-04-13T16:29:18Z

lib/Sema/TypeCheckStorage.cpp

+      // We consider the storage's mutability too because the wrapped property
+      // might be part of a class, in case of which nothing is mutating.
+      isGetterMutating = (mutability->Getter == PropertyWrapperMutability::Mutating)
+        ? (storage->isGetterMutating() || storage->isSetterMutating())


This block of code took me a minute to figure out. In the comment above, it might be useful to explain:

The storage getter is used to access the wrappedValue/projectedValue in both cases

If the getter/setter of the wrappedValue/projectedVar is mutating, you need to consider both the getter and setter of the backing storage because you need both in order to perform the writeback

I don't quite understand what you're saying, so let me explain my reasoning.

Here, we're trying to figure out if self should be an lvalue or not. self should be an lvalue if the current method being synthesised is mutating. If we're using property wrappers, we try to derive that by computing PropertyWrapperMutability.

I could've written this as [1]:

isGetterMutating = (mutability->Getter == PropertyWrapperMutability::Mutating); isSetterMutating = (mutability->Setter == PropertyWrapperMutability::Mutating);

However, that only applies if self was a struct. If self was a class, then neither the setter or the getter should be mutating, and in that case, the storage's getter/setter wouldn't be mutating too. So to handle the class scenario, the code looks like this [2]:

isGetterMutating = (mutability->Getter == PropertyWrapperMutability::Mutating) ? (storage->isGetterMutating() || storage->isSetterMutating()) : storage->isGetterMutating(); isSetterMutating = (mutability->Setter == PropertyWrapperMutability::Mutating) ? (storage->isGetterMutating() || storage->isSetterMutating()) : storage->isGetterMutating();

Actually, if PropertyWrapperMutability could handle this class scenario internally (i.e. if it could return the correct mutability for classes as well), that would be great. Then we could just use [1].

hborla · 2020-04-13T16:52:39Z

lib/Sema/TypeCheckStorage.cpp

+    mutability.Setter = getSetterMutatingness(firstWrapperInfo.*varMember,
+                                var->getInnermostDeclContext());
+
+    for (unsigned i : range(1, numWrappers)) {


I think this bit of code would make more sense if you first compute the PropertyWrapperMutability for each composed property wrapper, and then compute the mutability sequence for the last get/set from that.

Do you mean something like: Given Swift code like @A @B @C var member: Int, compute PropertyWrapperMutability for @A, then @A @B, then @A @B @C, and derive the lvalueness based on those results?

This isn't what I had in mind - I'm going to revisit your code here to deepen my understanding and figure out if what I had in mind is possible

hborla · 2020-04-14T23:55:11Z

To help get this landed, I've resolved merge conflicts and build failures due to request evaluator changes here: https://github.com/hborla/swift/tree/roop-buildStorageReference_lvalue. Tests are passing locally, but I've also discovered that the compiler can still crash in the case of a nonmutating set for the wrapped value in the property wrapper and a mutating didSet for the property with that attached property wrapper:

@propertyWrapper
struct State {
  var wrappedValue: Int {
    get { 0 }
    nonmutating set {}
  }
}

struct S {
  @State private var value: Int {
    mutating didSet {}
  }
}

I'm going to try to fix this up and open a new PR. Thank you so much @roop for working on this!

roop · 2020-04-15T20:18:48Z

Thanks for taking over this work.

I suspect the crash with the mutating didSet might have something to do with the changes in #26632. Specifically, my changes in IsSetterMutatingRequest::evaluate might now be wrong:

     if (auto mut = var->getPropertyWrapperMutability()) {
-      return mut->Setter == PropertyWrapperMutability::Mutating
-        && result;
+      bool isMutating = mut->Setter == PropertyWrapperMutability::Mutating;
+      if (var->getParsedAccessor(AccessorKind::DidSet)) {
+        // If there's a didSet, we call the getter for the 'oldValue', and so
+        // should consider the getter's mutatingness as well
+        isMutating |= (mut->Getter == PropertyWrapperMutability::Mutating);
+      }
+      return isMutating && result;
     }

I'll try to get that fixed in this PR tomorrow, which might be useful to integrate into yours.

theblixguy · 2020-04-15T20:24:34Z

That test case crashes on 5.1 and later as well, so I think it might be unrelated to your changes. But, you're right that your change should also take into consideration the result of isSimpleDidSet() on the parsed didSet accessor:

if (auto didSet = var->getParsedAccessor(AccessorKind::DidSet)) {
  if (!didSet->isSimpleDidSet()) {
    isMutating |= (mut->Getter == PropertyWrapperMutability::Mutating);
  }
}

hborla · 2020-04-16T00:26:19Z

I've posted a draft PR (#31054) that resolves merge conflicts/fixes build failures, addresses @theblixguy 's feedback above, and fixes the mutating didSet case. It's currently a draft because I want to better understand the algorithm in PropertyWrapperLValuenessRequest::evaluate for computing the the mutability sequence of the composition chain to figure out if what I had suggested above is possible. @roop I suggest that you close this PR and we can continue discussion on the new one. Thank you!

roop · 2020-04-16T07:36:34Z

It's currently a draft because I want to better understand the algorithm in PropertyWrapperLValuenessRequest::evaluate for computing the the mutability sequence of the composition chain to figure out if what I had suggested above is possible.

I've added an explanation of the algorithm in the new PR: #31054 (comment)

I'm closing this PR.

the description from swiftlang#29069

roop added 3 commits January 8, 2020 08:32

[Property wrappers] Move sr11637 compiler crasher to fixed directory

6c23146

roop changed the title ~~Build storage reference lvalue~~ Fix computation of lvalue-ness in buildStorageReference for property wrappers with non-standard mutatingness Jan 8, 2020

roop changed the title ~~Fix computation of lvalue-ness in buildStorageReference for property wrappers with non-standard mutatingness~~ Fix computation of lvalue-ness in buildStorageReference for property wrappers Jan 8, 2020

roop changed the title ~~Fix computation of lvalue-ness in buildStorageReference for property wrappers~~ [Property Wrappers] Fix computation of lvalue-ness in buildStorageReference Jan 8, 2020

roop requested review from DougGregor, jckarter and theblixguy January 22, 2020 11:04

theblixguy mentioned this pull request Mar 23, 2020

[Typechecker] Fix _modify for wrapped properties with observers #29931

Merged

hborla reviewed Apr 13, 2020

View reviewed changes

hborla mentioned this pull request Apr 16, 2020

[Property Wrappers] Fix computation of lvalue-ness in buildStorageReference #31054

Merged

roop closed this Apr 16, 2020

hborla added a commit to hborla/swift that referenced this pull request Apr 17, 2020

[NFC] Update the documentation for/in buildStorageReference using

765618b

the description from swiftlang#29069

roop mannequin mentioned this pull request Jan 8, 2020

[SR-11637] Property wrapper crash when wrappedValue getter is mutating and setter is nonmutating #54048

Closed

[Property Wrappers] Fix computation of lvalue-ness in buildStorageReference #29069

[Property Wrappers] Fix computation of lvalue-ness in buildStorageReference #29069

Uh oh!

Conversation

roop commented Jan 8, 2020

Implementation

Testing

Uh oh!

roop commented Jan 22, 2020

Uh oh!

hborla 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!

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!

hborla commented Apr 14, 2020

Uh oh!

roop commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theblixguy commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hborla commented Apr 16, 2020

Uh oh!

roop commented Apr 16, 2020

Uh oh!

Uh oh!

roop commented Apr 15, 2020 •

edited

Loading

theblixguy commented Apr 15, 2020 •

edited

Loading