[region-isolation] Fix actor isolated parameters to get an actor isolated error instead of a task isolated error.

Now that we actually know the region that non transferrable things belong to, we
can use this information to give a better diagnostic here.

A really nice effect of this is that we now emit that actor isolated parameters
are actually actor isolated instead of task isolated.

Author: gottesmm

include/swift/AST/ActorIsolation.h

@@ -274,6 +274,9 @@ class ActorIsolation {
     llvm_unreachable("Covered switch isn't covered?!");
   }
 
+  void printForDiagnostics(llvm::raw_ostream &os,
+                           StringRef openingQuotationMark = "'") const;
+
   SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
 };
 

include/swift/AST/DiagnosticsSIL.def

@@ -954,8 +954,8 @@ ERROR(regionbasedisolation_transfer_yields_race_stronglytransferred_binding, non
       "binding of non-Sendable type %0 accessed after being transferred; later accesses could race",
       (Type))
 ERROR(regionbasedisolation_arg_transferred, none,
-      "task-isolated value of type %0 transferred to %1 context; later accesses to value could race",
-      (Type, ActorIsolation))
+      "%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))

include/swift/SILOptimizer/Analysis/RegionAnalysis.h

@@ -195,14 +195,16 @@ class ValueIsolationRegionInfo {
     }
   }
 
+  void printForDiagnostics(llvm::raw_ostream &os) const;
+
   SWIFT_DEBUG_DUMP {
     print(llvm::dbgs());
     llvm::dbgs() << '\n';
   }
 
   std::optional<ActorIsolation> getActorIsolation() const {
     assert(kind == Actor);
-    assert(std::holds_alternative<NominalTypeDecl *>(data) &&
+    assert(std::holds_alternative<std::optional<ActorIsolation>>(data) &&
            "Doesn't have an actor isolation?!");
     return std::get<std::optional<ActorIsolation>>(data);
   }
@@ -221,6 +223,18 @@ class ValueIsolationRegionInfo {
     return std::get<SILValue>(data);
   }
 
+  bool hasActorIsolation() const {
+    return std::holds_alternative<std::optional<ActorIsolation>>(data);
+  }
+
+  bool hasActorInstance() const {
+    return std::holds_alternative<NominalTypeDecl *>(data);
+  }
+
+  bool hasTaskIsolatedValue() const {
+    return std::holds_alternative<SILValue>(data);
+  }
+
   [[nodiscard]] ValueIsolationRegionInfo
   merge(ValueIsolationRegionInfo other) const {
     // If we are greater than the other kind, then we are further along the
@@ -355,6 +369,7 @@ class regionanalysisimpl::RepresentativeValue {
 
   SILValue getValue() const { return value.get<SILValue>(); }
   SILValue maybeGetValue() const { return value.dyn_cast<SILValue>(); }
+  bool hasRegionIntroducingInst() const { return value.is<SILInstruction *>(); }
   SILInstruction *getActorRegionIntroducingInst() const {
     return value.get<SILInstruction *>();
   }
@@ -464,6 +479,10 @@ class RegionAnalysisValueMap {
   /// value" returns an empty SILValue.
   SILValue maybeGetRepresentative(Element trackableValueID) const;
 
+  /// Returns the fake "representative value" for this element if it
+  /// exists. Returns nullptr otherwise.
+  SILInstruction *maybeGetActorIntroducingInst(Element trackableValueID) const;
+
   ValueIsolationRegionInfo getIsolationRegion(Element trackableValueID) const;
   ValueIsolationRegionInfo getIsolationRegion(SILValue trackableValueID) const;
 
@@ -474,6 +493,15 @@ class RegionAnalysisValueMap {
   getTrackableValue(SILValue value,
                     bool isAddressCapturedByPartialApply = false) const;
 
+  /// An actor introducing inst is an instruction that doesn't have any
+  /// non-Sendable parameters and produces a new value that has to be actor
+  /// isolated.
+  ///
+  /// This is just for looking up the ValueIsolationRegionInfo for a
+  /// instructionInst if we have one. So it is a find like function.
+  std::optional<TrackableValue> getTrackableValueForActorIntroducingInst(
+      SILInstruction *introducingInst) const;
+
 private:
   std::optional<TrackableValue> getValueForId(TrackableValueID id) const;
   std::optional<TrackableValue> tryToTrackValue(SILValue value) const;

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -1098,34 +1098,49 @@ struct PartitionOpEvaluator {
       p.trackNewElement(op.getOpArgs()[0]);
       return;
     case PartitionOpKind::Transfer: {
+      // NOTE: We purposely do not check here if a transferred value is already
+      // transferred. Callers are expected to put a require for that
+      // purpose. This ensures that if we pass the same argument multiple times
+      // to the same transferring function as weakly transferred arguments, we
+      // do not get an error.
       assert(op.getOpArgs().size() == 1 &&
              "Transfer PartitionOp should be passed 1 argument");
       assert(p.isTrackingElement(op.getOpArgs()[0]) &&
              "Transfer PartitionOp's argument should already be tracked");
 
-      if (isActorDerived(op.getOpArgs()[0]) ||
-          isTaskIsolatedDerived(op.getOpArgs()[0]))
+      // If we know our direct value is actor derived... immediately emit an
+      // error.
+      if (isActorDerived(op.getOpArgs()[0]))
         return handleTransferNonTransferrable(op, op.getOpArgs()[0]);
 
-      // NOTE: We purposely do not check here if a transferred value is already
-      // transferred. Callers are expected to put a require for that
-      // purpose. This ensures that if we pass the same argument multiple times
-      // to the same transferring function as weakly transferred arguments, we
-      // do not get an error.
-
+      // Otherwise, we may have a value that is actor derived or task isolated
+      // from another value. We need to prefer actor derived.
+      //
       // While we are checking for actor derived, also check if our value or any
       // value in our region is closure captured and propagate that bit in our
       // transferred inst.
       bool isClosureCapturedElt =
           isClosureCaptured(op.getOpArgs()[0], op.getSourceOp());
-
       Region elementRegion = p.getRegion(op.getOpArgs()[0]);
+      std::optional<Element> actorDerivedElt;
+      std::optional<Element> taskDerivedElt;
       for (const auto &pair : p.range()) {
-        if (pair.second == elementRegion &&
-            (isActorDerived(pair.first) || isTaskIsolatedDerived(pair.first)))
-          return handleTransferNonTransferrable(op, op.getOpArgs()[0],
-                                                pair.first);
-        isClosureCapturedElt |= isClosureCaptured(pair.first, op.getSourceOp());
+        if (pair.second == elementRegion) {
+          if (isActorDerived(pair.first))
+            actorDerivedElt = pair.first;
+          if (isTaskIsolatedDerived(pair.first))
+            taskDerivedElt = pair.first;
+          isClosureCapturedElt |=
+              isClosureCaptured(pair.first, op.getSourceOp());
+        }
+      }
+
+      // Now try to add the actor derived elt first and then the task derived
+      // elt, preferring actor derived.
+      if (actorDerivedElt.has_value() || taskDerivedElt.has_value()) {
+        return handleTransferNonTransferrable(
+            op, op.getOpArgs()[0],
+            actorDerivedElt.has_value() ? *actorDerivedElt : *taskDerivedElt);
       }
 
       // Mark op.getOpArgs()[0] as transferred.

lib/AST/DiagnosticEngine.cpp

@@ -935,40 +935,12 @@ static void formatDiagnosticArgument(StringRef Modifier,
     Out << FormatOpts.OpeningQuotationMark << Arg.getAsLayoutConstraint()
         << FormatOpts.ClosingQuotationMark;
     break;
-  case DiagnosticArgumentKind::ActorIsolation:
+  case DiagnosticArgumentKind::ActorIsolation: {
     assert(Modifier.empty() && "Improper modifier for ActorIsolation argument");
-    switch (auto isolation = Arg.getAsActorIsolation()) {
-    case ActorIsolation::ActorInstance:
-      Out << "actor-isolated";
-      break;
-
-    case ActorIsolation::GlobalActor: {
-      if (isolation.isMainActor()) {
-        Out << "main actor-isolated";
-      } else {
-        Type globalActor = isolation.getGlobalActor();
-        Out << "global actor " << FormatOpts.OpeningQuotationMark
-          << globalActor.getString()
-          << FormatOpts.ClosingQuotationMark << "-isolated";
-      }
-      break;
-    }
-
-    case ActorIsolation::Erased:
-      Out << "@isolated(any)";
-      break;
-
-    case ActorIsolation::Nonisolated:
-    case ActorIsolation::NonisolatedUnsafe:
-    case ActorIsolation::Unspecified:
-      Out << "nonisolated";
-      if (isolation == ActorIsolation::NonisolatedUnsafe) {
-        Out << "(unsafe)";
-      }
-      break;
-    }
+    auto isolation = Arg.getAsActorIsolation();
+    isolation.printForDiagnostics(Out, FormatOpts.OpeningQuotationMark);
     break;
-
+  }
   case DiagnosticArgumentKind::Diagnostic: {
     assert(Modifier.empty() && "Improper modifier for Diagnostic argument");
     auto diagArg = Arg.getAsDiagnostic();

lib/AST/TypeCheckRequests.cpp

@@ -1684,6 +1684,38 @@ ActorIsolation ActorIsolation::subst(SubstitutionMap subs) const {
   llvm_unreachable("unhandled actor isolation kind!");
 }
 
+void ActorIsolation::printForDiagnostics(llvm::raw_ostream &os,
+                                         StringRef openingQuotationMark) const {
+  switch (*this) {
+  case ActorIsolation::ActorInstance:
+    os << "actor-isolated";
+    break;
+
+  case ActorIsolation::GlobalActor: {
+    if (isMainActor()) {
+      os << "main actor-isolated";
+    } else {
+      Type globalActor = getGlobalActor();
+      os << "global actor " << openingQuotationMark << globalActor.getString()
+         << openingQuotationMark << "-isolated";
+    }
+    break;
+  }
+  case ActorIsolation::Erased:
+    os << "@isolated(any)";
+    break;
+
+  case ActorIsolation::Nonisolated:
+  case ActorIsolation::NonisolatedUnsafe:
+  case ActorIsolation::Unspecified:
+    os << "nonisolated";
+    if (*this == ActorIsolation::NonisolatedUnsafe) {
+      os << "(unsafe)";
+    }
+    break;
+  }
+}
+
 void swift::simple_display(
     llvm::raw_ostream &out, const ActorIsolation &state) {
   if (state.preconcurrency())

lib/SILOptimizer/Analysis/RegionAnalysis.cpp

@@ -576,6 +576,32 @@ static bool isTransferrableFunctionArgument(SILFunctionArgument *arg) {
   return arg->isTransferring();
 }
 
+//===----------------------------------------------------------------------===//
+//                       MARK: ValueIsolationRegionInfo
+//===----------------------------------------------------------------------===//
+
+void ValueIsolationRegionInfo::printForDiagnostics(
+    llvm::raw_ostream &os) const {
+  switch (Kind(*this)) {
+  case Unknown:
+    llvm::report_fatal_error("Printing unknown for diagnostics?!");
+    return;
+  case Disconnected:
+    os << "disconnected";
+    return;
+  case Actor:
+    if (hasActorIsolation() && getActorIsolation()) {
+      getActorIsolation()->printForDiagnostics(os);
+    } else {
+      os << "actor-isolated";
+    }
+    return;
+  case Task:
+    os << "task-isolated";
+    return;
+  }
+}
+
 //===----------------------------------------------------------------------===//
 //                            MARK: TrackableValue
 //===----------------------------------------------------------------------===//
@@ -3093,6 +3119,17 @@ void RegionAnalysisFunctionInfo::runDataflow() {
 //                              MARK: Value Map
 //===----------------------------------------------------------------------===//
 
+SILInstruction *RegionAnalysisValueMap::maybeGetActorIntroducingInst(
+    Element trackableValueID) const {
+  if (auto value = getValueForId(trackableValueID)) {
+    auto rep = value->getRepresentative();
+    if (rep.hasRegionIntroducingInst())
+      return rep.getActorRegionIntroducingInst();
+  }
+
+  return nullptr;
+}
+
 std::optional<TrackableValue>
 RegionAnalysisValueMap::getValueForId(TrackableValueID id) const {
   auto iter = stateIndexToEquivalenceClass.find(id);
@@ -3281,18 +3318,40 @@ TrackableValue RegionAnalysisValueMap::getTrackableValue(
   }
 
   // See if we have a non-transferring argument from a function. In such a case,
-  // mark the value as task isolated.
+  // mark the value as actor isolated if self is actor isolated and task
+  // isolated otherwise.
   if (auto *fArg =
           dyn_cast<SILFunctionArgument>(iter.first->first.getValue())) {
     if (!isTransferrableFunctionArgument(fArg)) {
-      iter.first->getSecond().mergeIsolationRegionInfo(
-          ValueIsolationRegionInfo::getTaskIsolated(fArg));
+      auto *self =
+          iter.first->first.getValue()->getFunction()->maybeGetSelfArgument();
+      NominalTypeDecl *nomDecl = nullptr;
+      if (self &&
+          ((nomDecl = self->getType().getNominalOrBoundGenericNominal()))) {
+        iter.first->getSecond().mergeIsolationRegionInfo(
+            ValueIsolationRegionInfo::getActorIsolated(nomDecl));
+      } else {
+        iter.first->getSecond().mergeIsolationRegionInfo(
+            ValueIsolationRegionInfo::getTaskIsolated(fArg));
+      }
     }
   }
 
   return {iter.first->first, iter.first->second};
 }
 
+std::optional<TrackableValue>
+RegionAnalysisValueMap::getTrackableValueForActorIntroducingInst(
+    SILInstruction *inst) const {
+  auto *self = const_cast<RegionAnalysisValueMap *>(this);
+  auto iter = self->equivalenceClassValuesToState.find(inst);
+  if (iter == self->equivalenceClassValuesToState.end())
+    return {};
+
+  // Otherwise, we need to compute our flags.
+  return {{iter->first, iter->second}};
+}
+
 std::optional<TrackableValue>
 RegionAnalysisValueMap::tryToTrackValue(SILValue value) const {
   auto state = getTrackableValue(value);