RevenueCat / purchases-ios

In-app purchases and subscriptions made easy. Support for iOS, watchOS, tvOS, macOS, and visionOS.
https://www.revenuecat.com/
MIT License
2.36k stars 319 forks source link

Improve Swift Concurrency support #1041

Closed aboedo closed 2 years ago

aboedo commented 2 years ago

We're using Swift Concurrency in a few places. There are a couple of compiler flags that help prevent issues with Swift Concurrency, that we should leverage.

The flags are -Xfrontend -warn-concurrency -Xfrontend -enable-actor-data-race-checks

And can be enabled in Xcode -> RevenueCat Project -> RevenueCat target -> Build Settings -> Other Swift Flags.

image

Enabling them fires up a few warnings and errors. We should enable the compiler flags, and fix the warnings / errors. More context here: https://forums.swift.org/t/concurrency-in-swift-5-and-6/49337

https://app.shortcut.com/revenuecat/story/11686/enable-swift-compiler-flags-for-concurrency-and-fix-errors

NachoSoto commented 2 years ago

From the post:

If at any point the only way forward for the developer is to switch the language mode and fix 100 errors before they can make progress, we have failed.

Well that's kind of the case right now with these warnings. I worked on this for a while yesterday, and it's kinda of a pain to do right in a codebase that only partially supports Swift concurrency. A bunch of types in the standard library (Foundation, UIKit, etc) added @MainActor (which, I have to say, it's a great abstraction for being able to annotate "this class isn't thread safe", but...), which requires us to either:

The second option leads to my biggest pet-peeve of async/await: an explosion of everything needing to be async. FRP doesn't suffer from this as much. In our case not only does that require making lots of code asynchronous, but it's also impossible because we need to support pre-iOS 13.0. The first option works, but only delays the need for the second one further up the hierarchy.

Similarly, declarations can be marked with the “unsafe” form of a global actor, meaning that they should be considered to be a part of the global actor, but that should only be enforced in Swift 6 code or Swift 5 code that has adopted concurrency. For example:

Unfortunately @MainActor(unsafe) isn't helping us here either. It's not clear what "Swift 5 code that has adopted concurrency." refers to, but adding @MainActor(unsafe) to a method that uses a @MainActor type, and calling that method from a regular method still fails:

Property 'identifierForVendor' isolated to global actor 'MainActor' can not be referenced from this synchronous context

There's also problems with the need for APIs to have @Sendable closures, which the post talks about. @_unsafeSendable would help here, but that's not available yet.

There's a few improvements that I'll put in a PR in a bit, but for the most part unfortunately there isn't much that we can do for the time being. I say for the time being, but I'm scared of what this situation will look like in Swift 6.0.

aboedo commented 2 years ago

🤕 Xcode 14 will be a fun release. Thanks for taking a look at this!

NachoSoto commented 2 years ago

Good news: https://twitter.com/dgregor79/status/1470940523259056129?s=21

taquitos commented 2 years ago

Won't fix until Swift 5.6

pro-akbar commented 2 years ago

I am still facing this type of error

joshdholtz commented 2 years ago

@pro-akbar Hey! Would you be able to create a new issue and fill out all of your environment information in there? 😊 It's a bit easier to have a conversation in a new issue and reference this one. Thank you thank you! ❤️

AvdLee commented 2 years ago

I think it's a good moment for this SDK to start preparing for Swift 6 and adopt Sendable where possible.

It should be doable to add Sendable inheritance for some types. I've currently got these warnings:

CleanShot 2022-08-05 at 15 13 03@2x

aboedo commented 2 years ago

@AvdLee thanks for bringing this up!

We tried it out a while back but had some issues that we would hoping would go away in newer Xcode betas. We'll give it another shot.

NachoSoto commented 2 years ago

@AvdLee Sendable support should be ready in #1795. Would you be able to give that branch a try? If not we could release that as a beta for you to try it in your app.

joshdholtz commented 2 years ago

Not relevant to this issue but just wanted to say... Hi @AvdLee! 👋

NachoSoto commented 2 years ago

Should we close this? :)

aboedo commented 2 years ago

I think we're all done with this one! Closing

aboedo commented 2 years ago

Changes will be released in the next version, next Wednesday or sooner

AvdLee commented 2 years ago

Thanks a lot for the hard work on this @NachoSoto! And Hi back to you @joshdholtz ⛳️

joshdholtz commented 2 years ago

Wow... always rubbing in that you won that round of golf, aren't you? 😛

AvdLee commented 2 years ago

Of course, I need to create extra reason to meet up again for another round of golf 🤪