Skip to content

Commit 230a3a9

Browse files
committed
[rbi] Add a special diagnostic for when emitting error for a non-Sendable mutable box escaping from one concurrency domaint to another.
I am doing general work in this area to fix a larger bug. As I brought up tests for the new larger bug, I noticed some small improvements that I could make at the same time. In this case, I am porting a diagnostic that already exists for sending to closures to have a similar form for isolated closures. The diagnostic occurs when we error on escaping a non-Sendable box that contains a Sendable var. We would emit an error saying that 'x' (the Sendable value) is isolated in some way and that is why we are emitting an error. This is of course incorrect since we are erroring due to the mutable box that contains the sendable type being mutatable from multiple isolation domains. To see this in action, we used to emit the following diagnostic in this case: ```swift func testMutableNoncopyableSendableStructWithEscapingMainActorAsync() { var x = NoncopyableStructSendable() x = NoncopyableStructSendable() let _ = { escapingAsyncUse { @mainactor in useValue(x) // expected-error {{sending 'x' risks causing data races}} // expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}} } } } ``` After this change, we emit the following diagnostic which emphasizes the mutability of 'x' and how it is referenceable from multiple concurrency domains. ``` test.swift:48:7: error: sending 'x' risks causing data races [#SendingRisksDataRace] 46 | let _ = { 47 | escapingAsyncUse { @mainactor in 48 | useValue(x) | |- error: sending 'x' risks causing data races [#SendingRisksDataRace] | `- note: main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task 59 | } ``` rdar://166417084
1 parent 6b3e561 commit 230a3a9

File tree

3 files changed

+50
-9
lines changed

3 files changed

+50
-9
lines changed

include/swift/AST/DiagnosticsSIL.def

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,6 +1056,8 @@ NOTE(regionbasedisolation_named_value_used_after_explicit_sending, none,
10561056
NOTE(regionbasedisolation_named_isolated_closure_yields_race, none,
10571057
"%select{%1 |}0%2 is captured by a %3 closure. %3 uses in closure may race against later %4 uses",
10581058
(bool, StringRef, Identifier, StringRef, StringRef))
1059+
NOTE(regionbasedisolation_named_isolated_closure_yields_race_mutable, none,
1060+
"%0 closure captures reference to mutable %1 which remains modifiable by %select{%3 code|code in the current task}2", (StringRef, Identifier, bool, StringRef))
10591061

10601062
NOTE(regionbasedisolation_typed_use_after_sending, none,
10611063
"Passing value of non-Sendable type %0 as a 'sending' argument risks causing races in between local and caller code",

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1668,6 +1668,24 @@ class SendNeverSentDiagnosticEmitter {
16681668
callerIsolationStr);
16691669
}
16701670

1671+
void
1672+
emitNamedFunctionArgumentClosureMutable(SILLocation loc, Identifier name,
1673+
ApplyIsolationCrossing crossing) {
1674+
emitNamedOnlyError(loc, name);
1675+
1676+
auto calleeIsolationStr =
1677+
SILIsolationInfo::printActorIsolationForDiagnostics(
1678+
getFunction(), crossing.getCalleeIsolation());
1679+
auto callerIsolationStr =
1680+
SILIsolationInfo::printActorIsolationForDiagnostics(
1681+
getFunction(), crossing.getCallerIsolation());
1682+
diagnoseNote(
1683+
loc,
1684+
diag::regionbasedisolation_named_isolated_closure_yields_race_mutable,
1685+
calleeIsolationStr, name, getIsolationRegionInfo()->isTaskIsolated(),
1686+
callerIsolationStr);
1687+
}
1688+
16711689
void emitTypedSendingNeverSendableToSendingParam(SILLocation loc,
16721690
Type inferredType) {
16731691
diagnoseError(loc, diag::regionbasedisolation_type_send_yields_race,
@@ -2104,6 +2122,27 @@ bool SentNeverSendableDiagnosticInferrer::initForIsolatedPartialApply(
21042122
// going to be returning some form of a SILFunctionArgument which is always
21052123
// easy to find a name for.
21062124
if (auto rootValueAndName = inferNameAndRootHelper(op->get())) {
2125+
// See if our underlying value is a SILBox type that contains a Sendable
2126+
// type. In that case, we could have only emitted this error if we were
2127+
// escaping the box itself. In such a case, we want to emit a better
2128+
// diagnostic that mentions that the reason we are emitting an error is b/c
2129+
// the value is mutable.
2130+
if (auto boxTy = op->get()->getType().getAs<SILBoxType>();
2131+
boxTy && boxTy->getLayout()->isMutable() &&
2132+
llvm::all_of(boxTy->getLayout()->getFields(),
2133+
[&op](const SILField &field) -> bool {
2134+
auto fieldTy = field.getAddressType();
2135+
if (fieldTy.hasTypeParameter())
2136+
fieldTy =
2137+
op->getFunction()->mapTypeIntoEnvironment(fieldTy);
2138+
return SILIsolationInfo::isSendableType(
2139+
fieldTy, op->getFunction());
2140+
})) {
2141+
diagnosticEmitter.emitNamedFunctionArgumentClosureMutable(
2142+
diagnosticOp->getUser()->getLoc(), rootValueAndName->first, crossing);
2143+
return true;
2144+
}
2145+
21072146
diagnosticEmitter.emitNamedFunctionArgumentClosure(
21082147
diagnosticOp->getUser()->getLoc(), rootValueAndName->first, crossing);
21092148
return true;

test/Concurrency/transfernonsendable_closure_captures.swift

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func testMutableCopyableSendableStructWithEscapingMainActorAsync() {
5050
let _ = {
5151
escapingAsyncUse { @MainActor in
5252
useValue(x) // expected-error {{sending 'x' risks causing data races}}
53-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
53+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
5454
}
5555
}
5656
}
@@ -142,7 +142,7 @@ func testMutableNoncopyableSendableStructWithEscapingMainActorAsync() {
142142
let _ = {
143143
escapingAsyncUse { @MainActor in
144144
useValue(x) // expected-error {{sending 'x' risks causing data races}}
145-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
145+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
146146
}
147147
}
148148
}
@@ -191,7 +191,7 @@ func testNoncopyableNonsendableStructWithNonescapingMainActorAsync() {
191191
let x = NoncopyableStructNonsendable()
192192
let _ = {
193193
nonescapingAsyncUse { @MainActor in
194-
useValue(x) // expected-error {{sending 'x' risks causing data races}}
194+
useValue(x) // expected-error {{sending 'x' risks causing data races}}
195195
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
196196
}
197197
}
@@ -382,7 +382,7 @@ func testCopyableSendableClassWithEscapingMainActorAsyncWeakCapture() {
382382
let _ = { [weak x] in
383383
escapingAsyncUse { @MainActor in
384384
useValue(x) // expected-error {{sending 'x' risks causing data races}}
385-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
385+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
386386
}
387387
}
388388
}
@@ -393,7 +393,7 @@ func testMutableCopyableSendableClassWithEscapingMainActorAsyncWeakCapture() {
393393
let _ = { [weak x] in
394394
escapingAsyncUse { @MainActor in
395395
useValue(x) // expected-error {{sending 'x' risks causing data races}}
396-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
396+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
397397
}
398398
}
399399
}
@@ -424,7 +424,7 @@ func testCopyableSendableClassWithNonescapingMainActorAsyncWeakCapture() {
424424
let _ = { [weak x] in
425425
nonescapingAsyncUse { @MainActor in
426426
useValue(x) // expected-error {{sending 'x' risks causing data races}}
427-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
427+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
428428
}
429429
}
430430
}
@@ -435,7 +435,7 @@ func testMutableCopyableSendableClassWithNonescapingMainActorAsyncWeakCapture()
435435
let _ = { [weak x] in
436436
nonescapingAsyncUse { @MainActor in
437437
useValue(x) // expected-error {{sending 'x' risks causing data races}}
438-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
438+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
439439
}
440440
}
441441
}
@@ -556,7 +556,7 @@ func testGenericSendableWithEscapingMainActorAsync<T : ~Copyable>(_ value: consu
556556
let _ = {
557557
escapingAsyncUse { @MainActor in
558558
useValue(x) // expected-error {{sending 'x' risks causing data races}}
559-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
559+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
560560
}
561561
}
562562
}
@@ -569,7 +569,7 @@ func testMutableGenericSendableWithEscapingMainActorAsync<T : ~Copyable>(
569569
let _ = {
570570
escapingAsyncUse { @MainActor in
571571
useValue(x) // expected-error {{sending 'x' risks causing data races}}
572-
// expected-note @-1 {{task-isolated 'x' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
572+
// expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
573573
}
574574
}
575575
}

0 commit comments

Comments
 (0)