Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the KDS SwiftPM module to build in Swift 6 mode and addresses Swift 6 concurrency/safety requirements around font registration state and color model types.
Changes:
- Removed Swift 5 language-mode pin so KDS builds under Swift 6 defaults.
- Replaced the font-registration global flag with an atomic to satisfy Swift 6 synchronization requirements.
- Updated color models/extensions to align with Swift 6 rules (
Sendable/Identifiable, removal of@retroactive).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| KDS/Sources/KDS/Fonts/InterFont.swift | Uses Synchronization.Atomic to back isRegistered for Swift 6 safety. |
| KDS/Sources/KDS/Controllers/ColorsView.swift | Removes @retroactive and keeps Identifiable conformances compiling in Swift 6. |
| KDS/Sources/KDS/Colors/SemanticColor.swift | Adds Sendable to AdaptiveColor and color structs for Swift 6 concurrency checks. |
| KDS/Package.swift | Removes Swift 5 compatibility language-mode configuration to enable Swift 6 mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| set { | ||
| registeredInterfont = newValue | ||
| _ = registeredInterfont.exchange(newValue, ordering: .acquiringAndReleasing) |
There was a problem hiding this comment.
isRegistered setter uses exchange and discards the returned old value. If you only need to write the flag, prefer an atomic store operation (with an appropriate ordering) to make intent clearer and avoid the extra read/modify overhead.
| _ = registeredInterfont.exchange(newValue, ordering: .acquiringAndReleasing) | |
| registeredInterfont.store(newValue, ordering: .releasing) |
| } | ||
| set { | ||
| registeredInterfont = newValue | ||
| _ = registeredInterfont.exchange(newValue, ordering: .acquiringAndReleasing) |
There was a problem hiding this comment.
Switching the backing flag to an Atomic removes the data race on the boolean, but the overall registration flow is still not thread-safe: two callers can both observe isRegistered == false and proceed to register concurrently, potentially triggering the “calling this more than once” failure path. Consider using an atomic compare-and-swap (or another synchronization mechanism) to ensure only one thread performs registration.
| _ = registeredInterfont.exchange(newValue, ordering: .acquiringAndReleasing) | |
| if newValue { | |
| var expected = false | |
| _ = registeredInterfont.compareExchange( | |
| expected: &expected, | |
| desired: true, | |
| ordering: .acquiringAndReleasing | |
| ) | |
| } else { | |
| registeredInterfont.store(false, ordering: .releasing) | |
| } |
| import UIKit | ||
|
|
||
| public protocol AdaptiveColor { | ||
| public protocol AdaptiveColor: Sendable { |
There was a problem hiding this comment.
AdaptiveColor is a public protocol; making it inherit from Sendable is a source-breaking API change for any downstream module that conforms its own types to AdaptiveColor (those types must now be Sendable too). If the goal is only to satisfy Swift 6 checks for KDS’s concrete color types, consider keeping AdaptiveColor non-Sendable and adding Sendable only to SemanticColor/LegacyColor (or introducing a separate internal/sendable protocol).
| public protocol AdaptiveColor: Sendable { | |
| public protocol AdaptiveColor { |
amy-at-kickstarter
left a comment
There was a problem hiding this comment.
I'm curious about how much more effort it would take to make some (or all) of KDS live on @MainActor - since most of it probably shouldn't be used off of the UI thread. At a minimum, getting font registration on @MainActor seems valuable. I think everything else should be reasonably safe.
📲 What
This PR enables Swift 6 support for the KDS module.
🤔 Why
Swift 6 makes stronger guarantees about memory safety and concurrency, and can be accessed by Swift 5 modules.
🛠 How
Atomic<Bool>for the font registration static value, which gives Swift 6 the synchronization guarantee to make itSendableSendableconformance to the different Color types@retroactive Identifiablewhich is now a compile time error in Swift 6 mode✅ Acceptance criteria
App should build and run. Tests should work.