There is a data race safety hole where closures can be used to pass non-sendable state around in ways that should not be allowed. An example of the problem looks like this:
open class NS {
func use() {}
}
func send(_ ns: sending NS) {
Task.detached { ns.use() }
}
func test() {
let ns = NS()
let c = { ns }
send(c())
ns.use() // ⁉️ use-after-send goes undiagnosed
}
This sort of pattern is caught if the aliasing closure is replaced with a "regular" function, so the issue has something to do with how closures are handled. Debug logging suggests the issue is that the region of the closure's return value is never merged with that of the closure itself. The SIL for this function looks like (emphasis mine):
// test()
// Isolation: unspecified
sil hidden [ossa] @$s6output4testyyF : $@convention(thin) () -> () {
bb0:
%0 = metatype $@thick NS.Type // user: %2
// function_ref NS.__allocating_init()
%1 = function_ref @$s6output2NSCACycfC : $@convention(method) (@thick NS.Type) -> @owned NS // user: %2
%2 = apply %1(%0) : $@convention(method) (@thick NS.Type) -> @owned NS // user: %3
%3 = move_value [lexical] [var_decl] %2 : $NS // users: %15, %16, %18, %6, %4
debug_value %3 : $NS, let, name "ns" // id: %4
// function_ref closure #1 in test()
%5 = function_ref @$s6output4testyyFAA2NSCycfU_ : $@convention(thin) (@guaranteed NS) -> @owned NS // user: %7
%6 = copy_value %3 : $NS // user: %7
// 👇 closure formed
%7 = partial_apply [callee_guaranteed] %5(%6) : $@convention(thin) (@guaranteed NS) -> @owned NS // user: %8
// 👇 closure stored to local variable
%8 = move_value [lexical] [var_decl] %7 : $@callee_guaranteed () -> @owned NS // users: %10, %17, %9
debug_value %8 : $@callee_guaranteed () -> @owned NS, let, name "c" // id: %9
// 👇 closure copied (not sure why exactly... maybe due to "callee_guaranteed"?)
%10 = copy_value %8 : $@callee_guaranteed () -> @owned NS // users: %11, %14
// 👇 closure invoked
%11 = apply %10() : $@callee_guaranteed () -> @owned NS // user: %13
// function_ref send(_:)
%12 = function_ref @$s6output4sendyyAA2NSCnF : $@convention(thin) (@sil_sending @owned NS) -> () // user: %13
%13 = apply %12(%11) : $@convention(thin) (@sil_sending @owned NS) -> ()
destroy_value %10 : $@callee_guaranteed () -> @owned NS // id: %14
%15 = class_method %3 : $NS, #NS.use : (NS) -> () -> (), $@convention(method) (@guaranteed NS) -> () // user: %16
%16 = apply %15(%3) : $@convention(method) (@guaranteed NS) -> ()
destroy_value %8 : $@callee_guaranteed () -> @owned NS // id: %17
destroy_value %3 : $NS // id: %18
%19 = tuple () // user: %20
return %19 : $() // id: %20
} // end sil function '$s6output4testyyF'
Region analysis translates the partial_apply as so:
Visiting: %7 = partial_apply [callee_guaranteed] %5(%6) : $@convention(thin) (@guaranteed NS) -> @owned NS // user: %8
Semantics: special
┌─┬─╼ %7 = partial_apply [callee_guaranteed] %5(%6) : $@convention(thin) (@guaranteed NS) -> @owned NS // user: %8
│ └─╼ line:12:13
├─────╼ require %%3: %7 = partial_apply [callee_guaranteed] %5(%6) : $@convention(thin) (@guaranteed NS) -> @owned NS // user: %8
│ └╼ assign_direct %%5 = %%3: %7 = partial_apply [callee_guaranteed] %5(%6) : $@convention(thin) (@guaranteed NS) -> @owned NS // user: %8
└─────╼ Used Values
└╼ State: %%3. TrackableValueState[id: 3][is_no_alias: no][is_sendable: no][region_value_kind: disconnected].
Rep Value: %3 = move_value [lexical] [var_decl] %2 : $NS // users: %15, %16, %18, %6, %4
Type: $NS
└╼ State: %%5. TrackableValueState[id: 5][is_no_alias: no][is_sendable: no][region_value_kind: disconnected].
Rep Value: %7 = partial_apply [callee_guaranteed] %5(%6) : $@convention(thin) (@guaranteed NS) -> @owned NS // user: %8
Type: $@callee_guaranteed () -> @owned NS
This makes sense – it requires that the region of the captured NS instance (which has id %%3) must not have been sent, and then merges it into the same region as the closure itself (which has id %%5 in this example).
However, when we get to the invocation of the closure, we see this:
Visiting: %11 = apply %10() : $@callee_guaranteed () -> @owned NS // user: %13
Semantics: apply
┌─┬─╼ %11 = apply %10() : $@callee_guaranteed () -> @owned NS // user: %13
│ └─╼ line:14:10
├─────╼ require %%6: %11 = apply %10() : $@callee_guaranteed () -> @owned NS // user: %13
│ └╼ assign_fresh %%7: %11 = apply %10() : $@callee_guaranteed () -> @owned NS // user: %13
└─────╼ Used Values
└╼ State: %%6. TrackableValueState[id: 6][is_no_alias: no][is_sendable: no][region_value_kind: disconnected].
Rep Value: %8 = move_value [lexical] [var_decl] %7 : $@callee_guaranteed () -> @owned NS // users: %10, %17, %9
Type: $@callee_guaranteed () -> @owned NS
└╼ State: %%7. TrackableValueState[id: 7][is_no_alias: no][is_sendable: no][region_value_kind: disconnected].
Rep Value: %11 = apply %10() : $@callee_guaranteed () -> @owned NS // user: %13
Type: $NS
The important bit here is that the result of the apply gets translated into an assign_fresh operation. This means it gets its own "fresh" disconnected region during the dataflow.
In the RBI proposal it states this (apologies for the formatting... maybe we can enable the "math" discourse plugin somehow?):
Given a function $f$ with arguments $a_{i}$ and result that is assigned to variable $y$:
$$
y = f(a_{0}, ..., a_{n})
$$
All regions of non-
Sendablearguments $a_{i}$ are merged into one larger region after $f$ executes.If any of $a_{i}$ are non-
Sendableand $y$ is non-Sendable, then $y$ is in the same merged region as $a_{i}$. If all of the $a_{i}$ areSendable, then $y$ is within a new disconnected region that consists only of $y$.If $y$ is not a new variable, i.e. it's mutable, then
a) If $y$ was previously captured by reference in a closure, then the assignment to $y$ merges $y$'s new region into its old region.
b) If $y$ was not captured by reference, then $y$'s old region is forgotten.
Recall that the apply instruction in our scenario looks like:
%11 = apply %10() : $@callee_guaranteed () -> @owned NS
If we interpret the callee (i.e. %10) as the function f here, then by point 2., since it has no arguments and it produces a non-Sendable value, its result should be disconnected. But is that reasoning actually correct in this case? If we instead interpret f to be the apply instruction itself, then %10 is one of its arguments, which is ultimately the result of the original partial_apply (after a couple intermediate copies/assignments). So it would seem to me that this scenario should actually hit the first case in point 2. where the result gets merged into the same region as the arguments – i.e. tracked values %%7 and %%6 should wind up in the same region after the closure is invoked.
However, if we look at the dataflow logging, we see that does not occur:
Applying: assign_fresh %%7: %11 = apply %10() : $@callee_guaranteed () -> @owned NS // user: %13
Before: [(2 3 5 6)]
After: [(2 3 5 6)(7)]
And ultimately, when the closure's return value is eventually sent, the fact that %%7 was given its own region prevents the subsequent use-after-send from being detected:
// 👇 sent here
Applying: send %%7: %13 = apply %12(%11) : $@convention(thin) (@sil_sending @owned NS) -> ()
Before: [(2 3 5 6)(7)]
After: [(2 3 5 6){7}]
Applying: require %%3: %15 = class_method %3 : $NS, #NS.use : (NS) -> () -> (), $@convention(method) (@guaranteed NS) -> () // user: %16
Before: [(2 3 5 6){7}]
After: [(2 3 5 6){7}]
Applying: assign_direct %%10 = %%3: %15 = class_method %3 : $NS, #NS.use : (NS) -> () -> (), $@convention(method) (@guaranteed NS) -> () // user: %16
Before: [(2 3 5 6){7}]
After: [(2 3 5 6 10){7}]
Applying: require %%10: %16 = apply %15(%3) : $@convention(method) (@guaranteed NS) -> ()
Before: [(2 3 5 6 10){7}]
After: [(2 3 5 6 10){7}]
// 👇 should catch use-after-send here
Applying: require %%3: %16 = apply %15(%3) : $@convention(method) (@guaranteed NS) -> ()
Before: [(2 3 5 6 10){7}]
After: [(2 3 5 6 10){7}]
In terms of solutions to this problem... the simplest thing that comes to mind is changing the logic here in translateNonIsolationCrossingSILApply to somehow merge the callee operand in cases where that seems appropriate, rather than simply requiring that it not have been sent. I've tried a couple variations of this locally and while it appears to resolve this issue, it may have some undesirable fallout (some tests fail for reasons I've not yet determined), so open to other ideas.
Relevant bug report: [RBI] Capturing and returning non-Sendable values in closures can cause data races · Issue #87918 · swiftlang/swift · GitHub
Compiler explorer: Compiler Explorer