[region-isolation] Eliminate more "call site passes self" warnings

I just did a full pass through. There were some cases around nonisolated
closures defined in methods and global actor isolated things where we are now
emitting the wrong message. I am going to fix that in subsequent commits.

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -976,6 +976,9 @@ ERROR(regionbasedisolation_named_transfer_yields_race, none,
 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))
+NOTE(regionbasedisolation_transfer_non_transferrable_named_note, none,
+      "transferring %1 %0 to %2 callee could cause races between %2 and %1 uses",
+      (Identifier, ActorIsolation, ActorIsolation))
 NOTE(regionbasedisolation_named_info_isolated_capture, none,
      "%1 value %0 is captured by %2 closure. Later local uses could race",
      (Identifier, ActorIsolation, ActorIsolation))

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -51,6 +51,43 @@ using Region = PartitionPrimitives::Region;
 
 } // namespace
 
+//===----------------------------------------------------------------------===//
+//                              MARK: Utilities
+//===----------------------------------------------------------------------===//
+
+static Expr *inferArgumentExprFromApplyExpr(ApplyExpr *sourceApply,
+                                            FullApplySite fai,
+                                            const Operand *op) {
+
+  Expr *foundExpr = nullptr;
+
+  // If we have self, then infer it.
+  if (fai.hasSelfArgument() && op == &fai.getSelfArgumentOperand()) {
+    if (auto callExpr = dyn_cast<CallExpr>(sourceApply))
+      if (auto calledExpr =
+              dyn_cast<DotSyntaxCallExpr>(callExpr->getDirectCallee()))
+        foundExpr = calledExpr->getBase();
+  } else {
+    // Otherwise, try to infer using the operand of the ApplyExpr.
+    unsigned argNum = [&]() -> unsigned {
+      if (fai.isCalleeOperand(*op))
+        return op->getOperandNumber();
+      return fai.getAppliedArgIndex(*op);
+    }();
+    assert(argNum < sourceApply->getArgs()->size());
+
+    foundExpr = sourceApply->getArgs()->getExpr(argNum);
+
+    // If we have an erasure expression, lets use the original type. We do
+    // this since we are not saying the specific parameter that is the
+    // issue and we are using the type to explain it to the user.
+    if (auto *erasureExpr = dyn_cast<ErasureExpr>(foundExpr))
+      foundExpr = erasureExpr->getSubExpr();
+  }
+
+  return foundExpr;
+}
+
 //===----------------------------------------------------------------------===//
 //                             MARK: Diagnostics
 //===----------------------------------------------------------------------===//
@@ -578,22 +615,7 @@ void UseAfterTransferDiagnosticInferrer::initForApply(const Operand *op,
 
   assert(!fai.getArgumentConvention(*op).isIndirectOutParameter() &&
          "An indirect out parameter is never transferred");
-
-  Expr *foundExpr = nullptr;
-
-  // If we have self, then infer it.
-  if (fai.hasSelfArgument() && op == &fai.getSelfArgumentOperand()) {
-    foundExpr = getFoundExprForSelf(sourceApply);
-  } else {
-    // Otherwise, try to infer using the operand of the ApplyExpr.
-    unsigned argNum = [&]() -> unsigned {
-      if (fai.isCalleeOperand(*op))
-        return op->getOperandNumber();
-      return fai.getAppliedArgIndex(*op);
-    }();
-    assert(argNum < sourceApply->getArgs()->size());
-    foundExpr = getFoundExprForParam(sourceApply, argNum);
-  }
+  auto *foundExpr = inferArgumentExprFromApplyExpr(sourceApply, fai, op);
 
   auto inferredArgType =
       foundExpr ? foundExpr->findOriginalType() : baseInferredType;
@@ -727,7 +749,9 @@ void UseAfterTransferDiagnosticInferrer::init(const Operand *op) {
     // Before we do anything further, see if we can find a name and emit a name
     // error.
     SmallString<64> resultingName;
-    VariableNameInferrer inferrer(op->getFunction(), resultingName);
+    VariableNameInferrer::Options options;
+    options |= VariableNameInferrer::Flag::InferSelfThroughAllAccessors;
+    VariableNameInferrer inferrer(op->getFunction(), options, resultingName);
     auto &astContext = op->getFunction()->getASTContext();
     if (auto rootValue =
             inferrer.inferByWalkingUsesToDefsReturningRoot(op->get())) {
@@ -946,19 +970,41 @@ class TransferNonTransferrableDiagnosticInferrer {
     /// Used if we have a function argument passed as an explicitly strongly
     /// transferring argument to a function.
     FunctionArgumentApplyStronglyTransferred = 4,
+
+    /// Used if we have a named variable.
+    NamedIsolation = 5,
   };
 
-  struct UseDiagnosticInfo {
+  class UseDiagnosticInfo {
     UseDiagnosticInfoKind kind;
     std::optional<ApplyIsolationCrossing> transferredIsolationCrossing = {};
+    llvm::PointerUnion<Type, Identifier> inferredTypeOrIdentifier = {};
+
+  public:
+    UseDiagnosticInfoKind getKind() const { return kind; }
+    ApplyIsolationCrossing getIsolationCrossing() const {
+      return transferredIsolationCrossing.value();
+    }
+    Identifier getName() const {
+      return inferredTypeOrIdentifier.get<Identifier>();
+    }
+    Type getType() const { return inferredTypeOrIdentifier.get<Type>(); }
 
     static UseDiagnosticInfo forMiscUse() {
       return {UseDiagnosticInfoKind::MiscUse};
     }
 
     static UseDiagnosticInfo
-    forFunctionArgumentApply(ApplyIsolationCrossing isolation) {
-      return {UseDiagnosticInfoKind::FunctionArgumentApply, isolation};
+    forNamed(Identifier valueName, ApplyIsolationCrossing isolationCrossing) {
+      return {UseDiagnosticInfoKind::NamedIsolation, isolationCrossing,
+              valueName};
+    }
+
+    static UseDiagnosticInfo
+    forFunctionArgumentApply(ApplyIsolationCrossing isolation,
+                             Type inferredType) {
+      return {UseDiagnosticInfoKind::FunctionArgumentApply, isolation,
+              inferredType};
     }
 
     static UseDiagnosticInfo
@@ -971,9 +1017,12 @@ class TransferNonTransferrableDiagnosticInferrer {
     }
 
   private:
-    UseDiagnosticInfo(UseDiagnosticInfoKind kind,
-                      std::optional<ApplyIsolationCrossing> isolation = {})
-        : kind(kind), transferredIsolationCrossing(isolation) {}
+    UseDiagnosticInfo(
+        UseDiagnosticInfoKind kind,
+        std::optional<ApplyIsolationCrossing> isolation = {},
+        llvm::PointerUnion<Type, Identifier> inferredTypeOrIdentifier = {})
+        : kind(kind), transferredIsolationCrossing(isolation),
+          inferredTypeOrIdentifier(inferredTypeOrIdentifier) {}
   };
 
 private:
@@ -1024,15 +1073,12 @@ bool TransferNonTransferrableDiagnosticInferrer::initForIsolatedPartialApply(
 }
 
 bool TransferNonTransferrableDiagnosticInferrer::run() {
-  if (!isa<SILFunctionArgument>(info.nonTransferrableValue)) {
-    diagnosticInfo = UseDiagnosticInfo::forMiscUse();
-    return true;
-  }
-
   // We need to find the isolation info.
   auto loc = info.transferredOperand->getUser()->getLoc();
 
   if (auto *sourceApply = loc.getAsASTNode<ApplyExpr>()) {
+    auto *op = info.transferredOperand;
+
     std::optional<ApplyIsolationCrossing> isolation = {};
 
     // First try to get the apply from the isolation crossing.
@@ -1042,9 +1088,9 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
     // If we could not infer an isolation...
     if (!isolation) {
       // First see if we have a transferring argument.
-      if (auto fas = FullApplySite::isa(info.transferredOperand->getUser())) {
-        if (fas.getArgumentParameterInfo(*info.transferredOperand)
-                .hasOption(SILParameterInfo::Transferring)) {
+      if (auto fas = FullApplySite::isa(op->getUser())) {
+        if (fas.getArgumentParameterInfo(*op).hasOption(
+                SILParameterInfo::Transferring)) {
           diagnosticInfo =
               UseDiagnosticInfo::forFunctionArgumentApplyStronglyTransferred();
           return true;
@@ -1053,12 +1099,35 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
 
       // Otherwise, emit a "we don't know error" that tells the user to file a
       // bug.
-      diagnoseError(info.transferredOperand->getUser(),
-                    diag::regionbasedisolation_unknown_pattern);
+      diagnoseError(op->getUser(), diag::regionbasedisolation_unknown_pattern);
       return false;
     }
+    assert(isolation && "Expected non-null");
+
+    // See if we can infer a name from the value.
+    SmallString<64> resultingName;
+    VariableNameInferrer::Options options;
+    options |= VariableNameInferrer::Flag::InferSelfThroughAllAccessors;
+    VariableNameInferrer inferrer(op->getFunction(), options, resultingName);
+    if (inferrer.inferByWalkingUsesToDefsReturningRoot(op->get())) {
+      auto &astContext = op->getFunction()->getASTContext();
+      diagnosticInfo = UseDiagnosticInfo::forNamed(
+          astContext.getIdentifier(inferrer.getName()), *isolation);
+      return true;
+    }
 
-    diagnosticInfo = UseDiagnosticInfo::forFunctionArgumentApply(*isolation);
+    // Attempt to find the specific sugared ASTType if we can to emit a better
+    // diagnostic.
+    Type type = op->get()->getType().getASTType();
+    if (auto fas = FullApplySite::isa(info.transferredOperand->getUser())) {
+      if (auto *inferredArgExpr =
+              inferArgumentExprFromApplyExpr(sourceApply, fas, op)) {
+        type = inferredArgExpr->findOriginalType();
+      }
+    }
+
+    diagnosticInfo =
+        UseDiagnosticInfo::forFunctionArgumentApply(*isolation, type);
     return true;
   }
 
@@ -1094,7 +1163,7 @@ void TransferNonSendableImpl::emitTransferredNonTransferrableDiagnostics() {
 
     auto diagnosticInfo = diagnosticInferrer.getDiagnostic();
     auto loc = diagnosticInferrer.getLoc();
-    switch (diagnosticInfo.kind) {
+    switch (diagnosticInfo.getKind()) {
     case UseDiagnosticInfoKind::Invalid:
       llvm_unreachable("Should never see this");
     case UseDiagnosticInfoKind::MiscUse:
@@ -1103,9 +1172,8 @@ void TransferNonSendableImpl::emitTransferredNonTransferrableDiagnostics() {
       break;
     case UseDiagnosticInfoKind::FunctionArgumentApply: {
       diagnoseError(astContext, loc, diag::regionbasedisolation_arg_transferred,
-                    op->get()->getType().getASTType(),
-                    diagnosticInfo.transferredIsolationCrossing.value()
-                        .getCalleeIsolation())
+                    diagnosticInfo.getType(),
+                    diagnosticInfo.getIsolationCrossing().getCalleeIsolation())
           .highlight(op->getUser()->getLoc().getSourceRange());
       // Only emit the note if our value is different from the function
       // argument.
@@ -1124,8 +1192,7 @@ void TransferNonSendableImpl::emitTransferredNonTransferrableDiagnostics() {
     case UseDiagnosticInfoKind::FunctionArgumentClosure: {
       diagnoseError(astContext, loc, diag::regionbasedisolation_arg_transferred,
                     op->get()->getType().getASTType(),
-                    diagnosticInfo.transferredIsolationCrossing.value()
-                        .getCalleeIsolation())
+                    diagnosticInfo.getIsolationCrossing().getCalleeIsolation())
           .highlight(op->getUser()->getLoc().getSourceRange());
       // Only emit the note if our value is different from the function
       // argument.
@@ -1149,6 +1216,17 @@ void TransferNonSendableImpl::emitTransferredNonTransferrableDiagnostics() {
           .highlight(op->getUser()->getLoc().getSourceRange());
       break;
     }
+    case UseDiagnosticInfoKind::NamedIsolation:
+      diagnoseError(astContext, loc,
+                    diag::regionbasedisolation_named_transfer_yields_race,
+                    diagnosticInfo.getName());
+      diagnoseNote(
+          astContext, loc,
+          diag::regionbasedisolation_transfer_non_transferrable_named_note,
+          diagnosticInfo.getName(),
+          diagnosticInfo.getIsolationCrossing().getCallerIsolation(),
+          diagnosticInfo.getIsolationCrossing().getCalleeIsolation());
+      break;
     }
   }
 }

test/Concurrency/isolated_captures.swift

@@ -39,7 +39,8 @@ class NotSendable {
   let ns = NotSendable(x: 0)
   MyActor.ns = ns
 
-  // expected-region-isolation-warning@+1 {{call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller; this is an error in Swift 6}}
+  // expected-region-isolation-warning @+2 {{transferring 'ns' could cause a race}}
+  // expected-region-isolation-note @+1 {{transferring global actor 'MyActor'-isolated 'ns' to global actor 'YourActor'-isolated callee could cause races between global actor 'YourActor'-isolated and global actor 'MyActor'-isolated uses}}
   await { @YourActor in
     // expected-complete-warning@+1 {{capture of 'ns' with non-sendable type 'NotSendable' in an isolated closure; this is an error in Swift 6}}
     YourActor.ns = ns

test/Concurrency/sendable_checking.swift

@@ -251,11 +251,13 @@ final class NonSendable {
   func call() async {
     await 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 {{task isolated value of type 'NonSendable' transferred to main actor-isolated context; later accesses to value could race}}
+    // expected-tns-warning @-2 {{transferring 'self' could cause a race}}
+    // expected-tns-note @-3 {{transferring nonisolated 'self' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
 
     await self.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 {{task isolated value of type 'NonSendable' transferred to main actor-isolated context; later accesses to value could race}}
+    // expected-tns-warning @-2 {{transferring 'self' could cause a race}}
+    // expected-tns-note @-3 {{transferring nonisolated 'self' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
 
     _ = await x
     // expected-warning@-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to main actor-isolated property 'x' cannot cross actor boundary}}
@@ -293,13 +295,14 @@ func callNonisolatedAsyncClosure(
 ) async {
   await g(ns)
   // expected-targeted-and-complete-warning @-1 {{passing argument of non-sendable type 'NonSendable' outside of main actor-isolated context may introduce data races}}
-  // expected-tns-warning @-2 {{task isolated value of type 'NonSendable' transferred to nonisolated context; later accesses to value could race}}
+  // expected-tns-warning @-2 {{transferring 'ns' could cause a race}}
+  // expected-tns-note @-3 {{transferring main actor-isolated 'ns' to nonisolated callee could cause races between nonisolated and main actor-isolated uses}}
 
   let f: (NonSendable) async -> () = globalSendable // okay
   await f(ns)
   // expected-targeted-and-complete-warning@-1 {{passing argument of non-sendable type 'NonSendable' outside of main actor-isolated context may introduce data races}}
-  // expected-tns-warning @-2 {{task isolated value of type 'NonSendable' transferred to nonisolated context; later accesses to value could race}}
-
+  // expected-tns-warning @-2 {{transferring 'ns' could cause a race}}
+  // expected-tns-note @-3 {{transferring main actor-isolated 'ns' to nonisolated callee could cause races between nonisolated and main actor-isolated uses}}
 }
 
 @available(SwiftStdlib 5.1, *)