-
Notifications
You must be signed in to change notification settings - Fork 100
Swift 6: complete concurrency checking #825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
# Conflicts: # StreamChatSwiftUI.xcodeproj/project.pbxproj
Generated by 🚫 Danger |
replaceDeclaration ' Image?' ' NukeImage?' $f | ||
replaceDeclaration ' Image(' ' NukeImage(' $f | ||
replaceDeclaration 'struct Image:' 'struct NukeImage:' $f | ||
replaceDeclaration 'extension Image {' 'extension NukeImage {' $f | ||
replaceDeclaration 'Content == Image' 'Content == NukeImage' $f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nuke was upgraded to 12.8 (supports Swift 6), some of the types are not there anymore
static var `default`: Appearance = .init() | ||
nonisolated(unsafe) static var `default`: Appearance = .init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appearance
is a class in SwiftUI and its properties are accessible through InjectedValues.
@injected makes it impossible top make the Appearance
itself @MainActor
because @Injected(\.images) private var images
can't ensure main actor isolation when a type/view X is created.
The other option would be to use a lock within Appearance to make sure everything is concurrency safe, but that feels like too much because the type itself is called from main anyway (if not mistakenly calling from background threads).
@@ -66,7 +66,8 @@ jobs: | |||
build-xcode15: | |||
name: Build SDKs (Xcode 15) | |||
runs-on: macos-15 | |||
if: ${{ github.event.inputs.record_snapshots != 'true' }} | |||
#if: ${{ github.event.inputs.record_snapshots != 'true' }} | |||
if: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Xcode 15 builds are going away, disabling it for now and a cleanup happens separately
nonisolated(unsafe) | ||
public static var localizationProvider: @Sendable(_ key: String, _ table: String) -> String = { key, table in | ||
Bundle.streamChatUI.localizedString(forKey: key, value: nil, table: table) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative could be making it @MainActor
instead of opting out with nonisolated. But then all out generated L10n structs would need to be MainActor
as well which will cause some troubles, since main actor isolation is not available everywhere where L10n is used. Probably fine to keep it unsafe for now.
static let queryIdentifiers = ChannelListQueryIdentifier.allCases.sorted(using: KeyPathComparator(\.title)) | ||
static var queryIdentifiers: [ChannelListQueryIdentifier] { | ||
ChannelListQueryIdentifier.allCases.sorted(by: { $0.title < $1.title }) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was giving Sendable related error which did not feel right. Just switched to another sorting method.
@@ -42,7 +42,7 @@ class LoginViewModel: ObservableObject { | |||
return | |||
} | |||
|
|||
DispatchQueue.main.async { [weak self] in | |||
Task { @MainActor [weak self] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DispatchQueue gives Sendable error for self
@@ -6,7 +6,7 @@ import StreamChat | |||
import SwiftUI | |||
|
|||
/// View model for the `AddUsersView`. | |||
class AddUsersViewModel: ObservableObject { | |||
@MainActor class AddUsersViewModel: ObservableObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since view model factory is MainActor and view models are used from view, then it made more than sense to make all the view models main actor.
guard let self = self else { return } | ||
self.users = self.searchController.userArray | ||
self.loadingNextUsers = false | ||
MainActor.ensureIsolated { [weak self] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controllers do not ensure on the compilation level which actor is used. We can use any queue for controller completion handlers although the default is main. Therefore, we need to make sure that main is used. Applies to all the completion handlers and controller delegates.
@@ -172,7 +172,7 @@ public class ChatChannelInfoViewModel: ObservableObject, ChatChannelControllerDe | |||
loadAdditionalUsers() | |||
} | |||
|
|||
public func leaveConversationTapped(completion: @escaping () -> Void) { | |||
public func leaveConversationTapped(completion: @escaping @MainActor() -> Void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For avoiding actor isolation in the view. Better force main on the view model level.
@@ -152,15 +152,16 @@ public struct MentionsCommandHandler: CommandHandler { | |||
|
|||
private func searchAllUsers(for typingMention: String) -> Future<SuggestionInfo, Error> { | |||
Future { promise in | |||
nonisolated(unsafe) let unsafePromise = promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise is not sendable even when making SuggestionInfo sendable, added a unsafe check here for bypassing it.
@@ -80,8 +80,9 @@ public class PhotoAssetLoader: NSObject, ObservableObject { | |||
|
|||
exportSession.outputURL = outputURL | |||
exportSession.outputFileType = .mp4 | |||
nonisolated(unsafe) let unsafeSession = exportSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a Swift 6 friendly export function, but we can't use it because it does not support iOS 13. Suppressing the concurrency error here.
@@ -103,7 +103,7 @@ struct LazyGiphyView: View { | |||
var body: some View { | |||
LazyImage(imageURL: source) { state in | |||
if let imageContainer = state.imageContainer { | |||
NukeImage(imageContainer) | |||
Image(uiImage: imageContainer.image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nuke was upgraded to 12.8 and LazyImage has some breaking changes.
LazyImage(imageURL: linkAttachment.previewURL ?? linkAttachment.originalURL) { state in | ||
if let image = state.image { | ||
image | ||
.resizable() | ||
.aspectRatio(contentMode: .fill) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LazyImage has breaking changes and requires to be used like above for getting the resize to fill behavior
let color = containsUserReaction ? | ||
InjectedValues[\.colors].reactionCurrentUserColor : | ||
InjectedValues[\.colors].reactionOtherUserColor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color is struct and reactionCurrentUserColor is lazy var which triggers mutation. Swift 6 was not happy if colors was static var since then static var needs to be made concurrency safe. Ignored the error by switching to accessing injected values directly.
onFinish: @escaping @MainActor(MessageActionInfo) -> Void, | ||
onError: @escaping @MainActor(Error) -> Void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a though one since these functions are used in views, which means that it makes sense to force MainActor. Downside is that it changes public API and ends up changing some view factory methods related to actions (SDK user will get a warning after upgrading the SDK).
@@ -60,7 +60,7 @@ struct ReactionsOverlayContainer: View { | |||
|
|||
public extension ChatMessage { | |||
|
|||
func reactionOffsetX( | |||
@preconcurrency @MainActor func reactionOffsetX( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uses UIScreen.main which requires MainActor
@@ -5,7 +5,7 @@ | |||
import StreamChat | |||
|
|||
/// Data source providing the chat messages. | |||
protocol MessagesDataSource: AnyObject { | |||
@MainActor protocol MessagesDataSource: AnyObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is used by main actor view model, then makes sense to force it to main actor from the beginning. Note that then delegate implementation does not require nonisolated which makes it cleaner.
ImageCache.shared.trim(toCost: utils.messageListConfig.cacheSizeOnChatDismiss) | ||
if !channelDataSource.hasLoadedAllNextMessages { | ||
channelDataSource.loadFirstPage { _ in } | ||
MainActor.ensureIsolated { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isolated deinits are coming, but not available yet
private var images: Images { InjectedValues[\.images] } | ||
private var utils: Utils { InjectedValues[\.utils] } | ||
|
||
/// Context provided utils. | ||
private lazy var imageProcessor = utils.imageProcessor | ||
private lazy var imageMerger = utils.imageMerger | ||
private var imageProcessor: ImageProcessor { utils.imageProcessor } | ||
private var imageMerger: ImageMerging { utils.imageMerger } | ||
|
||
/// Placeholder images. | ||
private lazy var placeholder1 = images.userAvatarPlaceholder1 | ||
private lazy var placeholder2 = images.userAvatarPlaceholder2 | ||
private lazy var placeholder3 = images.userAvatarPlaceholder3 | ||
private lazy var placeholder4 = images.userAvatarPlaceholder4 | ||
private var placeholder1: UIImage { images.userAvatarPlaceholder1 } | ||
private var placeholder2: UIImage { images.userAvatarPlaceholder2 } | ||
private var placeholder3: UIImage { images.userAvatarPlaceholder3 } | ||
private var placeholder4: UIImage { images.userAvatarPlaceholder4 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making everything getter here avoids making lazy var properties concurrency safe. Note that these are indeed called from different threads.
@@ -36,7 +36,7 @@ open class ChannelHeaderLoader: ObservableObject { | |||
private var loadedImages = [ChannelId: UIImage]() | |||
private let didLoadImage = PassthroughSubject<ChannelId, Never>() | |||
|
|||
public init() { | |||
nonisolated public init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise Utils
can't init it in-place and adding any MainActor isolation to Utils is not feasible.
@@ -127,9 +127,9 @@ struct TabBarAccessor: UIViewControllerRepresentable { | |||
} | |||
|
|||
var isIphone: Bool { | |||
UIDevice.current.userInterfaceIdiom == .phone | |||
UITraitCollection.current.userInterfaceIdiom == .phone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UIDevice is main actor whereas UITraitCollection is not
/// Sweeps are performed in a background and can be performed in parallel | ||
/// with reading. | ||
var sweepInterval: TimeInterval = 30 | ||
/// The time interval between cache sweeps. The default value is 1 hour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything under StreamNuke does not need reviewing since it was Nuke's version upgrade without any manual changes.
@@ -14,7 +14,7 @@ public struct StreamChatError: Error { | |||
public let description: String? | |||
|
|||
/// The additional information dictionary. | |||
public let additionalInfo: [String: Any]? | |||
public nonisolated(unsafe) let additionalInfo: [String: Any]? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has Any, which can't be Sendable (note that Error is Sendable). Skipping the sendable check here (breaking change to change Any here)
onDismiss: @escaping @MainActor() -> Void, | ||
onError: @escaping @MainActor(Error) -> Void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will give a build warning for SDK users who override it in their view factory. Needs to be highlighted in the CHANGELOG
@preconcurrency @MainActor | ||
public lazy var audioSessionFeedbackGenerator: AudioSessionFeedbackGenerator = StreamAudioSessionFeedbackGenerator() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apple's feedback generator's init is MainActor, therefore it is really difficult to not have our wrapper nonisolated.
StreamChatSwiftUITests/Infrastructure/Mocks/MockBackgroundTaskScheduler.swift
Outdated
Show resolved
Hide resolved
@@ -22,7 +22,15 @@ public extension ChatClient { | |||
config: config, | |||
workerBuilders: [], | |||
environment: .init( | |||
apiClientBuilder: APIClient_Spy.init, | |||
apiClientBuilder: { | |||
APIClient_Spy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sendable warning if not written out with all the arguments
@@ -21,7 +21,7 @@ open class StreamChatTestCase: XCTestCase { | |||
|
|||
public var streamChat: StreamChat? | |||
|
|||
override open func setUp() { | |||
@MainActor override open func setUp() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many tests want to change main actor isolated types, therefore it is easier to require main actor in a single place
@@ -371,7 +371,7 @@ class MessageListPage { | |||
|
|||
enum ComposerMentions { | |||
static var cells: XCUIElementQuery { | |||
app.scrollViews["CommandsContainerView"].otherElements.matching(NSPredicate(format: "identifier LIKE 'MessageAvatarView'")) | |||
app.scrollViews["CommandsContainerView"].images.matching(NSPredicate(format: "identifier LIKE 'MessageAvatarView'")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nuke's LazyImage changes causes it
|
🔗 Issue Links
Resolves: IOS-736
🎯 Goal
📝 Summary
SWIFT_STRICT_CONCURRENCY
=complete
)Sendable
conformance to many types@preconcurrency @MainActor
requirement (e.g. view models)MainActor.ensureIsolated
for jumping on the main actor from controller delegates and completion handlers🛠 Implementation
🎨 Showcase
🧪 Manual Testing Notes
Full manual regression testing round is needed.
☑️ Contributor Checklist
docs-content
repo