Compiler allows converting `@concurrent` closure to `nonisolated(nonsending)` closure

[rayx]

Description

According to SE-0461 a @concurrent function shouldn't be converted to nonisolated(nonsending) function. However, I find compiler fails to prevent it in some scenarios. In the code below, both test1 and test2 should fail to compile, but test2 compiles.

@concurrent
func fn(_ ns: NS) async {}

func foo(_ fn: nonisolated(nonsending) (NS) async -> Void) {}

func test1() {
    foo(fn) // error: cannot convert '@Sendable (NS) async -> ()' to 'nonisolated(nonsending) (NS) async -> Void' because crossing of an isolation boundary requires parameter and result types to conform to 'Sendable' protocol
}

func test2() {
    let fn: @concurrent (NS) async -> Void = { _ in }
    foo(fn) // OK
}

Below is a more complete test program to verify that fn runs on global executor indeed. It also demonstrates how that could lead to a data race.

class NS {
    var value = 0
}

func currentIsolation(isolation actor: (any Actor)? = #isolation) -> String {
    return "\(actor, default: "non-isolated")"
}

func logStart(_ fnName: String, _ isolation: String) {
    print("BEGIN \(fnName) [\(isolation)]")
}

func logEnd(_ fnName: String) {
    print("END \(fnName)")
}

actor A {
    var ns = NS()

    func foo(_ fn: nonisolated(nonsending) (NS) async -> Void) async {
        logStart("foo", currentIsolation())
        await fn(self.ns)
        logEnd("foo")
    }

    func test() {
        let fn: @concurrent (NS) async -> Void = { ns in 
            logStart("fn", currentIsolation())
            _ = ns
            logEnd("fn")
        }

        Task { 
            await foo(fn)
        }

        // Task {
        //     await foo(fn)
        // }
    }
}

let a = A()
await a.test()
try? await Task.sleep(for: .seconds(1))

// test output:
// BEGIN foo [output.A]
// BEGIN fn [non-isolated]
// END fn
// END foo

Note the output. It shows that fn actually runs on global executor. If we starts multiple tasks (see the code commented out), there are multiple instances of fn running on global executor and mutating A.ns simultaneously.

Reproduction

See above

Expected behavior

test2 in example code should fail to compile.

Environment

nightly build. The issue can be reproduced in dev snapshot and Swift 6.2 too.

Additional information

No response

[xedin]

@rayx I think the answer here is that this behavior is correct and conversion in test2 is accepted because the conversion is to a non-escaping non-Sendable function and the fn is not used anywhere else in the body, that's why it's safe to convert it even if the argument is non-Sendable because there is no possibility of it transferring a non-Sendable state across isolation boundaries here.

cc @gottesmm to double-check my reasoning here.

[rayx]

@xedin It's possible to produce data race indeed. See the following program and its output. Note fn in task1 and fn in task2 have overlapped writable access to A.ns, which is a data race. However, I'm not sure the issue is with the conversion or with the fact that current compiler allows to pass a nonisolated(nonsending) closure to `foo(_:) as its parameter in multiple tasks. I think the latter is bug. While doing experiment about it, however, I find another diagnostic bug. I'm going to submit these bugs (they are related to the current issue) and get back to you.

EDIT: I'm quite sure this issue is caused by the conversion. See my analysis in the next comment.

class NS {
    var value = 0
}

func currentIsolation(isolation actor: (any Actor)? = #isolation) -> String {
    return "\(actor, default: "non-isolated")"
}

func logStart(_ fnName: String, _ isolation: String) {
    print("\(Task.name ?? "") | BEGIN \(fnName) [\(isolation)]")
}

func logEnd(_ fnName: String) {
    print("\(Task.name ?? "") | END \(fnName)")
}

actor A {
    var ns = NS()

    func foo(_ fn: nonisolated(nonsending) (NS) async -> Void) async {
        logStart("foo", currentIsolation())
        await fn(self.ns)
        logEnd("foo")
    }

    func test() {
        let fn: @concurrent (NS) async -> Void = { ns in 
            logStart("fn", currentIsolation())
            ns.value += 1
            try? await Task.sleep(for: .seconds(1))
            logEnd("fn")
        }

        Task(name: "task1") { 
            await foo(fn)
        }

        Task(name: "task2") {
            await foo(fn)
        }
    }
}

let a = A()
await a.test()
try? await Task.sleep(for: .seconds(3))

// output:
// 
// task1 | BEGIN foo [output.A]
// task1 | BEGIN fn [non-isolated]
// task2 | BEGIN foo [output.A]
// task2 | BEGIN fn [non-isolated]
// task2 | END fn
// task2 | END foo
// task1 | END fn
// task1 | END foo

[rayx]

I believe the conversion is the culprit. nonisolated(nonsending) closures have special features that @concurrent closures don't have:

1. A nonisolated(nonsending) closure capturing non-Sendable value might be passed to multiple task closures.

   An Example

   class NS {
       var value = 0
   }
   
   actor X {
       let ns = NS()
   
       func foo(_ fn: nonisolated(nonsending) () async -> Void) async {
           await fn()
       }
   
       func test() {
           let ns = NS()
           let fn: nonisolated(nonsending) () async -> Void = {
               ns.value += 1
               self.ns.value += 1
           }
   
           Task { 
               await foo(fn)
           }
   
   
           Task { 
               await foo(fn)
           }
       }
   }
   

2. It's OK to pass a value in actor isolated region (e.g. actor property) to nonisolated(nonsending) closure. But it isn't OK to pass that value to @concurrent closure.

In my test program, since fn doesn't captures value, the first difference doesn't play a role. The second difference, however, plays an important role, and it's the conversion that invalidate the difference and causes data race. The three examples below explains why the conversion hides the difference and causes compiler fail to catch the data race.

Example 1: No function conversion. fn is nonisolated(nonsending). The code compiles, as expected.

class NS {
    var value = 0
}

actor A {
    let ns = NS()

    func foo(_ fn: nonisolated(nonsending) (NS) async -> Void) async {
        await fn(self.ns)
    }

    func test() {
        let fn: nonisolated(nonsending) (NS) async -> Void = { ns in 
            ns.value += 1
        }

        Task { 
            await foo(fn)
        }


        Task { 
            await foo(fn)
        }
    }
}

Example 2: No function conversion. fn is @concurrent. The code doesn't compile, as expected.

actor B {
    let ns = NS()

    func foo(_ fn: @concurrent (NS) async -> Void) async {
        await fn(self.ns) // This doesn't compile, as expected.
    }

    func test() {
        let fn: @concurrent (NS) async -> Void = { ns in 
            ns.value += 1
        }

        Task { 
            await foo(fn)
        }


        Task { 
            await foo(fn)
        }
    }
}

Example 3: Having function conversion. fn is converted. The code compiles and causes data race.

actor C {
    let ns = NS()

    func foo(_ fn: nonisolated(nonsending)(NS) async -> Void) async {
        await fn(self.ns) // This comiles, because compiler sees fn as `nonisolated(nonsending)`.
    }

    func test() {
        let fn: @concurrent (NS) async -> Void = { ns in 
            ns.value += 1
        }

        Task { 
            await foo(fn)
        }


        Task { 
            await foo(fn)
        }
    }
}

The problem with example 3 is that while compiler thinks it passes an actor's property to a nonisolated(nonsending) closure, it actually passes it to a @concurrent closure. The conversion effectively does something like the following, which doesn't compile on nightly build if it's written explicitly (because a nonisolated(nonsending) function's parameter shouldn't be transferred anywhere). However, the implicit conversion makes compiler fails to catch the issue.

nonisolated(nonsending)
func wrapper(_ ns: NS) async {
	let fn: @concurrent (NS) async -> Void = { ns in
            ns.value += 1
    }
	fn(ns)
}

[rayx]

conversion in test2 is accepted because the conversion is to a non-escaping non-Sendable function and ...

I probably see what you meant. The original fn is escaping, @concurrent, and Sendable, so it can be captured by multiple task closures. The conversion occurs inside the task closure. I think that's the reason why the fact that foo's closure parameter is non-escaping doesn't help prevent the issue.