Merge pull request swiftlang#74705 from gottesmm/pr-93a2750af4fb0c65ca129d545c4af30cd9e718f9

[region-isolation] Given a .none #isolation parameter, infer the isolation from the callee rather than the isolated parameter.

Author: gottesmm

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1590,6 +1590,10 @@ class PartitionOpTranslator {
     SmallVector<SILValue, 8> assignOperands;
     SmallVector<SILValue, 8> assignResults;
 
+    // A helper we use to emit an unknown patten error if our merge is
+    // invalid. This ensures we guarantee that if we find an actor merge error,
+    // the compiler halts. Importantly this lets our users know 100% that if the
+    // compiler exits successfully, actor merge errors could not have happened.
     std::optional<SILDynamicMergedIsolationInfo> mergedInfo;
     if (resultIsolationInfoOverride) {
       mergedInfo = resultIsolationInfoOverride;
@@ -1600,12 +1604,26 @@ class PartitionOpTranslator {
     for (SILValue src : sourceValues) {
       if (auto value = tryToTrackValue(src)) {
         assignOperands.push_back(value->getRepresentative().getValue());
-        mergedInfo = mergedInfo->merge(value->getIsolationRegionInfo());
+        auto originalMergedInfo = mergedInfo;
+        (void)originalMergedInfo;
+        if (mergedInfo)
+          mergedInfo = mergedInfo->merge(value->getIsolationRegionInfo());
 
         // If we fail to merge, then we have an incompatibility in between some
         // of our arguments (consider isolated to different actors) or with the
         // isolationInfo we specified. Emit an unknown patten error.
         if (!mergedInfo) {
+          LLVM_DEBUG(
+              llvm::dbgs() << "Merge Failure!\n"
+                           << "Original Info: ";
+              if (originalMergedInfo)
+                  originalMergedInfo->printForDiagnostics(llvm::dbgs());
+              else llvm::dbgs() << "nil";
+              llvm::dbgs() << "\nValue: "
+                           << value->getRepresentative().getValue();
+              llvm::dbgs() << "Value Info: ";
+              value->getIsolationRegionInfo().printForDiagnostics(llvm::dbgs());
+              llvm::dbgs() << "\n");
           builder.addUnknownPatternError(src);
           continue;
         }
@@ -1614,7 +1632,7 @@ class PartitionOpTranslator {
 
     for (SILValue result : resultValues) {
       // If we had isolation info explicitly passed in... use our
-      // mergedInfo. Otherwise, we want to infer.
+      // resultIsolationInfoError. Otherwise, we want to infer.
       if (resultIsolationInfoOverride) {
         // We only get back result if it is non-Sendable.
         if (auto nonSendableValue =
@@ -1657,9 +1675,10 @@ class PartitionOpTranslator {
       // derived, introduce a fake element so we just propagate the actor
       // region.
       //
-      // NOTE: Here we check if we have mergedInfo rather than isolationInfo
-      // since we want to do this regardless of whether or not we passed in a
-      // specific isolation info unlike earlier when processing actual results.
+      // NOTE: Here we check if we have resultIsolationInfoOverride rather than
+      // isolationInfo since we want to do this regardless of whether or not we
+      // passed in a specific isolation info unlike earlier when processing
+      // actual results.
       if (assignOperands.size() && resultIsolationInfoOverride) {
         builder.addActorIntroducingInst(assignOperands.back(),
                                         resultIsolationInfoOverride);

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

@@ -364,21 +364,31 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
     }
 
     if (auto *isolatedOp = fas.getIsolatedArgumentOperandOrNullPtr()) {
-      // First pattern match from global actors being passed as isolated
-      // parameters. This gives us better type information. If we can pattern
-      // match... we should!
+      // First see if we have an enum inst.
       if (auto *ei = dyn_cast<EnumInst>(isolatedOp->get())) {
-        if (ei->getElement()->getParentEnum()->isOptionalDecl() &&
-            ei->hasOperand()) {
-          if (auto *ieri = dyn_cast<InitExistentialRefInst>(ei->getOperand())) {
-            CanType selfASTType = ieri->getFormalConcreteType();
-
-            if (auto *nomDecl = selfASTType->getAnyActor()) {
-              // The SILValue() parameter doesn't matter until we have isolation
-              // history.
-              if (nomDecl->isGlobalActor())
-                return SILIsolationInfo::getGlobalActorIsolated(SILValue(),
-                                                                nomDecl);
+        if (ei->getElement()->getParentEnum()->isOptionalDecl()) {
+          // Pattern match from global actors being passed as isolated
+          // parameters. This gives us better type information. If we can
+          // pattern match... we should!
+          if (ei->hasOperand()) {
+            if (auto *ieri =
+                    dyn_cast<InitExistentialRefInst>(ei->getOperand())) {
+              CanType selfASTType = ieri->getFormalConcreteType();
+
+              if (auto *nomDecl = selfASTType->getAnyActor()) {
+                // The SILValue() parameter doesn't matter until we have
+                // isolation history.
+                if (nomDecl->isGlobalActor())
+                  return SILIsolationInfo::getGlobalActorIsolated(SILValue(),
+                                                                  nomDecl);
+              }
+            }
+          } else {
+            // In this case, we have a .none so we are attempting to use the
+            // global queue. In such a case, we need to not use the enum as our
+            // value and instead need to grab the isolation of our apply.
+            if (auto isolationInfo = get(fas.getCallee())) {
+              return isolationInfo;
             }
           }
         }

test/Concurrency/transfernonsendable.swift

@@ -1767,3 +1767,30 @@ extension MyActor {
     }
   }
 }
+
+public struct TimeoutError: Error, CustomStringConvertible {
+  public var description: String { "Timed out" }
+}
+
+// We used to not merge the isolation below correctly causing us to emit a crash
+// due to undefined behavior. Make sure we do not crash or emit an unhandled
+// pattern error.
+public func doNotCrashOrEmitUnhandledPatternErrors<T: Sendable>(
+  _ duration: Duration,
+  _ body: @escaping @Sendable () async throws -> T
+) async throws -> T {
+  try await withThrowingTaskGroup(of: T.self) { taskGroup in
+    taskGroup.addTask {
+      try await Task.sleep(for: duration)
+      throw TimeoutError()
+    }
+    taskGroup.addTask {
+      return try await body()
+    }
+    for try await value in taskGroup {
+      taskGroup.cancelAll()
+      return value
+    }
+    throw CancellationError()
+  }
+}