Merge pull request #81571 from gottesmm/pr-d7a7ebea7af14e2f9f1ce266febd0c983e3fc888

[rbi] Treat a partial_apply as nonisolated(unsafe) if all of its captures are nonisolated(unsafe).

Author: gottesmm

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -2503,7 +2503,8 @@ class PartitionOpTranslator {
       }
     }
 
-    if (auto isolationRegionInfo = SILIsolationInfo::get(pai)) {
+    if (auto isolationRegionInfo = SILIsolationInfo::get(pai);
+        isolationRegionInfo && !isolationRegionInfo.isDisconnected()) {
       return translateIsolatedPartialApply(pai, isolationRegionInfo);
     }
 

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

@@ -357,6 +357,69 @@ inferIsolationInfoForTempAllocStack(AllocStackInst *asi) {
   return SILIsolationInfo::get(targetOperand->getUser());
 }
 
+static SILValue lookThroughNonVarDeclOwnershipInsts(SILValue v) {
+  while (true) {
+    if (auto *svi = dyn_cast<SingleValueInstruction>(v)) {
+      if (isa<CopyValueInst>(svi)) {
+        v = svi->getOperand(0);
+        continue;
+      }
+
+      if (auto *bbi = dyn_cast<BeginBorrowInst>(v)) {
+        if (!bbi->isFromVarDecl()) {
+          v = bbi->getOperand();
+          continue;
+        }
+        return v;
+      }
+
+      if (auto *mvi = dyn_cast<MoveValueInst>(v)) {
+        if (!mvi->isFromVarDecl()) {
+          v = mvi->getOperand();
+          continue;
+        }
+
+        return v;
+      }
+    }
+
+    return v;
+  }
+}
+
+/// See if \p pai has at least one nonisolated(unsafe) capture and that all
+/// captures are either nonisolated(unsafe) or sendable.
+static bool isPartialApplyNonisolatedUnsafe(PartialApplyInst *pai) {
+  bool foundOneNonIsolatedUnsafe = false;
+  for (auto &op : pai->getArgumentOperands()) {
+    if (SILIsolationInfo::isSendableType(op.get()))
+      continue;
+
+    // Normally we would not look through copy_value, begin_borrow, or
+    // move_value since this is meant to find the inherent isolation of a
+    // specific element. But since we are checking for captures rather
+    // than the actual value itself (just for unsafe nonisolated
+    // purposes), it is ok.
+    //
+    // E.x.: As an example of something we want to prevent, consider an
+    // invocation of a nonisolated function that is a parameter to an
+    // @MainActor function. That means from a region isolation
+    // perspective, the function parameter is in the MainActor region, but
+    // the actual function itself is not MainActor isolated, since the
+    // function will not hop onto the main actor.
+    //
+    // TODO: We should use some of the shared infrastructure to find the
+    // underlying value of op.get(). This is conservatively correct for
+    // now.
+    auto value = lookThroughNonVarDeclOwnershipInsts(op.get());
+    if (!SILIsolationInfo::get(value).isUnsafeNonIsolated())
+      return false;
+    foundOneNonIsolatedUnsafe = true;
+  }
+
+  return foundOneNonIsolatedUnsafe;
+}
+
 SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
   if (auto fas = FullApplySite::isa(inst)) {
     // Before we do anything, see if we have a sending result. In such a case,
@@ -436,12 +499,23 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
   }
 
   if (auto *pai = dyn_cast<PartialApplyInst>(inst)) {
+    // Check if we have any captures and if the isolation info for all captures
+    // are nonisolated(unsafe) or Sendable. In such a case, we consider the
+    // partial_apply to be nonisolated(unsafe). We purposely do not do this if
+    // the partial_apply does not have any parameters just out of paranoia... we
+    // shouldn't have such a partial_apply emitted by SILGen (it should use a
+    // thin to thick function or something like that)... but in that case since
+    // we do not have any nonisolated(unsafe), it doesn't make sense to
+    // propagate nonisolated(unsafe).
+    bool partialApplyIsNonIsolatedUnsafe = isPartialApplyNonisolatedUnsafe(pai);
+
     if (auto *ace = pai->getLoc().getAsASTNode<AbstractClosureExpr>()) {
       auto actorIsolation = ace->getActorIsolation();
 
       if (actorIsolation.isGlobalActor()) {
         return SILIsolationInfo::getGlobalActorIsolated(
-            pai, actorIsolation.getGlobalActor());
+                   pai, actorIsolation.getGlobalActor())
+            .withUnsafeNonIsolated(partialApplyIsNonIsolatedUnsafe);
       }
 
       if (actorIsolation.isActorInstanceIsolated()) {
@@ -465,13 +539,16 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
             if (auto *fArg = dyn_cast<SILFunctionArgument>(
                     actualIsolatedValue.getValue())) {
               if (auto info =
-                      SILIsolationInfo::getActorInstanceIsolated(pai, fArg))
+                      SILIsolationInfo::getActorInstanceIsolated(pai, fArg)
+                          .withUnsafeNonIsolated(
+                              partialApplyIsNonIsolatedUnsafe))
                 return info;
             }
           }
 
           return SILIsolationInfo::getActorInstanceIsolated(
-              pai, actorInstance, actorIsolation.getActor());
+                     pai, actorInstance, actorIsolation.getActor())
+              .withUnsafeNonIsolated(partialApplyIsNonIsolatedUnsafe);
         }
 
         // For now, if we do not have an actor instance, just create an actor
@@ -485,12 +562,16 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
         //
         // TODO: How do we want to resolve this.
         return SILIsolationInfo::getPartialApplyActorInstanceIsolated(
-            pai, actorIsolation.getActor());
+                   pai, actorIsolation.getActor())
+            .withUnsafeNonIsolated(partialApplyIsNonIsolatedUnsafe);
       }
 
       assert(actorIsolation.getKind() != ActorIsolation::Erased &&
              "Implement this!");
     }
+
+    if (partialApplyIsNonIsolatedUnsafe)
+      return SILIsolationInfo::getDisconnected(partialApplyIsNonIsolatedUnsafe);
   }
 
   // See if the memory base is a ref_element_addr from an address. If so, add

test/Concurrency/transfernonsendable_nonisolatedunsafe.swift

@@ -10,7 +10,7 @@
 // MARK: Declarations //
 ////////////////////////
 
-class NonSendableKlass { // expected-complete-note 98{{}}
+class NonSendableKlass {
   var field: NonSendableKlass? = nil
 }
 
@@ -821,3 +821,209 @@ actor ActorContainingSendableStruct {
 }
 
 
+////////////////////
+// MARK: Closures //
+////////////////////
+
+func closureTests() async {
+  func sendingClosure(_ x: sending () -> ()) {
+  }
+
+  func testLetOneNSVariableError() async {
+    let x = NonSendableKlass()
+    sendingClosure { _ = x } // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+    // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+    sendingClosure { _ = x } // expected-note {{access can happen concurrently}}
+  }
+
+  func testLetNonIsolatedUnsafeNSVariableNoError() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    sendingClosure { _ = x }
+    sendingClosure { _ = x }
+  }
+
+  func testLetOneNSVariableSVariableError() async {
+    let x = NonSendableKlass()
+    let y = CustomActorInstance()
+    sendingClosure { // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+      // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+      _ = x
+      _ = y
+    }
+    sendingClosure { // expected-note {{access can happen concurrently}}
+      _ = x
+      _ = y
+    }
+  }
+
+  func testLetNonIsolatedUnsafeNSSVariableNoError() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    let y = CustomActorInstance()
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+  }
+
+  func testLetTwoNSVariableError() async {
+    let x = NonSendableKlass()
+    let y = NonSendableKlass()
+    sendingClosure { // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+      // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+      _ = x
+      _ = y
+    }
+    sendingClosure { // expected-note {{access can happen concurrently}}
+      _ = x
+      _ = y
+    }
+  }
+
+  func testLetTwoNSVariableError2() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    let y = NonSendableKlass()
+    sendingClosure { // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+      // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+      _ = x
+      _ = y
+    }
+    sendingClosure { // expected-note {{access can happen concurrently}}
+      _ = x
+      _ = y
+    }
+  }
+
+  func testLetTwoNSVariableError3() async {
+    nonisolated(unsafe) let x = NonSendableKlass()
+    nonisolated(unsafe) let y = NonSendableKlass()
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+  }
+
+  func testVarOneNSVariableError() async {
+    var x = NonSendableKlass()
+    x = NonSendableKlass()
+
+    sendingClosure { _ = x } // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+    // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+    sendingClosure { _ = x } // expected-note {{access can happen concurrently}}
+  }
+
+  func testVarNonIsolatedUnsafeNSVariableNoError() async {
+    nonisolated(unsafe) var x = NonSendableKlass()
+    x = NonSendableKlass()
+
+    sendingClosure { _ = x }
+    sendingClosure { _ = x }
+  }
+
+  func testVarOneNSVariableSVariableError() async {
+    var x = NonSendableKlass()
+    x = NonSendableKlass()
+    var y = CustomActorInstance()
+    y = CustomActorInstance()
+    sendingClosure { // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+      // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+      _ = x
+      _ = y
+    }
+    sendingClosure { // expected-note {{access can happen concurrently}}
+      _ = x
+      _ = y
+    }
+  }
+
+  func testVarNonIsolatedUnsafeNSSVariableNoError() async {
+    nonisolated(unsafe) var x = NonSendableKlass()
+    x = NonSendableKlass()
+    var y = CustomActorInstance()
+    y = CustomActorInstance()
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+  }
+
+  func testVarTwoNSVariableError() async {
+    var x = NonSendableKlass()
+    x = NonSendableKlass()
+    var y = NonSendableKlass()
+    y = NonSendableKlass()
+    sendingClosure { // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+      // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+      _ = x
+      _ = y
+    }
+    sendingClosure { // expected-note {{access can happen concurrently}}
+      _ = x
+      _ = y
+    }
+  }
+
+  func testVarTwoNSVariableError2() async {
+    nonisolated(unsafe) var x = NonSendableKlass()
+    x = NonSendableKlass()
+    var y = NonSendableKlass()
+    y = NonSendableKlass()
+    sendingClosure { // expected-warning {{sending value of non-Sendable type '() -> ()' risks causing data races}}
+      // expected-note @-1 {{Passing value of non-Sendable type '() -> ()' as a 'sending' argument to local function 'sendingClosure' risks causing races in between local and caller code}}
+      _ = x
+      _ = y
+    }
+    sendingClosure { // expected-note {{access can happen concurrently}}
+      _ = x
+      _ = y
+    }
+  }
+
+  func testVarTwoNSVariableError3() async {
+    nonisolated(unsafe) var x = NonSendableKlass()
+    x = NonSendableKlass()
+    nonisolated(unsafe) var y = NonSendableKlass()
+    y = NonSendableKlass()
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+    sendingClosure {
+      _ = x
+      _ = y
+    }
+  }
+
+  func testWithTaskDetached() async {
+    let x1 = NonSendableKlass()
+    Task.detached { _ = x1 } // 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 static method 'detached(priority:operation:)' risks causing races in between local and caller code}}
+    Task.detached { _ = x1 } // expected-note {{access can happen concurrently}}
+
+    nonisolated(unsafe) let x2 = NonSendableKlass()
+    Task.detached { _ = x2 }
+    Task.detached { _ = x2 }
+
+    nonisolated(unsafe) let x3a = NonSendableKlass()
+    nonisolated(unsafe) let x3b = NonSendableKlass()
+    Task.detached { _ = x3a; _ = x3b }
+    Task.detached { _ = x3a; _ = x3b }
+
+    nonisolated(unsafe) let x4a = NonSendableKlass()
+    let x4b = NonSendableKlass()
+    Task.detached { _ = x4a; _ = x4b } // 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 static method 'detached(priority:operation:)' risks causing races in between local and caller code}}
+    Task.detached { _ = x4a; _ = x4b } // expected-note {{access can happen concurrently}}
+  }
+}