Resolve Compile Errors with Strict Swift Concurrency Checking #214

KunJeongPark · 2023-10-31T16:37:11Z

Reason for changes

Apple has suggested you work towards complete checking, not just to avoid potential problems, but also in preparation for the expected checking that a future release of Swift 6 will enforce.

In WWDC 2022 video "Eliminate data races using Swift Concurrency", they state "Complete checking approximates the intended Swift 6 semantics to completely eliminate data races".

Summary of changes

Resolve compile errors with Swift Concurrency Check set to "Complete" in preparation for Swift 6
- References to properties in UIDevice, UIApplication in structs or classes result in an error unless
  the struct or classes or specific functions are referencing these properties are also explicitly guaranteed to run
  on main thread.
- Unit tests using waitForExpectations function needed to be annotated with @mainactor to resolve compile errors
- In PaymentButton's subclasses, their Representable structs that wrap the UIKit elements for SwiftUI views, needed to have initializers that are annotated @mainactor to resolve compile errors.
  
  PaymentButton is a subclass of UIButton which is annotated as @mainactor. I think it is not enforced for the UIButton subclass initializers because of backward compatibility.

I will address some Sendable warnings in another PR.
We can decide when we want to implement these changes.
It's good to be proactive in at least spiking potential scope of work before Swift 6.

Checklist

~~- [ ] Added a changelog entry~~

Authors

@KunJeongPark

KunJeongPark · 2023-10-31T18:06:56Z

Sources/CorePayments/AnalyticsEventData.swift

@@ -101,7 +102,7 @@ struct AnalyticsEventData: Encodable {
        self.correlationID = correlationID
    }

-    func encode(to encoder: Encoder) throws {
+    nonisolated func encode(to encoder: Encoder) throws {
        var topLevel = encoder.container(keyedBy: TopLevelKeys.self)


This just gave warning during compile time but caused failure in Cocoapods test in the pipeline.

Curious if you could add the Sendable protocol to this struct to resolve this issue?

struct AnalyticsEventData: Encodable, Sendable {

No, it does not.

sshropshire · 2023-10-31T19:26:59Z

Sources/CorePayments/Networking/AnalyticsService.swift

@@ -49,7 +49,7 @@ public struct AnalyticsService {
        do {
            let clientID = coreConfig.clientID

-            let eventData = AnalyticsEventData(
+            let eventData = await AnalyticsEventData(


q: This is an asynchronous constructor?

Because AnalyticsEventData was marked with @mainactor, to instantiate it in async function that isn't isolated to the @mainactor, I needed "await".

jaxdesmarais · 2023-10-31T19:34:31Z

It seems like there is also an intermediary Targeted level of checking before Complete that we can consider for making this move progressively. I found this medium article helpful for talking about these checks. Since we are an SDK we likely won't be able to release with Swift 6 until a bit after the release since that is a breaking change. With that in mind we should consider if we want to jump right to Complete or start with Targeted.

KunJeongPark · 2023-10-31T22:00:28Z

It seems like there is also an intermediary Targeted level of checking before Complete that we can consider for making this move progressively. I found this medium article helpful for talking about these checks. Since we are an SDK we likely won't be able to release with Swift 6 until a bit after the release since that is a breaking change. With that in mind we should consider if we want to jump right to Complete or start with Targeted.

I checked with Targeted option and we have no errors or warnings other than same old warnings without the concurrency checking. I think for the compile errors with complete option, the only breaking change would be for the buttons if merchant is not instantiating them on main thread.

I think marking AnalyticsEventData @mainactor is ok since the work in there is lightweight. Alternative would be to fetch device information somewhere else and store it for access here, I don't see an obvious place to store the info. I can implement something like DeviceInfoCache. I thought @mainactor was the simplest way to deal with compile errors.

I think it would be surprising if any version of Swift in the future would enforce this one, any reference to UIDevice's read only properties, for example, giving compile errors without explicit main thread context.

jaxdesmarais

Are the test updates that we needed to make an indication at all that these would be a breaking change for merchants or are these changes mainly behind the scenes? What changes would this cause for merchant demo apps that also have swift strict concurrency checking enabled?

I also don't see anywhere in this PR where we are enabling this setting. Will that be in a future PR along with a changelog entry if needed?

jaxdesmarais · 2023-11-06T15:05:47Z

UnitTests/CardPaymentsTests/CardClient_Tests.swift

@@ -5,6 +5,7 @@ import AuthenticationServices
 @testable import CardPayments
 @testable import TestShared

+@MainActor


For the tests, it looks like we may have the opportunity to make the test async and await on waitForExpectations instead (from this swift forum). Not sure if you've had a chance to try this yet.

addressed in bd8cd78

jaxdesmarais · 2023-11-06T15:09:54Z

Sources/CardPayments/CardClient.swift

@@ -161,6 +161,7 @@ public class CardClient: NSObject {

 extension CardClient: ASWebAuthenticationPresentationContextProviding {

+    @MainActor


I'm curious what portion of this method needs to be called on the main thread. Is it our default to ASPresentationAnchor if we cannot find a window or something else?

We start the authentication session on the main thread in the start function.
Since UIApplication is marked @mainactor, there is compile error with accessing its properties in this function with complete checking option since it checks for explicit main thread context. This seems extreme to me, as it would break many implementations for others as well.

KunJeongPark · 2023-11-06T16:50:43Z

all that these would be a breaking change for merchants or are these changes mainly behind the scenes? What changes would this cause for merchant demo apps that also have swift strict concurrency checking enabled?

The AnalyticEventData change, I think that's just us, so no breaking change for merchant. PresentationAnchor change, that function is used behind the scenes as part of ASWebAuthenticationSession, so I don't think it will affect merchants' implementations. It would only be problematic if merchant has the strict checking and we don't.
The SwiftUI buttons, the change is only for the Representables, for merchants who are using it in SwiftUI and buttons on SwiftUIViews, I don't think there should be any difference in the way it is instantiated by the merchant.

I didn't submit the complete concurrency checking option but I fixed compile errors that occurred as a result of that option check.

jaxdesmarais · 2023-11-07T15:58:49Z

I didn't submit the complete concurrency checking option but I fixed compile errors that occurred as a result of that option check.

Personally, I think if we are adding the code to support complete strict concurrency checking, we should enable it in the SDK at the same time. This way it is clear to merchants/us when we added support and what the necessary code changes were. But curious to hear how other folks feel.

jaxdesmarais · 2023-11-09T14:39:48Z

@KunJeongPark I just came across this Swift Forum thread on Swift 6. There are a few things of note with regards to this PR:

Readiness of data-race safety by default
There are still a number of bugs and holes in the Sendable checking model that admit data races under strict concurrency checking. Equally important, strict concurrency checking currently issues a significant number of false positive reports of data races, making the complete checking difficult to program against.

The Swift 6 language mode will only be declared ready once the Language Steering Group determines that the programming model is effective and usable. The remaining language work to complete data-race safety for the Swift 6 language mode will fall into two categories:

Fill all holes in the strict concurrency model so that data races are diagnosed statically, or dynamically where static safety cannot be proven.
Mitigate false reports of data races on patterns that are proven to be safe.
Swift 5.10 contains a number of significant bug fixes in the actor isolation and Sendable checking. In addition, the following language changes are currently being designed and implemented to undergo the Swift evolution review process:

SE-0411: Isolated default value expressions 17
Inferring @Sendable for methods and key-path literals 15
Strict concurrency checking for global and static variables 9
Improved control over closure actor isolation 13
Lifting restrictions on non-Sendable values through isolated value regions 12

Together, these changes fill the remaining major holes in strict concurrency checking and improve the usability of strict concurrency checking by introducing more Sendable inference and enabling safe ways to transfer non-Sendable values across isolation boundaries. The Language Steering Group acknowledges that other language changes in the area of concurrency are important, but the changes above are necessary for defining the Swift 6 language mode. Other concurrency features are additive and can be explored independently.

Based on these comments and since we do not have a major version planned to update to either Swift 5.10 (that includes some of these fixes) or Swift 6 (that does not yet have a targeted release date) it seems like we should hold off on these changes until the lauguage has ironed out some of these bugs. Curious to know what other folks think!

KunJeongPark · 2023-11-13T16:39:53Z

@KunJeongPark I just came across this Swift Forum thread on Swift 6. There are a few things of note with regards to this PR:

Readiness of data-race safety by default
There are still a number of bugs and holes in the Sendable checking model that admit data races under strict concurrency checking. Equally important, strict concurrency checking currently issues a significant number of false positive reports of data races, making the complete checking difficult to program against.
The Swift 6 language mode will only be declared ready once the Language Steering Group determines that the programming model is effective and usable. The remaining language work to complete data-race safety for the Swift 6 language mode will fall into two categories:
Fill all holes in the strict concurrency model so that data races are diagnosed statically, or dynamically where static safety cannot be proven.
Mitigate false reports of data races on patterns that are proven to be safe.
Swift 5.10 contains a number of significant bug fixes in the actor isolation and Sendable checking. In addition, the following language changes are currently being designed and implemented to undergo the Swift evolution review process:
SE-0411: Isolated default value expressions 17
Inferring @Sendable for methods and key-path literals 15
Strict concurrency checking for global and static variables 9
Improved control over closure actor isolation 13
Lifting restrictions on non-Sendable values through isolated value regions 12
Together, these changes fill the remaining major holes in strict concurrency checking and improve the usability of strict concurrency checking by introducing more Sendable inference and enabling safe ways to transfer non-Sendable values across isolation boundaries. The Language Steering Group acknowledges that other language changes in the area of concurrency are important, but the changes above are necessary for defining the Swift 6 language mode. Other concurrency features are additive and can be explored independently.

Based on these comments and since we do not have a major version planned to update to either Swift 5.10 (that includes some of these fixes) or Swift 6 (that does not yet have a targeted release date) it seems like we should hold off on these changes until the lauguage has ironed out some of these bugs. Curious to know what other folks think!

Those are great finds from the forums. I agree, I did have a conversation with some people and consensus is to mark possible issues and keep the branches updated periodically and revisit as warnings and errors in Xcode get refined for next versions.

I will keep making changes, checking forums, keeping the branches updated and I look forward to input from all of you whenever you have a chance. I think it's good for us to be ahead of it and start thinking about possible issues.

resolve compile errors with strict concurrency checking

89742c1

KunJeongPark requested a review from a team as a code owner October 31, 2023 16:37

KunJeongPark added 2 commits October 31, 2023 10:00

fix button tests

3dfe4d4

fix tests and Cocoapods lint failure on AnalyticsEventData encode

1571833

KunJeongPark commented Oct 31, 2023

View reviewed changes

sshropshire reviewed Oct 31, 2023

View reviewed changes

fix more unit test errors

37bfb51

KunJeongPark mentioned this pull request Nov 6, 2023

Fix Sendable Warnings with Strict Concurrency Checking #215

Open

jaxdesmarais reviewed Nov 6, 2023

View reviewed changes

use async unit test for CardClient

bd8cd78

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve Compile Errors with Strict Swift Concurrency Checking #214

Resolve Compile Errors with Strict Swift Concurrency Checking #214

KunJeongPark commented Oct 31, 2023 •

edited

KunJeongPark Oct 31, 2023 •

edited

warmkesselj Nov 16, 2023

KunJeongPark Nov 17, 2023

sshropshire Oct 31, 2023

KunJeongPark Oct 31, 2023

jaxdesmarais commented Oct 31, 2023

KunJeongPark commented Oct 31, 2023 •

edited

jaxdesmarais left a comment

jaxdesmarais Nov 6, 2023

KunJeongPark Nov 6, 2023 •

edited

jaxdesmarais Nov 6, 2023

KunJeongPark Nov 6, 2023 •

edited

KunJeongPark commented Nov 6, 2023 •

edited

jaxdesmarais commented Nov 7, 2023 •

edited

jaxdesmarais commented Nov 9, 2023

KunJeongPark commented Nov 13, 2023 •

edited

		@@ -161,6 +161,7 @@ public class CardClient: NSObject {

		extension CardClient: ASWebAuthenticationPresentationContextProviding {

		@MainActor

Resolve Compile Errors with Strict Swift Concurrency Checking #214

Are you sure you want to change the base?

Resolve Compile Errors with Strict Swift Concurrency Checking #214

Conversation

KunJeongPark commented Oct 31, 2023 • edited

Reason for changes

Summary of changes

Checklist

Authors

KunJeongPark Oct 31, 2023 • edited

Choose a reason for hiding this comment

warmkesselj Nov 16, 2023

Choose a reason for hiding this comment

KunJeongPark Nov 17, 2023

Choose a reason for hiding this comment

sshropshire Oct 31, 2023

Choose a reason for hiding this comment

KunJeongPark Oct 31, 2023

Choose a reason for hiding this comment

jaxdesmarais commented Oct 31, 2023

KunJeongPark commented Oct 31, 2023 • edited

jaxdesmarais left a comment

Choose a reason for hiding this comment

jaxdesmarais Nov 6, 2023

Choose a reason for hiding this comment

KunJeongPark Nov 6, 2023 • edited

Choose a reason for hiding this comment

jaxdesmarais Nov 6, 2023

Choose a reason for hiding this comment

KunJeongPark Nov 6, 2023 • edited

Choose a reason for hiding this comment

KunJeongPark commented Nov 6, 2023 • edited

jaxdesmarais commented Nov 7, 2023 • edited

jaxdesmarais commented Nov 9, 2023

KunJeongPark commented Nov 13, 2023 • edited

KunJeongPark commented Oct 31, 2023 •

edited

KunJeongPark Oct 31, 2023 •

edited

KunJeongPark commented Oct 31, 2023 •

edited

KunJeongPark Nov 6, 2023 •

edited

KunJeongPark Nov 6, 2023 •

edited

KunJeongPark commented Nov 6, 2023 •

edited

jaxdesmarais commented Nov 7, 2023 •

edited

KunJeongPark commented Nov 13, 2023 •

edited