Merge pull request #86151 from gottesmm/pr-9798df40033186940bfad19e0ff54d20221159e7

[rbi] Add a special diagnostic for when emitting error for a non-Sendable mutable box escaping from one concurrency domaint to another.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -1056,6 +1056,8 @@ NOTE(regionbasedisolation_named_value_used_after_explicit_sending, none,
 NOTE(regionbasedisolation_named_isolated_closure_yields_race, none,
       "%select{%1 |}0%2 is captured by a %3 closure. %3 uses in closure may race against later %4 uses",
       (bool, StringRef, Identifier, StringRef, StringRef))
+NOTE(regionbasedisolation_named_isolated_closure_yields_race_mutable, none,
+     "%0 closure captures reference to mutable %1 which remains modifiable by %select{%3 code|code in the current task}2", (StringRef, Identifier, bool, StringRef))
 
 NOTE(regionbasedisolation_typed_use_after_sending, none,
       "Passing value of non-Sendable type %0 as a 'sending' argument risks causing races in between local and caller code",
@@ -1096,7 +1098,7 @@ NOTE(regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value_
      "closure captures %0 which is accessible to code in the current task",
      (DeclName))
 NOTE(regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_boxed_value_task_isolated, none,
-     "closure captures reference to mutable %kind0 which is accessible to code in the current task",
+     "closure captures reference to mutable %kind0 which remains modifiable by code in the current task",
      (const ValueDecl *))
 NOTE(regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value_region, none,
      "closure captures %1 which is accessible to %0 code",

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

@@ -1668,6 +1668,24 @@ class SendNeverSentDiagnosticEmitter {
                  callerIsolationStr);
   }
 
+  void
+  emitNamedFunctionArgumentClosureMutable(SILLocation loc, Identifier name,
+                                          ApplyIsolationCrossing crossing) {
+    emitNamedOnlyError(loc, name);
+
+    auto calleeIsolationStr =
+        SILIsolationInfo::printActorIsolationForDiagnostics(
+            getFunction(), crossing.getCalleeIsolation());
+    auto callerIsolationStr =
+        SILIsolationInfo::printActorIsolationForDiagnostics(
+            getFunction(), crossing.getCallerIsolation());
+    diagnoseNote(
+        loc,
+        diag::regionbasedisolation_named_isolated_closure_yields_race_mutable,
+        calleeIsolationStr, name, getIsolationRegionInfo()->isTaskIsolated(),
+        callerIsolationStr);
+  }
+
   void emitTypedSendingNeverSendableToSendingParam(SILLocation loc,
                                                    Type inferredType) {
     diagnoseError(loc, diag::regionbasedisolation_type_send_yields_race,
@@ -2104,6 +2122,27 @@ bool SentNeverSendableDiagnosticInferrer::initForIsolatedPartialApply(
   // going to be returning some form of a SILFunctionArgument which is always
   // easy to find a name for.
   if (auto rootValueAndName = inferNameAndRootHelper(op->get())) {
+    // See if our underlying value is a SILBox type that contains a Sendable
+    // type. In that case, we could have only emitted this error if we were
+    // escaping the box itself. In such a case, we want to emit a better
+    // diagnostic that mentions that the reason we are emitting an error is b/c
+    // the value is mutable.
+    if (auto boxTy = op->get()->getType().getAs<SILBoxType>();
+        boxTy && boxTy->getLayout()->isMutable() &&
+        llvm::all_of(boxTy->getLayout()->getFields(),
+                     [&op](const SILField &field) -> bool {
+                       auto fieldTy = field.getAddressType();
+                       if (fieldTy.hasTypeParameter())
+                         fieldTy =
+                             op->getFunction()->mapTypeIntoEnvironment(fieldTy);
+                       return SILIsolationInfo::isSendableType(
+                           fieldTy, op->getFunction());
+                     })) {
+      diagnosticEmitter.emitNamedFunctionArgumentClosureMutable(
+          diagnosticOp->getUser()->getLoc(), rootValueAndName->first, crossing);
+      return true;
+    }
+
     diagnosticEmitter.emitNamedFunctionArgumentClosure(
         diagnosticOp->getUser()->getLoc(), rootValueAndName->first, crossing);
     return true;

test/Concurrency/transfernonsendable.swift

@@ -1794,7 +1794,7 @@ extension MyActor {
 func nonSendableAllocBoxConsumingParameter(x: consuming SendableKlass) async throws {
   try await withThrowingTaskGroup(of: Void.self) { group in
     group.addTask { // expected-warning {{passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure}}
-      useValue(x) // expected-note {{closure captures reference to mutable parameter 'x' which is accessible to code in the current task}}
+      useValue(x) // expected-note {{closure captures reference to mutable parameter 'x' which remains modifiable by code in the current task}}
     }
 
     try await group.waitForAll()
@@ -1807,7 +1807,7 @@ func nonSendableAllocBoxConsumingVar() async throws {
 
   try await withThrowingTaskGroup(of: Void.self) { group in
     group.addTask { // expected-warning {{passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure}}
-      useValue(x) // expected-note {{closure captures reference to mutable var 'x' which is accessible to code in the current task}}
+      useValue(x) // expected-note {{closure captures reference to mutable var 'x' which remains modifiable by code in the current task}}
     }
 
     try await group.waitForAll()
@@ -1865,7 +1865,7 @@ func testFunctionIsNotEmpty(input: SendableKlass) async throws {
   var result: [SendableKlass] = []
   try await withThrowingTaskGroup(of: Void.self) { taskGroup in // expected-warning {{no calls to throwing functions occur within 'try' expression}}
     taskGroup.addTask { // expected-warning {{passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure}}
-      result.append(input) // expected-note {{closure captures reference to mutable var 'result' which is accessible to code in the current task}}
+      result.append(input) // expected-note {{closure captures reference to mutable var 'result' which remains modifiable by code in the current task}}
     }
   }
 }

test/Concurrency/transfernonsendable_closure_captures.swift

@@ -50,7 +50,7 @@ func testMutableCopyableSendableStructWithEscapingMainActorAsync() {
   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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }
@@ -142,7 +142,7 @@ func testMutableNoncopyableSendableStructWithEscapingMainActorAsync() {
   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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }
@@ -191,7 +191,7 @@ func testNoncopyableNonsendableStructWithNonescapingMainActorAsync() {
   let x = NoncopyableStructNonsendable()
   let _ = {
     nonescapingAsyncUse { @MainActor in
-      useValue(x)  // expected-error {{sending 'x' risks causing data races}}
+      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}}
     }
   }
@@ -382,7 +382,7 @@ func testCopyableSendableClassWithEscapingMainActorAsyncWeakCapture() {
   let _ = { [weak x] in
     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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }
@@ -393,7 +393,7 @@ func testMutableCopyableSendableClassWithEscapingMainActorAsyncWeakCapture() {
   let _ = { [weak x] in
     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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }
@@ -424,7 +424,7 @@ func testCopyableSendableClassWithNonescapingMainActorAsyncWeakCapture() {
   let _ = { [weak x] in
     nonescapingAsyncUse { @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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }
@@ -435,7 +435,7 @@ func testMutableCopyableSendableClassWithNonescapingMainActorAsyncWeakCapture()
   let _ = { [weak x] in
     nonescapingAsyncUse { @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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }
@@ -556,7 +556,7 @@ func testGenericSendableWithEscapingMainActorAsync<T : ~Copyable>(_ value: consu
   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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }
@@ -569,7 +569,7 @@ func testMutableGenericSendableWithEscapingMainActorAsync<T : ~Copyable>(
   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}}
+      // expected-note @-1 {{main actor-isolated closure captures reference to mutable 'x' which remains modifiable by code in the current task}}
     }
   }
 }