SE-0258: Property Wrappers (second review)

Evolution Proposal Reviews
42

This example is using the Fluent, an ORM for the Vapor framework. The example code essentially constructs a SQL query like this and runs it:

SELECT * FROM users WHERE name = "Foo"

The reason we want to access backing storage is to get the name of the property that we are filtering on. While it is possible with a normal key-path, using the backing storage makes it much easier. We can use Mirror instead of weird Codable hacks. It is also more performant because we can store the the property's name in the backing storage after accessing it. We can also easily set a custom name for the SQL column:

final class User: Model {
    @Field(name: "full_name") var name: String
}
2 Likes
43

I think this is where having some specific SwiftUI examples would help. @Douglas_Gregor presented Modern Swift API Design - WWDC19 - Videos - Apple Developer the Binding struct (about minute 39:00) which uses dynamic member look up. Unfortunately the documentation does not show the full declaration. Apple Developer Documentation but they use https://developer.apple.com/documentation/swiftui/bindingconvertible

The slide at 39:00 gives it more justice but using the public documentation you can deduce something like the below.

@dynamicMemberLookup @propertyWrapper 
struct SomeBinding<Value> {
    // Other stuff
    subscript<Subject>(dynamicMember keyPath: WritableKeyPath<Self.Value, Subject>) -> SomeBinding<Subject> { get }
}

This is why I think we are getting composition.

final class User: Model {
  @SomeBinding @Field(name: "full_name") var name: String
}
1 Like
44

The UserDefaults example needs to store the string key, so there is storage. The proposal doesn’t have a way to completely suppress the stored property, but I don’t think we need one: the property wrapper type can be an empty struct and it will take no storage.

  • Doug
1 Like
45

I have been involved with the proposal since the 3rd pitch, and I am very much in favor. However, I must echo the concerns raised by others regarding access levels and regarding access to both the wrapper and its wrapperValue property.

Access levels:

While I can live with internal for wrappers which expose a wrapperValue property, I would prefer to see the same level of control offered to properties themselves.

$ & $$:

With this proliferation of sigils, I am reminded of Perl, but not in a positive way. I think having both will be confusing, and having $foo change meaning with the introduction of wrapperValue to the type can only lead to confusion. Code which uses $foo will break if the wrapper adds wrapperValue.

I don't see value in having access to both, but if this must remain, access to the backing storage variable should always be through $$.

4 Likes
46
  • What is your evaluation of the proposal?

As it stands right now, I'm pretty neutral. I expressed in the pitch thread my dissatisfaction with the variable meaning of $foo, and believe that such a terse syntactic construct should be reserved for something tied closely to the declaration of foo itself, i.e., the backing storage (and not wrapperValue). I do not believe that the small gains in conciseness justify the loss in clarity. Writing $foo.binding feels much more "Swifty" and far less magical to me, and so I remain against the idea of wrapperValue in general.

As far as the access control issue goes, I agree with those who have expressed that the current model is too complex. Personally, I agree with @DevAndArtist and others who have argued that the default visibility of the storage should be internal. From my understanding, a likely future direction is to provide an explicit syntax for modifying the wrapper's access level (it would be great if that was implemented as part of this proposal, but it is not a showstopper IMO), and once that comes along it will be pretty broken to have these arcane access level rules for wrappers in addition to the ability to specify things explicitly.

(Aside: one other thought I had about alternative syntax for access level of the wrapper is to allow a modifier to be applied to the annotation itself, e.g.:

public @Wrapper public var foo: Int = 0

)

Other than those caveats, I'm very much in favor of this proposal.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes. This will help abstract away many common patterns.

  • Does this proposal fit well with the feel and direction of Swift?

Besides the exceptions noted above, I believe so.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

N/A

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Followed along with the pitch threads and the previous review. Gave the newest proposal a quick reading.

1 Like
47

  • What is your evaluation of the proposal?
    +.99 for sure. I think this feature is great. Unfortunately, I think it might have gotten camelled a bit at the end there with the "Delegating access to the storage property" $$ stuff.

  • Is the problem being addressed significant enough to warrant a change to Swift?
    Yes

  • Does this proposal fit well with the feel and direction of Swift?
    Yes

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Been following this review since the beginning through WWDC.

Most of the churn seems to be around access control of properties and functions in the propertyWrapper. I think everyone would be happy if we could fulfill these use cases for propertyWrapper properties/functions:

  • A way to designate properties/functions as private to the propertyWrapper
  • A way to designate properties/functions as accessible only by the class who's property is backed by that propertyWrapper
  • A way to designate properties/functions as accessible by any object that has access the the property backed by a propertyWrapper

Given Swift prior art, the first and third seem to be pretty straightforward by having private on the former and internal/public on the latter.

The second one, however, makes me feel like we're still missing that access control piece for designating that middle ground of access control between visibility to all and no visibility. We currently have fileprivate to try to fulfill that need, but I think this pitch exposes the weaknesses of using file colocation as the only way to do that.

That's all to say that my vote to solve this problem would be to rename fileprivate to something a bit less file specific (privileged?), and to allow only the class implementing the property backed by a wrapper to see those functions and properties.

48

True. Although I suppose you could imagine a more macro like incarnation of propertyWrappers where it would be possible.

49

It’s not a use-case-based decision; it’s based on the precedent of other generated entities (such as the memberwise initializer of a struct) being “internal” by default. The reasoning here is that having something implicit be public is making a long-term commitment on the authors behalf without them necessarily realizing that they are making this commitment.

I’m open to @tanner0101’s arguments that we should be expose the $ property more widely, because the wrapper can have meaningful API. It’s meaningful to expose both foo and $foo for these property wrappers (like Field, but also more generally any link to data where you want more coordination APIs).

However, I think we do need to be mindful of not accidentally exposing implementation details of a property that happens to be implemented in terms of a property wrapper. That’s the argument made by folks who want the storage property to always be private—it’s an implementation detail that shouldn’t be exposed even outside of the current type, much less publicly.

There’s no solution that will make everyone 100% happy here. The wrapperValue approach in the proposal puts the decision in the hands of property wrapper type authors: add wrapperValue to have $foo be exposed outside of the type, omit it to have $foo be kept as private.

I think having $foo follow the access of the original property (when wrappedValue is present) is reasonable. It still has the problem of accidentally exposing $foo as API, because wrapperValue is declared completely separate from the foo property. That said, property wrapper types are expected to be well-documented to describe their effect, and the defaults skew toward exposing less.

  • Doug
5 Likes
50

I really like this proposal, with two caveats:

  1. I don’t see why you would want to expose the backing storage at all when a wrapperValue is provided, that seems to defeat the purpose. Perhaps there are technical reasons.

  2. I think it is very unfortunate, but perhaps unavoidable, that this introduces another case where a variable may look uninitialised but may have a value. This was always true for optionals, it’s a shame if this becomes a trend.

The second one may be hard to avoid, but I think we should try. Perhaps require such properties to be initialised with =.init() or BackingType.initialValue.

On a related note, in the ArenaStorage example, and others, the backing initialiser has a term initialValue, but when the property is initialised that term stays on the attribute line:
@ArenaStorage(arena: currentConnectionArena, initialValue: "Hello") and the property itself appears unitialised. Is this a typo? If not it’s an avoidable case of 2.

1 Like
51

Wouldn’t one solution be to expose nothing, unless you implement wrapperValue, then you expose that as $myValue. This gives the implementer full control, even over access levels.

If you want the behaviour in the proposal, return self and make the property internal. If you want to make it private, do that. If you want to expose a pointer or something, return that, or don’t implement it at all and expose nothing. Everyone’s happy...

2 Likes
52

(Taking off my review-manager hat again.)

In my mind, there are really two kinds of property wrappers:

  • property wrappers that are just a convenient way to get some common behavior in the implementation, like @Lazy or @Cloning or whatever, and
  • property wrappers that represent a more fundamental aspect of the property, where you can't really understand the property without knowing that it's a @Blah property.

The former are likely to only provide low-level operations on the wrapper type. For example, it would be reasonable for @Lazy to have a reset() method, but that's probably something that only the property's defining context should be able to use, because executing the lazy initializer twice might mess up a lot of invariants for the enclosing type.

The latter are more likely to offer high-level operations. For example, the entire point of a @Subscribable wrapper type is that it offers APIs to subscribe to changes to the property, and it's probably the right choice for those APIs to be available to anyone who can access the property itself. (The restriction to at most internal seems unfortunate to me.) It's unlikely that such a property would ever be made non-@Subscribable, and having to write public(wrapper) @Subscribable on pretty much every such property seems like a really unfortunate usability cost.

It seems to me that that difference is inherent to a wrapper type rather than inherent to a particular use of it. Whether the presence of a wrapperValue property is a reasonable proxy for that difference, I have no opinions about.

2 Likes
53

Yes I agree with you on this. I may have expressed myself poorly, and/or misunderstood the proposal, but it looks like you would be exposing two synthesised properties when you implement wrapperValue, and I think that adds more complexity than it is worth. If you want to expose something other than the actual backing property, it seems to me that you won't want to also expose the backing property.

The way I see it, and again I may be misunderstanding, there are really only two fundamental cases:

  1. You either want to expose something other than the "primitive" property or
  2. You don't want to expose anything else

As I understand it, the proposal gives us two other options:

A. Expose only the actual backing property
B. Expose it, plus another implementer chosen property.

That seems arbitrary, what if you want to expose 2 more properties, or 3? Since you can always expose properties on the backing type, it doesn't seems very important to be able to also expose properties directly on the enclosing type.

My proposal was to expose nothing by default, except the primitive. If the implementer provides a wrapperValue then that is exposed behind the dollar sign. This gives more control to the implementer, and it's conceptually much simpler. You never have to think about two different wrapperValues. And again, you can expose pointer properties and whatever else you want, as properties of the wrapperValue.

EDIT:

I agree with your last point, I think it should be the implementer that decides the access level of the backing type. In my scenario that is controlled by the access level on the wrapperValue property.

3 Likes
54

Hello,

I am sorry if this has been discussed before, please point me to the right spot if it was.

Is there a way in current design for property delegates to somehow refer to data on the containing object at init time?

For example, I want a property delegate that uses some storage mechanism using an ID key. Something commonly found in ORM systems.
Simplified:

@propertyDelegate
struct DBField {
  var value: { get { db.find(id)[key] }} 
}

struct User {
  let id: Int
  @DBField(id = XXX, key="name") name: String
}

Right now, it looks to me that this is impossible, and requires initialization in init:

struct User {
  @DBField name: String
  init(id: Int) { self.$name = DBField(id=id, key="name") }
}

Which, admittedly, is not terrible, but would be awesome if it could be simplified to a single line.

55

I don't think it's possible. It's not any different than the following illegal code:

struct S {
   let x: Int
   let y = x
}
57

+1. I've been a strong supporter of the discussions of this type of abstraction since Swift 3 discussions on the same topic. Glad we've resolved them, and this particular design looks great. Not 100% on the wrappedValue and wrapperValue stuff, but I also wonder if this will just come around with a bit of time.

Yes. This seems really important for further framework development as Swift matures, as shown by SwiftUI.

Yes. I did at first think this felt too magical, but the wider attribute story seems to have solidified.

N/A

Read the proposals, and have silently followed this discussions. I felt I had very little extra to contribute. Lots of great talk and digestion of the topic.

58

It would be wonderful to have the first as default (backing store as private, no exposed API) and second be an optional thing (backing store with custom access level) that property wrapper author enables explicitly when defining the property wrapper.

59

I'm very much backing @tanner0101's post here. The current implementation is a real hit and miss. It achieves its goal brilliantly in Apple's use case, but there are many use cases already appearing in this thread that aren't covered, while I think they should be.

6 Likes
60

I gently disagree, for three reasons:

1. The problem isn’t really that bad.

The example code you gave to expose the wrapped field values is unnecessarily verbose; as of Swift 5.1, it could be:

public final class User: Model {
    @Field public var id: Int?
    public var idField: Field<Int?> { $id }

    @Field public var name: String
    public var nameField: Field<IString> { $name }
}

That one extra line per field is not at all ideal, agreed! A convenience for this would be nice. But it’s also not terrible, not a showstopper.

2. What you suggest could be an incremental improvement.

A convenience for the code above such as public(wrapper) is an additive change.

Your proposal of exposing the wrapper by default is not additive. However, I tend to think making the wrapper private by default is the correct approach in most cases — and making the wrapper exposed by default is dangerous. I don’t want to accidentally expose control over how my types internally manage their properties’ contracts.

There are cases like your Field example where exposing both the value and the wrapper should be the default for wrappers of that type. That could be an optional on the wrapper itself, which is also an additive change.

Either way, this feels like a tricky separate discussion that belongs in a follow-up proposal and doesn’t need to delay acceptance of this one.

2 Likes
61

If the wrapper implements wrapperValue to return a different type, it starts to be rather confusing, as well as very annoying:

struct S {
    @StatefulWrapper var x: Int
    public var xObserver: Observer<Int> { $x } // huh? Where'd that come from?! 😯
}
1 Like
62

But why is then 'internal by default' like everything else not the default? That's the only thing I'm worried about. I think especially with some SwiftUI code we would want to write internal wrappers that are also accessible from other files and not stack everything into one file and call it a day. If the introduction of access control specified by the user is only a matter of time, then there speaks nothing against that the user can write private(wrapper) when it's available, and does not need to learn new access control rules for a new feature.

2 Likes