[rbi] Cleanup handling of how we send parameters to fix issues with inout sending.

We previously were not "unsending" inout sending parameters after sending them
so they could not be used again in the caller and could not be forwarded into
other 'inout sending' parameters. While looking at the code I
realized it was pretty obtuse/confusing so I cleaned up the logic and fixed a
few other issues in the process.

Now we follow the following pattern in the non-isolation crossing case:

1. We first require the callee operand.
2. We then merge/require all of the non-explicitly sent parameters.
3. We then through all of the parameters and require/send all of the sending parameters.
4. At the end of processing, we unsend all of the sending parameters that were
'inout sending' parameters.

In the case of isolation crossing applies we:

1. Require all parameters that are not explicitly marked as sending and then
send them all at once. We are really just saving a little work by not merging
them into one large region and then requiring/sending that region once.

2. Then for each sending parameter, we require/send them one by one interleaving
the requires/sends. This ensures that if a value is passed to different
explicitly sending parameters, we get an error.

3. Then once we have finished processing results, we perform an undo send on all
of the 'inout sending' params.

rdar://154440896

Author: gottesmm

include/swift/SIL/ApplySite.h

@@ -645,14 +645,27 @@ class ApplySite {
     return getSubstCalleeConv().getParamInfoForSILArg(calleeArgIndex);
   }
 
+  /// Returns true if \p op is the callee operand of this apply site
+  /// and not an argument operand.
+  ///
+  /// If this instruction is not a full apply site, this always returns false.
+  bool isCalleeOperand(const Operand &op) const;
+
+  /// Returns true if this is an 'out' parameter.
   bool isSending(const Operand &oper) const {
-    if (isIndirectErrorResultOperand(oper))
+    if (isIndirectErrorResultOperand(oper) || oper.isTypeDependent() ||
+        isCalleeOperand(oper))
       return false;
     if (isIndirectResultOperand(oper))
       return getSubstCalleeType()->hasSendingResult();
     return getArgumentParameterInfo(oper).hasOption(SILParameterInfo::Sending);
   }
 
+  /// Returns true if this operand is an 'inout sending' parameter.
+  bool isInOutSending(const Operand &oper) const {
+    return isSending(oper) && getArgumentConvention(oper).isInoutConvention();
+  }
+
   /// Return true if 'operand' is addressable after type substitution in the
   /// caller's context.
   bool isAddressable(const Operand &operand) const;
@@ -1037,6 +1050,13 @@ inline bool ApplySite::isIndirectErrorResultOperand(const Operand &op) const {
   return fas.isIndirectErrorResultOperand(op);
 }
 
+inline bool ApplySite::isCalleeOperand(const Operand &op) const {
+  auto fas = asFullApplySite();
+  if (!fas)
+    return false;
+  return fas.isCalleeOperand(op);
+}
+
 } // namespace swift
 
 #endif

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1723,9 +1723,13 @@ struct PartitionOpBuilder {
         PartitionOp::Send(lookupValueID(representative), op));
   }
 
-  void addUndoSend(SILValue representative, SILInstruction *unsendingInst) {
+  void addUndoSend(TrackableValue value, SILInstruction *unsendingInst) {
+    if (value.isSendable())
+      return;
+
+    auto representative = value.getRepresentative().getValue();
     assert(valueHasID(representative) &&
-           "value should already have been encountered");
+           "sent value should already have been encountered");
 
     currentInstPartitionOps->emplace_back(
         PartitionOp::UndoSend(lookupValueID(representative), unsendingInst));
@@ -2430,7 +2434,7 @@ class PartitionOpTranslator {
                 }))
           continue;
 
-        builder.addUndoSend(trackedArgValue.getRepresentative().getValue(), ai);
+        builder.addUndoSend(trackedArgValue, ai);
       }
     }
   }
@@ -2567,60 +2571,53 @@ class PartitionOpTranslator {
     // gather our non-sending parameters.
     SmallVector<Operand *, 8> nonSendingParameters;
     SmallVector<Operand *, 8> sendingIndirectResults;
-    if (fas.getNumArguments()) {
-      // NOTE: We want to process indirect parameters as if they are
-      // parameters... so we process them in nonSendingParameters.
-      for (auto &op : fas.getOperandsWithoutSelf()) {
-        // If op is the callee operand, skip it.
-        if (fas.isCalleeOperand(op))
-          continue;
 
-        if (fas.isSending(op)) {
-          if (fas.isIndirectResultOperand(op)) {
-            sendingIndirectResults.push_back(&op);
-            continue;
-          }
+    // NOTE: We want to process indirect parameters as if they are
+    // parameters... so we process them in nonSendingParameters.
+    for (auto &op : fas->getAllOperands()) {
+      // If op is the callee operand or type dependent operand, skip it.
+      if (op.isTypeDependent())
+        continue;
 
-          // Attempt to lookup the value we are passing as sending. We want to
-          // require/send value if it is non-Sendable and require its base if it
-          // is non-Sendable as well.
-          if (auto lookupResult = tryToTrackValue(op.get())) {
-            builder.addRequire(*lookupResult);
-            builder.addSend(lookupResult->value, &op);
-          }
-        } else {
-          nonSendingParameters.push_back(&op);
+      if (fas.isCalleeOperand(op)) {
+        if (auto calleeResult = tryToTrackValue(op.get())) {
+          builder.addRequire(*calleeResult);
         }
+        continue;
       }
-    }
 
-    // If our self parameter was sending, send it. Otherwise, just
-    // stick it in the non self operand values array and run multiassign on
-    // it.
-    if (fas.hasSelfArgument()) {
-      auto &selfOperand = fas.getSelfArgumentOperand();
-      if (fas.getArgumentParameterInfo(selfOperand)
-              .hasOption(SILParameterInfo::Sending)) {
-        if (auto lookupResult = tryToTrackValue(selfOperand.get())) {
-          builder.addRequire(*lookupResult);
-          builder.addSend(lookupResult->value, &selfOperand);
-        }
-      } else {
-        nonSendingParameters.push_back(&selfOperand);
+      // If our parameter is not sending, just add it to the non-sending
+      // parameters array and continue.
+      if (!fas.isSending(op)) {
+        nonSendingParameters.push_back(&op);
+        continue;
       }
-    }
 
-    // Require our callee operand if it is non-Sendable.
-    //
-    // DISCUSSION: Even though we do not include our callee operand in the same
-    // region as our operands/results, we still need to require that it is live
-    // at the point of application. Otherwise, we will not emit errors if the
-    // closure before this function application is already in the same region as
-    // a sent value. In such a case, the function application must error.
-    if (auto calleeResult = tryToTrackValue(fas.getCallee())) {
-      builder.addRequire(*calleeResult);
+      // Otherwise, first handle indirect result operands.
+      if (fas.isIndirectResultOperand(op)) {
+        sendingIndirectResults.push_back(&op);
+        continue;
+      }
+
+      // Attempt to lookup the value we are passing as sending. We want to
+      // require/send value if it is non-Sendable and require its base if it
+      // is non-Sendable as well.
+      if (auto lookupResult = tryToTrackValue(op.get())) {
+        builder.addRequire(*lookupResult);
+        builder.addSend(lookupResult->value, &op);
+      }
     }
 
+    SWIFT_DEFER {
+      for (auto &op : fas->getAllOperands()) {
+        if (!fas.isInOutSending(op))
+          continue;
+        if (auto lookupResult = tryToTrackValue(op.get())) {
+          builder.addUndoSend(lookupResult->value, op.getUser());
+        }
+      }
+    };
+
     SmallVector<SILValue, 8> applyResults;
     getApplyResults(*fas, applyResults);
 
@@ -2688,36 +2685,77 @@ class PartitionOpTranslator {
 
   /// Handles the semantics for SIL applies that cross isolation.
   ///
-  /// Semantically this causes all arguments of the applysite to be sent.
+  /// Semantically we are attempting to implement the following:
+  ///
+  /// * Step 1: Require all non-sending parameters and then send those
+  ///   parameters. We perform all of the requires first and the sends second
+  ///   since all of the parameters are getting sent to the same isolation
+  ///   domains and become part of the same region in our callee. So in a
+  ///   certain sense, we are performing a require over the entire merge of the
+  ///   parameter regions and then send each constituant part of the region
+  ///   without requiring again so we do not emit use-after-send diagnostics.
+  ///
+  /// * Step 2: Require/Send each of the sending parameters one by one. This
+  ///   includes both 'sending' and 'inout sending' parameters. We purposely
+  ///   interleave the require/send operations to ensure that if one passes a
+  ///   value twice to different 'sending' or 'inout sending' parameters, we
+  ///   will emit an error.
+  ///
+  /// * Step 3: Unsend each of the unsending parameters. Since our caller
+  ///   ensures that 'inout sending' parameters are disconnected on return and
+  ///   are in different regions from all other parameters, we can just simply
+  ///   unsend the parameter so we can use it again later.
   void translateIsolationCrossingSILApply(FullApplySite applySite) {
-    // Require all operands first before we emit a send.
-    for (auto op : applySite.getArguments()) {
-      if (auto lookupResult = tryToTrackValue(op)) {
+    SmallVector<TrackableValue, 8> inoutSendingParams;
+
+    // First go through and require all of our operands that are not 'sending'
+    for (auto &op : applySite.getArgumentOperands()) {
+      if (applySite.isSending(op))
+        continue;
+      if (auto lookupResult = tryToTrackValue(op.get()))
         builder.addRequire(*lookupResult);
-      }
     }
 
-    auto handleSILOperands = [&](MutableArrayRef<Operand> operands) {
-      for (auto &op : operands) {
-        if (auto lookupResult = tryToTrackValue(op.get())) {
-          builder.addSend(lookupResult->value, &op);
-        }
+    // Then go through our operands again and send all of our non-sending
+    // parameters. We do not interleave these sends with our requires since we
+    // are considering these values to be merged into the same region. We could
+    // also merge them in the caller but there is no point in doing so
+    // semantically since the values cannot be used again locally.
+    for (auto &op : applySite.getOperandsWithoutIndirectResults()) {
+      if (applySite.isSending(op))
+        continue;
+      if (auto lookupResult = tryToTrackValue(op.get())) {
+        builder.addSend(lookupResult->value, &op);
       }
     };
 
-    auto handleSILSelf = [&](Operand *self) {
-      if (auto lookupResult = tryToTrackValue(self->get())) {
-        builder.addSend(lookupResult->value, self);
+    // Then go through our 'sending' params and require/send each in sequence.
+    //
+    // We do this interleaved so that if a value is passed to multiple 'sending'
+    // parameters, we emit errors.
+    for (auto &op : applySite.getOperandsWithoutIndirectResults()) {
+      if (!applySite.isSending(op))
+        continue;
+      auto lookupResult = tryToTrackValue(op.get());
+      if (!lookupResult)
+        continue;
+      builder.addRequire(*lookupResult);
+      builder.addSend(lookupResult->value, &op);
+    }
+
+    // Now use a SWIFT_DEFER so that when the function is done executing, we
+    // unsend 'inout sending' params.
+    SWIFT_DEFER {
+      for (auto &op : applySite.getOperandsWithoutIndirectResults()) {
+        if (!applySite.isInOutSending(op))
+          continue;
+        auto lookupResult = tryToTrackValue(op.get());
+        if (!lookupResult)
+          continue;
+        builder.addUndoSend(lookupResult->value, *applySite);
       }
     };
 
-    if (applySite.hasSelfArgument()) {
-      handleSILOperands(applySite.getOperandsWithoutIndirectResultsOrSelf());
-      handleSILSelf(&applySite.getSelfArgumentOperand());
-    } else {
-      handleSILOperands(applySite.getOperandsWithoutIndirectResults());
-    }
-
     // Create a new assign fresh for each one of our values and unless our
     // return value is sending, emit an extra error bit on the results that are
     // non-Sendable.

test/Concurrency/transfernonsendable_inout_sending_params.swift

@@ -80,6 +80,9 @@ struct MyError : Error {}
 
 func takeClosure(_ x: sending () -> ()) {}
 func takeClosureAndParam(_ x: NonSendableKlass, _ y: sending () -> ()) {}
+func useInOutSending(_ x: inout sending NonSendableKlass) {}
+func useInOutSending(_ x: inout sending NonSendableKlass,
+                     _ y: inout sending NonSendableKlass) {}
 
 ///////////////////////////////
 // MARK: InOut Sending Tests //
@@ -162,6 +165,17 @@ func testWrongIsolationGlobalIsolation(_ x: inout sending NonSendableKlass) {
 } // expected-warning {{'inout sending' parameter 'x' cannot be main actor-isolated at end of function}}
 // expected-note @-1 {{main actor-isolated 'x' risks causing races in between main actor-isolated uses and caller uses since caller assumes value is not actor isolated}}
 
+func passInOutSendingToInOutSending(_ x: inout sending NonSendableKlass) {
+  useInOutSending(&x)
+}
+
+func passInOutSendingMultipleTimes(_ x: inout NonSendableStruct) {
+  useInOutSending(&x.first, &x.second) // expected-warning {{sending 'x.first' risks causing data races}}
+  // expected-note @-1 {{task-isolated 'x.first' is passed as a 'sending' parameter}}
+  // expected-warning @-2 {{sending 'x.second' risks causing data races}}
+  // expected-note @-3 {{task-isolated 'x.second' is passed as a 'sending' parameter}}
+}
+
 //////////////////////////////////////
 // MARK: Return Inout Sending Tests //
 //////////////////////////////////////

test/Concurrency/transfernonsendable_sending_params.swift

@@ -69,11 +69,22 @@ func transferArgAsync(_ x: sending NonSendableKlass) async {
 func transferArgWithOtherParam(_ x: sending NonSendableKlass, _ y: NonSendableKlass) {
 }
 
+@MainActor
+func transferArgWithOtherParamIsolationCrossing(_ x: sending NonSendableKlass, _ y: NonSendableKlass) async {
+}
+
 func transferArgWithOtherParam2(_ x: NonSendableKlass, _ y: sending NonSendableKlass) {
 }
 
+@MainActor
+func transferArgWithOtherParam2IsolationCrossing(_ x: NonSendableKlass, _ y: sending NonSendableKlass) async {
+}
+
 func twoTransferArg(_ x: sending NonSendableKlass, _ y: sending NonSendableKlass) {}
 
+@MainActor
+func twoTransferArgIsolationCrossing(_ x: sending NonSendableKlass, _ y: sending NonSendableKlass) async {}
+
 @MainActor var globalKlass = NonSendableKlass()
 
 struct MyError : Error {}
@@ -114,6 +125,22 @@ func testSimpleTransferUseOfOtherParamNoError2() {
   useValue(k)
 }
 
+func testSimpleTransferUseOfOtherParamError() async {
+  let k = NonSendableKlass()
+  await transferArgWithOtherParamIsolationCrossing(k, k) // expected-warning {{sending 'k' risks causing data races}}
+  // expected-note @-1 {{sending 'k' to main actor-isolated global function 'transferArgWithOtherParamIsolationCrossing' risks causing data races between main actor-isolated and local nonisolated uses}}
+  // expected-note @-2 {{access can happen concurrently}}
+}
+
+// TODO: Improve this error message. We should say that we are emitting an
+// error. Also need to add SILLocation to ApplyInst.
+func testSimpleTransferUseOfOtherParam2Error() async {
+  let k = NonSendableKlass()
+  await transferArgWithOtherParam2IsolationCrossing(k, k) // expected-warning {{sending 'k' risks causing data races}}
+  // expected-note @-1 {{sending 'k' to main actor-isolated global function 'transferArgWithOtherParam2IsolationCrossing' risks causing data races between main actor-isolated and local nonisolated uses}}
+  // expected-note @-2 {{access can happen concurrently}}
+}
+
 @MainActor func transferToMain2(_ x: sending NonSendableKlass, _ y: NonSendableKlass, _ z: NonSendableKlass) async {
 }
 
@@ -358,6 +385,13 @@ func doubleArgument() async {
   // expected-note @-2 {{access can happen concurrently}}
 }
 
+func doubleArgumentIsolationCrossing() async {
+  let x = NonSendableKlass()
+  await twoTransferArgIsolationCrossing(x, x) // expected-warning {{sending 'x' risks causing data races}}
+  // expected-note @-1 {{'x' used after being passed as a 'sending' parameter}}
+  // expected-note @-2 {{access can happen concurrently}}
+}
+
 func testTransferSrc(_ x: sending NonSendableKlass) async {
   let y = NonSendableKlass()
   await transferToMain(y) // expected-warning {{sending 'y' risks causing data races}}