Skip to content
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

[Swift 6] use isolated(any) for factory closures #237

Closed

Conversation

mlink
Copy link

@mlink mlink commented Oct 12, 2024

The 2.4 update added Sendable conformance to the factory closures which breaks using GAIT types in Container and elsewhere. For example:

@DaemonActor
public final class XPCDaemon {
    public static let shared = XPCDaemon()

    public init() {}
}

@MainActor
public final class State {
    public init() {}
}

extension Container {
    @DaemonActor var daemon: Factory<XPCDaemon> {
        Factory(self) { XPCDaemon.shared } // Global actor 'DaemonActor'-isolated class property 'shared' can not be referenced from a Sendable closure
    }

    // also a problem for `MainActor` types
    @MainActor var state: Factory<State> {
        self { .init() } // Converting function value of type '@MainActor @Sendable () -> State' to '@Sendable () -> State' loses global actor 'MainActor'
    }
}

By adding isolated(any) to the factory closures we can explicitly mark which global actor the closure should be isolated on. The above can now be written without any errors or intermediate wrapper types:

extension Container {
    @DaemonActor var daemon: Factory<XPCDaemon> {
        Factory(self) { @DaemonActor in XPCDaemon.shared }
    }

    @MainActor var state: Factory<State> {
        self { @MainActor in .init() }
    }
}

Some important details to consider. From the 431 evolution proposal it mentions that functions that use isolated(any) are assumed to cross an isolation boundary and are thus implicitly async and must be awaited. It seems the compiler allows these functions to be assigned to properties where isolated(any) is not used and then called without using await. Not sure if this is intended or possibly a compiler bug. The proposal also mentions that when the compiler can detect that an isolation region is not crossed then it seems the function wouldn't need to be awaited. All of those conditions would be satisfied if the getter in Container is using the GAIT (e.g. @MainActor) and the factory closure is also using the GAIT. Then if no isolation region is crossed no await is needed otherwise an await will be required by the compiler.

Currently it's possible to write something like this:

extension Container {
    // missing `@MainActor` attribute
    var state: Factory<State> {
        self { @MainActor in .init() }
    }
}

and then use it like this:

    @MainActor func doSomething() {
        DispatchQueue.global().async {
            _ = Container.shared.state() // crash here without compiler error because `state()` is `nonisolated`
        }
    }

Which causes a quick quit easter egg. The only way I can think of avoiding this situation is if the calls to the factory closure were always awaited on which sort of defeats the purpose of having everything assigned to the same global actor. I'm open to ideas if anyone has any clues of how to force the compiler to recognize the isolation context. It would be nice if this worked:

extension Container {
    @MainActor var state: Factory<State> {
        // `isolated(any)` picks up `@MainActor` from property definition, which would require using the `@MainActor` attribute for the property definition
        self { .init() }
    }
}

From the 431 proposal:

A function value with this type dynamically carries the isolation of the function that was used to initialize it.

Perhaps there's something I'm not understanding there or the feature isn't working the way I think that it says it's supposed to.

@hmlongco
Copy link
Owner

It's not this PR but check out the concurrency branch for 2.4.2.

@Arideno
Copy link

Arideno commented Nov 14, 2024

Hello @hmlongco, will concurrency branch be merged soon?

@tareksabry1337
Copy link

@Arideno +1 👀

@hmlongco
Copy link
Owner

Concurrency was merged

@hmlongco hmlongco closed this Nov 26, 2024
@balazserdeszlogmein
Copy link

This PR broke the package for Swift 5: #250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants