Merge pull request #84181 from rjmccall/isolation-fixes

Isolation fixes for closures and defer bodies

Author: rjmccall

include/swift/AST/ActorIsolation.h

@@ -28,6 +28,7 @@ class raw_ostream;
 
 namespace swift {
 class DeclContext;
+class Initializer;
 class ModuleDecl;
 class VarDecl;
 class NominalTypeDecl;
@@ -434,6 +435,10 @@ InferredActorIsolation getInferredActorIsolation(ValueDecl *value);
 ActorIsolation
 __AbstractClosureExpr_getActorIsolation(AbstractClosureExpr *CE);
 
+/// Determine how the given initialization context is isolated.
+ActorIsolation getActorIsolation(Initializer *init,
+                                 bool ignoreDefaultArguments = false);
+
 /// Determine how the given declaration context is isolated.
 /// \p getClosureActorIsolation allows the specification of actor isolation for
 /// closures that haven't been saved been saved to the AST yet. This is useful

include/swift/AST/Decl.h

@@ -3287,6 +3287,9 @@ class ValueDecl : public Decl {
   /// `AbstractStorageDecl`, returns `false`.
   bool isAsync() const;
 
+  /// Returns whether this function represents a defer body.
+  bool isDeferBody() const;
+
 private:
   bool isObjCDynamic() const {
     return isObjC() && isDynamic();

include/swift/SIL/SILDeclRef.h

@@ -38,6 +38,7 @@ namespace swift {
   enum class EffectsKind : uint8_t;
   class AbstractFunctionDecl;
   class AbstractClosureExpr;
+  class ActorIsolation;
   class ValueDecl;
   class FuncDecl;
   class ClosureExpr;
@@ -508,6 +509,8 @@ struct SILDeclRef {
     return result;
   }
 
+  ActorIsolation getActorIsolation() const;
+
   /// True if the decl ref references a thunk from a natively foreign
   /// declaration to Swift calling convention.
   bool isForeignToNativeThunk() const;

lib/AST/Decl.cpp

@@ -9383,7 +9383,18 @@ void ParamDecl::setDefaultExpr(Expr *E) {
 }
 
 void ParamDecl::setTypeCheckedDefaultExpr(Expr *E) {
-  assert(E || getDefaultArgumentKind() == DefaultArgumentKind::Inherited);
+  // The type-checker will only produce a null expression here if the
+  // default argument is inherited, so if we're called with a null pointer
+  // in any other case, it must be from a request cycle. Don't crash;
+  // just wrap the original expression with an ErrorExpr and proceed.
+  if (!E && getDefaultArgumentKind() != DefaultArgumentKind::Inherited) {
+    auto *initExpr = getStructuralDefaultExpr();
+    assert(initExpr);
+    auto &ctx = getASTContext();
+    E = new (ctx) ErrorExpr(initExpr->getSourceRange(), ErrorType::get(ctx),
+                            initExpr);
+  }
+
   setDefaultExpr(E);
 
   auto *defaultInfo = DefaultValueAndFlags.getPointer();
@@ -11808,6 +11819,12 @@ PrecedenceGroupDecl *InfixOperatorDecl::getPrecedenceGroup() const {
       nullptr);
 }
 
+bool ValueDecl::isDeferBody() const {
+  if (auto fn = dyn_cast<FuncDecl>(this))
+    return fn->isDeferBody();
+  return false;
+}
+
 bool FuncDecl::isDeferBody() const {
   return getBaseIdentifier() == getASTContext().getIdentifier("$defer");
 }
@@ -11999,55 +12016,93 @@ ActorIsolation swift::getActorIsolationOfContext(
     DeclContext *dc,
     llvm::function_ref<ActorIsolation(AbstractClosureExpr *)>
         getClosureActorIsolation) {
-  auto &ctx = dc->getASTContext();
   auto dcToUse = dc;
-  // Defer bodies share actor isolation of their enclosing context.
-  if (auto FD = dyn_cast<FuncDecl>(dcToUse)) {
-    if (FD->isDeferBody()) {
-      dcToUse = FD->getDeclContext();
-    }
+
+  // Defer bodies share the actor isolation of their enclosing context.
+  // We don't actually have to do this check here because
+  // getActorIsolation does consider it already, but it's nice to
+  // avoid some extra request evaluation in a trivial case.
+  while (auto FD = dyn_cast<FuncDecl>(dcToUse)) {
+    if (!FD->isDeferBody()) break;
+    dcToUse = FD->getDeclContext();
   }
+
   if (auto *vd = dyn_cast_or_null<ValueDecl>(dcToUse->getAsDecl()))
     return getActorIsolation(vd);
 
-  // In the context of the initializing or default-value expression of a
-  // stored property:
-  //   - For a static stored property, the isolation matches the VarDecl.
-  //     Static properties are initialized upon first use, so the isolation
-  //     of the initializer must match the isolation required to access the
-  //     property.
-  //   - For a field of a nominal type, the expression can require the same
-  //     actor isolation as the field itself. That default expression may only
-  //     be used from inits that meet the required isolation.
-  if (auto *var = dcToUse->getNonLocalVarDecl()) {
-    // If IsolatedDefaultValues are enabled, treat this context as having
-    // unspecified isolation. We'll compute the required isolation for
-    // the initializer and validate that it matches the isolation of the
-    // var itself in the DefaultInitializerIsolation request.
-    if (ctx.LangOpts.hasFeature(Feature::IsolatedDefaultValues))
-      return ActorIsolation::forUnspecified();
-
-    return getActorIsolation(var);
-  }
-
   if (auto *closure = dyn_cast<AbstractClosureExpr>(dcToUse)) {
     return getClosureActorIsolation(closure);
   }
 
+  if (auto *init = dyn_cast<Initializer>(dcToUse)) {
+    // FIXME: force default argument initializers to report a meaningless
+    // isolation in order to break a bunch of cycles with the way that
+    // isolation is computed for them.
+    return getActorIsolation(init, /*ignoreDefaultArguments*/ true);
+  }
+
   if (isa<TopLevelCodeDecl>(dcToUse)) {
+    auto &ctx = dc->getASTContext();
     if (dcToUse->isAsyncContext() ||
-        dcToUse->getASTContext().LangOpts.StrictConcurrencyLevel >=
-            StrictConcurrency::Complete) {
-      if (Type mainActor = dcToUse->getASTContext().getMainActorType())
+        ctx.LangOpts.StrictConcurrencyLevel >= StrictConcurrency::Complete) {
+      if (Type mainActor = ctx.getMainActorType())
         return ActorIsolation::forGlobalActor(mainActor)
-            .withPreconcurrency(
-                !dcToUse->getASTContext().isSwiftVersionAtLeast(6));
+            .withPreconcurrency(!ctx.isSwiftVersionAtLeast(6));
     }
   }
 
   return ActorIsolation::forUnspecified();
 }
 
+ActorIsolation swift::getActorIsolation(Initializer *init,
+                                        bool ignoreDefaultArguments) {
+  switch (init->getInitializerKind()) {
+  case InitializerKind::PatternBinding:
+    // In the context of the initializing or default-value expression of a
+    // stored property:
+    //   - For a static stored property, the isolation matches the VarDecl.
+    //     Static properties are initialized upon first use, so the isolation
+    //     of the initializer must match the isolation required to access the
+    //     property.
+    //   - For a field of a nominal type, the expression can require the same
+    //     actor isolation as the field itself. That default expression may only
+    //     be used from inits that meet the required isolation.
+    if (auto *var = init->getNonLocalVarDecl()) {
+      auto &ctx = var->getASTContext();
+
+      // If IsolatedDefaultValues are enabled, treat this context as having
+      // unspecified isolation. We'll compute the required isolation for
+      // the initializer and validate that it matches the isolation of the
+      // var itself in the DefaultInitializerIsolation request.
+      if (ctx.LangOpts.hasFeature(Feature::IsolatedDefaultValues))
+        return ActorIsolation::forUnspecified();
+
+      return getActorIsolation(var);
+    }
+
+    return ActorIsolation::forUnspecified();
+
+  case InitializerKind::DefaultArgument: {
+    auto defArgInit = cast<DefaultArgumentInitializer>(init);
+
+    // A hack when used from getActorIsolationOfContext to maintain the
+    // current behavior and avoid request cycles.
+    if (ignoreDefaultArguments)
+      return ActorIsolation::forUnspecified();
+
+    auto fn = cast<ValueDecl>(defArgInit->getParent()->getAsDecl());
+    auto param = getParameterAt(fn, defArgInit->getIndex());
+    assert(param);
+    return param->getInitializerIsolation();
+  }
+
+  case InitializerKind::PropertyWrapper:
+  case InitializerKind::CustomAttribute:
+    return ActorIsolation::forUnspecified();
+  }
+  llvm_unreachable("bad initializer kind");
+}
+
 bool swift::isSameActorIsolated(ValueDecl *value, DeclContext *dc) {
     auto valueIsolation = getActorIsolation(value);
     auto dcIsolation = getActorIsolationOfContext(dc);

lib/SIL/IR/SILDeclRef.cpp

@@ -1921,3 +1921,21 @@ bool SILDeclRef::isCalleeAllocatedCoroutine() const {
 
   return getASTContext().SILOpts.CoroutineAccessorsUseYieldOnce2;
 }
+
+ActorIsolation SILDeclRef::getActorIsolation() const {
+  // Deallocating destructor is always nonisolated. Isolation of the deinit
+  // applies only to isolated deallocator and destroyer.
+  if (kind == SILDeclRef::Kind::Deallocator) {
+    return ActorIsolation::forNonisolated(false);
+  }
+
+  // Default argument generators use the isolation of the initializer,
+  // not the general isolation of the function.
+  if (isDefaultArgGenerator()) {
+    auto param = getParameterAt(getDecl(), defaultArgIndex);
+    assert(param);
+    return param->getInitializerIsolation();
+  }
+
+  return getActorIsolationOfContext(getInnermostDeclContext());
+}

lib/SIL/IR/SILFunctionType.cpp

@@ -1261,6 +1261,11 @@ class Conventions {
 
   ConventionsKind getKind() const { return kind; }
 
+  bool hasCallerIsolationParameter() const {
+    return kind == ConventionsKind::Default ||
+           kind == ConventionsKind::Deallocator;
+  }
+
   virtual ParameterConvention
   getIndirectParameter(unsigned index,
                        const AbstractionPattern &type,
@@ -1700,14 +1705,10 @@ class DestructureInputs {
       };
     }
 
-    // If we are an async function that is unspecified or nonisolated, insert an
-    // isolated parameter if NonisolatedNonsendingByDefault is enabled.
-    //
-    // NOTE: The parameter is not inserted for async functions imported
-    // from ObjC because they are handled in a special way that doesn't
-    // require it.
+    // If the function has nonisolated(nonsending) isolation, insert the
+    // implicit isolation parameter.
     if (IsolationInfo && IsolationInfo->isCallerIsolationInheriting() &&
-        extInfoBuilder.isAsync() && !Foreign.async) {
+        Convs.hasCallerIsolationParameter()) {
       auto actorProtocol = TC.Context.getProtocol(KnownProtocolKind::Actor);
       auto actorType =
           ExistentialType::get(actorProtocol->getDeclaredInterfaceType());
@@ -2412,32 +2413,7 @@ swift::getSILFunctionTypeActorIsolation(CanAnyFunctionType substFnInterfaceType,
   }
 
   if (constant) {
-    // TODO: It should to be possible to `getActorIsolation` if
-    // reference is to a decl instead of trying to get isolation
-    // from the reference kind, the attributes, or the context.
-
-    if (constant->kind == SILDeclRef::Kind::Deallocator) {
-      return ActorIsolation::forNonisolated(false);
-    }
-
-    if (auto *decl = constant->getAbstractFunctionDecl()) {
-      if (auto *nonisolatedAttr =
-              decl->getAttrs().getAttribute<NonisolatedAttr>()) {
-        if (nonisolatedAttr->isNonSending())
-          return ActorIsolation::forCallerIsolationInheriting();
-      }
-
-      if (decl->getAttrs().hasAttribute<ConcurrentAttr>()) {
-        return ActorIsolation::forNonisolated(false /*unsafe*/);
-      }
-    }
-
-    if (auto *closure = constant->getAbstractClosureExpr()) {
-      if (auto isolation = closure->getActorIsolation())
-        return isolation;
-    }
-
-    return getActorIsolationOfContext(constant->getInnermostDeclContext());
+    return constant->getActorIsolation();
   }
 
   if (substFnInterfaceType->hasExtInfo() &&

lib/SILGen/SILGen.cpp

@@ -801,32 +801,6 @@ static bool isEmittedOnDemand(SILModule &M, SILDeclRef constant) {
   return false;
 }
 
-static ActorIsolation getActorIsolationForFunction(SILFunction &fn) {
-  if (auto constant = fn.getDeclRef()) {
-    if (constant.kind == SILDeclRef::Kind::Deallocator) {
-      // Deallocating destructor is always nonisolated. Isolation of the deinit
-      // applies only to isolated deallocator and destroyer.
-      return ActorIsolation::forNonisolated(false);
-    }
-
-    // If we have a closure expr, check if our type is
-    // nonisolated(nonsending). In that case, we use that instead.
-    if (auto *closureExpr = constant.getAbstractClosureExpr()) {
-      if (auto actorIsolation = closureExpr->getActorIsolation())
-        return actorIsolation;
-    }
-
-    // If we have actor isolation for our constant, put the isolation onto the
-    // function. If the isolation is unspecified, we do not return it.
-    if (auto isolation =
-            getActorIsolationOfContext(constant.getInnermostDeclContext()))
-      return isolation;
-  }
-
-  // Otherwise, return for unspecified.
-  return ActorIsolation::forUnspecified();
-}
-
 SILFunction *SILGenModule::getFunction(SILDeclRef constant,
                                        ForDefinition_t forDefinition) {
   // If we already emitted the function, return it.
@@ -853,7 +827,7 @@ SILFunction *SILGenModule::getFunction(SILDeclRef constant,
       });
 
   F->setDeclRef(constant);
-  F->setActorIsolation(getActorIsolationForFunction(*F));
+  F->setActorIsolation(constant.getActorIsolation());
 
   assert(F && "SILFunction should have been defined");
 
@@ -1384,7 +1358,7 @@ void SILGenModule::preEmitFunction(SILDeclRef constant, SILFunction *F,
   F->setDeclRef(constant);
 
   // Set our actor isolation.
-  F->setActorIsolation(getActorIsolationForFunction(*F));
+  F->setActorIsolation(constant.getActorIsolation());
 
   LLVM_DEBUG(llvm::dbgs() << "lowering ";
              F->printName(llvm::dbgs());

lib/SILGen/SILGenConcurrency.cpp

@@ -190,7 +190,7 @@ void SILGenFunction::emitExpectedExecutorProlog() {
     }
 
     case ActorIsolation::CallerIsolationInheriting:
-      assert(F.isAsync());
+      assert(F.isAsync() || F.isDefer());
       setExpectedExecutorForParameterIsolation(*this, actorIsolation);
       break;
 
@@ -255,7 +255,7 @@ void SILGenFunction::emitExpectedExecutorProlog() {
           RegularLocation::getDebugOnlyLocation(F.getLocation(), getModule()),
           executor,
           /*mandatory*/ false);
-    } else {
+    } else if (wantDataRaceChecks) {
       // For a synchronous function, check that we're on the same executor.
       // Note: if we "know" that the code is completely Sendable-safe, this
       // is unnecessary. The type checker will need to make this determination.