Fix a bunch of bugs with the isolation of local funcs. Since we

use local funcs to implement `defer`, this also fixes several
bugs with that feature, such as it breaking in nonisolated
functions when a default isolation is in effect in the source file.

Change how we compute isolation of local funcs. The rule here is
supposed to be that non-`@Sendable` local funcs are isolated the
same as their enclosing context. Unlike closure expressions, this
is unconditional: in instance-isolated functions, the isolation
does not depend on whether `self` is captured. But the computation
was wrong: it didn't translate global actor isolation between
contexts, it didn't turn parameter isolation into capture isolation,
and it fell through for several other kinds of parent isolation,
causing the compiler to try to apply default isolation instead.
I've extracted the logic from the closure expression path into a
common function and used it for both paths.

The capture computation logic was forcing a capture of the
enclosing isolation in local funcs, but only for async functions.
Presumably this was conditional because async functions need the
isolation for actor hops, but sync functions don't really need it.
However, this was causing crashes with `-enable-actor-data-race-checks`.
(I didn't investigate whether it also failed with the similar
assertion we do with preconcurrency.) For now, I've switched this
to capture the isolated instance unconditionally. If we need to
be more conservative by either only capturing when data-race checks
are enabled or disabling the checks when the isolation isn't captured,
we can look into that.

Fix a bug in capture isolation checking. We were ignoring captures
of nonisolated declarations in order to implement the rule that
permits `nonisolated(unsafe)` variables to be captured in
non-sendable closures. This check needs to only apply to variables!
The isolation of a local func has nothing to do with its sendability
as a capture.

That fix exposed a problem where we were being unnecessarily
restrictive with generic local func declarations because we didn't
consider them to have sendable type. This was true even if the
genericity was purely from being declared in a generic context,
but it doesn't matter, they ought to be sendable regardless.

Finally, fix a handful of bugs where global actor types were not
remapped properly in SILGen.

Author: rjmccall

include/swift/AST/ActorIsolation.h

@@ -154,7 +154,8 @@ class ActorIsolation {
   explicit ActorIsolation(Kind kind = Unspecified, bool isSILParsed = false)
       : pointer(nullptr), kind(kind), isolatedByPreconcurrency(false),
         silParsed(isSILParsed), encodedParameterIndex(0) {
-    assert(kind != ActorInstance);
+    // SIL's use of this has weaker invariants for now.
+    assert(kind != ActorInstance || isSILParsed);
   }
 
   static ActorIsolation forUnspecified() {
@@ -209,21 +210,14 @@ class ActorIsolation {
   static std::optional<ActorIsolation> forSILString(StringRef string) {
     auto kind =
         llvm::StringSwitch<std::optional<ActorIsolation::Kind>>(string)
-            .Case("unspecified",
-                  std::optional<ActorIsolation>(ActorIsolation::Unspecified))
-            .Case("actor_instance",
-                  std::optional<ActorIsolation>(ActorIsolation::ActorInstance))
-            .Case("nonisolated",
-                  std::optional<ActorIsolation>(ActorIsolation::Nonisolated))
-            .Case("nonisolated_unsafe", std::optional<ActorIsolation>(
-                                            ActorIsolation::NonisolatedUnsafe))
-            .Case("global_actor",
-                  std::optional<ActorIsolation>(ActorIsolation::GlobalActor))
-            .Case("global_actor_unsafe",
-                  std::optional<ActorIsolation>(ActorIsolation::GlobalActor))
+            .Case("unspecified", ActorIsolation::Unspecified)
+            .Case("actor_instance", ActorIsolation::ActorInstance)
+            .Case("nonisolated", ActorIsolation::Nonisolated)
+            .Case("nonisolated_unsafe", ActorIsolation::NonisolatedUnsafe)
+            .Case("global_actor", ActorIsolation::GlobalActor)
+            .Case("global_actor_unsafe", ActorIsolation::GlobalActor)
             .Case("caller_isolation_inheriting",
-                  std::optional<ActorIsolation>(
-                      ActorIsolation::CallerIsolationInheriting))
+                  ActorIsolation::CallerIsolationInheriting)
             .Default(std::nullopt);
     if (kind == std::nullopt)
       return std::nullopt;

lib/AST/ActorIsolation.cpp

@@ -191,6 +191,9 @@ bool ActorIsolation::isEqual(const ActorIsolation &lhs,
     auto *lhsActor = lhs.getActorInstance();
     auto *rhsActor = rhs.getActorInstance();
     if (lhsActor && rhsActor) {
+      if (lhsActor == rhsActor)
+        return true;
+
       // FIXME: This won't work for arbitrary isolated parameter captures.
       if ((lhsActor->isSelfParameter() && rhsActor->isSelfParamCapture()) ||
           (lhsActor->isSelfParamCapture() && rhsActor->isSelfParameter())) {

lib/AST/ConformanceLookup.cpp

@@ -269,8 +269,13 @@ static ProtocolConformanceRef getBuiltinTupleTypeConformance(
   return ProtocolConformanceRef::forMissingOrInvalid(type, protocol);
 }
 
+// We can end up checking builtin conformances for generic function types
+// when e.g. we're checking whether a captured local func declaration is
+// sendable. That's fine, we can answer that question in the abstract
+// without needing to generally support conformances on generic function
+// types.
 using EitherFunctionType =
-    llvm::PointerUnion<const SILFunctionType *, const FunctionType *>;
+    llvm::PointerUnion<const SILFunctionType *, const AnyFunctionType *>;
 
 /// Whether the given function type conforms to Sendable.
 static bool isSendableFunctionType(EitherFunctionType eitherFnTy) {
@@ -288,7 +293,7 @@ static bool isSendableFunctionType(EitherFunctionType eitherFnTy) {
     representation = *converted;
 
   } else {
-    auto functionType = eitherFnTy.get<const FunctionType *>();
+    auto functionType = eitherFnTy.get<const AnyFunctionType *>();
 
     if (functionType->isSendable())
       return true;
@@ -332,7 +337,7 @@ static bool isBitwiseCopyableFunctionType(EitherFunctionType eitherFnTy) {
   if (auto silFnTy = eitherFnTy.dyn_cast<const SILFunctionType *>()) {
     representation = silFnTy->getRepresentation();
   } else {
-    auto fnTy = eitherFnTy.get<const FunctionType *>();
+    auto fnTy = eitherFnTy.get<const AnyFunctionType *>();
     representation = convertRepresentation(fnTy->getRepresentation());
   }
 
@@ -621,7 +626,7 @@ LookupConformanceInModuleRequest::evaluate(
   }
 
   // Function types can conform to protocols.
-  if (auto functionType = type->getAs<FunctionType>()) {
+  if (auto functionType = type->getAs<AnyFunctionType>()) {
     return getBuiltinFunctionTypeConformance(type, functionType, protocol);
   }
 

lib/SILGen/SILGenConcurrency.cpp

@@ -195,7 +195,8 @@ void SILGenFunction::emitExpectedExecutorProlog() {
 
     case ActorIsolation::GlobalActor:
       if (F.isAsync() || wantDataRaceChecks) {
-        setExpectedExecutorForGlobalActor(*this, actorIsolation.getGlobalActor());
+        auto globalActorType = F.mapTypeIntoContext(actorIsolation.getGlobalActor());
+        setExpectedExecutorForGlobalActor(*this, globalActorType);
       }
       break;
     }
@@ -226,7 +227,8 @@ void SILGenFunction::emitExpectedExecutorProlog() {
 
     case ActorIsolation::GlobalActor:
       if (wantExecutor) {
-        setExpectedExecutorForGlobalActor(*this, actorIsolation.getGlobalActor());
+        auto globalActorType = F.mapTypeIntoContext(actorIsolation.getGlobalActor());
+        setExpectedExecutorForGlobalActor(*this, globalActorType);
         break;
       }
     }
@@ -573,9 +575,10 @@ SILGenFunction::emitFunctionTypeIsolation(SILLocation loc,
 
   // Emit global actor isolation by loading .shared from the global actor,
   // erasing it into `any Actor`, and injecting that into Optional.
-  case FunctionTypeIsolation::Kind::GlobalActor:
+  case FunctionTypeIsolation::Kind::GlobalActor: {
     return emitGlobalActorIsolation(loc,
              isolation.getGlobalActorType()->getCanonicalType());
+  }
 
   // Emit @isolated(any) isolation by loading the actor reference from the
   // function.
@@ -646,9 +649,11 @@ SILGenFunction::emitClosureIsolation(SILLocation loc, SILDeclRef constant,
   case ActorIsolation::Erased:
     llvm_unreachable("closures cannot directly have erased isolation");
 
-  case ActorIsolation::GlobalActor:
-    return emitGlobalActorIsolation(loc,
-             isolation.getGlobalActor()->getCanonicalType());
+  case ActorIsolation::GlobalActor: {
+    auto globalActorType = F.mapTypeIntoContext(isolation.getGlobalActor())
+                               ->getCanonicalType();
+    return emitGlobalActorIsolation(loc, globalActorType);
+  }
 
   case ActorIsolation::ActorInstance: {
     assert(isolation.isActorInstanceForCapture());
@@ -703,8 +708,10 @@ SILGenFunction::emitExecutor(SILLocation loc, ActorIsolation isolation,
     return emitLoadActorExecutor(loc, self);
   }
 
-  case ActorIsolation::GlobalActor:
-    return emitLoadGlobalActorExecutor(isolation.getGlobalActor());
+  case ActorIsolation::GlobalActor: {
+    auto globalActorType = F.mapTypeIntoContext(isolation.getGlobalActor());
+    return emitLoadGlobalActorExecutor(globalActorType);
+  }
   }
   llvm_unreachable("covered switch");
 }

lib/Sema/TypeCheckCaptures.cpp

@@ -762,7 +762,7 @@ CaptureInfo CaptureInfoRequest::evaluate(Evaluator &evaluator,
     finder.checkType(type, AFD->getLoc());
   }
 
-  if (AFD->isLocalCapture() && AFD->hasAsync()) {
+  if (AFD->isLocalCapture()) {
     // If a local function inherits isolation from the enclosing context,
     // make sure we capture the isolated parameter, if we haven't already.
     auto actorIsolation = getActorIsolation(AFD);

lib/Sema/TypeCheckConcurrency.cpp

@@ -3045,13 +3045,13 @@ namespace {
         // If the closure won't execute concurrently with the context in
         // which the declaration occurred, it's okay.
         auto decl = capture.getDecl();
-        auto isolation = getActorIsolation(decl);
 
         // 'nonisolated' local variables are always okay to capture in
         // 'Sendable' closures because they can be accessed from anywhere.
         // Note that only 'nonisolated(unsafe)' can be applied to local
         // variables.
-        if (isolation.isNonisolated())
+        if (isa<VarDecl>(decl) &&
+            getActorIsolation(decl).isNonisolated())
           continue;
 
         auto *context = localFunc.getAsDeclContext();
@@ -4833,6 +4833,70 @@ namespace {
   };
 } // end anonymous namespace
 
+/// Compute the isolation of a closure or local function from its parent
+/// isolation.
+///
+/// Doesn't apply preconcurrency because it's generally easier for the
+/// caller to do so.
+static ActorIsolation
+computeClosureIsolationFromParent(DeclContext *closure,
+                                  ActorIsolation parentIsolation,
+                                  bool checkIsolatedCapture) {
+  // We must have parent isolation determined to get here.
+  switch (parentIsolation) {
+  case ActorIsolation::CallerIsolationInheriting:
+  case ActorIsolation::Nonisolated:
+  case ActorIsolation::NonisolatedUnsafe:
+  case ActorIsolation::Unspecified:
+    return ActorIsolation::forNonisolated(
+        parentIsolation == ActorIsolation::NonisolatedUnsafe);
+
+  case ActorIsolation::Erased:
+    llvm_unreachable("context cannot have erased isolation");
+
+  case ActorIsolation::GlobalActor:
+    // This should already be an interface type, so we don't need to remap
+    // it between the contexts.
+    return ActorIsolation::forGlobalActor(parentIsolation.getGlobalActor());
+
+  case ActorIsolation::ActorInstance: {
+    // In non-@Sendable local functions, we always inherit the enclosing
+    // isolation, forcing a capture of it if necessary.
+    if (isa<FuncDecl>(closure)) {
+      // We should always have a VarDecl in this case, where we got the
+      // ActorIsolation from a context; the non-VarDecl cases are only used
+      // locally within isolation checking.
+      auto actor = parentIsolation.getActorInstance();
+      assert(actor);
+      return ActorIsolation::forActorInstanceCapture(actor);
+    }
+
+    if (checkIsolatedCapture) {
+      auto closureAsFn = AnyFunctionRef::fromFunctionDeclContext(closure);
+      if (auto param = closureAsFn.getCaptureInfo().getIsolatedParamCapture())
+        return ActorIsolation::forActorInstanceCapture(param);
+
+      auto *explicitClosure = dyn_cast<ClosureExpr>(closure);
+      // @_inheritActorContext(always) forces the isolation capture.
+      if (explicitClosure && explicitClosure->alwaysInheritsActorContext()) {
+        if (parentIsolation.isActorInstanceIsolated()) {
+          if (auto *param = parentIsolation.getActorInstance())
+            return ActorIsolation::forActorInstanceCapture(param);
+        }
+        return parentIsolation;
+      }
+    } else {
+      // If we don't have capture information during code completion, assume
+      // that the closure captures the `isolated` parameter from the parent
+      // context.
+      return parentIsolation;
+    }
+
+    return ActorIsolation::forNonisolated(/*unsafe=*/false);
+  }
+  }
+}
+
 ActorIsolation ActorIsolationChecker::determineClosureIsolation(
     AbstractClosureExpr *closure) const {
   bool preconcurrency = false;
@@ -4889,48 +4953,8 @@ ActorIsolation ActorIsolationChecker::determineClosureIsolation(
         closure->getParent(), getClosureActorIsolation);
     preconcurrency |= parentIsolation.preconcurrency();
 
-    // We must have parent isolation determined to get here.
-    switch (parentIsolation) {
-    case ActorIsolation::CallerIsolationInheriting:
-    case ActorIsolation::Nonisolated:
-    case ActorIsolation::NonisolatedUnsafe:
-    case ActorIsolation::Unspecified:
-      return ActorIsolation::forNonisolated(
-          parentIsolation == ActorIsolation::NonisolatedUnsafe);
-
-    case ActorIsolation::Erased:
-      llvm_unreachable("context cannot have erased isolation");
-
-    case ActorIsolation::GlobalActor: {
-      Type globalActor = closure->mapTypeIntoContext(
-          parentIsolation.getGlobalActor()->mapTypeOutOfContext());
-      return ActorIsolation::forGlobalActor(globalActor);
-    }
-
-    case ActorIsolation::ActorInstance: {
-      if (checkIsolatedCapture) {
-        if (auto param = closure->getCaptureInfo().getIsolatedParamCapture())
-          return ActorIsolation::forActorInstanceCapture(param);
-
-        auto *explicitClosure = dyn_cast<ClosureExpr>(closure);
-        // @_inheritActorContext(always) forces the isolation capture.
-        if (explicitClosure && explicitClosure->alwaysInheritsActorContext()) {
-          if (parentIsolation.isActorInstanceIsolated()) {
-            if (auto *param = parentIsolation.getActorInstance())
-              return ActorIsolation::forActorInstanceCapture(param);
-          }
-          return parentIsolation;
-        }
-      } else {
-        // If we don't have capture information during code completion, assume
-        // that the closure captures the `isolated` parameter from the parent
-        // context.
-        return parentIsolation;
-      }
-
-      return ActorIsolation::forNonisolated(/*unsafe=*/false);
-    }
-    }
+    return computeClosureIsolationFromParent(closure, parentIsolation,
+                                             checkIsolatedCapture);
   }();
 
   // Apply computed preconcurrency.
@@ -5127,7 +5151,8 @@ getIsolationFromAttributes(const Decl *decl, bool shouldDiagnose = true,
     std::optional<ActorIsolation> result;
     if (selfTypeDecl) {
       if (selfTypeDecl->isAnyActor()) {
-        result = ActorIsolation::forActorInstanceSelf(selfTypeDecl);
+        result = ActorIsolation::forActorInstanceSelf(
+          const_cast<AbstractFunctionDecl*>(cast<AbstractFunctionDecl>(decl)));
       } else {
         // If the declaration is in an extension that has one of the isolation
         // attributes, use that.
@@ -6316,50 +6341,37 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
     return inferred;
   };
 
+  // If this is an accessor, use the actor isolation of its storage
+  // declaration. All of the logic for FuncDecls below only applies to
+  // non-accessor functions.
+  if (auto accessor = dyn_cast<AccessorDecl>(value)) {
+    return getInferredActorIsolation(accessor->getStorage());
+  }
+
   // If this is a local function, inherit the actor isolation from its
   // context if it global or was captured.
   if (auto func = dyn_cast<FuncDecl>(value)) {
-    if (func->isLocalCapture() && !func->isSendable()) {
-      auto *dc = func->getDeclContext();
+    auto *dc = func->getDeclContext();
+    if (dc->isLocalContext() && !func->isSendable()) {
       llvm::PointerUnion<Decl *, AbstractClosureExpr *> inferenceSource;
       if (auto *closure = dyn_cast<AbstractClosureExpr>(dc)) {
         inferenceSource = closure;
       } else {
         inferenceSource = dc->getAsDecl();
       }
 
-      switch (auto enclosingIsolation = getActorIsolationOfContext(dc)) {
-      case ActorIsolation::Nonisolated:
-      case ActorIsolation::CallerIsolationInheriting:
-      case ActorIsolation::NonisolatedUnsafe:
-      case ActorIsolation::Unspecified:
-        // Do nothing.
-        break;
-
-      case ActorIsolation::Erased:
-        llvm_unreachable("context cannot have erased isolation");
-
-      case ActorIsolation::ActorInstance:
-        return {
-          inferredIsolation(enclosingIsolation),
-          IsolationSource(inferenceSource, IsolationSource::LexicalContext)
-        };
-
-      case ActorIsolation::GlobalActor:
-        return {
-          inferredIsolation(enclosingIsolation),
-          IsolationSource(inferenceSource, IsolationSource::LexicalContext)
-        };
-      }
+      auto enclosingIsolation = getActorIsolationOfContext(dc);
+      auto isolation =
+        computeClosureIsolationFromParent(func, enclosingIsolation,
+                                          /*checkIsolatedCapture*/true)
+          .withPreconcurrency(enclosingIsolation.preconcurrency());
+      return {
+        inferredIsolation(isolation),
+        IsolationSource(inferenceSource, IsolationSource::LexicalContext)
+      };
     }
   }
 
-  // If this is an accessor, use the actor isolation of its storage
-  // declaration.
-  if (auto accessor = dyn_cast<AccessorDecl>(value)) {
-    return getInferredActorIsolation(accessor->getStorage());
-  }
-
   if (auto var = dyn_cast<VarDecl>(value)) {
     auto &ctx = var->getASTContext();
     if (!ctx.LangOpts.isConcurrencyModelTaskToThread() &&
@@ -6601,7 +6613,8 @@ bool HasIsolatedSelfRequest::evaluate(
     }
   }
   if (attrIsolation) {
-    return attrIsolation == ActorIsolation::forActorInstanceSelf(selfTypeDecl);
+    return attrIsolation->getKind() == ActorIsolation::ActorInstance &&
+           attrIsolation->isActorInstanceForSelfParameter();
   }
 
   // If this is a variable, check for a property wrapper that alters its
@@ -7861,7 +7874,7 @@ ActorIsolation swift::getActorIsolationForReference(ValueDecl *decl,
     // as needing to enter the actor.
     if (auto nominal = ctor->getDeclContext()->getSelfNominalTypeDecl()) {
       if (nominal->isAnyActor())
-        return ActorIsolation::forActorInstanceSelf(decl);
+        return ActorIsolation::forActorInstanceSelf(ctor);
     }
 
     // Fall through to treat initializers like any other declaration.