Merge pull request #72264 from gottesmm/eliminate-rest-of-task-or-actor-isolated

[region-isolation] Eliminate most of the rest of the "task or actor" isolated error

Author: gottesmm

include/swift/AST/DiagnosticsSIL.def

@@ -968,34 +968,21 @@ NOTE(regionbasedisolation_isolated_since_in_same_region_basename, none,
 ERROR(regionbasedisolation_named_transfer_yields_race, none,
       "transferring %0 may cause a race",
       (Identifier))
-ERROR(regionbasedisolation_stronglytransfer_assignment_yields_race_name, none,
-      "assigning %0 to transferring parameter %1 may cause a race",
-      (Identifier, Identifier))
-NOTE(regionbasedisolation_named_transfer_into_transferring_param, none,
-      "%0 %1 is passed as a transferring parameter; Uses in callee may race with later %0 uses",
-      (StringRef, Identifier))
 
 NOTE(regionbasedisolation_named_info_transfer_yields_race, none,
      "%0 is transferred from %1 caller to %2 callee. Later uses in caller could race with potential uses in callee",
      (Identifier, ActorIsolation, ActorIsolation))
-NOTE(regionbasedisolation_transfer_non_transferrable_named_note, none,
+NOTE(regionbasedisolation_named_transfer_non_transferrable, none,
       "transferring %1 %0 to %2 callee could cause races between %2 and %1 uses",
       (Identifier, ActorIsolation, ActorIsolation))
-NOTE(regionbasedisolation_named_info_isolated_capture, none,
-     "%1 value %0 is captured by %2 closure. Later local uses could race",
-     (Identifier, ActorIsolation, ActorIsolation))
-NOTE(regionbasedisolation_named_arg_info, none,
-     "Transferring task-isolated function argument %0 could yield races with caller uses",
-     (Identifier))
-NOTE(regionbasedisolation_named_stronglytransferred_binding, none,
-     "Cannot access a transferring parameter after the parameter has been transferred", ())
-NOTE(regionbasedisolation_note_arg_passed_to_strongly_transferred_param, none,
-     "%0 value of type %1 passed as a strongly transferred parameter; later accesses could race",
-     (StringRef, Identifier, Type))
-ERROR(regionbasedisolation_named_arg_passed_to_strongly_transferred_param, none,
-      "%0 %1 passed as a strongly transferred parameter; Uses in callee could race with later %0 uses",
-      (StringRef, Identifier))
+NOTE(regionbasedisolation_named_transfer_into_transferring_param, none,
+     "%0 %1 is passed as a transferring parameter; Uses in callee may race with later %0 uses",
+     (StringRef, Identifier))
+NOTE(regionbasedisolation_named_notransfer_transfer_into_result, none,
+     "%0 %1 cannot be a transferring result. %0 uses may race with caller uses",
+     (StringRef, Identifier))
 
+// Misc Error.
 ERROR(regionbasedisolation_task_or_actor_isolated_transferred, none,
       "task or actor isolated value cannot be transferred", ())
 

include/swift/SILOptimizer/Utils/VariableNameUtils.h

@@ -17,6 +17,7 @@
 #ifndef SWIFT_SILOPTIMIZER_UTILS_VARIABLENAMEUTILS_H
 #define SWIFT_SILOPTIMIZER_UTILS_VARIABLENAMEUTILS_H
 
+#include "swift/Basic/Defer.h"
 #include "swift/Basic/OptionSet.h"
 #include "swift/SIL/ApplySite.h"
 #include "swift/SIL/DebugUtils.h"
@@ -40,8 +41,76 @@ class VariableNameInferrer {
   using Options = OptionSet<Flag>;
 
 private:
-  /// The stacklist that we use to process from use->
-  StackList<PointerUnion<SILInstruction *, SILValue>> variableNamePath;
+  template <typename T, unsigned smallSize>
+  class VariableNamePathArray {
+    SmallVector<T, smallSize> data;
+
+    unsigned lastSnapShotIndex = 0;
+    unsigned insertionPointIndex = 0;
+
+  public:
+    VariableNamePathArray() : data() {}
+
+    ArrayRef<T> getData() const {
+      assert(insertionPointIndex <= data.size());
+      return ArrayRef<T>(data).take_front(insertionPointIndex);
+    }
+
+    void print(llvm::raw_ostream &os) const {
+      os << "LastSnapShotIndex: " << lastSnapShotIndex << '\n';
+      os << "InsertionPointIndex: " << insertionPointIndex << '\n';
+    }
+
+    SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
+
+    /// Pushes a snapshot and returns the old index.
+    unsigned pushSnapShot() & {
+      // After we run with a snapshot, we want:
+      //
+      // 1. lastSnapShotIndex to return to its value before the continuation
+      // ran.
+      // 2. The insertion point index becomes lastSnapShotIndex.
+      unsigned oldSnapShotIndex = lastSnapShotIndex;
+      lastSnapShotIndex = insertionPointIndex;
+      return oldSnapShotIndex;
+    }
+
+    void popSnapShot(unsigned oldIndex) & {
+      insertionPointIndex = lastSnapShotIndex;
+      lastSnapShotIndex = oldIndex;
+    }
+
+    void returnSnapShot(unsigned oldIndex) & { lastSnapShotIndex = oldIndex; }
+
+    void push_back(const T &newValue) & {
+      SWIFT_DEFER { assert(insertionPointIndex <= data.size()); };
+      if (insertionPointIndex == data.size()) {
+        data.push_back(newValue);
+        ++insertionPointIndex;
+        return;
+      }
+
+      data[insertionPointIndex] = newValue;
+      ++insertionPointIndex;
+    }
+
+    [[nodiscard]] T pop_back_val() & {
+      SWIFT_DEFER { assert(insertionPointIndex <= data.size()); };
+      assert(!lastSnapShotIndex &&
+             "Can only pop while lastSnapShotIndex is not set");
+      --insertionPointIndex;
+      return data[insertionPointIndex];
+    }
+
+    bool empty() const { return !insertionPointIndex; }
+  };
+
+  /// The stacklist that we use to print out variable names.
+  ///
+  /// Has to be a small vector since we push/pop the last segment start. This
+  /// lets us speculate when processing phis.
+  VariableNamePathArray<PointerUnion<SILInstruction *, SILValue>, 4>
+      variableNamePath;
 
   /// The root value of our string.
   ///
@@ -59,12 +128,12 @@ class VariableNameInferrer {
 
 public:
   VariableNameInferrer(SILFunction *fn, SmallString<64> &resultingString)
-      : variableNamePath(fn), resultingString(resultingString) {}
+      : variableNamePath(), resultingString(resultingString) {}
 
   VariableNameInferrer(SILFunction *fn, Options options,
                        SmallString<64> &resultingString)
-      : variableNamePath(fn), resultingString(resultingString),
-        options(options) {}
+      : variableNamePath(), resultingString(resultingString), options(options) {
+  }
 
   /// Attempts to infer a name from just uses of \p searchValue.
   ///
@@ -133,7 +202,12 @@ class VariableNameInferrer {
 
   /// Do not call this directly. Used just to improve logging for
   /// findDebugInfoProvidingValue.
-  SILValue findDebugInfoProvidingValueHelper(SILValue searchValue);
+  SILValue findDebugInfoProvidingValueHelper(SILValue searchValue,
+                                             ValueSet &visitedValues);
+
+  /// A special helper for handling phi values. Do not call this directly.
+  SILValue findDebugInfoProvidingValuePhiArg(SILValue incomingValue,
+                                             ValueSet &visitedValues);
 
   /// Given an initialized once allocation inst without a ValueDecl or a
   /// DebugVariable provided name, attempt to find a root value from its

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

@@ -985,15 +985,14 @@ class TransferNonTransferrableDiagnosticEmitter {
   void emitNamedIsolation(SILLocation loc, Identifier name,
                           ApplyIsolationCrossing isolationCrossing) {
     emitNamedOnlyError(loc, name);
-    diagnoseNote(
-        loc, diag::regionbasedisolation_transfer_non_transferrable_named_note,
-        name, isolationCrossing.getCallerIsolation(),
-        isolationCrossing.getCalleeIsolation());
+    diagnoseNote(loc,
+                 diag::regionbasedisolation_named_transfer_non_transferrable,
+                 name, isolationCrossing.getCallerIsolation(),
+                 isolationCrossing.getCalleeIsolation());
   }
 
-  void emitNamedFunctionArgumentApplyStronglyTransferred(
-      SILLocation loc, Identifier varName,
-      ValueIsolationRegionInfo isolationRegionInfo) {
+  void emitNamedFunctionArgumentApplyStronglyTransferred(SILLocation loc,
+                                                         Identifier varName) {
     emitNamedOnlyError(loc, varName);
     SmallString<64> descriptiveKindStr;
     {
@@ -1005,6 +1004,18 @@ class TransferNonTransferrableDiagnosticEmitter {
     diagnoseNote(loc, diag, descriptiveKindStr, varName);
   }
 
+  void emitNamedTransferringReturn(SILLocation loc, Identifier varName) {
+    emitNamedOnlyError(loc, varName);
+    SmallString<64> descriptiveKindStr;
+    {
+      llvm::raw_svector_ostream os(descriptiveKindStr);
+      getIsolationRegionInfo().printForDiagnostics(os);
+    }
+    auto diag =
+        diag::regionbasedisolation_named_notransfer_transfer_into_result;
+    diagnoseNote(loc, diag, descriptiveKindStr, varName);
+  }
+
 private:
   ASTContext &getASTContext() const {
     return getOperand()->getFunction()->getASTContext();
@@ -1106,7 +1117,7 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
         SmallString<64> resultingName;
         if (auto varName = inferNameFromValue(op->get())) {
           diagnosticEmitter.emitNamedFunctionArgumentApplyStronglyTransferred(
-              loc, *varName, diagnosticEmitter.getIsolationRegionInfo());
+              loc, *varName);
           return true;
         }
 
@@ -1171,6 +1182,33 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
     }
   }
 
+  if (auto *ri = dyn_cast<ReturnInst>(op->getUser())) {
+    auto fType = ri->getFunction()->getLoweredFunctionType();
+    if (fType->getNumResults() &&
+        fType->getResults()[0].hasOption(SILResultInfo::IsTransferring)) {
+      assert(llvm::all_of(fType->getResults(),
+                          [](SILResultInfo resultInfo) {
+                            return resultInfo.hasOption(
+                                SILResultInfo::IsTransferring);
+                          }) &&
+             "All result info must be the same... if that changes... update "
+             "this code!");
+      SmallString<64> resultingName;
+      if (auto name = inferNameFromValue(op->get())) {
+        diagnosticEmitter.emitNamedTransferringReturn(loc, *name);
+        return true;
+      }
+    } else {
+      assert(llvm::none_of(fType->getResults(),
+                           [](SILResultInfo resultInfo) {
+                             return resultInfo.hasOption(
+                                 SILResultInfo::IsTransferring);
+                           }) &&
+             "All result info must be the same... if that changes... update "
+             "this code!");
+    }
+  }
+
   diagnosticEmitter.emitUnknownUse(loc);
   return true;
 }

lib/SILOptimizer/Utils/VariableNameUtils.cpp

@@ -214,7 +214,8 @@ VariableNameInferrer::findDebugInfoProvidingValue(SILValue searchValue) {
     return SILValue();
   LLVM_DEBUG(llvm::dbgs() << "Searching for debug info providing value for: "
                           << searchValue);
-  SILValue result = findDebugInfoProvidingValueHelper(searchValue);
+  ValueSet valueSet(searchValue->getFunction());
+  SILValue result = findDebugInfoProvidingValueHelper(searchValue, valueSet);
   if (result) {
     LLVM_DEBUG(llvm::dbgs() << "Result: " << result);
   } else {
@@ -223,12 +224,45 @@ VariableNameInferrer::findDebugInfoProvidingValue(SILValue searchValue) {
   return result;
 }
 
-SILValue
-VariableNameInferrer::findDebugInfoProvidingValueHelper(SILValue searchValue) {
+SILValue VariableNameInferrer::findDebugInfoProvidingValuePhiArg(
+    SILValue incomingValue, ValueSet &visitedValues) {
+  // We use pushSnapShot to run recursively and if we fail to find a
+  // value, we just pop our list to the last snapshot end of list. If we
+  // succeed, we do not pop and just return recusive value. Our user
+  // will consume variableNamePath at this point.
+  LLVM_DEBUG(llvm::dbgs() << "Before pushing a snap shot!\n";
+             variableNamePath.print(llvm::dbgs()));
+
+  unsigned oldSnapShotIndex = variableNamePath.pushSnapShot();
+  LLVM_DEBUG(llvm::dbgs() << "After pushing a snap shot!\n";
+             variableNamePath.print(llvm::dbgs()));
+
+  if (SILValue recursiveValue =
+          findDebugInfoProvidingValueHelper(incomingValue, visitedValues)) {
+    LLVM_DEBUG(llvm::dbgs() << "Returned: " << recursiveValue);
+    variableNamePath.returnSnapShot(oldSnapShotIndex);
+    return recursiveValue;
+  }
+
+  variableNamePath.popSnapShot(oldSnapShotIndex);
+  LLVM_DEBUG(llvm::dbgs() << "After popping a snap shot!\n";
+             variableNamePath.print(llvm::dbgs()));
+  return SILValue();
+}
+
+SILValue VariableNameInferrer::findDebugInfoProvidingValueHelper(
+    SILValue searchValue, ValueSet &visitedValues) {
   assert(searchValue);
 
   while (true) {
     assert(searchValue);
+
+    // If we already visited the value, return SILValue(). This prevents issues
+    // caused by looping phis. We treat this as a failure and visit the either
+    // phi values.
+    if (!visitedValues.insert(searchValue))
+      return SILValue();
+
     LLVM_DEBUG(llvm::dbgs() << "Value: " << *searchValue);
 
     if (auto *allocInst = dyn_cast<AllocationInst>(searchValue)) {
@@ -276,6 +310,12 @@ VariableNameInferrer::findDebugInfoProvidingValueHelper(SILValue searchValue) {
       continue;
     }
 
+    if (auto *uedi = dyn_cast<UncheckedEnumDataInst>(searchValue)) {
+      variableNamePath.push_back(uedi);
+      searchValue = uedi->getOperand();
+      continue;
+    }
+
     if (auto *tei = dyn_cast<TupleExtractInst>(searchValue)) {
       variableNamePath.push_back(tei);
       searchValue = tei->getOperand();
@@ -294,6 +334,23 @@ VariableNameInferrer::findDebugInfoProvidingValueHelper(SILValue searchValue) {
       continue;
     }
 
+    if (auto *e = dyn_cast<UncheckedTakeEnumDataAddrInst>(searchValue)) {
+      variableNamePath.push_back(e);
+      searchValue = e->getOperand();
+      continue;
+    }
+
+    // Enums only have a single possible parent and is used sometimes like a
+    // transformation (e.x.: constructing an optional). We want to look through
+    // them and add the case to the variableNamePath.
+    if (auto *e = dyn_cast<EnumInst>(searchValue)) {
+      if (e->hasOperand()) {
+        variableNamePath.push_back(e);
+        searchValue = e->getOperand();
+        continue;
+      }
+    }
+
     if (auto *dti = dyn_cast_or_null<DestructureTupleInst>(
             searchValue->getDefiningInstruction())) {
       // Append searchValue, so we can find the specific tuple index.
@@ -317,6 +374,27 @@ VariableNameInferrer::findDebugInfoProvidingValueHelper(SILValue searchValue) {
       }
     }
 
+    // If we have a phi argument, visit each of the incoming values and pick the
+    // first one that gives us a name.
+    if (auto *phiArg = dyn_cast<SILPhiArgument>(searchValue)) {
+      if (auto *term = phiArg->getSingleTerminator()) {
+        if (auto *swi = dyn_cast<SwitchEnumInst>(term)) {
+          if (auto value = findDebugInfoProvidingValuePhiArg(swi->getOperand(),
+                                                             visitedValues))
+            return value;
+        }
+      }
+
+      SmallVector<SILValue, 8> incomingValues;
+      if (phiArg->getIncomingPhiValues(incomingValues)) {
+        for (auto value : incomingValues) {
+          if (auto resultValue =
+                  findDebugInfoProvidingValuePhiArg(value, visitedValues))
+            return resultValue;
+        }
+      }
+    }
+
     auto getNamePathComponentFromCallee = [&](FullApplySite call) -> SILValue {
       // Use the name of the property being accessed if we can get to it.
       if (isa<FunctionRefBaseInst>(call.getCallee()) ||
@@ -473,6 +551,11 @@ void VariableNameInferrer::popSingleVariableName() {
       return;
     }
 
+    if (auto *uedi = dyn_cast<UncheckedEnumDataInst>(inst)) {
+      resultingString += getNameFromDecl(uedi->getElement());
+      return;
+    }
+
     if (auto *sei = dyn_cast<StructElementAddrInst>(inst)) {
       resultingString += getNameFromDecl(sei->getField());
       return;
@@ -484,6 +567,16 @@ void VariableNameInferrer::popSingleVariableName() {
       return;
     }
 
+    if (auto *uedi = dyn_cast<UncheckedTakeEnumDataAddrInst>(inst)) {
+      resultingString += getNameFromDecl(uedi->getElement());
+      return;
+    }
+
+    if (auto *ei = dyn_cast<EnumInst>(inst)) {
+      resultingString += getNameFromDecl(ei->getElement());
+      return;
+    }
+
     resultingString += "<unknown decl>";
     return;
   }