Merge pull request #71154 from gottesmm/transfernonsendable-partialapply-is-crossing

[region-isolation] Values captured by an actor isolated closure are transferred into that closure

Author: gottesmm

include/swift/AST/CaptureInfo.h

@@ -25,12 +25,18 @@
 
 namespace swift {
 class CapturedValue;
-}
+} // namespace swift
+
+namespace swift {
+namespace Lowering {
+class TypeConverter;
+} // namespace Lowering
+} // namespace swift
 
 namespace llvm {
 class raw_ostream;
 template <> struct DenseMapInfo<swift::CapturedValue>;
-}
+} // namespace llvm
 
 namespace swift {
 class ValueDecl;
@@ -41,6 +47,8 @@ class VarDecl;
 /// CapturedValue includes both the declaration being captured, along with flags
 /// that indicate how it is captured.
 class CapturedValue {
+  friend class Lowering::TypeConverter;
+
 public:
   using Storage =
       llvm::PointerIntPair<llvm::PointerUnion<ValueDecl*, OpaqueValueExpr*>, 2,
@@ -69,9 +77,17 @@ class CapturedValue {
   CapturedValue(ValueDecl *Val, unsigned Flags, SourceLoc Loc)
       : Value(Val, Flags), Loc(Loc) {}
 
-  CapturedValue(OpaqueValueExpr *Val, unsigned Flags)
+private:
+  // This is only used in TypeLowering when forming Lowered Capture
+  // Info. OpaqueValueExpr captured value should never show up in the AST
+  // itself.
+  //
+  // NOTE: AbstractClosureExpr::getIsolationCrossing relies upon this and
+  // asserts that it never sees one of these.
+  explicit CapturedValue(OpaqueValueExpr *Val, unsigned Flags)
       : Value(Val, Flags), Loc(SourceLoc()) {}
 
+public:
   static CapturedValue getDynamicSelfMetadata() {
     return CapturedValue((ValueDecl *)nullptr, 0, SourceLoc());
   }

include/swift/AST/DiagnosticsSIL.def

@@ -898,14 +898,23 @@ WARNING(regionbasedisolation_transfer_yields_race_no_isolation, none,
 WARNING(regionbasedisolation_transfer_yields_race_with_isolation, none,
         "transferring value of non-Sendable type %0 from %1 context to %2 context; later accesses could race",
         (Type, ActorIsolation, ActorIsolation))
+WARNING(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))
 WARNING(regionbasedisolation_transfer_yields_race_transferring_parameter, none,
         "transferring value of non-Sendable type %0 into transferring parameter; later accesses could race",
         (Type))
 WARNING(regionbasedisolation_transfer_yields_race_stronglytransferred_binding, none,
         "binding of non-Sendable type %0 accessed after being transferred; later accesses could race",
         (Type))
+WARNING(regionbasedisolation_arg_transferred, none,
+        "task isolated value of type %0 transferred to %1 context; later accesses to value could race",
+        (Type, ActorIsolation))
 NOTE(regionbasedisolation_maybe_race, none,
      "access here could race", ())
+NOTE(regionbasedisolation_isolated_since_in_same_region_basename, none,
+     "value is %0 since it is in the same region as %1",
+     (StringRef, DeclBaseName))
 ERROR(regionbasedisolation_unknown_pattern, none,
       "pattern that the region based isolation checker does not understand how to check. Please file a bug",
       ())

include/swift/AST/Expr.h

@@ -3846,6 +3846,20 @@ class AbstractClosureExpr : public DeclContext, public Expr {
     Bits.AbstractClosureExpr.Discriminator = InvalidDiscriminator;
   }
 
+  /// If we find that a capture x of this AbstractClosureExpr belongs to a
+  /// different isolation domain than the closure, add an ApplyIsolationCrossing
+  /// to foundIsolationCrossing.
+  ///
+  /// \p foundIsolationCrossings an out parameter that contains the
+  /// ApplyIsolationCrossing if any of the captures are isolation crossing and
+  /// the index of the capture in the capture array. We return the index since
+  /// all captures may not cross isolation boundaries and we may need to be able
+  /// to look up the corresponding capture at the SIL level by index.
+  void getIsolationCrossing(
+      SmallVectorImpl<
+          std::tuple<CapturedValue, unsigned, ApplyIsolationCrossing>>
+          &foundIsolationCrossings);
+
   CaptureInfo getCaptureInfo() const { return Captures; }
   void setCaptureInfo(CaptureInfo captures) { Captures = captures; }
 

lib/AST/Expr.cpp

@@ -2927,3 +2927,55 @@ Type ThrownErrorDestination::getContextErrorType() const {
   auto conversion = storage.get<Conversion *>();
   return conversion->conversion->getType();
 }
+
+void AbstractClosureExpr::getIsolationCrossing(
+    SmallVectorImpl<std::tuple<CapturedValue, unsigned, ApplyIsolationCrossing>>
+        &foundIsolationCrossings) {
+  /// For each capture...
+  for (auto pair : llvm::enumerate(getCaptureInfo().getCaptures())) {
+    auto capture = pair.value();
+
+    // First check quickly if we have dynamic self metadata. This is Sendable
+    // data so we don't care about it.
+    if (capture.isDynamicSelfMetadata())
+      continue;
+
+    auto declIsolation = swift::getActorIsolation(capture.getDecl());
+
+    // Assert that we do not have an opaque value capture. These only appear in
+    // TypeLowering and should never appear in the AST itself.
+    assert(!capture.getOpaqueValue() &&
+           "This should only be created in TypeLowering");
+
+    // If our decl is actor isolated...
+    if (declIsolation.isActorIsolated()) {
+      // And our closure is also actor isolated, then add the apply isolation
+      // crossing if the actors are different.
+      if (actorIsolation.isActorIsolated()) {
+        if (declIsolation.getActor() == actorIsolation.getActor()) {
+          continue;
+        }
+
+        foundIsolationCrossings.emplace_back(
+            capture, pair.index(),
+            ApplyIsolationCrossing(declIsolation, actorIsolation));
+        continue;
+      }
+
+      // Otherwise, our closure is not actor isolated but our decl is actor
+      // isolated. Add it.
+      foundIsolationCrossings.emplace_back(
+          capture, pair.index(),
+          ApplyIsolationCrossing(declIsolation, actorIsolation));
+      continue;
+    }
+
+    if (!actorIsolation.isActorIsolated()) {
+      continue;
+    }
+
+    foundIsolationCrossings.emplace_back(
+        capture, pair.index(),
+        ApplyIsolationCrossing(declIsolation, actorIsolation));
+  }
+}

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -1574,15 +1574,66 @@ class PartitionOpTranslator {
     }
   }
 
+  /// Handles the semantics for SIL applies that cross isolation.
+  ///
+  /// Semantically this causes all arguments of the applysite to be transferred.
+  void translateIsolatedPartialApply(PartialApplyInst *pai) {
+    ApplySite applySite(pai);
+
+    // For each argument operand.
+    for (auto &op : applySite.getArgumentOperands()) {
+      // See if we tracked it.
+      if (auto value = tryToTrackValue(op.get())) {
+        // If we are tracking it, transfer it and if it is actor derived, mark
+        // our partial apply as actor derived.
+        builder.addTransfer(value->getRepresentative().getValue(), &op);
+      }
+    }
+
+    // Now that we have transferred everything into the partial_apply, perform
+    // an assign fresh for the partial_apply. If we use any of the transferred
+    // values later, we will error, so it is safe to just create a new value.
+    auto paiValue = tryToTrackValue(pai).value();
+    SILValue rep = paiValue.getRepresentative().getValue();
+    markValueAsActorDerived(rep);
+    translateSILAssignFresh(rep);
+  }
+
   void translateSILPartialApply(PartialApplyInst *pai) {
     assert(!isIsolationBoundaryCrossingApply(pai));
 
     // First check if our partial_apply is fed into an async let begin. If so,
     // handle it especially.
+    //
+    // NOTE: If it is an async_let, then the closure itself will be Sendable. We
+    // treat passing in a value into the async Sendable closure as transferring
+    // it into the closure.
     if (isAsyncLetBeginPartialApply(pai)) {
       return translateSILPartialApplyAsyncLetBegin(pai);
     }
 
+    // Then check if our partial apply is Sendable. In such a case, we will have
+    // emitted an earlier warning in Sema.
+    //
+    // DISCUSSION: The reason why we can treat values passed into an async let
+    // as transferring safely but it is unsafe to do this for arbitrary Sendable
+    // closures is that we do not know how many times the Sendable closure will
+    // be executed. It is possible to have different invocations of the Sendable
+    // closure to cause races with the captured non-Sendable value. In contrast
+    // since we know an async let runs exactly once, we do not need to worry
+    // about such a possibility. If we had the ability in the language to
+    // specify that a closure is run at most once or that it is always run
+    // serially, we could lift this restriction... so for now we leave in the
+    // Sema warning and just bail here.
+    if (pai->getFunctionType()->isSendableType())
+      return;
+
+    if (auto *ace = pai->getLoc().getAsASTNode<AbstractClosureExpr>()) {
+      if (ace->getActorIsolation().isActorIsolated()) {
+        return translateIsolatedPartialApply(pai);
+      }
+    }
+
     SILMultiAssignOptions options;
     for (auto arg : pai->getOperandValues()) {
       if (auto value = tryToTrackValue(arg)) {