Ensure access to PDFDocument is always concurrency safe #58
Conversation
|
@PSchmiedmayer as explained in the PR description. I requested review on this PR draft already to discuss potential ways forward with this PR as it inevitably causes breaking changes of recently introduced API and uses Swift 6 language features that might not be available to people still using the Swift 5 toolchain. |
|
Thank you for taking a look at this @Supereg! Thank you for breaking down the breaking and non-breaking changes. As far as I can see these are breaking changes in a need that a user would need to update to an Xcode version that supports Swift 6, but would still compile the package in combination with an other package or application that only uses the Swift 5 compilation mode without any errors in the source code? And to add this so we don't run into this in the future: We should add a GitHub Action job that builds for Swift 6 and is required for all future PRs. I am wondering if the general workflow should be: We merge #52 and #54 trying to avoid any breaking changes (1.X) sooner than later (CC @AdritRao & @RealLast). A more general question: Should we handle supporting Swift 6 only (dropping compiling with Swift 5.X) as a breaking change across all our packages or is there a way to handle these are elements as minor changes, just requiring developers to update Xcode but ensuring that elements still build with the latest Xcode versions. |
Yes, the user would need to have the latest Xcode version with a Swift 6 toolchain. However, the
This sounds like a good approach.
If we do not consider requiring updating the Xcode version as a source breaking change, we can see this as a minor change (just updating the swift tools version in the |
|
Agree; let's wait until Xcode 16 is released as a stable release and then tag it as a minor when we bump the swift tools version to Swift 6. As long as it can be used with apps that still support Swift 6 we should be fine. We just might need to tag these releases in the reverse order of the dependency tree and we might not be able to catch all of them ... something to discuss in our next meeting. |
There was a problem hiding this comment.
Thank you for working on this @Supereg; I would be happy to see this merged in combination with the other PRs open here; we might at some point move to tag a new major version of Onboarding anyways, e.g., moving consent into a few target or even a new package. Some of the OnboardingStack elements can also move into SpeziViews in the long run.
| // Something the compiler doesn't realize here is that we can send the `PDFDocument` because it is located in a non-Sendable, non-Copyable | ||
| // type and accessing it will consume the enclosing type. Therefore, the PDFDocument instance can only be accessed once (even in async method) | ||
| // and that is fully checked at compile time by the compiler :rocket: | ||
| // See similar discussion: https://forums.swift.org/t/swift-6-consume-optional-noncopyable-property-and-transfer-sending-it-out/72414/3 |
There was a problem hiding this comment.
Thank you for the context!
Ensure access to PDFDocument is always concurrency safe
♻️ Current situation & Problem
The PR #51 added several interactions with
PDFDocumentthat cannot be proven to be concurrency safe. This makes the package incompatible with Swift 6 and unable to compile. The issue was tracked in #57.This PR attempts to resolve these issues. However, this not possible without breaking existing API. So see the initial state of the PR as a starting point for discussion on how we version this change and how we provide compatibility with Swift 5.10 environment as most of the changes require Swift 6 features.
PDFDocumentis a non-[Sendable](https://developer.apple.com/documentation/swift/sendable) type (and probably won't be ever as it is an open class type inheriting from NSObject, see Sendable Classes). However, non-Sendable types can cross actor boundaries when passed assendingparameters. This keyword ensures that the caller won't maintain any references and ownership is fully transferred to the callee (for non-Sendable types). As thePDFDocumentis just a result type we mean to transfer to theStandardthat implements theConsentConstraintand don't require to maintain ownership within SpeziOnboarding, we can easily send the PDFDocument to the Standard and provide ownership to the Standard.EDIT: Here is a great deep dive into non-copyable types: Consume noncopyable types in Swift.
⚙️ Release Notes
All changes are annotated if they require Swift 6 language features and if they are considered a breaking change. Note that we assume framework users to operate in Swift 5 mode. As Swift 6 isn't released yet, we do not consider any changes to concurrency annotations that do not create compiler errors in Swift 5 (even with strict concurrency checking) as a breaking change.
The following changes were made according to the approach explained above:
OnboardingConstraintreceives itsPDFDocumentas asendingparameter. This requires Swift 6 and is a breaking change.OnboardingDataSourcereceives thePDFDocumentas asendingparameter. This requires Swift 6 and is not a breaking change.ConsentDocumentExportwas turned into a non-Copyable type that is non-Sendable. The access to the PDF isconsumingand returns thePDFDocumentas asendingresult. This requires Swift 6 and is a breaking change.consumingcan only be applied tofuncdeclarations andsendingonly to results types offuncdeclarations. Therefore, we needed to replace theasyncpdfproperty.consumePDFmethod async as the documentation suggest that this might be required in the future. Is that really something that will come?ConsentConstraintwas updated to receive theConsentDocumentExportas asendingandconsumingparameter. This requires Swift 6 and is a breaking change.📚 Documentation
TBA
✅ Testing
The test app was updated and existing unit tests will cover the refactored code.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: