Enable building with strict concurrency checking enabled #8084
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tgoyne
force-pushed
the
tg/sendable
branch
3 times, most recently
from
aa89752 to
84fd2d8
Compare
leemaguire
reviewed
Jan 3, 2023
| @@ -30,7 +30,7 @@ RLM_HEADER_AUDIT_BEGIN(nullability, sendability) | |||
|
|
|||
| Schemas map to collections of tables in the core database. | |||
| */ | |||
| RLM_SWIFT_SENDABLE | |||
| RLM_SWIFT_SENDABLE // not actually immutable, but the public API kinda is | |||
There was a problem hiding this comment.
Is it dangerous in any way having the schema classes partially mutable given the above comment?
There was a problem hiding this comment.
Yes, but it's not a new problem. This is a place where Sendable annotations are pointing out that we're doing something which requires care. All of the mutation needs to happen before we expose the objects as part of the shared schema, and there's nothing which will warn us if we get that wrong.
leemaguire
reviewed
Jan 3, 2023
leemaguire
reviewed
Jan 3, 2023
| // (and is in practice) but isn't marked @MainActor. | ||
| func unsafeInvokeAsMainActor(_ fn: @MainActor () -> Void) { | ||
| assert(Thread.isMainThread) | ||
| withoutActuallyEscaping(fn) { fn in |
leemaguire
approved these changes
Jan 3, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This drops Xcode 13.1 and 13.2 support because
@preconcurrencywas added in Swift 5.6 and guarding it with#if swiftwould have required a large amount of duplication. 13.2 is over a year old, so it's about time to drop it anyway.Everything except for the Realm Swift tests are now built with complete strict concurrency checks. The RS tests are built with targeted due to them otherwise hitting a very large number of warnings related to various Apple frameworks not yet being updated for Sendable checking yet. Hopefully Xcode 15 will finish annotating the SDKs.
XCTestCase.wait(for:)is@MainActor, so all the tests which wait for expectations need to be as well. I ended up just marking SwiftSyncTestCase as@MainActoras a result. Xcode 13 has a bug where overridden setUp and tearDown methods don't inherit this, so they need to be explicitly marked as@MainActoras well.I introduced a
@Lockedproperty wrapper for the tests which guards access to a value with a lock. This is required for all the tests where we declare a variable, set it in a Sendable closure, and then check it afterwards. Unfortunately, sendability checking for property wrappers is currently buggy (swiftlang/swift#61358), so silly things are required to avoid warnings from this.A pile of test things have been hoisted out of their containing scope to avoid having to pointlessly capture
selfwhenselfisn't actually used in the function and isn't Sendable.I rewrote the watch tests to use expectations rather than semaphores. This both fixed a pile of warnings and made them work a bit better.
There turned out to be a whole bunch more obj-c things which needed to be marked as Sendable. I also marked the sendable types which already couldn't be reasonably subclassed as final as subclassing is complicated for sendability.
Migration is now a typealias for RLMMigration rather than a wrapper. This has no effect on the user-visible API but it eliminates some complications around translating the migration blocks between the obj-c and swift versions (and it really didn't work correctly before).
RLMMigrationRealmwas just long-obsolete; object store has taken care of what it was doing for years.RLMSyncManager wasn't actually particularly thread-safe. It has some properties which had to be set before the first synchronized Realm is opened so in practice it'd work out as ensuring you set the properties in time would sort of inherently involve doing something single-threaded, but it wasn't properly Sendable. All of the properties are now
atomicinstead. In the process I noticed that theappIDproperty is long obsolete and doesn't make sense any more.Future's callback currently isn't Sendable. I am fairly confident that's a bug as that's the entire point of a Future, so I'm unsafeBitCasting it to Sendable. Similarly,
DynamicProperty.update()currently isn't marked as@MainActorbut I don't see how it could possibly work otherwise, so that uses an unsafe cast as well.BoundCollection.remove()turned out to have a bug: it looked up an index, started a write transaction, then deleted the object at the index. This could possibly delete the wrong object if a write modifying the collection happened on a background thread.MongoClient had some pointless overloads which could simply be default function arguments instead.