swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.65k stars 10.38k forks source link

Calling an isolated function in a non-Sendable closure shouldn't be sending self #77067

Open mattmassicotte opened 1 month ago

mattmassicotte commented 1 month ago

Description

I'm not sure how it's possible that self could be sent in this situation. The closures involved are not @Sendable, so the isolation should never change.

- error: sending 'self' risks causing data races
- note: 'isolation'-isolated 'self' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses

Reproduction

class MyClass {
    var handler: (@escaping () -> Void) -> Void = { _ in }

    func doThing(isolation: isolated (any Actor)?) {
        handler {
            // error: sending 'self' risks causing data races
            self.doOtherThing(isolation: isolation)
        }
    }

    func doOtherThing(isolation: isolated (any Actor)?) {
    }
}

Expected behavior

I would expect this to compile error-free.

Environment

swift-driver version: 1.115 Apple Swift version 6.0.2 (swiftlang-6.0.2.1.2 clang-1600.0.26.4) Target: arm64-apple-macosx14.0

Additional information

No response

ollieatkinson commented 1 month ago

Strangely, marking handler as @Sendable makes the error go away

class MyClass {
    let handler: @Sendable (@escaping () -> Void) -> Void = { _ in }

    func doThing(isolation: isolated (any Actor)?) {
        handler {
            self.doOtherThing(isolation: isolation)
        }
    }

    func doOtherThing(isolation: isolated (any Actor)?) {
    }
}

Removing actor isolation parameters also makes it pass: (obvious, but wanted to provide a complete picture)

class MyClass {
    let handler: (@escaping () -> Void) -> Void = { _ in }

    func doThing() {
        handler {
            self.doOtherThing()
        }
    }

    func doOtherThing() {
    }
}

Also works when avoiding a closure's implicit capture of isolation:

class MyClass {
    var handler: (@escaping () -> Void) -> Void = { _ in }

    func doThing(isolation: isolated (any Actor)?) {
        func _doOtherThing() {
            doOtherThing(isolation: isolation)
        }
        handler(_doOtherThing)
    }

    func doOtherThing(isolation: isolated (any Actor)?) {
    }
}
mattmassicotte commented 1 month ago

Here is another workaround someone found for me, which I think pretty much confirms that this is a bug of some kind.

class MyClass {
    var handler: (@escaping () -> Void) -> Void = { _ in }

    func doThing(isolation: isolated (any Actor)?) {
        handler {
            self.doOtherThing(isolation: #isolation)
        }
    }

    func doOtherThing(isolation: isolated (any Actor)?) {
    }
}
mattmassicotte commented 1 month ago

Welp, actually that workaround is actually a way to pass the non-Sendable type across boundaries.

class MyClass {
    var handler: (@escaping () -> Void) -> Void = { $0() }

    func doThing(isolation: isolated (any Actor)?) {
        print("in", isolation as Any)

        handler {
            self.doOtherThing(isolation: #isolation)
        }
    }

    func doOtherThing(isolation: isolated (any Actor)?) {
        print("out", isolation as Any)
    }
}

let value = MyClass()

value.doThing(isolation: #isolation)

output:

in Optional(Swift.MainActor)
out nil
mattmassicotte commented 1 month ago

@gottesmm this is a potentially interesting one. This is both a data race safety hole, on top of being a difficult-to-workaround problem in general.

CrystDragon commented 3 weeks ago

Welp, actually that workaround is actually a way to pass the non-Sendable type across boundaries.

output:

in Optional(Swift.MainActor)
out nil

Hi, I just read your comments, and I'm confused how you deduced the code is "to pass the non-Sendable type across boundaries"? If I understand it correctly, in a non-async caller, a nil-value argument for an isolated parameter tells nothing about the caller's dynamic isolation, because that argument can only be nil.

mattmassicotte commented 3 weeks ago

I'm afraid I don't fully understand the question. Are you saying you think this isn't a data race hole? Or that the isolation differences make sense?

The type is non-Sendable, so it is should be impossible to access self from two different isolation domains.

mattmassicotte commented 3 weeks ago

(this is also potentially similar to https://github.com/swiftlang/swift/issues/77301)

CrystDragon commented 3 weeks ago

I'm afraid I don't fully understand the question. Are you saying you think this isn't a data race hole? Or that the isolation differences make sense?

The type is non-Sendable, so it is should be impossible to access self from two different isolation domains.

That't right, I think there isn't a hole in this case, the difference of the 2 printed #isolation is OK I believe.
Just the same as what you said in your original post, the actual dynamic isolation should never change, especially when the code is just calling non-async functions.

mattmassicotte commented 3 weeks ago

hmm maybe, I'd have to think on this more. But my gut says that this is a bug. I don't see how differences between static isolation and runtime behavior should ever be ok...