Merge pull request #82427 from gottesmm/pr-9d8b2e21000560a9c3a3b0143c3fb3b22a0ca75a

[rbi] Remove code that caused us to misidentify certain captured parameters as sending.

Author: gottesmm

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -538,34 +538,23 @@ static bool isAsyncLetBeginPartialApply(PartialApplyInst *pai) {
 
 /// Returns true if this is a function argument that is able to be sent in the
 /// body of our function.
+///
+/// Needs to stay in sync with SILIsolationInfo::get(SILArgument *).
 static bool canFunctionArgumentBeSent(SILFunctionArgument *arg) {
   // Indirect out parameters can never be sent.
   if (arg->isIndirectResult() || arg->isIndirectErrorResult())
     return false;
 
-  // If we have a function argument that is closure captured by a Sendable
-  // closure, allow for the argument to be sent.
-  //
-  // DISCUSSION: The reason that we do this is that in the case of us
-  // having an actual Sendable closure there are two cases we can see:
-  //
-  // 1. If we have an actual Sendable closure, the AST will emit an
-  // earlier error saying that we are capturing a non-Sendable value in a
-  // Sendable closure. So we want to squelch the error that we would emit
-  // otherwise. This only occurs when we are not in swift-6 mode since in
-  // swift-6 mode we will error on the earlier error... but in the case of
-  // us not being in swift 6 mode lets not emit extra errors.
-  //
-  // 2. If we have an async-let based Sendable closure, we want to allow
-  // for the argument to be sent in the async let's statement and
-  // not emit an error.
-  //
-  // TODO: Once the async let refactoring change this will no longer be needed
-  // since closure captures will have sending parameters and be
-  // non-Sendable.
-  if (arg->isClosureCapture() &&
-      arg->getFunction()->getLoweredFunctionType()->isSendable())
-    return true;
+  // If we have a closure capture...
+  if (arg->isClosureCapture()) {
+    // And that closure capture is from an async let, treat it as sending. This
+    // is because we allow for disconnected values to be sent into async let
+    // closures.
+    if (auto declRef = arg->getFunction()->getDeclRef();
+        declRef && declRef.isAsyncLetClosure) {
+      return true;
+    }
+  }
 
   // Otherwise, we only allow for the argument to be sent if it is explicitly
   // marked as a 'sending' parameter.

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

@@ -1020,16 +1020,36 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) {
   if (fArg->isSending())
     return SILIsolationInfo::getDisconnected(false /*nonisolated(unsafe)*/);
 
+  // Check if we have a closure captured parameter that is nonisolated(unsafe)
+  // at its original declaration sign. In such a case, we want to propagate that
+  // bit.
+  bool isClosureCapturedNonisolatedUnsafe = [&]() -> bool {
+    if (!fArg->isClosureCapture())
+      return false;
+    auto *decl = fArg->getDecl();
+    if (!decl)
+      return false;
+    auto *attr = decl->getAttrs().getAttribute<NonisolatedAttr>();
+    if (!attr)
+      return false;
+    return attr->isUnsafe();
+  }();
+
   // If we have a closure capture that is not an indirect result or indirect
   // result error, we want to treat it as sending so that we properly handle
   // async lets.
   //
   // This pattern should only come up with async lets. See comment in
-  // isTransferrableFunctionArgument.
+  // canFunctionArgumentBeSent in RegionAnalysis.cpp. This code needs to stay in
+  // sync with that code.
   if (!fArg->isIndirectResult() && !fArg->isIndirectErrorResult() &&
-      fArg->isClosureCapture() &&
-      fArg->getFunction()->getLoweredFunctionType()->isSendable())
-    return SILIsolationInfo::getDisconnected(false /*nonisolated(unsafe)*/);
+      fArg->isClosureCapture()) {
+    if (auto declRef = arg->getFunction()->getDeclRef();
+        declRef && declRef.isAsyncLetClosure) {
+      return SILIsolationInfo::getDisconnected(
+          isClosureCapturedNonisolatedUnsafe);
+    }
+  }
 
   // Before we do anything further, see if we have an isolated parameter. This
   // handles isolated self and specifically marked isolated.
@@ -1040,12 +1060,20 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) {
     if (auto funcIsolation = fArg->getFunction()->getActorIsolation();
         funcIsolation && funcIsolation->isCallerIsolationInheriting()) {
       return SILIsolationInfo::getTaskIsolated(fArg)
-          .withNonisolatedNonsendingTaskIsolated(true);
+          .withNonisolatedNonsendingTaskIsolated(true)
+          .withUnsafeNonIsolated(isClosureCapturedNonisolatedUnsafe);
     }
 
     auto astType = isolatedArg->getType().getASTType();
     if (astType->lookThroughAllOptionalTypes()->getAnyActor()) {
-      return SILIsolationInfo::getActorInstanceIsolated(fArg, isolatedArg);
+      if (auto isolation =
+              SILIsolationInfo::getActorInstanceIsolated(fArg, isolatedArg)) {
+        return isolation.withUnsafeNonIsolated(
+            isClosureCapturedNonisolatedUnsafe);
+      }
+      // If we had an isolated parameter that was an actor type, but we did not
+      // find any isolation for the actor instance... return {}.
+      return {};
     }
   }
 
@@ -1087,12 +1115,14 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) {
   }
 
   // Otherwise, if we do not have an isolated argument and are not in an
-  // allocator, then we might be isolated via global isolation.
+  // allocator, then we might be isolated via global isolation or in a global
+  // actor isolated initializer.
   if (auto functionIsolation = fArg->getFunction()->getActorIsolation()) {
     if (functionIsolation->isActorIsolated()) {
       if (functionIsolation->isGlobalActor()) {
         return SILIsolationInfo::getGlobalActorIsolated(
-            fArg, functionIsolation->getGlobalActor());
+                   fArg, functionIsolation->getGlobalActor())
+            .withUnsafeNonIsolated(isClosureCapturedNonisolatedUnsafe);
       }
 
       return SILIsolationInfo::getActorInstanceIsolated(
@@ -1101,7 +1131,8 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) {
     }
   }
 
-  return SILIsolationInfo::getTaskIsolated(fArg);
+  return SILIsolationInfo::getTaskIsolated(fArg).withUnsafeNonIsolated(
+      isClosureCapturedNonisolatedUnsafe);
 }
 
 /// Infer isolation region from the set of protocol conformances.

test/Concurrency/sendable_checking.swift

@@ -463,13 +463,15 @@ func preconcurrencyContext(_: @escaping @Sendable () -> Void) {}
 struct DowngradeForPreconcurrency {
   func capture(completion: @escaping @MainActor () -> Void) {
     preconcurrencyContext {
-      Task {
-        completion()
+        Task { // expected-tns-warning {{passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure}}
+            completion() // expected-tns-note {{closure captures 'completion' which is accessible to code in the current task}}
         // expected-warning@-1 {{capture of 'completion' with non-Sendable type '@MainActor () -> Void' in a '@Sendable' closure}}
         // expected-warning@-2 {{capture of 'completion' with non-Sendable type '@MainActor () -> Void' in an isolated closure}}
         // expected-note@-3 2 {{a function type must be marked '@Sendable' to conform to 'Sendable'}}
         // expected-warning@-4 {{expression is 'async' but is not marked with 'await'; this is an error in the Swift 6 language mode}}
         // expected-note@-5 {{calls to parameter 'completion' from outside of its actor context are implicitly asynchronous}}
+        // expected-complete-and-tns-warning @-7 {{passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure}}
+        // expected-complete-and-tns-note @-7 {{closure captures 'completion' which is accessible to code in the current task}}
       }
     }
   }

test/Concurrency/transfernonsendable.swift

@@ -1671,13 +1671,12 @@ extension MyActor {
         _ = self
         _ = sc
 
-        Task { // expected-warning {{sending value of non-Sendable type '() async -> ()' risks causing data races}}
-          // expected-note @-1 {{Passing value of non-Sendable type '() async -> ()' as a 'sending' argument to initializer 'init(name:priority:operation:)' risks causing races in between local and caller code}}
-          _ = sc
+        Task { // expected-warning {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
+          _ = sc // expected-note {{closure captures 'self'-isolated 'sc'}}
         }
 
-        Task { // expected-note {{access can happen concurrently}}
-          _ = sc
+        Task { // expected-warning {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
+          _ = sc // expected-note {{closure captures 'self'-isolated 'sc'}}
         }
       }
     }
@@ -2001,3 +2000,22 @@ nonisolated(nonsending) func testCallNonisolatedNonsending(_ x: NonSendableKlass
   // expected-ni-note @-1 {{sending nonisolated(nonsending) task-isolated 'x' to nonisolated global function 'useValueAsyncConcurrent' risks causing data races between nonisolated and nonisolated(nonsending) task-isolated uses}}
   // expected-ni-ns-note @-2 {{sending task-isolated 'x' to @concurrent global function 'useValueAsyncConcurrent' risks causing data races between @concurrent and task-isolated uses}}
 }
+
+func avoidThinkingClosureParameterIsSending() {
+  @MainActor
+  final class Foo {
+    let value = NonSendableKlass()
+
+    func perform() {
+        Task { [value] in
+          await value.asyncCall() // expected-ni-warning {{sending 'value' risks causing data races}}
+          // expected-ni-note @-1 {{sending main actor-isolated 'value' to nonisolated instance method 'asyncCall()' risks causing data races between nonisolated and main actor-isolated uses}}
+        }
+
+        Task {
+          await value.asyncCall() // expected-ni-warning {{sending 'self.value' risks causing data races}}
+          // expected-ni-note @-1 {{sending main actor-isolated 'self.value' to nonisolated instance method 'asyncCall()' risks causing data races between nonisolated and main actor-isolated uses}}
+        }
+    }
+  }
+}

test/Concurrency/transfernonsendable_nonisolatedunsafe.swift

@@ -823,9 +823,9 @@ actor ActorContainingSendableStruct {
 }
 
 
-////////////////////
-// MARK: Closures //
-////////////////////
+////////////////////////////
+// MARK: Sending Closures //
+////////////////////////////
 
 func closureTests() async {
   func sendingClosure(_ x: sending () -> ()) {
@@ -1043,3 +1043,64 @@ func closureTests() async {
     sendingClosure(y) // expected-note {{access can happen concurrently}}
   }
 }
+
+///////////////////////////////////////
+// MARK: Closure Capture Propagation //
+///////////////////////////////////////
+//
+// DISCUSSION: For tests that involve closure captures with nonisolated(unsafe)
+// and suppressing errors in the closure itself. Tests where the suppressed
+// error is outside the closure itself are in other places in the file.
+
+enum NonisolatedUnsafeCapture {
+  func testSimple() {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    let _ = {
+      await transferToMainDirect(x)
+      print(x)
+    }
+  }
+
+  func testAsyncLet() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    async let y: () = {
+      await transferToMainDirect(x)
+      await transferToMainDirect(x)
+    }()
+    _ = await y
+  }
+
+  func testIsolatedParamViaActor() async {
+    actor A {
+      let y = NonSendableKlass()
+      func test() {
+        nonisolated(unsafe) let x = y
+        _ = {
+          await transferToMainDirect(x)
+        }
+      }
+    }
+  }
+
+  func testExplicitIsolatedParam() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    _ = { (y: isolated CustomActorInstance) in
+      await transferToMainDirect(x)
+    }
+  }
+
+  func testIsolatedParamNonisolatedNonSending() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    let _: nonisolated(nonsending) () async -> () = {
+      await transferToMainDirect(x)
+      await transferToMainDirect(x)
+    }
+  }
+
+  func testGlobalActorIsolated() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    _ = { @MainActor in
+      await transferToCustom(x)
+    }
+  }
+}

test/Concurrency/transfernonsendable_typed_errors.swift

@@ -61,13 +61,12 @@ extension MyActor {
         _ = self
         _ = sc
 
-        Task { // expected-error {{sending value of non-Sendable type '() async -> ()' risks causing data races}}
-          // expected-note @-1 {{Passing value of non-Sendable type '() async -> ()' as a 'sending' argument to initializer 'init(name:priority:operation:)' risks causing races in between local and caller code}}
-          _ = sc
+        Task { // expected-error {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
+          _ = sc // expected-note {{closure captures 'self'-isolated 'sc'}}
         }
 
-        Task { // expected-note {{access can happen concurrently}}
-          _ = sc
+        Task { // expected-error {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
+          _ = sc // expected-note {{closure captures 'self'-isolated 'sc'}}
         }
       }
     }