Skip to content
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.

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

[SR-11138] Nested property wrappers result in too strict 'mutability' keywords. #53534

Closed
mortenbekditlevsen mannequin opened this issue Jul 15, 2019 · 11 comments
Closed
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@mortenbekditlevsen
Copy link
Mannequin

mortenbekditlevsen mannequin commented Jul 15, 2019

Previous ID SR-11138
Radar rdar://problem/53407949
Original Reporter @mortenbekditlevsen
Type Bug
Status Closed
Resolution Done
Environment

Swift 5.1 prerelease from Xcode 11 beta 3.

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug
Assignee @mortenbekditlevsen
Priority Medium

md5: 276670fd883a17c2a90bf7d0fb6d93f4

Issue Description:

With the Property Wrapper feature, the synthesized setter and getter are marked as mutating or nonmutating based on the corresponding keywords on the getter and setter of the wrappedValue.

For nested property wrappers this currently appears to be decided solely on the outermost wrapper - and this behavior could be improved.

Imagine the property wrappers A and B and the following nesting:

@A @B var c: Int

If A is a value type and has a mutating setter, and B is a reference type (with a nonmutating setter), this will currently (in Xcode 11 beta 3) generate a mutating setter for c.

But the synthesized setter is:

set { _c.wrappedValue.wrappedValue = newValue }

In other words - only the getter of A is called while just the setter on reference type B is called.

Since B is a reference type in this example nothing on _c is being mutated, so the setter might as well have been generated as nonmutating.

For a discussion about this please see the Swift Forums thread: Question about nested property wrappers and mutability

My own humble attempt at an 'algorithm' for deciding the mutability keywords. I hope that I am not mixing things up:

For a chain of nested property wrappers, the mutating keyword on the synthesized getter can be calculated as follows:

Consider the wrappers from the outermost to the innermost in order:

If a wrapper with a mutating getter appears before one with a nonmutating setter, then the synthesized getter must be mutating - ** otherwise it can be nonmutating.

Similarly for setters:

Consider the wrappers from the outermost to (but not including) the innermost:

If a wrapper with a mutating getter appears before one with a nonmutating setter OR if the innermost wrapper has a mutating setter, then the synthesized setter must be mutating. Otherwise it can be nonmutating.

Examples:

@A @b @C @d @e var f: Int

If A and B have nonmutating getters and C is a reference type (and thus has a nonmutating setter), then even though D and E had mutating getters and setters, both getter and setter of _f can be nonmutating. The mutating getter of D would not matter due to the nonmutating setter of C.

But if A, B, and C had nonmutating getters and mutating setters, then if D had a mutating getter, then both the getter and setter of _f should be mutating.

If A, B, C, D and E all had nonmutating getters and mutating setters, then _f would have a nonmutating getter and a mutating setter.

@belkadan
Copy link
Contributor

As Doug explained, the current behavior is correct. Just because the mutating setter does not currently mutate the struct doesn't mean it couldn't do so in the future. Swift simply doesn't have the expressive power to say "mutating iff this generic parameter is not a reference type".

@mortenbekditlevsen
Copy link
Mannequin Author

mortenbekditlevsen mannequin commented Jul 16, 2019

But my point is that the mutating setter is never being called.
I think that John McCall is of the same opinion. Have you read his comments on the topic?
I am not certain what you mean by ‘currently’? How could it change in the future?

Consider:
A is a struct with a wrappedValue with nonmutating getter and mutating setter.
B is a class and as such it’s wrappedValue has nonmutating getter and setter.

@A @b var c: Int
The synthesized setter of c never calls the wrappedValue setter of A. Only the wrappedValue getter of A and the wrappedValue setter of B.
As such the synthesized setter can be nonmutating just fine.

So I am not suggesting any change to Swift except how the synthesized getter and setter is marked with respect to mutability.

Am I completely missing something here?

@belkadan
Copy link
Contributor

:-/ Either you're missing something about how the setter is synthesized, or I'm missing something about how property wrappers are composed. @DougGregor?

@mortenbekditlevsen
Copy link
Mannequin Author

mortenbekditlevsen mannequin commented Jul 17, 2019

Right - I am a bit nervous that I am wasting everyone's time due to my own misunderstanding. :-/

But I'll try explaining how I think the property wrappers are composed. Maybe someone can see where my understanding is wrong and correct me.

For the example above:

@A @B var c: Int

My understanding is that c is synthesized as:

var c: Int {

get { _c.wrappedValue.wrappedValue }

set { _c.wrappedValue.wrappedValue = newValue }

}

So: The type of _c is A<B<Int>>

Calling the setter of c with a value of 1 will:

  1. get the wrappedValue of _c. This value has the type B<Int>.

  2. call the wrappedValue setter on this value with 1

  3. Since B is a reference type in this example, no mutation of the B<Int> value occurs and thus the setter on _c.wrappedValue is this not being called.

So - if this interpretation is correct we only have:

The wrappedValue getter of A is called. The wrappedValue setter of B is called. That's all.

And when _c is not mutated, the setter might just as well be marked as nonmutating.

If my interpretation is not correct I hope that you can show me which of my assumptions are wrong.

@mortenbekditlevsen
Copy link
Mannequin Author

mortenbekditlevsen mannequin commented Jul 19, 2019

@belkadan Did you by any chance have time to look at the description of my understanding of how the property wrappers are composed in the comment above?
I am very curious to hear if I am making any sense at all. :-)

@belkadan
Copy link
Contributor

I'm deferring to Doug as the author of the property wrappers proposal, but Doug has been super busy.

@mortenbekditlevsen
Copy link
Mannequin Author

mortenbekditlevsen mannequin commented Jul 21, 2019

Thanks!
The briefest example I could make, that demonstrates the issue is:

(sorry, jira messes up my formatting when I post this - here is a gist with the code instead:
https://gist.github.com/mortenbekditlevsen/078d4ae3cd935d9e743ba51a1ac2b019)

@propertyWrapper

struct A<T> {

**private** **var** \_storage: T

**var** wrappedValue: T {

    **get** { \_storage }

    **mutating** **set** { \_storage = newValue }

}

}

@propertyWrapper

class B<T> {

**private** **var** \_storage: T

**var** wrappedValue: T {

    **get** { \_storage }

    **set** { \_storage = newValue }

}

**init**(**\_** t: T) {

    **self**.\_storage = t

}

}

struct DontMutateMe {

@A @B **var** c: Int

**func** doesNotMutate() {

// c = 1

_c.wrappedValue.wrappedValue = 1

}

}

Here setting the value 'manually' through _c demonstrates that no mutation is necessary while the out-commented line does not compile.

@belkadan
Copy link
Contributor

@jckarter explained to me that you're correct, using key path composition as an analogy. Thanks for pointing it out and talking through it!

@swift-ci create

@mortenbekditlevsen
Copy link
Mannequin Author

mortenbekditlevsen mannequin commented Jul 22, 2019

Thanks a lot, Jordan. I’m happy to hear that I wasn’t imagining things. 🙂 Looking forward to see this amazing language feature be even more awesome!

@jckarter
Copy link
Member

Here's a fix in the master branch: #26326 You should be able to try it out in future snapshots of the master branch.

@mortenbekditlevsen
Copy link
Mannequin Author

mortenbekditlevsen mannequin commented Jul 30, 2019

Thank you so much! I look forward to trying it out in a future snapshot. :-) The 'composeWith' algorithm is a very elegant solution - and really excellent that it also deals with wrappers with missing setters.

Do you think that there is any chance that the fix will be cherry-picked to swift-5.1-branch?

Thanks again! :-)

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Development

No branches or pull requests

2 participants