Merge pull request #72710 from gottesmm/pr-feb1fe7a002640b270451f2187f141f4ed5da65d

[region-isolation] Propagate global actor-ness of functions/closures.

Author: gottesmm

include/swift/SIL/SILFunction.h

@@ -348,7 +348,7 @@ class SILFunction
   unsigned BlockListChangeIdx = 0;
 
   /// The isolation of this function.
-  std::optional<ActorIsolation> actorIsolation;
+  ActorIsolation actorIsolation = ActorIsolation::forUnspecified();
 
   /// The function's bare attribute. Bare means that the function is SIL-only
   /// and does not require debug info.
@@ -1374,9 +1374,7 @@ class SILFunction
     actorIsolation = newActorIsolation;
   }
 
-  std::optional<ActorIsolation> getActorIsolation() const {
-    return actorIsolation;
-  }
+  ActorIsolation getActorIsolation() const { return actorIsolation; }
 
   //===--------------------------------------------------------------------===//
   // Block List Access

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -208,6 +208,10 @@ class SILIsolationInfo {
     return SILIsolationInfo();
   }
 
+  static SILIsolationInfo getGlobalActorIsolated(Type globalActorType) {
+    return getActorIsolated(ActorIsolation::forGlobalActor(globalActorType));
+  }
+
   static SILIsolationInfo getTaskIsolated(SILValue value) {
     return {Kind::Task, value};
   }
@@ -901,8 +905,8 @@ struct PartitionOpEvaluator {
       // our transferring operand. If so, we can squelch this.
       if (auto functionIsolation =
               transferringOp->getUser()->getFunction()->getActorIsolation()) {
-        if (functionIsolation->isActorIsolated() &&
-            SILIsolationInfo::getActorIsolated(*functionIsolation) ==
+        if (functionIsolation.isActorIsolated() &&
+            SILIsolationInfo::getActorIsolated(functionIsolation) ==
                 SILIsolationInfo::get(transferringOp->getUser()))
           return;
       }

lib/SILGen/SILGen.cpp

@@ -725,6 +725,13 @@ SILFunction *SILGenModule::getFunction(SILDeclRef constant,
         return IGM.getFunction(constant, NotForDefinition);
       });
 
+  // If we have global actor isolation for our constant, put the isolation onto
+  // the function.
+  if (auto isolation =
+          getActorIsolationOfContext(constant.getInnermostDeclContext())) {
+    F->setActorIsolation(isolation);
+  }
+
   assert(F && "SILFunction should have been defined");
 
   emittedFunctions[constant] = F;
@@ -1217,9 +1224,12 @@ void SILGenModule::preEmitFunction(SILDeclRef constant, SILFunction *F,
   if (F->getLoweredFunctionType()->isPolymorphic())
     F->setGenericEnvironment(Types.getConstantGenericEnvironment(constant));
 
-  // Set the actor isolation of the function to its innermost decl context.
-  F->setActorIsolation(
-      getActorIsolationOfContext(constant.getInnermostDeclContext()));
+  // If we have global actor isolation for our constant, put the isolation onto
+  // the function.
+  if (auto isolation =
+          getActorIsolationOfContext(constant.getInnermostDeclContext())) {
+    F->setActorIsolation(isolation);
+  }
 
   // Create a debug scope for the function using astNode as source location.
   F->setDebugScope(new (M) SILDebugScope(Loc, F));

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1848,7 +1848,7 @@ class PartitionOpTranslator {
     }
 
     // If our self parameter was transferring, transfer it. Otherwise, just
-    // stick it in the non seld operand values array and run multiassign on
+    // stick it in the non self operand values array and run multiassign on
     // it.
     if (fas.hasSelfArgument()) {
       auto &selfOperand = fas.getSelfArgumentOperand();
@@ -1864,6 +1864,12 @@ class PartitionOpTranslator {
       }
     }
 
+    // Add our callee to non-transferring parameters. This ensures that if it is
+    // actor isolated, that propagates into our results. This is especially
+    // important since our callee could be dynamically isolated and we cannot
+    // know that until we perform dataflow.
+    nonTransferringParameters.push_back(fas.getCallee());
+
     SmallVector<SILValue, 8> applyResults;
     getApplyResults(*fas, applyResults);
 
@@ -3283,12 +3289,59 @@ TrackableValue RegionAnalysisValueMap::getTrackableValue(
     }
   }
 
-  // Then see if we have a sendable value. By default we assume values are not
-  // sendable.
   if (auto *defInst = value.getDefiningInstruction()) {
-    // Though these values are technically non-Sendable, we can safely and
-    // consistently treat them as Sendable.
-    if (isa<ClassMethodInst, FunctionRefInst>(defInst)) {
+    // Treat function ref as either actor isolated or sendable.
+    if (auto *fri = dyn_cast<FunctionRefInst>(defInst)) {
+      auto isolation = fri->getReferencedFunction()->getActorIsolation();
+      if (isolation.isActorIsolated()) {
+        iter.first->getSecond().mergeIsolationRegionInfo(
+            SILIsolationInfo::getActorIsolated(isolation));
+        return {iter.first->first, iter.first->second};
+      }
+
+      // Otherwise, lets look at the AST and see if our function ref is from an
+      // autoclosure.
+      if (auto *autoclosure = fri->getLoc().getAsASTNode<AutoClosureExpr>()) {
+        if (auto *funcType = autoclosure->getType()->getAs<AnyFunctionType>()) {
+          if (funcType->hasGlobalActor()) {
+            if (funcType->hasGlobalActor()) {
+              iter.first->getSecond().mergeIsolationRegionInfo(
+                  SILIsolationInfo::getActorIsolated(
+                      ActorIsolation::forGlobalActor(
+                          funcType->getGlobalActor())));
+              return {iter.first->first, iter.first->second};
+            }
+          }
+
+          if (auto *resultFType =
+                  funcType->getResult()->getAs<AnyFunctionType>()) {
+            if (resultFType->hasGlobalActor()) {
+              iter.first->getSecond().mergeIsolationRegionInfo(
+                  SILIsolationInfo::getActorIsolated(
+                      ActorIsolation::forGlobalActor(
+                          resultFType->getGlobalActor())));
+              return {iter.first->first, iter.first->second};
+            }
+          }
+        }
+      }
+
+      iter.first->getSecond().addFlag(TrackableValueFlag::isSendable);
+      return {iter.first->first, iter.first->second};
+    }
+
+    if (auto *cmi = dyn_cast<ClassMethodInst>(defInst)) {
+      if (auto *declRefExpr = cmi->getLoc().getAsASTNode<DeclRefExpr>()) {
+        // See if we are actor isolated. If so, treat this as non-Sendable so we
+        // propagate actor isolation.
+        if (auto isolation = getActorIsolation(declRefExpr->getDecl())) {
+          if (isolation.isActorIsolated()) {
+            iter.first->getSecond().mergeIsolationRegionInfo(
+                SILIsolationInfo::getActorIsolated(isolation));
+            return {iter.first->first, iter.first->second};
+          }
+        }
+      }
       iter.first->getSecond().addFlag(TrackableValueFlag::isSendable);
       return {iter.first->first, iter.first->second};
     }
@@ -3379,6 +3432,71 @@ TrackableValue RegionAnalysisValueMap::getTrackableValue(
     }
   }
 
+  // See if we have a convert function from a Sendable actor isolated function,
+  // we want to treat the result of the convert function as being actor isolated
+  // so that we cannot escape the value.
+  //
+  // NOTE: At this point, we already know that cfi's result is not sendable,
+  // since we would have exited above already.
+  if (auto *cfi = dyn_cast<ConvertFunctionInst>(iter.first->first.getValue())) {
+    SILValue operand = cfi->getOperand();
+    if (operand->getType().getAs<SILFunctionType>()->isSendable()) {
+      SILValue newValue = operand;
+      do {
+        operand = newValue;
+
+        newValue = lookThroughOwnershipInsts(operand);
+        if (auto *ttfi = dyn_cast<ThinToThickFunctionInst>(newValue)) {
+          newValue = ttfi->getOperand();
+        }
+
+        if (auto *cfi = dyn_cast<ConvertFunctionInst>(newValue)) {
+          newValue = cfi->getOperand();
+        }
+
+        if (auto *pai = dyn_cast<PartialApplyInst>(newValue)) {
+          newValue = pai->getCallee();
+        }
+      } while (newValue != operand);
+
+      if (auto *ai = dyn_cast<ApplyInst>(operand)) {
+        if (auto *callExpr = ai->getLoc().getAsASTNode<ApplyExpr>()) {
+          if (auto *callType = callExpr->getType()->getAs<AnyFunctionType>()) {
+            if (callType->hasGlobalActor()) {
+              iter.first->getSecond().mergeIsolationRegionInfo(
+                  SILIsolationInfo::getGlobalActorIsolated(
+                      callType->getGlobalActor()));
+              return {iter.first->first, iter.first->second};
+            }
+          }
+        }
+      }
+
+      if (auto *fri = dyn_cast<FunctionRefInst>(operand)) {
+        if (auto actorIsolation =
+                fri->getReferencedFunction()->getActorIsolation()) {
+          if (actorIsolation.isActorIsolated()) {
+            iter.first->getSecond().mergeIsolationRegionInfo(
+                SILIsolationInfo::getActorIsolated(actorIsolation));
+            return {iter.first->first, iter.first->second};
+          }
+        }
+
+        // See if the function ref statically is known to have actor isolation.
+        //
+        // TODO: We should make it so that the closure constructed has actor
+        // isolation.
+        if (auto value = tryToTrackValue(fri)) {
+          auto isolation = value->getIsolationRegionInfo();
+          if (isolation.isActorIsolated()) {
+            iter.first->getSecond().mergeIsolationRegionInfo(isolation);
+            return {iter.first->first, iter.first->second};
+          }
+        }
+      }
+    }
+  }
+
   return {iter.first->first, iter.first->second};
 }
 

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -1152,13 +1152,18 @@ class TransferNonTransferrableDiagnosticInferrer {
   bool run();
 
 private:
-  bool initForIsolatedPartialApply(Operand *op, AbstractClosureExpr *ace);
+  /// \p actualCallerIsolation is used to override the caller isolation we use
+  /// when emitting the error if the closure would have the incorrect one.
+  bool initForIsolatedPartialApply(
+      Operand *op, AbstractClosureExpr *ace,
+      std::optional<ActorIsolation> actualCallerIsolation = {});
 };
 
 } // namespace
 
 bool TransferNonTransferrableDiagnosticInferrer::initForIsolatedPartialApply(
-    Operand *op, AbstractClosureExpr *ace) {
+    Operand *op, AbstractClosureExpr *ace,
+    std::optional<ActorIsolation> actualCallerIsolation) {
   SmallVector<std::tuple<CapturedValue, unsigned, ApplyIsolationCrossing>, 8>
       foundCapturedIsolationCrossing;
   ace->getIsolationCrossing(foundCapturedIsolationCrossing);
@@ -1169,8 +1174,15 @@ bool TransferNonTransferrableDiagnosticInferrer::initForIsolatedPartialApply(
   for (auto &p : foundCapturedIsolationCrossing) {
     if (std::get<1>(p) == opIndex) {
       auto loc = RegularLocation(std::get<0>(p).getLoc());
+      auto crossing = std::get<2>(p);
+      auto declIsolation = crossing.getCallerIsolation();
+      auto closureIsolation = crossing.getCalleeIsolation();
+      if (!bool(declIsolation) && actualCallerIsolation) {
+        declIsolation = *actualCallerIsolation;
+      }
       diagnosticEmitter.emitNamedFunctionArgumentClosure(
-          loc, std::get<0>(p).getDecl()->getBaseIdentifier(), std::get<2>(p));
+          loc, std::get<0>(p).getDecl()->getBaseIdentifier(),
+          ApplyIsolationCrossing(declIsolation, closureIsolation));
       return true;
     }
   }
@@ -1220,6 +1232,15 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
     }
     assert(isolation && "Expected non-null");
 
+    // Then if we are calling a closure expr. If so, we should use the loc of
+    // the closure.
+    if (auto *closureExpr =
+            dyn_cast<AbstractClosureExpr>(sourceApply->getFn())) {
+      initForIsolatedPartialApply(op, closureExpr,
+                                  isolation->getCallerIsolation());
+      return true;
+    }
+
     // See if we can infer a name from the value.
     SmallString<64> resultingName;
     if (auto name = inferNameFromValue(op->get())) {

lib/SILOptimizer/Utils/PartitionUtils.cpp

@@ -91,7 +91,7 @@ SILIsolationInfo SILIsolationInfo::get(SILFunctionArgument *arg) {
   // should be marked as actor isolated.
   if (auto *self = arg->getFunction()->maybeGetSelfArgument()) {
     if (auto functionIsolation = arg->getFunction()->getActorIsolation()) {
-      if (functionIsolation->isActorIsolated()) {
+      if (functionIsolation.isActorIsolated()) {
         if (auto *nomDecl = self->getType().getNominalOrBoundGenericNominal()) {
           if (auto isolationInfo =
                   SILIsolationInfo::getActorIsolated(nomDecl)) {

test/Concurrency/Inputs/GlobalActorIsolatedFunction.swift

@@ -0,0 +1,2 @@
+
+@MainActor public func mainActorFunction() {}

test/Concurrency/isolated_captures.swift

@@ -39,9 +39,9 @@ class NotSendable {
   let ns = NotSendable(x: 0)
   MyActor.ns = ns
 
-  // expected-region-isolation-warning @+2 {{transferring 'ns' may 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-region-isolation-warning @+3 {{transferring 'ns' may cause a race}}
+    // 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
   }()
@@ -61,8 +61,9 @@ class NotSendable {
   let ns = NotSendable(x: 0)
   ns.stash()
 
-  // FIXME: Region isolation should diagnose this (https://github.com/apple/swift/issues/71533)
   await { @YourActor in
+    // expected-region-isolation-warning @+3 {{transferring 'ns' may cause a race}}
+    // 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
   }()
@@ -81,8 +82,9 @@ class NotSendable {
 @MyActor func exhibitRace3() async {
   let ns = NotSendable()
 
-  // FIXME: Region isolation should diagnose this (https://github.com/apple/swift/issues/71533)
   await { @YourActor in
+    // expected-region-isolation-warning @+3 {{transferring 'ns' may cause a race}}
+    // 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
   }()