[region-isolation] Always require before transferring.

We purposely do not treat a PartitionOpKind::Transfer as a require since that
would cause us to error when we weakly transfer the same parameter multiple
times to the same function. This is safe since all of the function parameters
will be in the same region.

To ensure that this fixed the multiple strong transferring issue (which is
exposed by requiring before transferring), I also had to muck around a little
with how we emit errors to ensure that we emit errors if the transfer
instruction is also the require.

Author: gottesmm

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1625,6 +1625,7 @@ class PartitionOpTranslator {
       if (auto value = tryToTrackValue(op.get())) {
         // If we are tracking it, transfer it and if it is actor derived, mark
         // our partial apply as actor derived.
+        builder.addRequire(value->getRepresentative().getValue());
         builder.addTransfer(value->getRepresentative().getValue(), &op);
       }
     }
@@ -1731,6 +1732,7 @@ class PartitionOpTranslator {
             fas.getArgumentParameterInfo(op).hasOption(
                 SILParameterInfo::Transferring)) {
           if (auto value = tryToTrackValue(op.get())) {
+            builder.addRequire(value->getRepresentative().getValue());
             builder.addTransfer(value->getRepresentative().getValue(), &op);
           }
         } else {
@@ -1747,6 +1749,7 @@ class PartitionOpTranslator {
       if (fas.getArgumentParameterInfo(selfOperand)
               .hasOption(SILParameterInfo::Transferring)) {
         if (auto value = tryToTrackValue(selfOperand.get())) {
+          builder.addRequire(value->getRepresentative().getValue());
           builder.addTransfer(value->getRepresentative().getValue(),
                               &selfOperand);
         }
@@ -1816,7 +1819,7 @@ class PartitionOpTranslator {
     assert((sourceApply || bool(applySite.getIsolationCrossing())) &&
            "only ApplyExpr's should cross isolation domains");
 
-    // require all operands
+    // Require all operands first before we emit transferring.
     for (auto op : applySite.getArguments())
       if (auto value = tryToTrackValue(op))
         builder.addRequire(value->getRepresentative().getValue());
@@ -1938,6 +1941,7 @@ class PartitionOpTranslator {
     // Transfer src. This ensures that we cannot use src again locally in this
     // function... which makes sense since its value is now in the transferring
     // parameter.
+    builder.addRequire(srcRoot.getRepresentative().getValue());
     builder.addTransfer(srcRoot.getRepresentative().getValue(), srcOperand);
 
     // Then check if we are assigning into an aggregate projection. In such a

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -214,7 +214,10 @@ void RequireLiveness::processDefBlock() {
 
   // Then walk from our transferInst to the end of the block looking for the
   // first require inst. Once we find it... return.
-  for (auto ii = std::next(transferInst->getIterator()),
+  //
+  // NOTE: We start walking at the transferInst since the transferInst could use
+  // the requireInst as well.
+  for (auto ii = transferInst->getIterator(),
             ie = transferInst->getParent()->end();
        ii != ie; ++ii) {
     if (!allRequires.contains(&*ii))
@@ -1083,11 +1086,6 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
                << "Transfer Op. Number: " << transferOp->getOperandNumber()
                << ". User: " << *transferOp->getUser());
 
-    RequireLiveness liveness(blockLivenessInfoGeneration, transferOp,
-                             blockLivenessInfo);
-    ++blockLivenessInfoGeneration;
-    liveness.process(requireInsts);
-
     UseAfterTransferDiagnosticInferrer diagnosticInferrer(
         regionInfo->getValueMap());
     diagnosticInferrer.init(transferOp);
@@ -1104,6 +1102,40 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
       continue;
     }
 
+    // Then look for our requires before we emit any error. We want to emit a
+    // single we don't understand error if we do not find the require.
+    bool didEmitRequireNote = false;
+    InstructionSet requireInstsUnique(function);
+    RequireLiveness liveness(blockLivenessInfoGeneration, transferOp,
+                             blockLivenessInfo);
+    ++blockLivenessInfoGeneration;
+    liveness.process(requireInsts);
+
+    SmallVector<SILInstruction *, 8> requireInstsForError;
+    for (auto *require : requireInsts) {
+      // We can have multiple of the same require insts if we had a require
+      // and an assign from the same instruction. Our liveness checking
+      // above doesn't care about that, but we still need to make sure we do
+      // not emit twice.
+      if (!requireInstsUnique.insert(require))
+        continue;
+
+      // If this was not a last require, do not emit an error.
+      if (!liveness.finalRequires.contains(require))
+        continue;
+
+      requireInstsForError.push_back(require);
+      didEmitRequireNote = true;
+    }
+
+    // If we did not emit a require, emit an "unknown pattern" error that
+    // tells the user to file a bug. This importantly ensures that we can
+    // guarantee that we always find the require if we successfully compile.
+    if (!didEmitRequireNote) {
+      diagnoseError(transferOp, diag::regionbasedisolation_unknown_pattern);
+      continue;
+    }
+
     using UseDiagnosticInfoKind =
         UseAfterTransferDiagnosticInferrer::UseDiagnosticInfoKind;
     for (auto &info : applyUses) {
@@ -1170,29 +1202,13 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
             .highlight(info.getLoc().getSourceRange());
         break;
       }
+    }
 
-      // Ok, we now have our requires... emit the errors.
-      bool didEmitRequire = false;
-
-      InstructionSet requireInstsUnique(function);
-      for (auto *require : requireInsts) {
-        // We can have multiple of the same require insts if we had a require
-        // and an assign from the same instruction. Our liveness checking
-        // above doesn't care about that, but we still need to make sure we do
-        // not emit twice.
-        if (!requireInstsUnique.insert(require))
-          continue;
-
-        // If this was not a last require, do not emit an error.
-        if (!liveness.finalRequires.contains(require))
-          continue;
-
-        diagnoseNote(require, diag::regionbasedisolation_maybe_race)
-            .highlight(require->getLoc().getSourceRange());
-        didEmitRequire = true;
-      }
-
-      assert(didEmitRequire && "Should have a require for all errors?!");
+    // Now actually emit the require notes.
+    while (!requireInstsForError.empty()) {
+      auto *require = requireInstsForError.pop_back_val();
+      diagnoseNote(require, diag::regionbasedisolation_maybe_race)
+          .highlight(require->getLoc().getSourceRange());
     }
   }
 }

test/Concurrency/transfernonsendable.swift

@@ -1133,10 +1133,16 @@ func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive4() async {
   var cls = {}
 
   for _ in 0..<1024 {
+    // TODO: The error here is b/c of the loop carry. We should probably store
+    // the operand instead of just the require inst, so we can better separate a
+    // loop carry use and an error due to multiple params. Then we could emit
+    // the error on test below. This is still correct though, just not as
+    // good... that is QoI though.
     await transferToMain(test) // expected-tns-warning {{transferring non-Sendable value 'test' could yield races with later accesses}}
     // expected-tns-note @-1 {{'test' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}
-    // expected-complete-warning @-2 {{passing argument of non-sendable type 'StructFieldTests' into main actor-isolated context may introduce data races}}
-    test = StructFieldTests() // expected-tns-note {{access here could race}}
+    // expected-tns-note @-2 {{access here could race}}
+    // expected-complete-warning @-3 {{passing argument of non-sendable type 'StructFieldTests' into main actor-isolated context may introduce data races}}
+    test = StructFieldTests()
     cls = {
       useInOut(&test.varSendableNonTrivial)
     }
@@ -1167,7 +1173,7 @@ func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive5() async {
   }
 
   test.varSendableNonTrivial = SendableKlass()
-  useValue(test)  // expected-tns-note {{access here could race}}
+  useValue(test)
 }
 
 func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive6() async {
@@ -1321,7 +1327,7 @@ func controlFlowTest2() async {
     x = NonSendableKlass()
   }
 
-  useValue(x) // expected-tns-note {{access here could race}}
+  useValue(x)
 }
 
 ////////////////////////

test/Concurrency/transfernonsendable_isolationcrossing_partialapply.swift

@@ -25,6 +25,7 @@ struct CustomActor {
 
 func useValue<T>(_ t: T) {}
 @MainActor func transferToMain<T>(_ t: T) {}
+@CustomActor func transferToCustom<T>(_ t: T) {}
 
 /////////////////
 // MARK: Tests //
@@ -86,3 +87,15 @@ func normalFunc_testLocal_2() {
   }
   useValue(x) // expected-note {{access here could race}}
 }
+
+// We error here since we are performing a double transfer.
+//
+// TODO: Add special transfer use so we can emit a double transfer error
+// diagnostic.
+func transferBeforeCaptureErrors() async {
+  let x = NonSendableKlass()
+  await transferToCustom(x) // expected-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to global actor 'CustomActor'-isolated context}}
+  let _ = { @MainActor in // expected-note {{access here could race}}
+    useValue(x)
+  }
+}

test/Concurrency/transfernonsendable_strong_transferring_params.swift

@@ -38,6 +38,8 @@ func transferArgWithOtherParam(_ x: transferring Klass, _ y: Klass) {
 func transferArgWithOtherParam2(_ x: Klass, _ y: transferring Klass) {
 }
 
+func twoTransferArg(_ x: transferring Klass, _ y: transferring Klass) {}
+
 @MainActor var globalKlass = Klass()
 
 /////////////////
@@ -282,3 +284,15 @@ func mergeDoesNotEliminateEarlierTransfer2(_ x: transferring NonSendableStruct)
 
   useValue(y) // expected-note {{access here could race}}
 }
+
+func doubleArgument() async {
+  let x = Klass()
+  twoTransferArg(x, x) // expected-warning {{binding of non-Sendable type 'Klass' accessed after being transferred}}
+  // expected-note @-1 {{access here could race}}
+}
+
+func testTransferSrc(_ x: transferring Klass) async {
+  let y = Klass()
+  await transferToMain(y) // expected-warning {{transferring value of non-Sendable type 'Klass' from nonisolated context to main actor-isolated context}}
+  x = y // expected-note {{access here could race}}
+}