Swift 6 Compatibility and class property warning

[adilrc]

Problem Statement

I'm using your SDK with Xcode 15.3, and I'm getting a warning about a static property not being concurrency-safe. Can you please update the SDK to make it Swift 6 compatible and fix the warning?

Here's the warning I'm getting in PostHogSDK.swift:54:
Class property 'shared' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor; this is an error in Swift 6

Thanks!

Solution Brainstorm

No response

[marandaneto]

@adilrc thanks for reporting, did you change any configuration to get this warning? I am also using Xcode 15.3 but I don't get this warning. (Swift v6 isn't released yet either).

[adilrc]

Yes I do have strict concurrency checks enabled in my package along with Swift 5.10.

.target(
  ...
  swiftSettings: [
    .enableUpcomingFeature("StrictConcurrency")
  ]
)

[marandaneto]

Yep I figured via Xcode project file as well SWIFT_STRICT_CONCURRENCY = complete;
I'm just taking a look into it.

[dehesa]

I believe @adilrc made a small typo. The SPM setting is:

.enableExperimentalFeature("StrictConcurrency"),

You can control the type of strict concurrency by following the attribute with = and then the level; i.e. minimal, targeted, complete. The absence of any mean complete. For example:

.enableExperimentalFeature("StrictConcurrency=targeted"),

If I may offer some advice in how to go about removing the concurrency warnings and making PostHogSDK safer:

• Ideally you would make PostHogSDK Sendable. This way the SDK can cross thread boundaries. I already see you are using locks when setting up some functions; therefore you could mark the whole class PostHogSDK as @unchecked Sendable. However, I think you are forwarding mutations on places unprotected by locks (e.g. getDistinctId() calls isEnabled() which relies in the unprotected enabled private variable).
• Since making PostHogSDK sendable is probably too much work. You could mark the static PostHog.shared constant as nonisolated(unsafe). This will remove the warning for users including the SDK as a static library while not making any untrue promises to the compiler. By the way, you don't need the self-executing closure there.

public class PostHogSDK: NSObject {
  @objc public nonisolated(unsafe) static let shared = PostHogSDK(PostHogConfig(apiKey: ""))
}

[marandaneto]

enabled is only mutated in the setup and close methods which are manually locked with setupLock so it's not racing.
Reading enabled won't be an issue.

[dehesa]

Yes, the enabled case is not really a problem because a Boolean is an "Atomic"-like variable. It was just an example. If you would have a similar situation where your write several properties within a lock but the reading is not locked, you can have memory corruptions or incorrect settings. For example:

• Your code is writing (with a lock) in thread 1 a property that stores several bytes of memory.
• Another part of the code is reading from a different thread (say thread 2) one or several of these properties. Since the reading is not locked, the thread 2 might have part of the property already stored in a cache line, but it will need to access the second part. Therefore, you might compose the property/properties with content from two different states.

In any case, this all goes to say that making the whole SDK properly conform to Swift 6 strict concurrency might be hard (not impossible, though). Since we have a zero warning policy in our CI, PostHog SDK is currently not letting us jump into Xcode 15.3. We might have to hardcode some exception for PostHog, but I would rather avoid that. That is why i was suggesting the nonisolated(unsafe) workaround.

[marandaneto]

Yep makes sense, I'm checking how to remove all the warnings without breaking changes, see PR.
At some point, we will migrate to Sendable everywhere + async-await approach but we need to be sure that back compatibility won't be a problem anymore, likely when dropping older versions of Swift.

[dehesa]

Seems like a good plan! I briefly checked your PR and it looks like you are applying major changes. For example:

• You are marking a lot of functions with @MainActor that previously were not. That is a major compatibility breakage for people using the SDK in a background thread. Since you already have a lot of locks, why not using locks instead of forcing people to switch to the main thread?
• You are making classes @unchecked Sendable and you are not marking them as final. Open classes would inherit such promise to the compiler about not worrying about concurrency. By the way, I would recommend marking almost all classes as final. You would get a big performance boost since all the methods would be called statically.

Regardless, my suggestion is to perform minimal changes to the current PostHog version. For the library users, we only have a single warning when accessing PostHog.shared. Why not just silence the compiler in that single instance with nonisolated(unsafe) and make a minor release. Later on you can perform major changes and release as PostHog 4.0?

[marandaneto]

Hey @dehesa, good points.

The @MainActor annotations will be reverted, I wanted to get a feeling of how much work would be to fix (or silence) all the warnings in the SDK as well.
Some things will need to be marked as @MainActor but not for this patch, reasons are that some iOS classes are marked as @MainActor, for example, UIDevice and UIScreen, and they are used to enrich the events with device context.

I was not aware of the performance boost by marking them as final, thanks for sharing.

It's just a draft PR that I am using to learn more about the new strict concurrency since I have not played with it before, no intention to merge it as it is but thanks for calling it out.

[marandaneto]

@dehesa the problem is that nonisolated(unsafe) is only available in Swift 5.10 right

[dehesa]

You are correct! I believe you can do the following:

#if swift(>=5.10)
@objc public nonisolated(unsafe) static let shared = PostHogSDK(PostHogConfig(apiKey: ""))
#else
@objc public static let shared = PostHogSDK(PostHogConfig(apiKey: ""))
#endif

[dehesa]

Hey @marandaneto. Any chance you can do a hotfix or minor release to remove the warning from our CI?

[marandaneto]

Hey @marandaneto. Any chance you can do a hotfix or minor release to remove the warning from our CI?

sorry the delay, https://github.com/PostHog/posthog-ios/releases/tag/3.3.0