Merge pull request #73446 from gottesmm/rdar127580781

[region-isolation] Some diagnostic tweaks

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -943,7 +943,7 @@ NOTE(sil_referencebinding_inout_binding_here, none,
 //===----------------------------------------------------------------------===//
 
 NOTE(regionbasedisolation_maybe_race, none,
-     "use here could race", ())
+     "risks concurrent access", ())
 ERROR(regionbasedisolation_unknown_pattern, none,
       "pattern that the region based isolation checker does not understand how to check. Please file a bug",
       ())
@@ -953,10 +953,10 @@ ERROR(regionbasedisolation_unknown_pattern, none,
 //
 
 ERROR(regionbasedisolation_transfer_yields_race_no_isolation, none,
-      "transferring value of non-Sendable type %0; later accesses could race",
+      "sending value of non-Sendable type %0; later accesses could race",
       (Type))
 ERROR(regionbasedisolation_transfer_yields_race_with_isolation, none,
-      "transferring value of non-Sendable type %0 from %1 context to %2 context; later accesses could race",
+      "sending value of non-Sendable type %0 from %1 context to %2 context; later accesses could race",
       (Type, ActorIsolation, ActorIsolation))
 ERROR(regionbasedisolation_isolated_capture_yields_race, none,
       "%1 closure captures value of non-Sendable type %0 from %2 context; later accesses to value could race",
@@ -965,7 +965,7 @@ ERROR(regionbasedisolation_transfer_yields_race_stronglytransferred_binding, non
       "value of non-Sendable type %0 accessed after being transferred; later accesses could race",
       (Type))
 ERROR(regionbasedisolation_arg_transferred, none,
-      "%0 value of type %1 transferred to %2 context; later accesses to value could race",
+      "sending %0 value of type %1 to %2 context; later accesses to value could race",
       (StringRef, Type, ActorIsolation))
 ERROR(regionbasedisolation_arg_passed_to_strongly_transferred_param, none,
       "%0 value of type %1 passed as a strongly transferred parameter; later accesses could race",
@@ -979,31 +979,38 @@ NOTE(regionbasedisolation_isolated_since_in_same_region_basename, none,
 //
 
 ERROR(regionbasedisolation_named_transfer_yields_race, none,
-      "transferring %0 may cause a data race",
+      "sending %0 risks causing data races",
       (Identifier))
 
 NOTE(regionbasedisolation_named_info_transfer_yields_race, none,
-     "transferring %1 %0 to %2 callee could cause races in between callee %2 and local %3 uses",
+     "sending %1%0 to %2 callee risks causing data races between %2 and local %3 uses",
      (Identifier, StringRef, ActorIsolation, ActorIsolation))
+NOTE(regionbasedisolation_named_info_transfer_yields_race_callee, none,
+     "sending %1%0 to %2 %3 %4 risks causing data races between %2 and local %5 uses",
+     (Identifier, StringRef, ActorIsolation, DescriptiveDeclKind, DeclName, 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))
+      "sending %1%0 to %2 callee risks causing data races between %2 and %3 uses",
+      (Identifier, StringRef, ActorIsolation, StringRef))
+NOTE(regionbasedisolation_named_transfer_non_transferrable_callee, none,
+      "sending %1%0 to %2 %3 %4 risks causing data races between %2 and %5 uses",
+      (Identifier, StringRef, ActorIsolation, DescriptiveDeclKind, DeclName, StringRef))
 NOTE(regionbasedisolation_named_transfer_into_transferring_param, none,
-     "%0 %1 is passed as a transferring parameter; Uses in callee may race with later %0 uses",
+     "%0%1 is passed as a transferring parameter; Uses in callee may race with later %0uses",
      (StringRef, Identifier))
 NOTE(regionbasedisolation_named_notransfer_transfer_into_result, none,
-     "%0 %1 cannot be a transferring result. %0 uses may race with caller uses",
-     (StringRef, Identifier))
+     "%0%1 cannot be a transferring result. %2 uses may race with caller uses",
+     (StringRef, Identifier, StringRef))
 NOTE(regionbasedisolation_named_stronglytransferred_binding, none,
       "%0 used after being passed as a transferring parameter; Later uses could race",
       (Identifier))
 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",
+      "%0%1 is captured by a %2 closure. %2 uses in closure may race against later %3 uses",
       (StringRef, Identifier, ActorIsolation, ActorIsolation))
 
 // Misc Error.
 ERROR(regionbasedisolation_task_or_actor_isolated_transferred, none,
-      "task or actor isolated value cannot be transferred", ())
+      "task or actor isolated value cannot be sent", ())
 
 // TODO: print the name of the nominal type
 ERROR(deinit_not_visible, none,

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -85,6 +85,50 @@ getDiagnosticBehaviorLimitForValue(SILValue value) {
              : DiagnosticBehavior::Ignore;
 }
 
+static std::optional<SILDeclRef> getDeclRefForCallee(SILInstruction *inst) {
+  auto fas = FullApplySite::isa(inst);
+  if (!fas)
+    return {};
+
+  SILValue calleeOrigin = fas.getCalleeOrigin();
+
+  while (true) {
+    // Intentionally don't lookup through dynamic_function_ref and
+    // previous_dynamic_function_ref as the target of those functions is not
+    // statically known.
+    if (auto *fri = dyn_cast<FunctionRefInst>(calleeOrigin)) {
+      if (auto *callee = fri->getReferencedFunctionOrNull()) {
+        if (auto declRef = callee->getDeclRef())
+          return declRef;
+      }
+    }
+
+    if (auto *mi = dyn_cast<MethodInst>(calleeOrigin)) {
+      return mi->getMember();
+    }
+
+    if (auto *pai = dyn_cast<PartialApplyInst>(calleeOrigin)) {
+      calleeOrigin = pai->getCalleeOrigin();
+      continue;
+    }
+
+    return {};
+  }
+}
+
+static std::optional<std::pair<DescriptiveDeclKind, DeclName>>
+getTransferringApplyCalleeInfo(SILInstruction *inst) {
+  auto declRef = getDeclRefForCallee(inst);
+  if (!declRef)
+    return {};
+
+  auto *decl = declRef->getDecl();
+  if (!decl->hasName())
+    return {};
+
+  return {{decl->getDescriptiveKind(), decl->getName()}};
+}
+
 static Expr *inferArgumentExprFromApplyExpr(ApplyExpr *sourceApply,
                                             FullApplySite fai,
                                             const Operand *op) {
@@ -472,6 +516,12 @@ class UseAfterTransferDiagnosticEmitter {
     return getDiagnosticBehaviorLimitForValue(transferOp->get());
   }
 
+  /// If we can find a callee decl name, return that. None otherwise.
+  std::optional<std::pair<DescriptiveDeclKind, DeclName>>
+  getTransferringCalleeInfo() const {
+    return getTransferringApplyCalleeInfo(transferOp->getUser());
+  }
+
   void
   emitNamedIsolationCrossingError(SILLocation loc, Identifier name,
                                   SILIsolationInfo namedValuesIsolationInfo,
@@ -485,13 +535,26 @@ class UseAfterTransferDiagnosticEmitter {
     // Then emit the note with greater context.
     SmallString<64> descriptiveKindStr;
     {
-      llvm::raw_svector_ostream os(descriptiveKindStr);
-      namedValuesIsolationInfo.printForDiagnostics(os);
+      if (!namedValuesIsolationInfo.isDisconnected()) {
+        llvm::raw_svector_ostream os(descriptiveKindStr);
+        namedValuesIsolationInfo.printForDiagnostics(os);
+        os << ' ';
+      }
+    }
+
+    if (auto calleeInfo = getTransferringCalleeInfo()) {
+      diagnoseNote(
+          loc,
+          diag::regionbasedisolation_named_info_transfer_yields_race_callee,
+          name, descriptiveKindStr, isolationCrossing.getCalleeIsolation(),
+          calleeInfo->first, calleeInfo->second,
+          isolationCrossing.getCallerIsolation());
+    } else {
+      diagnoseNote(
+          loc, diag::regionbasedisolation_named_info_transfer_yields_race, name,
+          descriptiveKindStr, isolationCrossing.getCalleeIsolation(),
+          isolationCrossing.getCallerIsolation());
     }
-    diagnoseNote(
-        loc, diag::regionbasedisolation_named_info_transfer_yields_race, name,
-        descriptiveKindStr, isolationCrossing.getCalleeIsolation(),
-        isolationCrossing.getCallerIsolation());
     emitRequireInstDiagnostics();
   }
 
@@ -557,8 +620,11 @@ class UseAfterTransferDiagnosticEmitter {
 
     SmallString<64> descriptiveKindStr;
     {
-      llvm::raw_svector_ostream os(descriptiveKindStr);
-      namedValuesIsolationInfo.printForDiagnostics(os);
+      if (!namedValuesIsolationInfo.isDisconnected()) {
+        llvm::raw_svector_ostream os(descriptiveKindStr);
+        namedValuesIsolationInfo.printForDiagnostics(os);
+        os << ' ';
+      }
     }
 
     diagnoseNote(
@@ -987,6 +1053,12 @@ class TransferNonTransferrableDiagnosticEmitter {
     return getDiagnosticBehaviorLimitForValue(info.transferredOperand->get());
   }
 
+  /// If we can find a callee decl name, return that. None otherwise.
+  std::optional<std::pair<DescriptiveDeclKind, DeclName>>
+  getTransferringCalleeInfo() const {
+    return getTransferringApplyCalleeInfo(info.transferredOperand->getUser());
+  }
+
   /// Return the isolation region info for \p getNonTransferrableValue().
   SILIsolationInfo getIsolationRegionInfo() const {
     return info.isolationRegionInfo;
@@ -1013,8 +1085,7 @@ class TransferNonTransferrableDiagnosticEmitter {
       getIsolationRegionInfo().printForDiagnostics(os);
     }
     diagnoseError(loc, diag::regionbasedisolation_arg_transferred,
-                  StringRef(descriptiveKindStr), type,
-                  crossing.getCalleeIsolation())
+                  descriptiveKindStr, type, crossing.getCalleeIsolation())
         .highlight(getOperand()->getUser()->getLoc().getSourceRange())
         .limitBehaviorIf(getBehaviorLimit());
   }
@@ -1024,8 +1095,11 @@ class TransferNonTransferrableDiagnosticEmitter {
     emitNamedOnlyError(loc, name);
     SmallString<64> descriptiveKindStr;
     {
-      llvm::raw_svector_ostream os(descriptiveKindStr);
-      getIsolationRegionInfo().printForDiagnostics(os);
+      if (!getIsolationRegionInfo().isDisconnected()) {
+        llvm::raw_svector_ostream os(descriptiveKindStr);
+        getIsolationRegionInfo().printForDiagnostics(os);
+        os << ' ';
+      }
     }
     diagnoseNote(loc,
                  diag::regionbasedisolation_named_isolated_closure_yields_race,
@@ -1059,22 +1133,42 @@ class TransferNonTransferrableDiagnosticEmitter {
                           ApplyIsolationCrossing isolationCrossing) {
     emitNamedOnlyError(loc, name);
     SmallString<64> descriptiveKindStr;
+    SmallString<64> descriptiveKindStrWithSpace;
     {
-      llvm::raw_svector_ostream os(descriptiveKindStr);
-      getIsolationRegionInfo().printForDiagnostics(os);
+      if (!getIsolationRegionInfo().isDisconnected()) {
+        {
+          llvm::raw_svector_ostream os(descriptiveKindStr);
+          getIsolationRegionInfo().printForDiagnostics(os);
+        }
+        descriptiveKindStrWithSpace = descriptiveKindStr;
+        descriptiveKindStrWithSpace.push_back(' ');
+      }
+    }
+    if (auto calleeInfo = getTransferringCalleeInfo()) {
+      diagnoseNote(
+          loc,
+          diag::regionbasedisolation_named_transfer_non_transferrable_callee,
+          name, descriptiveKindStrWithSpace,
+          isolationCrossing.getCalleeIsolation(), calleeInfo->first,
+          calleeInfo->second, descriptiveKindStr);
+    } else {
+      diagnoseNote(loc,
+                   diag::regionbasedisolation_named_transfer_non_transferrable,
+                   name, descriptiveKindStrWithSpace,
+                   isolationCrossing.getCalleeIsolation(), descriptiveKindStr);
     }
-    diagnoseNote(
-        loc, diag::regionbasedisolation_named_transfer_non_transferrable, name,
-        descriptiveKindStr, isolationCrossing.getCalleeIsolation());
   }
 
   void emitNamedFunctionArgumentApplyStronglyTransferred(SILLocation loc,
                                                          Identifier varName) {
     emitNamedOnlyError(loc, varName);
     SmallString<64> descriptiveKindStr;
     {
-      llvm::raw_svector_ostream os(descriptiveKindStr);
-      getIsolationRegionInfo().printForDiagnostics(os);
+      if (!getIsolationRegionInfo().isDisconnected()) {
+        llvm::raw_svector_ostream os(descriptiveKindStr);
+        getIsolationRegionInfo().printForDiagnostics(os);
+        os << ' ';
+      }
     }
     auto diag =
         diag::regionbasedisolation_named_transfer_into_transferring_param;
@@ -1084,13 +1178,21 @@ class TransferNonTransferrableDiagnosticEmitter {
   void emitNamedTransferringReturn(SILLocation loc, Identifier varName) {
     emitNamedOnlyError(loc, varName);
     SmallString<64> descriptiveKindStr;
+    SmallString<64> descriptiveKindStrWithSpace;
     {
-      llvm::raw_svector_ostream os(descriptiveKindStr);
-      getIsolationRegionInfo().printForDiagnostics(os);
+      if (!getIsolationRegionInfo().isDisconnected()) {
+        {
+          llvm::raw_svector_ostream os(descriptiveKindStr);
+          getIsolationRegionInfo().printForDiagnostics(os);
+        }
+        descriptiveKindStrWithSpace = descriptiveKindStr;
+        descriptiveKindStrWithSpace.push_back(' ');
+      }
     }
     auto diag =
         diag::regionbasedisolation_named_notransfer_transfer_into_result;
-    diagnoseNote(loc, diag, descriptiveKindStr, varName);
+    diagnoseNote(loc, diag, descriptiveKindStrWithSpace, varName,
+                 descriptiveKindStr);
   }
 
 private:

test/ClangImporter/transferring.swift

@@ -27,14 +27,14 @@ func funcTestTransferringResult() async {
   // Just to show that without the transferring param, we generate diagnostics.
   let x2 = NonSendableCStruct()
   let y2 = returnUserDefinedFromGlobalFunction(x2)
-  await transferToMain(x2) // expected-error {{transferring 'x2' may cause a data race}}
-  // expected-note @-1 {{transferring disconnected 'x2' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
-  useValue(y2) // expected-note {{use here could race}}
+  await transferToMain(x2) // expected-error {{sending 'x2' risks causing data races}}
+  // expected-note @-1 {{sending 'x2' to main actor-isolated global function 'transferToMain' risks causing data races between main actor-isolated and local nonisolated uses}}
+  useValue(y2) // expected-note {{risks concurrent access}}
 }
 
 func funcTestTransferringArg() async {
   let x = NonSendableCStruct()
-  transferUserDefinedIntoGlobalFunction(x) // expected-error {{transferring 'x' may cause a data race}}
+  transferUserDefinedIntoGlobalFunction(x) // expected-error {{sending 'x' risks causing data races}}
   // expected-note @-1 {{'x' used after being passed as a transferring parameter}}
-  useValue(x) // expected-note {{use here could race}}
+  useValue(x) // expected-note {{risks concurrent access}}
 }

test/ClangImporter/transferring_objc.swift

@@ -25,9 +25,9 @@ func methodTestTransferringResult() async {
 func methodTestTransferringArg() async {
   let x = MyType()
   let s = NSObject()
-  let _ = x.getResultWithTransferringArgument(s)  // expected-error {{transferring 's' may cause a data race}}
+  let _ = x.getResultWithTransferringArgument(s)  // expected-error {{sending 's' risks causing data races}}
   // expected-note @-1 {{'s' used after being passed as a transferring parameter; Later uses could race}}
-  useValue(s) // expected-note {{use here could race}}
+  useValue(s) // expected-note {{risks concurrent access}}
 }
 
 // Make sure we just ignore the swift_attr if it is applied to something like a
@@ -45,14 +45,14 @@ func funcTestTransferringResult() async {
   // Just to show that without the transferring param, we generate diagnostics.
   let x2 = NSObject()
   let y2 = returnNSObjectFromGlobalFunction(x2)
-  await transferToMain(x2) // expected-error {{transferring 'x2' may cause a data race}}
-  // expected-note @-1 {{transferring disconnected 'x2' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
-  useValue(y2) // expected-note {{use here could race}}
+  await transferToMain(x2) // expected-error {{sending 'x2' risks causing data races}}
+  // expected-note @-1 {{sending 'x2' to main actor-isolated global function 'transferToMain' risks causing data races between main actor-isolated and local nonisolated uses}}
+  useValue(y2) // expected-note {{risks concurrent access}}
 }
 
 func funcTestTransferringArg() async {
   let x = NSObject()
-  transferNSObjectToGlobalFunction(x) // expected-error {{transferring 'x' may cause a data race}}
+  transferNSObjectToGlobalFunction(x) // expected-error {{sending 'x' risks causing data races}}
   // expected-note @-1 {{'x' used after being passed as a transferring parameter; Later uses could race}}
-  useValue(x) // expected-note {{use here could race}}
+  useValue(x) // expected-note {{risks concurrent access}}
 }

test/Concurrency/async_task_locals_basic_warnings_bug_isolation.swift

@@ -18,7 +18,7 @@ actor Test {
                      _ body: (consuming NonSendableValue, isolated (any Actor)?) -> Void) async {
     Self.$local.withValue(12) {
       // Unexpected errors here:
-      //  error: unexpected warning produced: transferring 'body' may cause a data race; this is an error in the Swift 6 language mode
+      //  error: unexpected warning produced: transferring 'body' risks causing data races; this is an error in the Swift 6 language mode
       //  error: unexpected note produced: actor-isolated 'body' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses
       body(NonSendableValue(), isolation)
     }

test/Concurrency/isolated_captures.swift

@@ -40,7 +40,7 @@ class NotSendable {
   MyActor.ns = ns
 
   await { @YourActor in
-    // expected-region-isolation-warning @+3 {{transferring 'ns' may cause a data race}}
+    // expected-region-isolation-warning @+3 {{sending 'ns' risks causing data races}}
     // expected-region-isolation-note @+2 {{global actor 'MyActor'-isolated 'ns' is captured by a global actor 'YourActor'-isolated closure. global actor 'YourActor'-isolated uses in closure may race against later global actor 'MyActor'-isolated uses}}
     // expected-complete-warning@+1 {{capture of 'ns' with non-sendable type 'NotSendable' in an isolated closure; this is an error in the Swift 6 language mode}}
     YourActor.ns = ns
@@ -62,7 +62,7 @@ class NotSendable {
   ns.stash()
 
   await { @YourActor in
-    // expected-region-isolation-warning @+3 {{transferring 'ns' may cause a data race}}
+    // expected-region-isolation-warning @+3 {{sending 'ns' risks causing data races}}
     // expected-region-isolation-note @+2 {{global actor 'MyActor'-isolated 'ns' is captured by a global actor 'YourActor'-isolated closure. global actor 'YourActor'-isolated uses in closure may race against later global actor 'MyActor'-isolated uses}}
     // expected-complete-warning@+1 {{capture of 'ns' with non-sendable type 'NotSendable' in an isolated closure; this is an error in the Swift 6 language mode}}
     YourActor.ns = ns
@@ -83,7 +83,7 @@ class NotSendable {
   let ns = NotSendable()
 
   await { @YourActor in
-    // expected-region-isolation-warning @+3 {{transferring 'ns' may cause a data race}}
+    // expected-region-isolation-warning @+3 {{sending 'ns' risks causing data races}}
     // expected-region-isolation-note @+2 {{global actor 'MyActor'-isolated 'ns' is captured by a global actor 'YourActor'-isolated closure. global actor 'YourActor'-isolated uses in closure may race against later global actor 'MyActor'-isolated uses}}
     // expected-complete-warning@+1 {{capture of 'ns' with non-sendable type 'NotSendable' in an isolated closure; this is an error in the Swift 6 language mode}}
     YourActor.ns = ns