-
Notifications
You must be signed in to change notification settings - Fork 105
[V5] Use Swift 6 with complete concurrency checking #902
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: v5
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
Public Interface- public struct AddedVoiceRecording: Identifiable, Equatable
+ public struct AddedVoiceRecording: Identifiable, Equatable, Sendable
- public class ChatChannelInfoViewModel: ObservableObject, ChatChannelControllerDelegate
+ @MainActor public class ChatChannelInfoViewModel: ObservableObject, ChatChannelControllerDelegate
- public func leaveConversationTapped(completion: @escaping () -> Void)
+ public func leaveConversationTapped(completion: @escaping @MainActor() -> Void)
public struct AppearanceKey: EnvironmentKey
- public static let defaultValue: Appearance
+ public static var defaultValue: Appearance
- open class ChannelHeaderLoader: ObservableObject
+ @MainActor open class ChannelHeaderLoader: ObservableObject
- public init()
+ nonisolated public init()
- public class PinnedMessagesViewModel: ObservableObject
+ @MainActor public class PinnedMessagesViewModel: ObservableObject
- public struct AddedAsset: Identifiable, Equatable
+ public struct AddedAsset: Identifiable, Equatable, Sendable
- public struct CustomAttachment: Identifiable, Equatable
+ public struct CustomAttachment: Identifiable, Equatable, Sendable
- public class PollAttachmentViewModel: ObservableObject, PollControllerDelegate
+ @MainActor public class PollAttachmentViewModel: ObservableObject, PollControllerDelegate
public class InstantCommandsHandler: CommandHandler
- public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping (Error?) -> Void)
+ public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping @Sendable(Error?) -> Void)
- public class ChannelAvatarsMerger: ChannelAvatarsMerging
+ public final class ChannelAvatarsMerger: ChannelAvatarsMerging
- open class ChatChannelListViewModel: ObservableObject, ChatChannelListControllerDelegate, ChatMessageSearchControllerDelegate
+ @MainActor open class ChatChannelListViewModel: ObservableObject, ChatChannelListControllerDelegate, ChatMessageSearchControllerDelegate
- public struct ConfirmationPopup
+ public struct ConfirmationPopup: Sendable
- public protocol AudioSessionFeedbackGenerator
+ @MainActor public protocol AudioSessionFeedbackGenerator
- open class ReactionsOverlayViewModel: ObservableObject, ChatMessageControllerDelegate
+ @MainActor open class ReactionsOverlayViewModel: ObservableObject, ChatMessageControllerDelegate
- public struct PollsEntryConfig
+ public struct PollsEntryConfig: Sendable
public class CommandsHandler: CommandHandler
- public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping (Error?) -> Void)
+ public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping @Sendable(Error?) -> Void)
- open class MessageActionsViewModel: ObservableObject
+ @MainActor open class MessageActionsViewModel: ObservableObject
- public struct PollsConfig
+ public struct PollsConfig: Sendable
- public struct PaddingsConfig
+ public struct PaddingsConfig: Sendable
- public enum StreamChatErrorCode: Int
+ public enum StreamChatErrorCode: Int, Sendable
- open class MoreChannelActionsViewModel: ObservableObject
+ @MainActor open class MoreChannelActionsViewModel: ObservableObject
open class NukeImageLoader: ImageLoading
- open func loadImage(using urlRequest: URLRequest,cachingKey: String?,completion: @escaping ((Result<UIImage, Error>) -> Void))
+ open func loadImage(using urlRequest: URLRequest,cachingKey: String?,completion: @escaping @MainActor(Result<UIImage, Error>) -> Void)
- open func loadImages(from urls: [URL],placeholders: [UIImage],loadThumbnails: Bool,thumbnailSize: CGSize,imageCDN: ImageCDN,completion: @escaping (([UIImage]) -> Void))
+ open func loadImages(from urls: [URL],placeholders: [UIImage],loadThumbnails: Bool,thumbnailSize: CGSize,imageCDN: ImageCDN,completion: @escaping @MainActor([UIImage]) -> Void)
- open func loadImage(url: URL?,imageCDN: ImageCDN,resize: Bool = true,preferredSize: CGSize? = nil,completion: @escaping ((Result<UIImage, Error>) -> Void))
+ open func loadImage(url: URL?,imageCDN: ImageCDN,resize: Bool = true,preferredSize: CGSize? = nil,completion: @escaping @MainActor(Result<UIImage, Error>) -> Void)
- public class ViewModelsFactory
+ @MainActor public class ViewModelsFactory
extension ChannelAction
- public static func defaultActions(for channel: ChatChannel,chatClient: ChatClient,onDismiss: @escaping () -> Void,onError: @escaping (Error) -> Void)-> [ChannelAction]
+ @MainActor public static func defaultActions(for channel: ChatChannel,chatClient: ChatClient,onDismiss: @escaping () -> Void,onError: @escaping (Error) -> Void)-> [ChannelAction]
- open class ChatThreadListViewModel: ObservableObject, ChatThreadListControllerDelegate, EventsControllerDelegate
+ @MainActor open class ChatThreadListViewModel: ObservableObject, ChatThreadListControllerDelegate, EventsControllerDelegate
extension DateFormatter
- public static var messageListDateOverlay: DateFormatter
+ @MainActor public static var messageListDateOverlay: DateFormatter
- public struct InjectedChannelInfo
+ public struct InjectedChannelInfo: Sendable
public struct StreamChatError: Error
- public let additionalInfo: [String: Any]?
+ public nonisolated let additionalInfo: [String: Any]?
- public protocol ChannelAvatarsMerging
+ public protocol ChannelAvatarsMerging: Sendable
- open class ChatChannelViewModel: ObservableObject, MessagesDataSource
+ @MainActor open class ChatChannelViewModel: ObservableObject, MessagesDataSource
public class Appearance
- public static var localizationProvider: (_ key: String, _ table: String) -> String
+ nonisolated public static var localizationProvider: @Sendable(_ key: String, _ table: String) -> String
extension CommandHandler
- public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping (Error?) -> Void)
+ public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping @Sendable(Error?) -> Void)
- public struct ChannelItemMutedLayoutStyle: Hashable
+ public struct ChannelItemMutedLayoutStyle: Hashable, Sendable
- public static var `default`: ChannelItemMutedLayoutStyle
+ public static let `default`: ChannelItemMutedLayoutStyle
- public static var topRightCorner: ChannelItemMutedLayoutStyle
+ public static let topRightCorner: ChannelItemMutedLayoutStyle
- public static var afterChannelName: ChannelItemMutedLayoutStyle
+ public static let afterChannelName: ChannelItemMutedLayoutStyle
public class Utils
- public lazy var audioSessionFeedbackGenerator: AudioSessionFeedbackGenerator
+ @MainActor public lazy var audioSessionFeedbackGenerator: AudioSessionFeedbackGenerator
open class WaveformView: UIView
- public struct Content: Equatable
+ public struct Content: Equatable, Sendable
- public struct ChannelListSearchType: Equatable
+ public struct ChannelListSearchType: Equatable, Sendable
- public static var channels
+ public static let channels
- public static var messages
+ public static let messages
public class UnmuteCommandHandler: TwoStepMentionCommand
- override public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping (Error?) -> Void)
+ override public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping @Sendable(Error?) -> Void)
public final class DefaultVideoPreviewLoader: VideoPreviewLoader
- public func loadPreviewForVideo(at url: URL,completion: @escaping (Result<UIImage, Error>) -> Void)
+ public func loadPreviewForVideo(at url: URL,completion: @escaping @MainActor(Result<UIImage, Error>) -> Void)
open class StreamImageCDN: ImageCDN
- public static var streamCDNURL
+ public static let streamCDNURL
- open class MessageViewModel: ObservableObject
+ @MainActor open class MessageViewModel: ObservableObject
- public struct ChannelSelectionInfo: Identifiable
+ public struct ChannelSelectionInfo: Identifiable, Sendable
- public enum AssetType
+ public enum AssetType: Sendable
- open class MessageComposerViewModel: ObservableObject
+ @MainActor open class MessageComposerViewModel: ObservableObject
- open func sendMessage(quotedMessage: ChatMessage?,editedMessage: ChatMessage?,isSilent: Bool = false,skipPush: Bool = false,skipEnrichUrl: Bool = false,extraData: [String: RawJSON] = [:],completion: @escaping () -> Void)
+ open func sendMessage(quotedMessage: ChatMessage?,editedMessage: ChatMessage?,isSilent: Bool = false,skipPush: Bool = false,skipEnrichUrl: Bool = false,extraData: [String: RawJSON] = [:],completion: @escaping @MainActor() -> Void)
- public struct MessageActionInfo
+ public struct MessageActionInfo: Sendable
open class TwoStepMentionCommand: CommandHandler
- open func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping (Error?) -> Void)
+ open func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping @Sendable(Error?) -> Void)
public class MuteCommandHandler: TwoStepMentionCommand
- override public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping (Error?) -> Void)
+ override public func executeOnMessageSent(composerCommand: ComposerCommand,completion: @escaping @Sendable(Error?) -> Void)
- public struct AudioRecordingInfo: Equatable
+ public struct AudioRecordingInfo: Equatable, Sendable
- public struct ChannelAction: Identifiable
+ public struct ChannelAction: Identifiable, @unchecked Sendable
- public var navigationDestination: AnyView?
+ nonisolated public var navigationDestination: AnyView?
public struct ComposerConfig
- public static var defaultAttachmentPayloadConverter: (ChatMessage) -> [AnyAttachmentPayload]
+ nonisolated public static var defaultAttachmentPayloadConverter: (ChatMessage) -> [AnyAttachmentPayload]
- public protocol CommandHandler
+ @MainActor public protocol CommandHandler
- public struct TypingSuggestion
+ public struct TypingSuggestion: Sendable
- public protocol ViewFactory: AnyObject
+ @MainActor public protocol ViewFactory: AnyObject |
@@ -43,14 +43,10 @@ only_rules: | |||
- trailing_comma | |||
- trailing_newline | |||
- trailing_semicolon | |||
- trailing_whitespace |
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 was breaking markdown tests which only happened in this PR because I did a change in MessageView_Test.swift
which then led linter to lint the whole file and apply fixes. Some tests need soft linebreak which is
(double space)
SDK Size
|
@@ -1,6 +1,6 @@ | |||
// The MIT License (MIT) |
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.
Do not review anything under StreamNuke, skip it
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.
Maybe we should consider removing it? We don't have gifs from here, and we might consider using AsyncImage wrapper?
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.
Not in the scope of this PR though
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.
If there is some time to do it in v5, we should. I'll add a ticket for that to the v5 list.
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.
We have a bit more work here - we need to update the InjectedValues to make its usage better, plus we should think about the external dependencies.
token: token, | ||
cid: cid | ||
) | ||
Task { @MainActor 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.
We should have some consistency in how we run things on the main thread. On the other PR this was done with an abstraction.
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 this piece is not part of the SDK and part of the demo app, I did not want to duplicate here. So demo app uses Task, SDK uses that abstraction (mostly)
@@ -19,7 +18,9 @@ class ReactionsIconProvider { | |||
|
|||
static func color(for reaction: MessageReactionType, userReactionIDs: Set<MessageReactionType>) -> Color? { | |||
let containsUserReaction = userReactionIDs.contains(reaction) | |||
let color = containsUserReaction ? colors.reactionCurrentUserColor : colors.reactionOtherUserColor | |||
let color = containsUserReaction ? | |||
InjectedValues[\.colors].reactionCurrentUserColor : |
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.
We should probably update the InjectedValues mechanism, using it like this kind of defeats its purpose.
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.
I see, the issue here is that colors is static var here and it has mutable properties so compiler is:
Static property 'colors' is not concurrency-safe because it is nonisolated global shared mutable state
On the second look, all of this should be MainActor. Let me see.
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.
Ah yeah, I'll revert and just make some things here @mainactor.
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.
Done, avoids this change
public init() { | ||
// Public init. | ||
} | ||
|
||
@Injected(\.utils) private var utils | ||
@Injected(\.images) private var images | ||
private var images: Images { InjectedValues[\.images] } |
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.
Same here, we should definitely do sth about this.
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.
The case here is that ChannelAvatarsMerger is called from background threads. @injected properties have mutable state and therefore must be concurrency safe. @injected is not, InjectedValues is internally nonisolated(unsafe) which makes the compiler happy. Alternative approach would be to pass Images and Utils in with the initialiser.
@@ -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.
Why nonisolated?
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.
Utils creates this class in its init method, but Utils does not force MainActor. It is slightly bigger change because Utils is used in model objects (ChatChannel) what is Sendable. It is going to have bigger side-effects (all the callsites must ensure MainActor). Now the question is, should we try to make Utils @mainactor, maybe, but probably makes sense to have it in a separate ticket.
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.
If Utils is MainActor, then the high-level StreamChat must become MainActor as well.
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 now I took a shortcut here.
@@ -1,6 +1,6 @@ | |||
// The MIT License (MIT) |
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.
Maybe we should consider removing it? We don't have gifs from here, and we might consider using AsyncImage wrapper?
@@ -1,6 +1,6 @@ | |||
// The MIT License (MIT) |
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.
Not in the scope of this PR though
|
||
import Foundation | ||
|
||
enum StreamConcurrency { |
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.
We should move this to Core maybe?
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.
I'll add a note to consider it to the ticket we have about adding Stream Core to chat.
|
StreamConcurrency.onMain { | ||
if let value = value, value != itemHeight { | ||
itemHeight = value | ||
} |
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.
If we make the view MainActor, doesn't it fix this? Kinda weird that we need to do this in a View, which can only be used on the main thread either way 🤔
StreamConcurrency.onMain { | ||
messageCachingUtils.clearCache() | ||
if messageController == nil { | ||
utils.channelControllerFactory.clearCurrentController() | ||
cleanupAudioPlayer() | ||
ImageCache.shared.trim(toCost: utils.messageListConfig.cacheSizeOnChatDismiss) | ||
if !channelDataSource.hasLoadedAllNextMessages { | ||
channelDataSource.loadFirstPage { _ 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.
If the view model is MainActor, why do we need to use StreamConcurrency.onMain here? 🤔
nonisolated(unsafe) let unsafeOnDismiss = onDismiss | ||
nonisolated(unsafe) let unsafeOnError = onError |
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.
Why is this needed? Aren't the completion blocks MainActor?
nonisolated(unsafe) let unsafeOnDismiss = onDismiss | ||
nonisolated(unsafe) let unsafeOnError = onError |
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.
Can we make the completion blocks main actor?
🔗 Issue Links
Resolves: IOS-736
🎯 Goal
Use Swift 6 with strict concurrency checking set to complete
📝 Summary
🛠 Implementation
Tried to make it as concise as possible, therefore in some cases there is
nonisolated(unsafe)
, because fixing them properly leads to huge refactoring (too large scope). In many cases this is actually all right.🧪 Manual Testing Notes
Needs full regression testing round.
☑️ Contributor Checklist
docs-content
repo