refactor: migrate app target to Swift 6 + defaultIsolation = MainActor#255
Merged
refactor: migrate app target to Swift 6 + defaultIsolation = MainActor#255
Conversation
Pre-strip baseline that must stay green when @mainactor annotations are removed from app-target types in a later commit.
Spec review flagged that the previous name (VerifiedProtoStoreStressTests) matched the storage protocol rather than the actor under test (VerifiedProtoService). Rename for grep-ability.
Rename test #1 to reflect that first-match return is structural, not an asserted exactly-once property. Drop the parallel-pollers test — each poller had its own state, so it didn't actually exercise shared fan-in. Cancellation coverage for pollForGiveRequest is sufficient.
Partial-applying a @mainactor instance method in a ternary against nil chokes the Swift 6 type-checker ("failed to produce diagnostic"). Wrapping in a trailing closure literal sidesteps the inference path.
Swift 6 deinits are nonisolated by default and cannot touch @MainActor-isolated stored properties without an explicit isolation declaration. Mark the deinit isolated so it inherits the type's @mainactor isolation and can read `observer` to remove the NotificationCenter registration.
JSONEncoder/Decoder hoisted out of Defaults<T> to module-level lets so the struct contains only a Sendable DefaultsKey. Defaults<T> conforms to Sendable where T: Codable & Sendable; the static-var sites in RatesController, Preferences, SessionAuthenticator, BetaFlags, and CurrencySelectionViewModel no longer trip Swift 6's nonisolated global-state diagnostic.
JSONRPCAPIClient (SolanaSwift) holds only an immutable URL and a thread-safe URLSession; each request builds a fresh URLRequest with no shared mutable state. @retroactive @unchecked Sendable is correct given those invariants. WalletRPC inherits Sendable so the @mainactor WalletConnection can send rpc across await boundaries without warning.
GradientStop is a value type holding only Sendable members. The explicit conformance lets Swift 6 prove that static [GradientStop] constants in ColorEditorControl are concurrency-safe.
Swift 6's Regex<...> type is not Sendable because the matcher engine has internal mutable state, but the regex value itself is read-only after construction. The static let pattern is loaded once and only read via match APIs — nonisolated(unsafe) with a SAFETY comment documents the invariant.
The file-scope LocalDefaults enums (and equivalent extensions) that host @defaults static vars are consumed exclusively from @mainactor types. Marking the host @mainactor moves the static var out of Swift 6's 'nonisolated global mutable state' category without an @unchecked / nonisolated(unsafe) escape hatch.
VerifiedProtoService exposes ratesPublisher and reserveStatesPublisher as nonisolated PassthroughSubject lets so RatesController can subscribe synchronously from @mainactor. Combine's PassthroughSubject is not Sendable, so Swift 6 flags the cross-isolation read; @preconcurrency on the import suppresses the warning the same way VerifiedProtoService already does for the publish side.
Four @mainactor classes had nonisolated deinits touching MainActor-isolated stored properties (Operations.runTask, NotificationController.observers, PushController.activeObserver). Annotating the deinit to inherit the type's isolation matches the prior Updateable.deinit fix and avoids the @unchecked / nonisolated(unsafe) escape hatches.
NotificationController.observe now takes a @sendable @mainactor handler, since the trailing-closure body is captured into a Task { @mainactor } that crosses isolation boundaries. Pinning the handler to MainActor lets it mutate the controller's @observable state synchronously while still satisfying the Sendable cross-actor send. resolveVerifiedState's cacheLookup parameter is now @sendable so its sole call site (SendCashOperation) can capture [ratesController] across the awaited lookup. RatesController is @mainactor @observable and therefore Sendable — the captured reference is safe to send.
Two more fixes uncovered when SWIFT_VERSION is flipped to 6.0: - The fileprivate Keychain extension that hosts SecureCodable static vars (keyAccount, userAccount, historicalAccounts) is consumed exclusively from the @mainactor AccountManager. Marking the extension @mainactor moves these statics out of Swift 6's 'nonisolated global mutable state' category, mirroring the prior LocalDefaults / UserDefaults extension fix. - Task.retry's shouldRetry and body closures are now 'sending', so callers on @mainactor (Session.updateProfile, updateUserFlags, receiveCashLink) can pass closures that capture isolated state without the compiler flagging the cross-context send. Tests are unaffected since 'sending' is strictly weaker than @sendable on the parameter side.
The Swift 6 flip uncovered three additional issues: - resolveVerifiedState's cacheLookup closure parameter was tightened to @sendable but that breaks the parameterized test that captures local vars to assert side effects. Switching to 'sending' is strictly weaker on the parameter side: still allows the production call from SendCashOperation to send the [ratesController]-capturing closure across isolation, but lets the tests keep their inline var captures. - Two test files read VerifiedProtoService.{rates,reserveStates}Publisher from non-isolated context. PassthroughSubject is non-Sendable, so Swift 6 flags the cross-isolation read. Importing Combine with @preconcurrency mirrors the shim already in RatesController and silences the warning at the import site. - The parameterized @test arguments in WalletConnectionStateTests evaluate static let contexts in a nonisolated position even though the suite struct is @mainactor. Marking those statics nonisolated removes the implicit MainActor isolation so the macro-generated args expression compiles.
All 12 SWIFT_VERSION entries flip from 5.0 to 6.0. Twelve source isolation fixes uncovered by previous attempts at this flip are fixed in the preceding commits — this change is a pure build-setting flip with no source diff.
Aligns nonisolated async functions to inherit caller isolation, required for the upcoming defaultIsolation = MainActor change. Surfaces one new diagnostic in WalletConnection: the WalletRPC methods inherit MainActor isolation from the caller and now require SolanaSwift's RequestConfiguration / SimulationResult to be Sendable. Demote with @preconcurrency import (with SAFETY note) — same pattern already used for Combine, Firebase, Accelerate, and SQLite.
Every type and top-level function in the app target is now implicitly @mainactor unless marked nonisolated. The 41 manual @mainactor annotations are now redundant; they will be removed in the next commit. This is a behavior-preserving change: the implicit isolation matches what the manual annotations were already declaring. Scoped to the 4 app-target configs only — applying it to the test targets broke compilation of test suites that legitimately hold their own @mainactor declarations (e.g. BaseUITestCase) and would have required restructuring across many test files. Source diagnostics required nonisolated annotations on types and extensions that were intentionally non-MainActor: - Database controller (class + 6 extension files + Schema tables + Models): SQLite layer designed for background callers via the connection's own dispatch queue - Coinbase actor's file-level logger and Codable response types - Deep-link Route (Path, Fragment) and external-wallet processing contexts referenced from background async paths - AppRouter.Destination and AppRouter.SheetPresentation enums whose CustomStringConvertible conformances are read from log metadata formatting in nonisolated contexts - CashCode.Payload (encoded on background paths) and Data.nonce/ Data.Error helpers it pulls in - ImageCompressor / ImageEncoder static helpers called from Task.detached - ErrorReporting Fault NSError subclass (NSError init isolation must match overridden declarations) - ApplePayWebView's deinit calling a String.messageHandlerName static; deinit is nonisolated - VerifiedStateResolution, GradientStop, IndexedCollection, IndexedElement, EnterAmountCalculator: pure value types with no UI state, accessed from tests
The app target sets SWIFT_DEFAULT_ACTOR_ISOLATION = MainActor, so every type-level @mainactor on Session, Controllers, Database, Navigation, Screens, ViewModels, Operations, Utilities, and Share is redundant. Removed. Closure isolation specifiers (Task { @mainactor in ... }) are preserved — they continue to convey meaningful intent in contexts that are not implicitly main-actor (Task.detached blocks). Test target stays nonisolated by default and does not inherit implicit main-actor isolation when extending production types across modules. A handful of TestSupport mocks/helpers gain explicit @mainactor to keep matching the (now implicit) production isolation: Mocks.swift (HistoryController/RatesController/Session.mock and makeMock), CurrencyLaunchProcessingViewModel+TestSupport, and RatesController+TestSupport. No behavior change: the implicit isolation matches the explicit annotations being removed.
The Color-bridging init goes through UIColor(swiftUIColor) which requires the main thread. Marking just the bridging init @mainactor preserves the type's nonisolated stance for static presets while encoding the UIKit thread requirement at the type system level. Without this, callers from a non-MainActor context crash inside UIKit at runtime.
ColorEditorControlTests calls ColorEditorControl.initialStops which constructs GradientStop(from:) — UIKit-bridging code. Same for the hsb(of:) helper in BillDesignerColorsTests, which calls UIColor's getHue directly. Marking the suites @mainactor matches the now-explicit contract on GradientStop.init(from:).
The publisher is nonisolated and emits on VerifiedProtoService's actor executor, not main. The @mainactor test reads the captured received array after Task.sleep, racing the sink closure's write. Adding .receive(on: DispatchQueue.main) serializes delivery to main, matching the test's isolation expectation.
Drop the migration-origin framing in favor of describing each test's ongoing purpose as a TSan + Main Thread Checker sentinel for the target type. Keeps the WHY content (what's stressed, why specific input shapes) and removes references that go stale post-merge.
- MessagingServiceFanInStressTests: replace inline testAccount literal with the existing PublicKey.jeffy helper. - NotificationController.observe: drop redundant @sendable on a @mainactor closure parameter (@mainactor closures are already implicitly Sendable in Swift 6). - WithdrawAmountScreen: explain why the currency-selection action is wrapped in a closure literal — partial-applying a @mainactor method in a ternary against nil trips the Swift 6 type-checker. - Defaults.swift: document the load-bearing invariant that @defaults static-var holders are safe only when the holder is @mainactor.
The file is implicitly @mainactor via the app target's defaultIsolation setting, and every reader of currencyNameAllowedPattern is also @mainactor (CurrencyCreationState is @observable). The escape hatch wasn't load-bearing — the regex inherits @mainactor from default isolation and that's sufficient for thread safety.
Add a brief inline SAFETY note above @preconcurrency import Combine naming the offending Combine types and the .receive(on:) routing that makes cross-isolation reads sound, plus follow-up triggers on the two @unchecked Sendable extensions (Database and JSONRPCAPIClient) so future readers know what upstream change unblocks removal.
Annotating WalletProcessingStateTests @mainactor (matching the sibling WalletConnectionStateTests pattern) lets production drop the over-broad nonisolated modifier on enum WalletProcessingState — the type now inherits MainActor from the app target's default isolation, which is where every production reader actually lives. Static-let test fixtures referenced from @test(arguments:) literals stay nonisolated because @test arguments evaluate at type init before MainActor context.
The previous shape (Task creation followed by immediate cancel) almost always observed Task.isCancelled before the runtime scheduled the loop body, so the cancellation rendezvous wasn't actually exercised. Extract runCancellationStress / runThrowingCancellationStress to await a brief warmup before cancel, ensuring cancellation lands mid-iteration where TSan and the actor's cancellation contract have something to verify.
Task.sleep is wallclock-flaky on loaded CI containers and wastes time on fast ones. confirmation makes 'one publisher emit' a structural property — the test waits exactly until the callback fires and fails fast otherwise. The DB-write test relies on the controller's own sink being registered first against the same PassthroughSubject and running its synchronous DB write before the test's confirm() sink, so awaiting the confirmation guarantees the row is present.
Define Tag.concurrency and Tag.stress so test plans can filter the stress suite (e.g., run-only-stress under TSan in CI). Apply to each of the 5 stress @suites. Replace the hardcoded sheet list in cyclingAllSheets_convergesOnEmptyState with Stack.allCases.map(\.sheet) so adding a top-level sheet automatically widens coverage.
Consolidated migration plan covering Steps 0-4. Step 1 shipped in this PR; Steps 2-4 are checked-off TODOs with self-contained execution notes. Includes a Step 1 retrospective (what surfaced, what to carry forward) and a How to continue section so a future session can pick up the work without conversation history.
raulriera
force-pushed
the
refactor/app-target-swift6
branch
from
20554da to
3a06b38
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Migrates the app target to Swift 6 language mode with default-MainActor isolation. The 41 manual MainActor annotations the previous attempt sprinkled across the app are now redundant and removed — implicit isolation matches what they were declaring. Adds a small set of concurrency stress tests as a TSan and Main Thread Checker sentinel for the actor-heavy paths.
Test plan