Merge pull request #72546 from gottesmm/eliminate-rest-of-the-named-variants

[region-isolation] Add named variants to the rest of the diagnostics

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -949,7 +949,7 @@ 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",
       (Type, ActorIsolation, ActorIsolation))
 ERROR(regionbasedisolation_transfer_yields_race_stronglytransferred_binding, none,
-      "binding of non-Sendable type %0 accessed after being transferred; later accesses could race",
+      "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",
@@ -981,6 +981,12 @@ NOTE(regionbasedisolation_named_transfer_into_transferring_param, none,
 NOTE(regionbasedisolation_named_notransfer_transfer_into_result, none,
      "%0 %1 cannot be a transferring result. %0 uses may race with caller uses",
      (StringRef, Identifier))
+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 %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,

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -491,10 +491,10 @@ class UseAfterTransferDiagnosticEmitter {
       emitUnknownPatternError();
   }
 
-  void emitNamedIsolationCrossingError(SILLocation loc, Identifier name,
-                                       SILIsolationInfo namesIsolationInfo,
-                                       ApplyIsolationCrossing isolationCrossing,
-                                       SILLocation variableDefinedLoc) {
+  void
+  emitNamedIsolationCrossingError(SILLocation loc, Identifier name,
+                                  SILIsolationInfo namedValuesIsolationInfo,
+                                  ApplyIsolationCrossing isolationCrossing) {
     // Emit the short error.
     diagnoseError(loc, diag::regionbasedisolation_named_transfer_yields_race,
                   name)
@@ -504,7 +504,7 @@ class UseAfterTransferDiagnosticEmitter {
     SmallString<64> descriptiveKindStr;
     {
       llvm::raw_svector_ostream os(descriptiveKindStr);
-      namesIsolationInfo.printForDiagnostics(os);
+      namedValuesIsolationInfo.printForDiagnostics(os);
     }
     diagnoseNote(
         loc, diag::regionbasedisolation_named_info_transfer_yields_race, name,
@@ -523,7 +523,24 @@ class UseAfterTransferDiagnosticEmitter {
     emitRequireInstDiagnostics();
   }
 
-  void emitUseOfStronglyTransferredValue(SILLocation loc, Type inferredType) {
+  void emitNamedUseOfStronglyTransferredValue(SILLocation loc,
+                                              Identifier name) {
+    // Emit the short error.
+    diagnoseError(loc, diag::regionbasedisolation_named_transfer_yields_race,
+                  name)
+        .highlight(loc.getSourceRange());
+
+    // Then emit the note with greater context.
+    diagnoseNote(
+        loc, diag::regionbasedisolation_named_stronglytransferred_binding, name)
+        .highlight(loc.getSourceRange());
+
+    // Finally the require points.
+    emitRequireInstDiagnostics();
+  }
+
+  void emitTypedUseOfStronglyTransferredValue(SILLocation loc,
+                                              Type inferredType) {
     diagnoseError(
         loc,
         diag::
@@ -542,6 +559,29 @@ class UseAfterTransferDiagnosticEmitter {
     emitRequireInstDiagnostics();
   }
 
+  void emitNamedIsolationCrossingDueToCapture(
+      SILLocation loc, Identifier name,
+      SILIsolationInfo namedValuesIsolationInfo,
+      ApplyIsolationCrossing isolationCrossing) {
+    // Emit the short error.
+    diagnoseError(loc, diag::regionbasedisolation_named_transfer_yields_race,
+                  name)
+        .highlight(loc.getSourceRange());
+
+    SmallString<64> descriptiveKindStr;
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      namedValuesIsolationInfo.printForDiagnostics(os);
+    }
+
+    diagnoseNote(
+        loc, diag::regionbasedisolation_named_isolated_closure_yields_race,
+        descriptiveKindStr, name, isolationCrossing.getCalleeIsolation(),
+        isolationCrossing.getCallerIsolation())
+        .highlight(loc.getSourceRange());
+    emitRequireInstDiagnostics();
+  }
+
   void emitTypedIsolationCrossingDueToCapture(
       SILLocation loc, Type inferredType,
       ApplyIsolationCrossing isolationCrossing) {
@@ -617,7 +657,7 @@ class UseAfterTransferDiagnosticInferrer {
   SILLocation baseLoc = SILLocation::invalid();
   Type baseInferredType;
 
-  struct Walker;
+  struct AutoClosureWalker;
 
 public:
   UseAfterTransferDiagnosticInferrer(
@@ -671,16 +711,25 @@ bool UseAfterTransferDiagnosticInferrer::initForIsolatedPartialApply(
     return false;
 
   unsigned opIndex = ApplySite(op->getUser()).getAppliedArgIndex(*op);
+  bool emittedDiagnostic = false;
   for (auto &p : foundCapturedIsolationCrossing) {
-    if (std::get<1>(p) == opIndex) {
-      diagnosticEmitter.emitTypedIsolationCrossingDueToCapture(
-          RegularLocation(std::get<0>(p).getLoc()), baseInferredType,
-          std::get<2>(p));
-      return true;
+    if (std::get<1>(p) != opIndex)
+      continue;
+    emittedDiagnostic = true;
+
+    if (auto rootValueAndName = inferNameAndRootFromValue(transferOp->get())) {
+      diagnosticEmitter.emitNamedIsolationCrossingDueToCapture(
+          RegularLocation(std::get<0>(p).getLoc()), rootValueAndName->first,
+          transferOp->getIsolationInfo(), std::get<2>(p));
+      continue;
     }
+
+    diagnosticEmitter.emitTypedIsolationCrossingDueToCapture(
+        RegularLocation(std::get<0>(p).getLoc()), baseInferredType,
+        std::get<2>(p));
   }
 
-  return false;
+  return emittedDiagnostic;
 }
 
 void UseAfterTransferDiagnosticInferrer::initForApply(Operand *op,
@@ -701,14 +750,19 @@ void UseAfterTransferDiagnosticInferrer::initForApply(Operand *op,
                                                isolationCrossing);
 }
 
-struct UseAfterTransferDiagnosticInferrer::Walker : ASTWalker {
+/// This walker visits an AutoClosureExpr and looks for uses of a specific
+/// captured value. We want to error on the uses in the autoclosure.
+struct UseAfterTransferDiagnosticInferrer::AutoClosureWalker : ASTWalker {
   UseAfterTransferDiagnosticInferrer &foundTypeInfo;
   ValueDecl *targetDecl;
+  SILIsolationInfo targetDeclIsolationInfo;
   SmallPtrSet<Expr *, 8> visitedCallExprDeclRefExprs;
 
-  Walker(UseAfterTransferDiagnosticInferrer &foundTypeInfo,
-         ValueDecl *targetDecl)
-      : foundTypeInfo(foundTypeInfo), targetDecl(targetDecl) {}
+  AutoClosureWalker(UseAfterTransferDiagnosticInferrer &foundTypeInfo,
+                    ValueDecl *targetDecl,
+                    SILIsolationInfo targetDeclIsolationInfo)
+      : foundTypeInfo(foundTypeInfo), targetDecl(targetDecl),
+        targetDeclIsolationInfo(targetDeclIsolationInfo) {}
 
   Expr *lookThroughExpr(Expr *expr) {
     while (true) {
@@ -761,9 +815,9 @@ struct UseAfterTransferDiagnosticInferrer::Walker : ASTWalker {
             if (declRef->getDecl() == targetDecl) {
               // Found our target!
               visitedCallExprDeclRefExprs.insert(declRef);
-              foundTypeInfo.diagnosticEmitter.emitTypedIsolationCrossing(
-                  foundTypeInfo.baseLoc, declRef->findOriginalType(),
-                  *isolationCrossing);
+              foundTypeInfo.diagnosticEmitter.emitNamedIsolationCrossingError(
+                  foundTypeInfo.baseLoc, targetDecl->getBaseIdentifier(),
+                  targetDeclIsolationInfo, *isolationCrossing);
               return Action::Continue(expr);
             }
           }
@@ -783,7 +837,16 @@ void UseAfterTransferDiagnosticInferrer::infer() {
            "We should never transfer an indirect out parameter");
     if (fas.getArgumentParameterInfo(*transferOp->getOperand())
             .hasOption(SILParameterInfo::Transferring)) {
-      return diagnosticEmitter.emitUseOfStronglyTransferredValue(
+
+      // First try to do the named diagnostic if we can find a name.
+      if (auto rootValueAndName =
+              inferNameAndRootFromValue(transferOp->get())) {
+        return diagnosticEmitter.emitNamedUseOfStronglyTransferredValue(
+            baseLoc, rootValueAndName->first);
+      }
+
+      // Otherwise, emit the typed diagnostic.
+      return diagnosticEmitter.emitTypedUseOfStronglyTransferredValue(
           baseLoc, baseInferredType);
     }
   }
@@ -804,20 +867,9 @@ void UseAfterTransferDiagnosticInferrer::infer() {
     // Before we do anything further, see if we can find a name and emit a name
     // error.
     if (auto rootValueAndName = inferNameAndRootFromValue(transferOp->get())) {
-      if (auto *svi =
-              dyn_cast<SingleValueInstruction>(rootValueAndName->second)) {
-        return diagnosticEmitter.emitNamedIsolationCrossingError(
-            baseLoc, rootValueAndName->first, transferOp->getIsolationInfo(),
-            *sourceApply->getIsolationCrossing(), svi->getLoc());
-      }
-
-      if (auto *fArg =
-              dyn_cast<SILFunctionArgument>(rootValueAndName->second)) {
-        return diagnosticEmitter.emitNamedIsolationCrossingError(
-            baseLoc, rootValueAndName->first, transferOp->getIsolationInfo(),
-            *sourceApply->getIsolationCrossing(),
-            RegularLocation(fArg->getDecl()->getLoc()));
-      }
+      return diagnosticEmitter.emitNamedIsolationCrossingError(
+          baseLoc, rootValueAndName->first, transferOp->getIsolationInfo(),
+          *sourceApply->getIsolationCrossing());
     }
 
     // Otherwise, try to infer from the ApplyExpr.
@@ -843,7 +895,7 @@ void UseAfterTransferDiagnosticInferrer::infer() {
   auto captureInfo =
       autoClosureExpr->getCaptureInfo().getCaptures()[captureIndex];
   auto *captureDecl = captureInfo.getDecl();
-  Walker walker(*this, captureDecl);
+  AutoClosureWalker walker(*this, captureDecl, transferOp->getIsolationInfo());
   autoClosureExpr->walk(walker);
 }
 

test/ClangImporter/transferring.swift

@@ -28,11 +28,11 @@ func funcTestTransferringResult() async {
   let x2 = NonSendableCStruct()
   let y2 = returnUserDefinedFromGlobalFunction(x2)
   await transferToMain(x2) // expected-error {{transferring value of non-Sendable type 'NonSendableCStruct' from nonisolated context to main actor-isolated context}}
-  useValue(y2) // expected-note {{access here could race}}
+  useValue(y2) // expected-note {{use here could race}}
 }
 
 func funcTestTransferringArg() async {
   let x = NonSendableCStruct()
-  transferUserDefinedIntoGlobalFunction(x) // expected-error {{binding of non-Sendable type 'NonSendableCStruct' accessed after being transferred}}
-  useValue(x) // expected-note {{access here could race}}
+  transferUserDefinedIntoGlobalFunction(x) // expected-error {{value of non-Sendable type 'NonSendableCStruct' accessed after being transferred; later accesses could race}}
+  useValue(x) // expected-note {{use here could race}}
 }

test/ClangImporter/transferring_objc.swift

@@ -25,8 +25,9 @@ func methodTestTransferringResult() async {
 func methodTestTransferringArg() async {
   let x = MyType()
   let s = NSObject()
-  let _ = x.getResultWithTransferringArgument(s)  // expected-error {{binding of non-Sendable type 'NSObject' accessed after being transferred}}
-  useValue(s) // expected-note {{access here could race}}
+  let _ = x.getResultWithTransferringArgument(s)  // expected-error {{transferring 's' may cause a race}}
+  // expected-note @-1 {{'s' used after being passed as a transferring parameter; Later uses could race}}
+  useValue(s) // expected-note {{use here could race}}
 }
 
 // Make sure we just ignore the swift_attr if it is applied to something like a
@@ -46,11 +47,12 @@ func funcTestTransferringResult() async {
   let y2 = returnNSObjectFromGlobalFunction(x2)
   await transferToMain(x2) // expected-error {{transferring 'x2' may cause a 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 {{access here could race}}
+  useValue(y2) // expected-note {{use here could race}}
 }
 
 func funcTestTransferringArg() async {
   let x = NSObject()
-  transferNSObjectToGlobalFunction(x) // expected-error {{binding of non-Sendable type 'NSObject' accessed after being transferred}}
-  useValue(x) // expected-note {{access here could race}}
+  transferNSObjectToGlobalFunction(x) // expected-error {{transferring 'x' may cause a race}}
+  // expected-note @-1 {{'x' used after being passed as a transferring parameter; Later uses could race}}
+  useValue(x) // expected-note {{use here could race}}
 }