-
Notifications
You must be signed in to change notification settings - Fork 357
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
Enable compilation with Swift 6 for most targets #7311
base: main
Are you sure you want to change the base?
Conversation
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.
Reviewed 5 of 294 files at r1, all commit messages.
Reviewable status: 5 of 294 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)
ios/PacketTunnel/DeviceCheck/DeviceCheckOperation.swift
line 137 at r1 (raw file):
dispatchGroup.enter() let accountTask = remoteService.getAccountData(accountNumber: accountNumber) { result in accountResult = result
I got this waring which is referring to the concurrency checking:
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.
Reviewed 115 of 294 files at r1, all commit messages.
Reviewable status: 115 of 294 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)
ios/MullvadREST/ApiHandlers/RESTTaskIdentifier.swift
line 13 at r1 (raw file):
extension REST { private static let nslock = NSLock() nonisolated(unsafe) private static var taskCount: UInt32 = 0
Would it make sense to encapsulate this state into an Actor?
db519d8
to
a24fed8
Compare
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.
Reviewable status: 115 of 294 files reviewed, 2 unresolved discussions (waiting on @acb-mv and @mojganii)
ios/MullvadREST/ApiHandlers/RESTTaskIdentifier.swift
line 13 at r1 (raw file):
Previously, acb-mv wrote…
Would it make sense to encapsulate this state into an Actor?
I believe it would have to be a globally shared actor to achieve the same result, which would then force all network operations to communicate with that actor, which Operation
cannot inherently do since they are not actors.
ios/PacketTunnel/DeviceCheck/DeviceCheckOperation.swift
line 137 at r1 (raw file):
Yes, this file is used by the PacketTunnelProvider
which is still using Swift 5.0 compilation mode, hence the warnings.
However we can silence those warnings.
163772e
to
39d30b7
Compare
31fc153
to
669bcdb
Compare
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.
Reviewed 280 of 294 files at r1, 1 of 2 files at r2, 5 of 5 files at r3, 3 of 4 files at r4, 11 of 11 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @mojganii)
ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
line 2 at r6 (raw file):
{ "originHash" : "c15149b2d59d9e9c72375f65339c04f41a19943e1117e682df27fc9f943fdc56",
Did this need to be updated or is it unintentional?
ios/MullvadRESTTests/Mocks/AnyTransport.swift
line 9 at r6 (raw file):
// @preconcurrency import Foundation
Why do we need to add preconcurrency here?
ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/SocksSectionHandler.swift
line 12 at r6 (raw file):
import UIKit /// Type responsible for handling cells in socks table view section.
Why not turn the whole thing into an actor instead?
ios/MullvadVPN/TunnelManager/SetAccountOperation.swift
line 111 at r6 (raw file):
Does nothing if device is already logged out. */ private func startLogoutFlow(completion: @escaping @Sendable () -> Void) {
Are the sendable attribute on funcs necessary when the class is sendable?
ios/PacketTunnelCore/Actor/PacketTunnelActor+Extensions.swift
line 9 at r6 (raw file):
// @preconcurrency import Combine
Is combine not ready for concurrency?
ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift
line 50 at r6 (raw file):
private let interactorFactory: SettingsInteractorFactory private var viewControllerFactory: SettingsViewControllerFactory? private let accessMethodRepository: AccessMethodRepositoryProtocol
Why do we need these three as instance vars now?
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.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv, @mojganii, and @rablador)
ios/MullvadRESTTests/Mocks/AnyTransport.swift
line 9 at r6 (raw file):
Previously, rablador (Jon Petersson) wrote…
Why do we need to add preconcurrency here?
Because Foundation isn't fully up-to-date when it comes to Swift 6 concurrency features.
We will get warnings about Sendable types that we cannot get around at the moment.
In this file, we'll a get a compilation error
capture of 'dispatchWork' with non-sendable type 'DispatchWorkItem' in a `@Sendable` closure
Since it's for a mock, it's simpler to disable checks than have a valid model for something that won't be run in a concurrent context in the first place.
ios/MullvadVPN/Coordinators/Settings/SettingsCoordinator.swift
line 50 at r6 (raw file):
Previously, rablador (Jon Petersson) wrote…
Why do we need these three as instance vars now?
I'm not sure anymore, I might have had compilation errors at some point.
I'll remove those, nice catch !
ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/SocksSectionHandler.swift
line 12 at r6 (raw file):
Previously, rablador (Jon Petersson) wrote…
Why not turn the whole thing into an actor instead?
It doesn't need to ?
Declaring something to run on the main actor with @MainActor
is not the same thing as turning a struct into an actor
.
We have to be careful to not conflate two different concepts here.
In Swift 6.0 mode, everything that runs on the UI thread, or uses any API that is expected to run on the UI Thread is declared @MainActor
and as such, should be declared to run on it.
This struct doesn't need any form of synchronisation, as it's only ever used on UI Thread.
ios/MullvadVPN/TunnelManager/SetAccountOperation.swift
line 111 at r6 (raw file):
Previously, rablador (Jon Petersson) wrote…
Are the sendable attribute on funcs necessary when the class is sendable?
It depends
It's not because a class is sendable that the closures in its API are sendable by default.
In general, you almost always want closures to be @Sendable
in the first place, otherwise you wouldn't be able to do the following
func doSomethingAsyncAndExecuteOnUIThread(_ completion: @escaping () -> Void) {
DispatchQueue.main.async { completion() } // Does not compile because `completion` is not `@Sendable`
}
ios/MullvadVPN.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
line 2 at r6 (raw file):
Previously, rablador (Jon Petersson) wrote…
Did this need to be updated or is it unintentional?
It's used by the Swift Package Manager to keep track of dependencies
It's added automatically.
ios/PacketTunnelCore/Actor/PacketTunnelActor+Extensions.swift
line 9 at r6 (raw file):
Previously, rablador (Jon Petersson) wrote…
Is combine not ready for concurrency?
That was overzealous on my part, and not needed, will remove.
d28dea6
to
6fd8052
Compare
6fd8052
to
d72caf5
Compare
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.
Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @mojganii)
ios/MullvadVPN/Coordinators/Settings/APIAccess/Common/SocksSectionHandler.swift
line 12 at r6 (raw file):
Previously, buggmagnet wrote…
It doesn't need to ?
Declaring something to run on the main actor with
@MainActor
is not the same thing as turning a struct into anactor
.
We have to be careful to not conflate two different concepts here.In Swift 6.0 mode, everything that runs on the UI thread, or uses any API that is expected to run on the UI Thread is declared
@MainActor
and as such, should be declared to run on it.This struct doesn't need any form of synchronisation, as it's only ever used on UI Thread.
Sorry, I meant to declare it @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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv and @buggmagnet)
This PR enables us to compile most targets with Swift 6.0 except for all the targets that consume NetworkExtensions (duplicated symbols galore and other compilation issues that I want to solve later)
It mostly declares a lot of types as
Sendable
or disables strict concurrency checks in order to compile.We will revisit the app piece by piece over time to remove all those
@unchecked Sendable
since most of the code is already working (as proven by shipping it, and not seeing crashes)This change is