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

Property Wrappers #754

Closed
petrpavlik opened this issue Aug 29, 2019 · 5 comments
Closed

Property Wrappers #754

petrpavlik opened this issue Aug 29, 2019 · 5 comments

Comments

@petrpavlik
Copy link
Contributor

Are there plans to start making use of property wrappers? I think that it could improve the syntax in certain areas, especially Property, so I quickly put together this swift package as a proof of concept https://github.com/petrpavlik/PropertyPropertyWrapper.

@andersio
Copy link
Member

andersio commented Sep 8, 2019

It shouldn't be hard to introduced. MutableProperty, Property and Atomic can all be annotated as a property delegate.

@pyrtsa
Copy link

pyrtsa commented Sep 16, 2019

This sounds like a nice addition to have. However, I'm concerned that adding the set accessors without thinking about the overall design might be leading to trouble.

Specifically, the implementation of Atomic<Value> found in petrpavlik/PropertyWrapperWrapper apparently has the shortcoming that read-and-write modifications aren't done without releasing the lock in between. AFAICT (although it's hard to reproduce) that causes a data race in really straightforward looking pieces of code such as:

@Atomic var counter: Int = 0
let queue = DispatchQueue.global()
let group = DispatchGroup()
queue.async(group: group) {
    counter += 1  // <- not really atomic
}
queue.async(group: group) {
    counter += 2  // <- not really atomic
}
group.notify(queue: queue) {
    print(counter)  // depending on locking order, might print 1 or 2 or 3
}

I find this especially problematic in the case of Atomic which has atomic in its name already.

I believe this could be fixed either:

  • by using the unofficial _modify accessor (probably a no-go until it becomes official), or

  • by hiding set altogether and directing writes to be done in terms of the modification interfaces of Atomic and MutableProperty type exposed through the projectedValue, something like:

    $counter.modify { $0 += 1 }
    

Edit: Oh, I realise this topic has been discussed in #731 (comment) too.

@pyrtsa
Copy link

pyrtsa commented Sep 16, 2019

Spent a bit more time investigating this. For at least Atomic<Value>, turning the type into a class-backed struct with _modify accessors seems to me like the sweet spot.

E.g. given this example,

final class Demo {
    @Atomic var aaa: Int = 0
    @Atomic private(set) bbb: Int = 10
    @Atomic fileprivate(set) ccc: Int = 100
    func method() { ... }
}
let demo = Demo()
  • demo.aaa, demo.aaa = 1, and demo.aaa += 2 can be accessed freely (internally),
    • the accessors demo.$aaa.withValue { ... }, demo.$aaa.swap(3), and demo.$aaa.modify { ... } are equally accessible;
  • demo.aaa += 2 only locks once during the read-and-write operation,
  • demo.bbb can be read by anyone but only modified by Demo.method (where of course the always-private generated _bbb is visible too),
  • demo.$bbb.withValue { ... } is accessible whenever demo.bbb is, and
  • the mutable interface demo.ccc as well as demo.$ccc are visible within that file.

A quick implementation which seemed to work, with minor changes to other ReactiveSwift code, is found in pyrtsa@9de23ce.

I'm assuming a similar treatment could be done to Property and MutableProperty too, but turning those into structs would be an even more breaking change, although something to that end was discussed in #19.

@petrpavlik
Copy link
Contributor Author

@andersio I've opened a PR for property wrapper for Property #762

@petrpavlik
Copy link
Contributor Author

This is solved by #781. There's an issue with access control leakage that's being tracked here #795

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants