Merge pull request swiftlang#72247 from gottesmm/diagnostic-improvements

[region-isolation] Change "pass non-transferable non-sendable value as transferring parameter" error to use the variables name + some other small improvements

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -939,8 +939,6 @@ ERROR(regionbasedisolation_unknown_pattern, none,
 // Old Transfer Non Sendable Diagnostics
 //
 
-ERROR(regionbasedisolation_selforargtransferred, none,
-      "call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller", ())
 ERROR(regionbasedisolation_transfer_yields_race_no_isolation, none,
       "transferring value of non-Sendable type %0; later accesses could race",
       (Type))
@@ -957,8 +955,8 @@ ERROR(regionbasedisolation_arg_transferred, none,
       "%0 value of type %1 transferred to %2 context; later accesses to value could race",
       (StringRef, Type, ActorIsolation))
 ERROR(regionbasedisolation_arg_passed_to_strongly_transferred_param, none,
-      "task-isolated value of type %0 passed as a strongly transferred parameter; later accesses could race",
-      (Type))
+      "%0 value of type %1 passed as a strongly transferred parameter; later accesses could race",
+      (StringRef, Type))
 NOTE(regionbasedisolation_isolated_since_in_same_region_basename, none,
      "value is %0 since it is in the same region as %1",
      (StringRef, DeclBaseName))
@@ -973,9 +971,9 @@ ERROR(regionbasedisolation_named_transfer_yields_race, none,
 ERROR(regionbasedisolation_stronglytransfer_assignment_yields_race_name, none,
       "assigning %0 to transferring parameter %1 may cause a race",
       (Identifier, Identifier))
-NOTE(regionbasedisolation_stronglytransfer_taskisolated_assign_note, none,
-      "%0 is a task-isolated value that is assigned into transferring parameter %1. Transferred uses of %1 may race with caller uses of %0",
-      (Identifier, Identifier))
+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",
+      (StringRef, 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",
@@ -991,6 +989,15 @@ NOTE(regionbasedisolation_named_arg_info, none,
      (Identifier))
 NOTE(regionbasedisolation_named_stronglytransferred_binding, none,
      "Cannot access a transferring parameter after the parameter has been transferred", ())
+NOTE(regionbasedisolation_note_arg_passed_to_strongly_transferred_param, none,
+     "%0 value of type %1 passed as a strongly transferred parameter; later accesses could race",
+     (StringRef, Identifier, Type))
+ERROR(regionbasedisolation_named_arg_passed_to_strongly_transferred_param, none,
+      "%0 %1 passed as a strongly transferred parameter; Uses in callee could race with later %0 uses",
+      (StringRef, Identifier))
+
+ERROR(regionbasedisolation_task_or_actor_isolated_transferred, none,
+      "task or actor isolated value cannot be transferred", ())
 
 // TODO: print the name of the nominal type
 ERROR(deinit_not_visible, none,

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -912,24 +912,25 @@ class TransferNonTransferrableDiagnosticEmitter {
                   diag::regionbasedisolation_unknown_pattern);
   }
 
-  void emitMiscUses(SILLocation loc) {
-    diagnoseError(loc, diag::regionbasedisolation_selforargtransferred);
+  void emitUnknownUse(SILLocation loc) {
+    // TODO: This will eventually be an unknown pattern error.
+    diagnoseError(
+        loc, diag::regionbasedisolation_task_or_actor_isolated_transferred);
   }
 
   void emitFunctionArgumentApply(SILLocation loc, Type type,
-                                 ValueIsolationRegionInfo regionInfo,
                                  ApplyIsolationCrossing crossing) {
     SmallString<64> descriptiveKindStr;
     {
       llvm::raw_svector_ostream os(descriptiveKindStr);
-      regionInfo.printForDiagnostics(os);
+      getIsolationRegionInfo().printForDiagnostics(os);
     }
     diagnoseError(loc, diag::regionbasedisolation_arg_transferred,
                   StringRef(descriptiveKindStr), type,
                   crossing.getCalleeIsolation())
         .highlight(getOperand()->getUser()->getLoc().getSourceRange());
 
-    if (regionInfo.isTaskIsolated()) {
+    if (getIsolationRegionInfo().isTaskIsolated()) {
       auto *fArg =
           cast<SILFunctionArgument>(info.nonTransferrable.get<SILValue>());
       if (fArg->getDecl()) {
@@ -964,23 +965,46 @@ class TransferNonTransferrableDiagnosticEmitter {
 
   void emitFunctionArgumentApplyStronglyTransferred(SILLocation loc,
                                                     Type type) {
-    diagnoseError(
-        loc,
-        diag::regionbasedisolation_arg_passed_to_strongly_transferred_param,
-        type)
+    SmallString<64> descriptiveKindStr;
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      getIsolationRegionInfo().printForDiagnostics(os);
+    }
+    auto diag =
+        diag::regionbasedisolation_arg_passed_to_strongly_transferred_param;
+    diagnoseError(loc, diag, descriptiveKindStr, type)
+        .highlight(getOperand()->getUser()->getLoc().getSourceRange());
+  }
+
+  void emitNamedOnlyError(SILLocation loc, Identifier name) {
+    diagnoseError(loc, diag::regionbasedisolation_named_transfer_yields_race,
+                  name)
         .highlight(getOperand()->getUser()->getLoc().getSourceRange());
   }
 
   void emitNamedIsolation(SILLocation loc, Identifier name,
                           ApplyIsolationCrossing isolationCrossing) {
-    diagnoseError(loc, diag::regionbasedisolation_named_transfer_yields_race,
-                  name);
+    emitNamedOnlyError(loc, name);
     diagnoseNote(
         loc, diag::regionbasedisolation_transfer_non_transferrable_named_note,
         name, isolationCrossing.getCallerIsolation(),
         isolationCrossing.getCalleeIsolation());
   }
 
+  void emitNamedFunctionArgumentApplyStronglyTransferred(
+      SILLocation loc, Identifier varName,
+      ValueIsolationRegionInfo isolationRegionInfo) {
+    emitNamedOnlyError(loc, varName);
+    SmallString<64> descriptiveKindStr;
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      getIsolationRegionInfo().printForDiagnostics(os);
+    }
+    auto diag =
+        diag::regionbasedisolation_named_transfer_into_transferring_param;
+    diagnoseNote(loc, diag, descriptiveKindStr, varName);
+  }
+
 private:
   ASTContext &getASTContext() const {
     return getOperand()->getFunction()->getASTContext();
@@ -1073,29 +1097,35 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
   auto loc = op->getUser()->getLoc();
 
   if (auto *sourceApply = loc.getAsASTNode<ApplyExpr>()) {
-    std::optional<ApplyIsolationCrossing> isolation = {};
+    // First see if we have a transferring argument.
+    if (auto fas = FullApplySite::isa(op->getUser())) {
+      if (fas.getArgumentParameterInfo(*op).hasOption(
+              SILParameterInfo::Transferring)) {
+
+        // See if we can infer a name from the value.
+        SmallString<64> resultingName;
+        if (auto varName = inferNameFromValue(op->get())) {
+          diagnosticEmitter.emitNamedFunctionArgumentApplyStronglyTransferred(
+              loc, *varName, diagnosticEmitter.getIsolationRegionInfo());
+          return true;
+        }
+
+        Type type = op->get()->getType().getASTType();
+        if (auto *inferredArgExpr =
+                inferArgumentExprFromApplyExpr(sourceApply, fas, op)) {
+          type = inferredArgExpr->findOriginalType();
+        }
+        diagnosticEmitter.emitFunctionArgumentApplyStronglyTransferred(loc,
+                                                                       type);
+        return true;
+      }
+    }
 
     // First try to get the apply from the isolation crossing.
-    if (auto value = sourceApply->getIsolationCrossing())
-      isolation = value;
+    auto isolation = sourceApply->getIsolationCrossing();
 
     // If we could not infer an isolation...
     if (!isolation) {
-      // First see if we have a transferring argument.
-      if (auto fas = FullApplySite::isa(op->getUser())) {
-        if (fas.getArgumentParameterInfo(*op).hasOption(
-                SILParameterInfo::Transferring)) {
-          Type type = op->get()->getType().getASTType();
-          if (auto *inferredArgExpr =
-              inferArgumentExprFromApplyExpr(sourceApply, fas, op)) {
-            type = inferredArgExpr->findOriginalType();
-          }
-          diagnosticEmitter.emitFunctionArgumentApplyStronglyTransferred(loc,
-                                                                         type);
-          return true;
-        }
-      }
-
       // Otherwise, emit a "we don't know error" that tells the user to file a
       // bug.
       diagnoseError(op->getUser(), diag::regionbasedisolation_unknown_pattern);
@@ -1120,9 +1150,7 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
       }
     }
 
-    auto isolationRegionInfo = diagnosticEmitter.getIsolationRegionInfo();
-    diagnosticEmitter.emitFunctionArgumentApply(loc, type, isolationRegionInfo,
-                                                *isolation);
+    diagnosticEmitter.emitFunctionArgumentApply(loc, type, *isolation);
     return true;
   }
 
@@ -1137,15 +1165,13 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
   // See if we are in SIL and have an apply site specified isolation.
   if (auto fas = FullApplySite::isa(op->getUser())) {
     if (auto isolation = fas.getIsolationCrossing()) {
-      auto isolationRegionInfo = diagnosticEmitter.getIsolationRegionInfo();
       diagnosticEmitter.emitFunctionArgumentApply(
-          loc, op->get()->getType().getASTType(), isolationRegionInfo,
-          *isolation);
+          loc, op->get()->getType().getASTType(), *isolation);
       return true;
     }
   }
 
-  diagnosticEmitter.emitMiscUses(loc);
+  diagnosticEmitter.emitUnknownUse(loc);
   return true;
 }
 

test/Concurrency/issue-57376.swift

@@ -26,35 +26,35 @@ func testAsyncSequence1Sendable<Seq: AsyncSequence>(_ seq: Seq) async throws whe
 }
 
 func testAsyncSequenceTypedPattern<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-targeted-and-complete-note {{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{54-54=, Sendable}}
-   async let result: Int = seq.reduce(0) { $0 + $1 } // expected-tns-warning {{call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller}}
+   async let result: Int = seq.reduce(0) { $0 + $1 } // expected-tns-warning {{task or actor isolated value cannot be transferred}}
    // expected-warning @-1 {{immutable value 'result' was never used; consider replacing with '_' or removing it}}
    // expected-targeted-and-complete-warning @-2 {{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 
 func testAsyncSequenceTypedPattern1<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-targeted-and-complete-note {{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{55-55=, Sendable}}
-   async let _: Int = seq.reduce(0) { $0 + $1 } // expected-tns-warning {{call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller}}
+   async let _: Int = seq.reduce(0) { $0 + $1 } // expected-tns-warning {{task or actor isolated value cannot be transferred}}
    // expected-targeted-and-complete-warning @-1 {{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 
 func testAsyncSequence<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-targeted-and-complete-note {{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{42-42=, Sendable}}
-   async let result = seq.reduce(0) { $0 + $1 } // expected-tns-warning {{call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller}}
+   async let result = seq.reduce(0) { $0 + $1 } // // expected-tns-warning {{task or actor isolated value cannot be transferred}}
    // expected-warning @-1 {{initialization of immutable value 'result' was never used; consider replacing with assignment to '_' or removing it}}
    // expected-targeted-and-complete-warning @-2 {{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 
 func testAsyncSequence1<Seq: AsyncSequence>(_ seq: Seq) async throws where Seq.Element == Int { // expected-targeted-and-complete-note {{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{43-43=, Sendable}}
-   async let _ = seq.reduce(0) { $0 + $1 } // expected-tns-warning {{call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller}}
+   async let _ = seq.reduce(0) { $0 + $1 } // // expected-tns-warning {{task or actor isolated value cannot be transferred}}
    // expected-targeted-and-complete-warning @-1 {{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 
 func testAsyncSequence3<Seq>(_ seq: Seq) async throws where Seq: AsyncSequence, Seq.Element == Int { // expected-targeted-and-complete-note {{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{28-28=: Sendable}}
-  async let result = seq // expected-tns-warning {{call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller}}
+  async let result = seq // // expected-tns-warning {{task or actor isolated value cannot be transferred}}
   // expected-targeted-and-complete-warning @-1 {{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
    //expected-warning @-2 {{initialization of immutable value 'result' was never used; consider replacing with assignment to '_' or removing it}}
 }
 
 func testAsyncSequence4<Seq>(_ seq: Seq) async throws where Seq: AsyncSequence, Seq.Element == Int { // expected-targeted-and-complete-note {{consider making generic parameter 'Seq' conform to the 'Sendable' protocol}} {{28-28=: Sendable}}
-  async let _ = seq // expected-tns-warning {{call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller}}
+  async let _ = seq // // expected-tns-warning {{task or actor isolated value cannot be transferred}}
    // expected-targeted-and-complete-warning @-1 {{capture of 'seq' with non-sendable type 'Seq' in 'async let' binding}}
 }
 

test/Concurrency/transfernonsendable.sil

@@ -222,39 +222,39 @@ bb0(%0 : @guaranteed $NonSendableStruct):
   debug_value %0 : $NonSendableStruct, var, name "myname"
   %2 = struct_extract %0 : $NonSendableStruct, #NonSendableStruct.ns
   %3 = copy_value %2 : $NonSendableKlass
-  return %3 : $NonSendableKlass // expected-warning {{call site passes `self`}}
+  return %3 : $NonSendableKlass // expected-warning {{task or actor isolated value cannot be transferred}}
 }
 
 sil [ossa] @synchronous_returns_transferring_globalactor_struct_structextract : $@convention(method) (@guaranteed MainActorIsolatedStruct) -> @sil_transferring @owned NonSendableKlass {
 bb0(%0 : @guaranteed $MainActorIsolatedStruct):
   debug_value %0 : $MainActorIsolatedStruct, var, name "myname"
   %2 = struct_extract %0 : $MainActorIsolatedStruct, #MainActorIsolatedStruct.ns
   %3 = copy_value %2 : $NonSendableKlass
-  return %3 : $NonSendableKlass // expected-warning {{call site passes `self`}}
+  return %3 : $NonSendableKlass // expected-warning {{task or actor isolated value cannot be transferred}}
 }
 
 sil [ossa] @synchronous_returns_transferring_globalactor_struct_structelementaddr : $@convention(method) (@in_guaranteed MainActorIsolatedStruct) -> @sil_transferring @owned NonSendableKlass {
 bb0(%0 : $*MainActorIsolatedStruct):
   debug_value %0 : $*MainActorIsolatedStruct, var, name "myname"
   %2 = struct_element_addr %0 : $*MainActorIsolatedStruct, #MainActorIsolatedStruct.ns
   %3 = load [copy] %2 : $*NonSendableKlass
-  return %3 : $NonSendableKlass // expected-warning {{call site passes `self`}}
+  return %3 : $NonSendableKlass // expected-warning {{task or actor isolated value cannot be transferred}}
 }
 
 sil [ossa] @synchronous_returns_transferring_globalactor_enum_uncheckedenumdata : $@convention(method) (@guaranteed MainActorIsolatedEnum) -> @sil_transferring @owned NonSendableKlass {
 bb0(%0 : @guaranteed $MainActorIsolatedEnum):
   debug_value %0 : $MainActorIsolatedEnum, var, name "myname"
   %2 = unchecked_enum_data %0 : $MainActorIsolatedEnum, #MainActorIsolatedEnum.second!enumelt
   %3 = copy_value %2 : $NonSendableKlass
-  return %3 : $NonSendableKlass // expected-warning {{call site passes `self`}}
+  return %3 : $NonSendableKlass // expected-warning {{task or actor isolated value cannot be transferred}}
 }
 
 sil [ossa] @synchronous_returns_transferring_globalactor_enum_uncheckedtakeenumdataaddr : $@convention(method) (@in MainActorIsolatedEnum) -> @sil_transferring @owned NonSendableKlass {
 bb0(%0 : $*MainActorIsolatedEnum):
   debug_value %0 : $*MainActorIsolatedEnum, var, name "myname"
   %2 = unchecked_take_enum_data_addr %0 : $*MainActorIsolatedEnum, #MainActorIsolatedEnum.second!enumelt
   %3 = load [take] %2 : $*NonSendableKlass
-  return %3 : $NonSendableKlass // expected-warning {{call site passes `self`}}
+  return %3 : $NonSendableKlass // expected-warning {{task or actor isolated value cannot be transferred}}
 }
 
 sil [ossa] @synchronous_returns_transferring_globalactor_enum_switchenum : $@convention(method) (@guaranteed MainActorIsolatedEnum) -> @sil_transferring @owned FakeOptional<NonSendableKlass> {
@@ -272,7 +272,7 @@ bb2(%1 : @guaranteed $NonSendableKlass):
   br bb3(%3 : $FakeOptional<NonSendableKlass>)
 
 bb3(%4 : @owned $FakeOptional<NonSendableKlass>):
-  return %4 : $FakeOptional<NonSendableKlass> // expected-warning {{call site passes `self`}}
+  return %4 : $FakeOptional<NonSendableKlass> // expected-warning {{task or actor isolated value cannot be transferred}}
 }
 
 sil [ossa] @warningIfCallingGetter : $@convention(method) @async (@sil_isolated @guaranteed MyActor) -> () {

test/Concurrency/transfernonsendable_strong_transferring_params.swift

@@ -333,10 +333,19 @@ func testTransferOtherParamTuple(_ x: transferring Klass, y: (Klass, Klass)) asy
   x = y.0
 }
 
-func useSugaredTypeNameWhenEmittingTaskIsolationError(_ x: @escaping @MainActor () async -> ()) {
+func taskIsolatedError(_ x: @escaping @MainActor () async -> ()) {
   func fakeInit(operation: transferring @escaping () async -> ()) {}
 
-  fakeInit(operation: x) // expected-warning {{task-isolated value of type '@MainActor () async -> ()' passed as a strongly transferred parameter}}
+  fakeInit(operation: x) // expected-warning {{transferring 'x' may cause a race}}
+  // expected-note @-1 {{task-isolated 'x' is passed as a transferring parameter; Uses in callee may race with later task-isolated uses}}
+}
+
+@MainActor func actorIsolatedError(_ x: @escaping @MainActor () async -> ()) {
+  func fakeInit(operation: transferring @escaping () async -> ()) {}
+
+  // TODO: This needs to say actor-isolated.
+  fakeInit(operation: x) // expected-warning {{transferring 'x' may cause a race}}
+  // expected-note @-1 {{task-isolated 'x' is passed as a transferring parameter; Uses in callee may race with later task-isolated uses}}
 }
 
 // Make sure we error here on only the second since x by being assigned a part
@@ -348,3 +357,9 @@ func testMergeWithTaskIsolated(_ x: transferring Klass, y: Klass) async {
   await transferToMain(x) // expected-warning {{transferring 'x' may cause a race}}
   // expected-note @-1 {{transferring nonisolated 'x' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
 }
+
+@MainActor func testMergeWithActorIsolated(_ x: transferring Klass, y: Klass) async {
+  x = y
+  await transferToCustom(x) // expected-warning {{transferring 'x' may cause a race}}
+  // expected-note @-1 {{transferring main actor-isolated 'x' to global actor 'CustomActor'-isolated callee could cause races between global actor 'CustomActor'-isolated and main actor-isolated uses}}
+}