[region-isolation] Improve the error we emit for closure literals captured as a sending parameter.

Specifically:

I changed the main error message to focus on the closure and that the closure
is being accessed concurrently.

If we find that we captured a value that is the actual isolation source, we
emit that the capture is actually actor isolated.

If the captured value is in the same region as the isolated value but is not
isolated, we instead say that the value is accessible from *-isolated code or
code within the current task.

If we find multiple captures and we do not which is the actual value that was
in the same region before we formed the partial apply, we just emit a note on
the captures saying that the closure captures the value.

I changed the diagnostics from using the phrase "task-isolated" to use some
variant of accessible to code in the current task.

The idea is that in all situations we provide a breadcrumb that the user can
start investigating rather than just saying that the closure is "task-isolated".

From a preconcurrency perspective, I made it so that we apply the preconcurrency
behavior of all of the captures. This means that if one of the captures is
preconcurrency, we apply the preconcurrency restriction to the closure. This is
one step towards making it so that preconcurrency applies at the region level...
we just are not completely there yet.

rdar://133798044
(cherry picked from commit 4bb2e4f)

Conflicts:
	include/swift/AST/DiagnosticsSIL.def
	lib/SILOptimizer/Mandatory/TransferNonSendable.cpp
	test/Concurrency/concurrent_value_checking.swift
	test/Concurrency/isolated_parameters.swift
	test/Concurrency/sendable_preconcurrency.swift
	test/Concurrency/sendable_without_preconcurrency.swift
	test/Concurrency/sendable_without_preconcurrency_2.swift
	test/Concurrency/transfernonsendable.swift
	test/Concurrency/transfernonsendable_global_actor_sending.swift

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -1041,6 +1041,23 @@ NOTE(regionbasedisolation_named_isolated_closure_yields_race, none,
       "%0%1 is captured by a %2 closure. %2 uses in closure may race against later %3 uses",
       (StringRef, Identifier, ActorIsolation, ActorIsolation))
 
+ERROR(regionbasedisolation_typed_tns_passed_sending_closure, none,
+     "passing closure as a 'sending' parameter risks causing data races between %0 and concurrent execution of the closure",
+     (StringRef))
+NOTE(regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value, none,
+     "closure captures %0 %1",
+     (StringRef, DeclName))
+NOTE(regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value_task_isolated, none,
+     "closure captures %0 which is accessible to code in the current task",
+     (DeclName))
+NOTE(regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value_region, none,
+     "closure captures %1 which is accessible to %0 code",
+     (StringRef, DeclName))
+
+NOTE(regionbasedisolation_typed_tns_passed_to_sending_closure_helper_multiple_value, none,
+     "closure captures non-Sendable %0",
+     (DeclName))
+
 NOTE(regionbasedisolation_named_transfer_nt_asynclet_capture, none,
       "sending %1 %0 into async let risks causing data races between nonisolated and %1 uses",
       (Identifier, StringRef))

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -141,6 +141,12 @@ class RepresentativeValue {
     return value.get<SILInstruction *>();
   }
 
+  bool operator==(SILValue other) const {
+    if (hasRegionIntroducingInst())
+      return false;
+    return getValue() == other;
+  }
+
   SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
 
 private:

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -58,6 +58,29 @@ using Region = PartitionPrimitives::Region;
 //                              MARK: Utilities
 //===----------------------------------------------------------------------===//
 
+static SILValue stripFunctionConversions(SILValue val) {
+  while (true) {
+    if (auto ti = dyn_cast<ThinToThickFunctionInst>(val)) {
+      val = ti->getOperand();
+      continue;
+    }
+
+    if (auto cfi = dyn_cast<ConvertFunctionInst>(val)) {
+      val = cfi->getOperand();
+      continue;
+    }
+
+    if (auto cvt = dyn_cast<ConvertEscapeToNoEscapeInst>(val)) {
+      val = cvt->getOperand();
+      continue;
+    }
+
+    break;
+  }
+
+  return val;
+}
+
 static std::optional<DiagnosticBehavior>
 getDiagnosticBehaviorLimitForValue(SILValue value) {
   auto *nom = value->getType().getNominalOrBoundGenericNominal();
@@ -72,6 +95,38 @@ getDiagnosticBehaviorLimitForValue(SILValue value) {
   return getConcurrencyDiagnosticBehaviorLimit(nom, fromDC);
 }
 
+static std::optional<DiagnosticBehavior>
+getDiagnosticBehaviorLimitForCapturedValue(CapturedValue value) {
+  ValueDecl *decl = value.getDecl();
+  auto *nom = decl->getInterfaceType()->getNominalOrBoundGenericNominal();
+  if (!nom)
+    return {};
+
+  auto *fromDC = decl->getInnermostDeclContext();
+  return getConcurrencyDiagnosticBehaviorLimit(nom, fromDC);
+}
+
+/// Find the most conservative diagnostic behavior by taking the max over all
+/// DiagnosticBehavior for the captured values.
+static std::optional<DiagnosticBehavior>
+getDiagnosticBehaviorLimitForCapturedValues(
+    ArrayRef<CapturedValue> capturedValues) {
+  using UnderlyingType = std::underlying_type<DiagnosticBehavior>::type;
+
+  std::optional<DiagnosticBehavior> diagnosticBehavior;
+  for (auto value : capturedValues) {
+    auto lhs = UnderlyingType(
+        diagnosticBehavior.value_or(DiagnosticBehavior::Unspecified));
+    auto rhs = UnderlyingType(
+        getDiagnosticBehaviorLimitForCapturedValue(value).value_or(
+            DiagnosticBehavior::Unspecified));
+    auto result = DiagnosticBehavior(std::max(lhs, rhs));
+    if (result != DiagnosticBehavior::Unspecified)
+      diagnosticBehavior = result;
+  }
+  return diagnosticBehavior;
+}
+
 static std::optional<SILDeclRef> getDeclRefForCallee(SILInstruction *inst) {
   auto fas = FullApplySite::isa(inst);
   if (!fas)
@@ -1318,6 +1373,98 @@ class TransferNonTransferrableDiagnosticEmitter {
         .limitBehaviorIf(getBehaviorLimit());
   }
 
+  /// Only use if we were able to find the actual isolated value.
+  void emitTypedSendingNeverSendableToSendingClosureParamDirectlyIsolated(
+      SILLocation loc, CapturedValue capturedValue) {
+    SmallString<64> descriptiveKindStr;
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      if (getIsolationRegionInfo().getIsolationInfo().isTaskIsolated()) {
+        os << "code in the current task";
+      } else {
+        getIsolationRegionInfo().printForDiagnostics(os);
+        os << " code";
+      }
+    }
+
+    diagnoseError(loc,
+                  diag::regionbasedisolation_typed_tns_passed_sending_closure,
+                  descriptiveKindStr)
+        .highlight(loc.getSourceRange())
+        .limitBehaviorIf(
+            getDiagnosticBehaviorLimitForCapturedValue(capturedValue));
+
+    auto capturedLoc = RegularLocation(capturedValue.getLoc());
+    if (getIsolationRegionInfo().getIsolationInfo().isTaskIsolated()) {
+      auto diag = diag::
+          regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value_task_isolated;
+      diagnoseNote(capturedLoc, diag, capturedValue.getDecl()->getName());
+      return;
+    }
+
+    descriptiveKindStr.clear();
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      getIsolationRegionInfo().printForDiagnostics(os);
+    }
+
+    auto diag = diag::
+        regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value;
+    diagnoseNote(capturedLoc, diag, descriptiveKindStr,
+                 capturedValue.getDecl()->getName());
+  }
+
+  void emitTypedSendingNeverSendableToSendingClosureParam(
+      SILLocation loc, ArrayRef<CapturedValue> capturedValues) {
+    SmallString<64> descriptiveKindStr;
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      if (getIsolationRegionInfo().getIsolationInfo().isTaskIsolated()) {
+        os << "code in the current task";
+      } else {
+        getIsolationRegionInfo().printForDiagnostics(os);
+        os << " code";
+      }
+    }
+
+    auto behaviorLimit =
+        getDiagnosticBehaviorLimitForCapturedValues(capturedValues);
+    diagnoseError(loc,
+                  diag::regionbasedisolation_typed_tns_passed_sending_closure,
+                  descriptiveKindStr)
+        .highlight(loc.getSourceRange())
+        .limitBehaviorIf(behaviorLimit);
+
+    if (capturedValues.size() == 1) {
+      auto captured = capturedValues.front();
+      auto capturedLoc = RegularLocation(captured.getLoc());
+      if (getIsolationRegionInfo().getIsolationInfo().isTaskIsolated()) {
+        auto diag = diag::
+            regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value_task_isolated;
+        diagnoseNote(capturedLoc, diag, captured.getDecl()->getName());
+        return;
+      }
+
+      descriptiveKindStr.clear();
+      {
+        llvm::raw_svector_ostream os(descriptiveKindStr);
+        getIsolationRegionInfo().printForDiagnostics(os);
+      }
+      auto diag = diag::
+          regionbasedisolation_typed_tns_passed_to_sending_closure_helper_have_value_region;
+      diagnoseNote(capturedLoc, diag, descriptiveKindStr,
+                   captured.getDecl()->getName());
+      return;
+    }
+
+    for (auto captured : capturedValues) {
+      auto capturedLoc = RegularLocation(captured.getLoc());
+      auto diag = diag::
+          regionbasedisolation_typed_tns_passed_to_sending_closure_helper_multiple_value;
+      diagnoseNote(capturedLoc, diag, captured.getDecl()->getName());
+    }
+  }
+
   void emitNamedOnlyError(SILLocation loc, Identifier name) {
     diagnoseError(loc, diag::regionbasedisolation_named_transfer_yields_race,
                   name)
@@ -1455,12 +1602,13 @@ class TransferNonTransferrableDiagnosticEmitter {
 class TransferNonTransferrableDiagnosticInferrer {
   struct AutoClosureWalker;
 
+  RegionAnalysisValueMap &valueMap;
   TransferNonTransferrableDiagnosticEmitter diagnosticEmitter;
 
 public:
   TransferNonTransferrableDiagnosticInferrer(
-      TransferredNonTransferrableInfo info)
-      : diagnosticEmitter(info) {}
+      RegionAnalysisValueMap &valueMap, TransferredNonTransferrableInfo info)
+      : valueMap(valueMap), diagnosticEmitter(info) {}
 
   /// Gathers diagnostics. Returns false if we emitted a "I don't understand
   /// error". If we emit such an error, we should bail without emitting any
@@ -1474,10 +1622,94 @@ class TransferNonTransferrableDiagnosticInferrer {
   bool initForIsolatedPartialApply(
       Operand *op, AbstractClosureExpr *ace,
       std::optional<ActorIsolation> actualCallerIsolation = {});
+
+  bool initForSendingPartialApply(FullApplySite fas, Operand *pai);
+
+  std::optional<unsigned>
+  getIsolatedValuePartialApplyIndex(PartialApplyInst *pai,
+                                    SILValue isolatedValue) {
+    for (auto &paiOp : ApplySite(pai).getArgumentOperands()) {
+      if (valueMap.getTrackableValue(paiOp.get()).getRepresentative() ==
+          isolatedValue) {
+        // isolated_any causes all partial apply parameters to be shifted by 1
+        // due to the implicit isolated any parameter.
+        unsigned isIsolatedAny = pai->getFunctionType()->getIsolation() ==
+                                 SILFunctionTypeIsolation::Erased;
+        return ApplySite(pai).getAppliedArgIndex(paiOp) - isIsolatedAny;
+      }
+    }
+
+    return {};
+  }
 };
 
 } // namespace
 
+bool TransferNonTransferrableDiagnosticInferrer::initForSendingPartialApply(
+    FullApplySite fas, Operand *paiOp) {
+  auto *pai =
+      dyn_cast<PartialApplyInst>(stripFunctionConversions(paiOp->get()));
+  if (!pai)
+    return false;
+
+  // For now we want this to be really narrow and to only apply to closure
+  // literals.
+  auto *ce = pai->getLoc().getAsASTNode<ClosureExpr>();
+  if (!ce)
+    return false;
+
+  // Ok, we now know we have a partial apply and it is a closure literal. Lets
+  // see if we can find the exact thing that caused the closure literal to be
+  // actor isolated.
+  auto isolationInfo = diagnosticEmitter.getIsolationRegionInfo();
+  if (isolationInfo->hasIsolatedValue()) {
+    // Now that we have the value, see if that value is one of our captured
+    // values.
+    auto isolatedValue = isolationInfo->getIsolatedValue();
+    auto matchingElt = getIsolatedValuePartialApplyIndex(pai, isolatedValue);
+    if (matchingElt) {
+      // Ok, we found the matching element. Lets emit our diagnostic!
+      auto capture = ce->getCaptureInfo().getCaptures()[*matchingElt];
+      diagnosticEmitter
+          .emitTypedSendingNeverSendableToSendingClosureParamDirectlyIsolated(
+              ce, capture);
+      return true;
+    }
+  }
+
+  // Ok, we are not tracking an actual isolated value or we do not capture the
+  // isolated value directly... we need to be smarter here. First lets gather up
+  // all non-Sendable values captured by the closure.
+  SmallVector<CapturedValue, 8> nonSendableCaptures;
+  for (auto capture : ce->getCaptureInfo().getCaptures()) {
+    auto *decl = capture.getDecl();
+    auto type = decl->getInterfaceType()->getCanonicalType();
+    auto silType = SILType::getPrimitiveObjectType(type);
+    if (!SILIsolationInfo::isNonSendableType(silType, pai->getFunction()))
+      continue;
+
+    auto *fromDC = decl->getInnermostDeclContext();
+    auto *nom = silType.getNominalOrBoundGenericNominal();
+    if (nom && fromDC) {
+      if (auto diagnosticBehavior =
+              getConcurrencyDiagnosticBehaviorLimit(nom, fromDC)) {
+        if (*diagnosticBehavior == DiagnosticBehavior::Ignore)
+          continue;
+      }
+    }
+    nonSendableCaptures.push_back(capture);
+  }
+
+  // If we do not have any non-Sendable captures... bail.
+  if (nonSendableCaptures.empty())
+    return false;
+
+  // Otherwise, emit the diagnostic.
+  diagnosticEmitter.emitTypedSendingNeverSendableToSendingClosureParam(
+      ce, nonSendableCaptures);
+  return true;
+}
+
 bool TransferNonTransferrableDiagnosticInferrer::initForIsolatedPartialApply(
     Operand *op, AbstractClosureExpr *ace,
     std::optional<ActorIsolation> actualCallerIsolation) {
@@ -1585,6 +1817,11 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
     if (auto fas = FullApplySite::isa(op->getUser())) {
       if (fas.getArgumentParameterInfo(*op).hasOption(
               SILParameterInfo::Sending)) {
+        // Before we do anything, lets see if we are passing a sendable closure
+        // literal. If we do, we want to emit a special error that states which
+        // captured value caused the actual error.
+        if (initForSendingPartialApply(fas, op))
+          return true;
 
         // See if we can infer a name from the value.
         SmallString<64> resultingName;
@@ -1724,7 +1961,8 @@ void TransferNonSendableImpl::emitTransferredNonTransferrableDiagnostics() {
       llvm::dbgs() << "Emitting transfer non transferrable diagnostics.\n");
 
   for (auto info : transferredNonTransferrableInfoList) {
-    TransferNonTransferrableDiagnosticInferrer diagnosticInferrer(info);
+    TransferNonTransferrableDiagnosticInferrer diagnosticInferrer(
+        regionInfo->getValueMap(), info);
     diagnosticInferrer.run();
   }
 }

test/Concurrency/concurrent_value_checking.swift

@@ -413,20 +413,20 @@ extension NotConcurrent {
   func f() { }
 
   func test() {
-    Task { // expected-tns-warning {{task-isolated value of type '() async -> ()' passed as a strongly transferred parameter}}
-      f()
+    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}}
+      f() // expected-tns-note {{closure captures 'self' which is accessible to code in the current task}}
     }
 
-    Task { // expected-tns-warning {{task-isolated value of type '() async -> ()' passed as a strongly transferred parameter}}
-      self.f()
+    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}}
+      self.f() // expected-tns-note {{closure captures 'self' which is accessible to code in the current task}}
     }
 
-    Task { [self] in // expected-tns-warning {{task-isolated value of type '() async -> ()' passed as a strongly transferred parameter}}
-      f()
+    Task { [self] in // 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}}
+      f() // expected-tns-note {{closure captures 'self' which is accessible to code in the current task}}
     }
 
-    Task { [self] in // expected-tns-warning {{task-isolated value of type '() async -> ()' passed as a strongly transferred parameter}}
-      self.f()
+    Task { [self] in // 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}}
+      self.f() // expected-tns-note {{closure captures 'self' which is accessible to code in the current task}}
     }
   }
 }

test/Concurrency/isolated_parameters.swift

@@ -501,9 +501,9 @@ func testNonSendableCaptures(ns: NotSendable, a: isolated MyActor) {
 
   // FIXME: The `a` in the capture list and `isolated a` are the same,
   // but the actor isolation checker doesn't know that.
-  Task { [a] in // expected-tns-warning {{'a'-isolated value of type '() async -> ()' passed as a strongly transferred parameter}}
+  Task { [a] in // expected-tns-warning {{passing closure as a 'sending' parameter risks causing data races between 'a'-isolated code and concurrent execution of the closure}}
     _ = a
-    _ = ns
+    _ = ns // expected-tns-note {{closure captures 'a'-isolated 'ns'}}
   }
 }
 

test/Concurrency/sendable_preconcurrency.swift

@@ -31,10 +31,13 @@ struct MyType3 {
 }
 
 func testA(ns: NS, mt: MyType, mt2: MyType2, mt3: MyType3, sc: StrictClass, nsc: NonStrictClass) async {
-  // This is task isolated since we are capturing function arguments.
-  Task { // expected-tns-warning {{task-isolated value of type '() async -> ()' passed as a strongly transferred parameter}}
+  // This is task isolated since we are capturing function arguments... but
+  // since we are merging NonStrictClass from a preconcurrency module, the whole
+  // error is squelched since we allow for preconcurrency to apply to the entire
+  // capture list.
+  Task {
     print(ns)
-    print(mt) // no warning: MyType is Sendable because we suppressed NonStrictClass's warning
+    print(mt)
     print(mt2)
     print(mt3)
     print(sc)