[region-isolation] Clean up use after transfer error to use the dynamic isolation information of the transfered operand value in its diagnostic message.

As an example of the change:

-  // expected-note @-1 {{'x' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}
+  // expected-note @-1 {{transferring disconnected 'x' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}

Part of the reason I am doing this is that I am going to be ensuring that we
handle a bunch more cases and I wanted to fix this diagnostic before I added
more incaranations of it to the tests.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -970,8 +970,8 @@ ERROR(regionbasedisolation_named_transfer_yields_race, none,
       (Identifier))
 
 NOTE(regionbasedisolation_named_info_transfer_yields_race, none,
-     "%0 is transferred from %1 caller to %2 callee. Later uses in caller could race with potential uses in callee",
-     (Identifier, ActorIsolation, ActorIsolation))
+     "transferring %1 %0 to %2 callee could cause races in between callee %2 and local %3 uses",
+     (Identifier, StringRef, ActorIsolation, ActorIsolation))
 NOTE(regionbasedisolation_named_transfer_non_transferrable, none,
       "transferring %1 %0 to %2 callee could cause races between %2 and %1 uses",
       (Identifier, StringRef, ActorIsolation))

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -324,58 +324,58 @@ class SILIsolationInfo {
 
 namespace swift {
 
-struct TransferringOperand {
+class TransferringOperand {
   using ValueType = llvm::PointerIntPair<Operand *, 1>;
   ValueType value;
 
-  TransferringOperand() : value() {}
-  TransferringOperand(Operand *op, bool isClosureCaptured)
-      : value(op, isClosureCaptured) {}
-  explicit TransferringOperand(Operand *op) : value(op, false) {}
-  TransferringOperand(ValueType newValue) : value(newValue) {}
+  /// The dynamic isolation info of the region of value when we transferred.
+  SILIsolationInfo isolationInfo;
+
+  TransferringOperand(ValueType newValue, SILIsolationInfo isolationRegionInfo)
+      : value(newValue), isolationInfo(isolationRegionInfo) {
+    assert(isolationInfo && "Should never see unknown isolation info");
+  }
+
+public:
+  TransferringOperand(Operand *op, bool isClosureCaptured,
+                      SILIsolationInfo isolationRegionInfo)
+      : TransferringOperand({op, isClosureCaptured}, isolationRegionInfo) {}
+  explicit TransferringOperand(Operand *op,
+                               SILIsolationInfo isolationRegionInfo)
+      : TransferringOperand({op, false}, isolationRegionInfo) {}
 
   operator bool() const { return bool(value.getPointer()); }
 
   Operand *getOperand() const { return value.getPointer(); }
 
+  SILValue get() const { return getOperand()->get(); }
+
   bool isClosureCaptured() const { return value.getInt(); }
 
   SILInstruction *getUser() const { return getOperand()->getUser(); }
 
-  bool operator<(const TransferringOperand &other) const {
-    return value < other.value;
-  }
+  SILIsolationInfo getIsolationInfo() const { return isolationInfo; }
 
-  bool operator>=(const TransferringOperand &other) const {
-    return !(value < other.value);
-  }
-
-  bool operator>(const TransferringOperand &other) const {
-    return value > other.value;
-  }
-
-  bool operator<=(const TransferringOperand &other) const {
-    return !(value > other.value);
-  }
-
-  bool operator==(const TransferringOperand &other) const {
-    return value == other.value;
-  }
+  unsigned getOperandNumber() const { return getOperand()->getOperandNumber(); }
 
   void print(llvm::raw_ostream &os) const {
     os << "Op Num: " << getOperand()->getOperandNumber() << ". "
        << "Capture: " << (isClosureCaptured() ? "yes. " : "no.  ")
-       << "User: " << *getUser();
+       << "IsolationInfo: ";
+    isolationInfo.print(os);
+    os << "\nUser: " << *getUser();
   }
 
   static void Profile(llvm::FoldingSetNodeID &id, Operand *op,
-                      bool isClosureCaptured) {
+                      bool isClosureCaptured,
+                      SILIsolationInfo isolationRegionInfo) {
     id.AddPointer(op);
     id.AddBoolean(isClosureCaptured);
+    isolationRegionInfo.Profile(id);
   }
 
   void Profile(llvm::FoldingSetNodeID &id) const {
-    Profile(id, getOperand(), isClosureCaptured());
+    Profile(id, getOperand(), isClosureCaptured(), isolationInfo);
   }
 
   SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
@@ -1367,8 +1367,8 @@ struct PartitionOpEvaluator {
       }
 
       // Mark op.getOpArgs()[0] as transferred.
-      auto *ptrSet =
-          ptrSetFactory.emplace(op.getSourceOp(), isClosureCapturedElt);
+      auto *ptrSet = ptrSetFactory.emplace(
+          op.getSourceOp(), isClosureCapturedElt, transferredRegionIsolation);
       p.markTransferred(op.getOpArgs()[0], ptrSet);
       return;
     }

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -2967,6 +2967,10 @@ bool BlockPartitionState::recomputeExitFromEntry(
         : PartitionOpEvaluatorBaseImpl(workingPartition, ptrSetFactory),
           translator(translator) {}
 
+    SILIsolationInfo getIsolationRegionInfo(Element elt) const {
+      return translator.getValueMap().getIsolationRegion(elt);
+    }
+
     bool isClosureCaptured(Element elt, Operand *op) const {
       auto iter = translator.getValueForId(elt);
       if (!iter)

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -421,9 +421,34 @@ struct TransferredNonTransferrableInfo {
         isolationRegionInfo(isolationRegionInfo) {}
 };
 
+/// A pointer to a TransferringOperand pointer that sorts according to the
+/// Operand pointer in the TransferringOperand rather than the pointer of the
+/// TransferringOperand itself.
+class TransferringOperandRef {
+  TransferringOperand *operand;
+
+public:
+  TransferringOperandRef(TransferringOperand *operand) : operand(operand) {}
+
+  TransferringOperand *operator*() const { return operand; }
+  TransferringOperand *operator->() const { return operand; }
+
+  bool operator==(const TransferringOperandRef &other) const {
+    return operand->getOperand() == other->getOperand();
+  }
+
+  bool operator!=(const TransferringOperandRef &other) const {
+    return !(*this == other);
+  }
+
+  bool operator<(const TransferringOperandRef &other) const {
+    return operand->getOperand() < other->getOperand();
+  }
+};
+
 class TransferNonSendableImpl {
   RegionAnalysisFunctionInfo *regionInfo;
-  SmallFrozenMultiMap<Operand *, SILInstruction *, 8>
+  SmallFrozenMultiMap<TransferringOperandRef, SILInstruction *, 8>
       transferOpToRequireInstMultiMap;
   SmallVector<TransferredNonTransferrableInfo, 8>
       transferredNonTransferrableInfoList;
@@ -467,6 +492,7 @@ class UseAfterTransferDiagnosticEmitter {
   }
 
   void emitNamedIsolationCrossingError(SILLocation loc, Identifier name,
+                                       SILIsolationInfo namesIsolationInfo,
                                        ApplyIsolationCrossing isolationCrossing,
                                        SILLocation variableDefinedLoc) {
     // Emit the short error.
@@ -475,10 +501,15 @@ class UseAfterTransferDiagnosticEmitter {
         .highlight(loc.getSourceRange());
 
     // Then emit the note with greater context.
-    diagnoseNote(loc,
-                 diag::regionbasedisolation_named_info_transfer_yields_race,
-                 name, isolationCrossing.getCallerIsolation(),
-                 isolationCrossing.getCalleeIsolation());
+    SmallString<64> descriptiveKindStr;
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      namesIsolationInfo.printForDiagnostics(os);
+    }
+    diagnoseNote(
+        loc, diag::regionbasedisolation_named_info_transfer_yields_race, name,
+        descriptiveKindStr, isolationCrossing.getCalleeIsolation(),
+        isolationCrossing.getCallerIsolation());
     emitRequireInstDiagnostics();
   }
 
@@ -492,7 +523,7 @@ class UseAfterTransferDiagnosticEmitter {
     emitRequireInstDiagnostics();
   }
 
-  void emitUseOfStringlyTransferredValue(SILLocation loc, Type inferredType) {
+  void emitUseOfStronglyTransferredValue(SILLocation loc, Type inferredType) {
     diagnoseError(
         loc,
         diag::
@@ -580,7 +611,7 @@ class UseAfterTransferDiagnosticEmitter {
 };
 
 class UseAfterTransferDiagnosticInferrer {
-  Operand *transferOp;
+  TransferringOperand *transferOp;
   UseAfterTransferDiagnosticEmitter diagnosticEmitter;
   RegionAnalysisValueMap &valueMap;
   SILLocation baseLoc = SILLocation::invalid();
@@ -590,13 +621,18 @@ class UseAfterTransferDiagnosticInferrer {
 
 public:
   UseAfterTransferDiagnosticInferrer(
-      Operand *transferOp, SmallVectorImpl<SILInstruction *> &requireInsts,
+      TransferringOperand *transferOp,
+      SmallVectorImpl<SILInstruction *> &requireInsts,
       RegionAnalysisValueMap &valueMap)
-      : transferOp(transferOp), diagnosticEmitter(transferOp, requireInsts),
+      : transferOp(transferOp),
+        diagnosticEmitter(transferOp->getOperand(), requireInsts),
         valueMap(valueMap), baseLoc(transferOp->getUser()->getLoc()),
-        baseInferredType(transferOp->get()->getType().getASTType()) {}
+        baseInferredType(
+            transferOp->getOperand()->get()->getType().getASTType()) {}
   void infer();
 
+  Operand *getTransferringOperand() const { return transferOp->getOperand(); }
+
 private:
   bool initForIsolatedPartialApply(Operand *op, AbstractClosureExpr *ace);
 
@@ -742,11 +778,12 @@ struct UseAfterTransferDiagnosticInferrer::Walker : ASTWalker {
 void UseAfterTransferDiagnosticInferrer::infer() {
   // Otherwise, see if our operand's instruction is a transferring parameter.
   if (auto fas = FullApplySite::isa(transferOp->getUser())) {
-    assert(!fas.getArgumentConvention(*transferOp).isIndirectOutParameter() &&
+    assert(!fas.getArgumentConvention(*transferOp->getOperand())
+                .isIndirectOutParameter() &&
            "We should never transfer an indirect out parameter");
-    if (fas.getArgumentParameterInfo(*transferOp)
+    if (fas.getArgumentParameterInfo(*transferOp->getOperand())
             .hasOption(SILParameterInfo::Transferring)) {
-      return diagnosticEmitter.emitUseOfStringlyTransferredValue(
+      return diagnosticEmitter.emitUseOfStronglyTransferredValue(
           baseLoc, baseInferredType);
     }
   }
@@ -757,7 +794,7 @@ void UseAfterTransferDiagnosticInferrer::infer() {
   // transfer error due to us transferring a value into it.
   if (auto *ace = loc.getAsASTNode<AbstractClosureExpr>()) {
     if (ace->getActorIsolation().isActorIsolated()) {
-      if (initForIsolatedPartialApply(transferOp, ace)) {
+      if (initForIsolatedPartialApply(transferOp->getOperand(), ace)) {
         return;
       }
     }
@@ -770,21 +807,21 @@ void UseAfterTransferDiagnosticInferrer::infer() {
       if (auto *svi =
               dyn_cast<SingleValueInstruction>(rootValueAndName->second)) {
         return diagnosticEmitter.emitNamedIsolationCrossingError(
-            baseLoc, rootValueAndName->first,
+            baseLoc, rootValueAndName->first, transferOp->getIsolationInfo(),
             *sourceApply->getIsolationCrossing(), svi->getLoc());
       }
 
       if (auto *fArg =
               dyn_cast<SILFunctionArgument>(rootValueAndName->second)) {
         return diagnosticEmitter.emitNamedIsolationCrossingError(
-            baseLoc, rootValueAndName->first,
+            baseLoc, rootValueAndName->first, transferOp->getIsolationInfo(),
             *sourceApply->getIsolationCrossing(),
             RegularLocation(fArg->getDecl()->getLoc()));
       }
     }
 
     // Otherwise, try to infer from the ApplyExpr.
-    return initForApply(transferOp, sourceApply);
+    return initForApply(transferOp->getOperand(), sourceApply);
   }
 
   if (auto fas = FullApplySite::isa(transferOp->getUser())) {
@@ -801,7 +838,7 @@ void UseAfterTransferDiagnosticInferrer::infer() {
 
   auto *i = transferOp->getUser();
   auto pai = ApplySite::isa(i);
-  unsigned captureIndex = pai.getAppliedArgIndex(*transferOp);
+  unsigned captureIndex = pai.getAppliedArgIndex(*transferOp->getOperand());
 
   auto captureInfo =
       autoClosureExpr->getCaptureInfo().getCaptures()[captureIndex];
@@ -823,8 +860,9 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
 
   LLVM_DEBUG(llvm::dbgs() << "Emitting use after transfer diagnostics.\n");
 
-  for (auto [transferOp, requireInsts] :
+  for (auto [transferOpRef, requireInsts] :
        transferOpToRequireInstMultiMap.getRange()) {
+    auto *transferOp = *transferOpRef;
 
     LLVM_DEBUG(llvm::dbgs()
                << "Transfer Op. Number: " << transferOp->getOperandNumber()
@@ -834,8 +872,8 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
     // single we don't understand error if we do not find the require.
     bool didEmitRequireNote = false;
     InstructionSet requireInstsUnique(function);
-    RequireLiveness liveness(blockLivenessInfoGeneration, transferOp,
-                             blockLivenessInfo);
+    RequireLiveness liveness(blockLivenessInfoGeneration,
+                             transferOp->getOperand(), blockLivenessInfo);
     ++blockLivenessInfoGeneration;
     liveness.process(requireInsts);
 
@@ -860,7 +898,8 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
     // 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);
+      diagnoseError(transferOp->getOperand(),
+                    diag::regionbasedisolation_unknown_pattern);
       continue;
     }
 
@@ -1213,20 +1252,20 @@ namespace {
 struct DiagnosticEvaluator final
     : PartitionOpEvaluatorBaseImpl<DiagnosticEvaluator> {
   RegionAnalysisFunctionInfo *info;
-  SmallFrozenMultiMap<Operand *, SILInstruction *, 8>
+  SmallFrozenMultiMap<TransferringOperandRef, SILInstruction *, 8>
       &transferOpToRequireInstMultiMap;
 
   /// First value is the operand that was transferred... second value is the
   /// non-transferrable value in the same region as that value. The second value
   /// is what is non-transferrable.
   SmallVectorImpl<TransferredNonTransferrableInfo> &transferredNonTransferrable;
 
-  DiagnosticEvaluator(Partition &workingPartition,
-                      RegionAnalysisFunctionInfo *info,
-                      SmallFrozenMultiMap<Operand *, SILInstruction *, 8>
-                          &transferOpToRequireInstMultiMap,
-                      SmallVectorImpl<TransferredNonTransferrableInfo>
-                          &transferredNonTransferrable)
+  DiagnosticEvaluator(
+      Partition &workingPartition, RegionAnalysisFunctionInfo *info,
+      SmallFrozenMultiMap<TransferringOperandRef, SILInstruction *, 8>
+          &transferOpToRequireInstMultiMap,
+      SmallVectorImpl<TransferredNonTransferrableInfo>
+          &transferredNonTransferrable)
       : PartitionOpEvaluatorBaseImpl(workingPartition,
                                      info->getOperandSetFactory()),
         info(info),
@@ -1261,7 +1300,7 @@ struct DiagnosticEvaluator final
                << "        ID:  %%" << transferredVal << "\n"
                << "        Rep: " << *rep << "        Transferring Op Num: "
                << transferringOp->getOperand()->getOperandNumber() << '\n');
-    transferOpToRequireInstMultiMap.insert(transferringOp->getOperand(),
+    transferOpToRequireInstMultiMap.insert({transferringOp},
                                            partitionOp.getSourceInst());
   }
 

test/Concurrency/experimental_feature_strictconcurrency.swift

@@ -32,7 +32,7 @@ func iterate(stream: AsyncStream<Int>) async {
   // declaration to be in a disconnected region
 
   // expected-region-isolation-warning @+3 {{transferring 'it' may cause a race}}
-  // expected-region-isolation-note @+2 {{'it' is transferred from main actor-isolated caller to nonisolated callee. Later uses in caller could race with potential uses in callee}}
+  // expected-region-isolation-note @+2 {{transferring disconnected 'it' to nonisolated callee could cause races in between callee nonisolated and local main actor-isolated uses}}
   // expected-region-isolation-note @+1 {{access here could race}}
   while let element = await it.next() {
     print(element)

test/Concurrency/sendable_checking.swift

@@ -278,7 +278,7 @@ func testNonSendableBaseArg() async {
   await t.update()
   // expected-targeted-and-complete-warning @-1 {{passing argument of non-sendable type 'NonSendable' into main actor-isolated context may introduce data races}}
   // expected-tns-warning @-2 {{transferring 't' may cause a race}}
-  // expected-tns-note @-3 {{'t' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}
+  // expected-tns-note @-3 {{transferring disconnected 't' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
 
   _ = await t.x
   // expected-warning @-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to main actor-isolated property 'x' cannot cross actor boundary}}