Proposed fix for a false positive region analysis diagnostic involving isolated closure captures

Development Compiler
1

Hello, it is I, with another region analysis topic... I was thinking about this false-positive diagnostic that is produced when distinct values from a merged region are captured by an isolated closure. The example looks like this:

open class C {
    func use() {}
}

func merge<T, U>(_ t: T, _ u: U) {}

func test() async {
    let a = C()
    let b = C()

    merge(a, b)

    await MainActor.run {
        a.use() // 🛑 error: sending 'a' risks causing data races
        b.use()
    }
}

I believe this is a false positive, since we start with values in two disconnected regions, those regions are then merged (but remain disconnected and un-sent), and they are finally captured by an isolated closure, at which point they are marked as sent. Since the caller does not use the values after passing the closure, the code should compile.

To further demonstrate that this seems like a spurious diagnostic, if we instead ensure the captures are "packaged together" first, then we no longer get the error:

func test() async {
    let a = C()
    let b = C()

    merge(a, b)

    let tup = (a, b)

    await MainActor.run { // ✅
        let (a, b) = tup
        a.use()
        b.use()
    }
}

I think the error today is produced for a fairly straightforward reason: when region analysis translates the partial application for the closure into partition operations, it produces require and send operations for its captures which are interleaved. We can see this in the pass's logging. The isolated partial apply emits these partition operations:

Translating Isolated Partial Apply!
    Semantics: special
 ┌─┬─╼  %27 = partial_apply [callee_guaranteed] %24(%25, %26) : $@convention(thin) @Sendable @substituted <τ_0_0> (@guaranteed C, @guaranteed C) -> (@out τ_0_0, @error any Error) for <()> // users: %49, %37, %28
 │ └─╼  line:13:25
 ├─────╼ require %%4:   %27 = partial_apply [callee_guaranteed] %24(%25, %26) : $@convention(thin) @Sendable @substituted <τ_0_0> (@guaranteed C, @guaranteed C) -> (@out τ_0_0, @error any Error) for <()> // users: %49, %37, %28
 │    └╼ send %%4:   %27 = partial_apply [callee_guaranteed] %24(%25, %26) : $@convention(thin) @Sendable @substituted <τ_0_0> (@guaranteed C, @guaranteed C) -> (@out τ_0_0, @error any Error) for <()> // users: %49, %37, %28
 │    └╼ require %%8:   %27 = partial_apply [callee_guaranteed] %24(%25, %26) : $@convention(thin) @Sendable @substituted <τ_0_0> (@guaranteed C, @guaranteed C) -> (@out τ_0_0, @error any Error) for <()> // users: %49, %37, %28
 │    └╼ send %%8:   %27 = partial_apply [callee_guaranteed] %24(%25, %26) : $@convention(thin) @Sendable @substituted <τ_0_0> (@guaranteed C, @guaranteed C) -> (@out τ_0_0, @error any Error) for <()> // users: %49, %37, %28
 └─────╼ Used Values
          └╼ State: %%4. TrackableValueState[id: 4][is_no_alias: no][is_sendable: no][region_value_kind: disconnected].
             Rep Value:   %5 = move_value [lexical] [var_decl] %4 : $C    // users: %13, %51, %42, %25, %6
             Type: $C
          └╼ State: %%8. TrackableValueState[id: 8][is_no_alias: no][is_sendable: no][region_value_kind: disconnected].
             Rep Value:   %10 = move_value [lexical] [var_decl] %9 : $C   // users: %15, %50, %41, %26, %11
             Type: $C

And when the dataflow reaches the closure, it first requires the region corresponding to a, sends it, and then requires the region for b and errors because b's region was sent in the prior step since a and b share a region.

This issue can be avoided by splitting up all the requires to occur before the sends, like what is done in the implementation of translateIsolationCrossingSILApply. This PR has an implementation along those lines, though it is simpler than the translateIsolationCrossingSILApply implementation because it doesn't consider handling sending captures (which I think are not currently possible to end up with in this case).

Does it make sense to make a change like this, and require all captures in isolated closures before sending them?

A related thought that came up when thinking through this – async lets have similar logic in that they interleave the require/send pairs for each of their captures. From poking around the implementation, I think that may be intentional, since the implicit closure for an async let has sending captures, and we would not want to allow the same value captured more than once via an alias to be sent twice (although I did find a different hole when thinking about this). So I'm assuming we wouldn't want to change the logic for async lets in the same way as the isolated closure capture case – does that sound right?

6 Likes