Merge pull request #69974 from hborla/isolated-default-value-fixes

hborla · web-flow · commit bd61b6675486 · 2023-11-27T12:51:27.000-08:00
[SE-0411] Fixes for isolated default values.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -5352,8 +5352,14 @@ ERROR(distributed_actor_isolated_non_self_reference,none,
       "distributed actor-isolated %kind0 can not be accessed from a "
       "non-isolated context",
       (const ValueDecl *))
+ERROR(conflicting_stored_property_isolation,none,
+      "%select{default|memberwise}0 initializer for %1 cannot be both %2 and %3",
+      (bool, Type, ActorIsolation, ActorIsolation))
+NOTE(property_requires_actor,none,
+      "initializer for %0 %1 is %2",
+      (DescriptiveDeclKind, Identifier, ActorIsolation))
 ERROR(isolated_default_argument_context,none,
-      "%0 default argument in a %1 context", 
+      "%0 default value in a %1 context", 
       (ActorIsolation, ActorIsolation))
 ERROR(conflicting_default_argument_isolation,none,
       "default argument cannot be both %0 and %1",
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -10637,6 +10637,7 @@ 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)) {
@@ -10657,6 +10658,13 @@ ActorIsolation swift::getActorIsolationOfContext(
   //     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);
   }
 
diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp
@@ -337,13 +337,64 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl,
   ctor->setSynthesized();
   ctor->setAccess(accessLevel);
 
+  if (ctx.LangOpts.hasFeature(Feature::IsolatedDefaultValues)) {
+    // If any of the type's actor-isolated properties:
+    //   1. Have non-Sendable type, or
+    //   2. Have an isolated initial value
+    // then the initializer must also be actor-isolated. If all
+    // isolated properties have Sendable type and a nonisolated
+    // default value, then the initializer can be nonisolated.
+    //
+    // These rules only apply for global actor isolation, because actor
+    // initializers apply Sendable checking to arguments at the call-site,
+    // and actor initializers do not run on the actor, so initial values
+    // cannot be actor-instance-isolated.
+    bool shouldAddNonisolated = true;
+    llvm::Optional<ActorIsolation> existingIsolation = llvm::None;
+    VarDecl *previousVar = nullptr;
+
+    // The memberwise init properties are also effectively what the
+    // default init uses, e.g. default initializers initialize via
+    // properties wrapped and init accessors.
+    for (auto var : decl->getMemberwiseInitProperties()) {
+      auto type = var->getTypeInContext();
+      auto isolation = getActorIsolation(var);
+      if (isolation.isGlobalActor()) {
+        if (!isSendableType(decl->getModuleContext(), type) ||
+            var->getInitializerIsolation().isGlobalActor()) {
+          // If different isolated stored properties require different
+          // global actors, it is impossible to initialize this type.
+          if (existingIsolation &&
+              *existingIsolation != isolation) {
+            ctx.Diags.diagnose(decl->getLoc(),
+                diag::conflicting_stored_property_isolation,
+                ICK == ImplicitConstructorKind::Memberwise,
+                decl->getDeclaredType(), *existingIsolation, isolation);
+            previousVar->diagnose(
+                diag::property_requires_actor,
+                previousVar->getDescriptiveKind(),
+                previousVar->getName(), *existingIsolation);
+            var->diagnose(
+                diag::property_requires_actor,
+                var->getDescriptiveKind(),
+                var->getName(), isolation);
+          }
+
+          existingIsolation = isolation;
+          previousVar = var;
+          shouldAddNonisolated = false;
+        }
+      }
+    }
+
+    if (shouldAddNonisolated) {
+      addNonIsolatedToSynthesized(decl, ctor);
+    }
+  }
+
   if (ICK == ImplicitConstructorKind::Memberwise) {
     ctor->setIsMemberwiseInitializer();
 
-    // FIXME: If 'IsolatedDefaultValues' is enabled, the memberwise init
-    // should be 'nonisolated' if none of the memberwise-initialized properties
-    // are global actor isolated and have non-Sendable type, and none of the
-    // initial values require global actor isolation.
     if (!ctx.LangOpts.hasFeature(Feature::IsolatedDefaultValues)) {
       addNonIsolatedToSynthesized(decl, ctor);
     }
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -3143,7 +3143,10 @@ namespace {
       if (!unsatisfiedIsolation)
         return false;
 
-      if (refineRequiredIsolation(*unsatisfiedIsolation))
+      bool onlyArgsCrossIsolation = callOptions.contains(
+          ActorReferenceResult::Flags::OnlyArgsCrossIsolation);
+      if (!onlyArgsCrossIsolation &&
+          refineRequiredIsolation(*unsatisfiedIsolation))
         return false;
 
       // At this point, we know a jump is made to the callee that yields
@@ -4757,14 +4760,19 @@ DefaultInitializerIsolation::evaluate(Evaluator &evaluator,
 
   Initializer *dc = nullptr;
   Expr *initExpr = nullptr;
+  ActorIsolation enclosingIsolation;
 
   if (auto *pbd = var->getParentPatternBinding()) {
     if (!var->isParentInitialized())
       return ActorIsolation::forUnspecified();
 
     auto i = pbd->getPatternEntryIndexForVarDecl(var);
+    if (!pbd->isInitializerChecked(i))
+      TypeChecker::typeCheckPatternBinding(pbd, i);
+
     dc = cast<Initializer>(pbd->getInitContext(i));
-    initExpr = var->getParentInitializer();
+    initExpr = pbd->getInit(i);
+    enclosingIsolation = getActorIsolation(var);
   } else if (auto *param = dyn_cast<ParamDecl>(var)) {
     // If this parameter corresponds to a stored property for a
     // memberwise initializer, the default argument is the default
@@ -4782,13 +4790,27 @@ DefaultInitializerIsolation::evaluate(Evaluator &evaluator,
 
     dc = param->getDefaultArgumentInitContext();
     initExpr = param->getTypeCheckedDefaultExpr();
+    enclosingIsolation =
+        getActorIsolationOfContext(param->getDeclContext());
   }
 
   if (!dc || !initExpr)
     return ActorIsolation::forUnspecified();
 
+  // If the default argument has isolation, it must match the
+  // isolation of the decl context.
   ActorIsolationChecker checker(dc);
-  return checker.computeRequiredIsolation(initExpr);
+  auto requiredIsolation = checker.computeRequiredIsolation(initExpr);
+  if (requiredIsolation.isActorIsolated()) {
+    if (enclosingIsolation != requiredIsolation) {
+      var->diagnose(
+          diag::isolated_default_argument_context,
+          requiredIsolation, enclosingIsolation);
+      return ActorIsolation::forUnspecified();
+    }
+  }
+
+  return requiredIsolation;
 }
 
 void swift::checkOverrideActorIsolation(ValueDecl *value) {
@@ -5859,8 +5881,8 @@ bool swift::isAccessibleAcrossActors(
 }
 
 ActorReferenceResult ActorReferenceResult::forSameConcurrencyDomain(
-    ActorIsolation isolation) {
-  return ActorReferenceResult{SameConcurrencyDomain, llvm::None, isolation};
+    ActorIsolation isolation, Options options) {
+  return ActorReferenceResult{SameConcurrencyDomain, options, isolation};
 }
 
 ActorReferenceResult ActorReferenceResult::forEntersActor(
@@ -5869,8 +5891,8 @@ ActorReferenceResult ActorReferenceResult::forEntersActor(
 }
 
 ActorReferenceResult ActorReferenceResult::forExitsActorToNonisolated(
-    ActorIsolation isolation) {
-  return ActorReferenceResult{ExitsActorToNonisolated, llvm::None, isolation};
+    ActorIsolation isolation, Options options) {
+  return ActorReferenceResult{ExitsActorToNonisolated, options, isolation};
 }
 
 // Determine if two actor isolation contexts are considered to be equivalent.
@@ -5906,10 +5928,24 @@ ActorReferenceResult ActorReferenceResult::forReference(
       declIsolation = declIsolation.subst(declRef.getSubstitutions());
   }
 
+  // Determine what adjustments we need to perform for cross-actor
+  // references.
+  Options options = llvm::None;
+
+  // FIXME: Actor constructors are modeled as isolated to the actor
+  // so that Sendable checking is applied to their arguments, but the
+  // call itself does not hop to another executor.
+  if (auto ctor = dyn_cast<ConstructorDecl>(declRef.getDecl())) {
+    if (auto nominal = ctor->getDeclContext()->getSelfNominalTypeDecl()) {
+      if (nominal->isAnyActor())
+        options |= Flags::OnlyArgsCrossIsolation;
+    }
+  }
+
   // If the entity we are referencing is not a value, we're in the same
   // concurrency domain.
   if (isNonValueReference(declRef.getDecl()))
-    return forSameConcurrencyDomain(declIsolation);
+    return forSameConcurrencyDomain(declIsolation, options);
 
   // Compute the isolation of the context, if not provided.
   ActorIsolation contextIsolation = ActorIsolation::forUnspecified();
@@ -5928,11 +5964,11 @@ ActorReferenceResult ActorReferenceResult::forReference(
     if (isAsyncDecl(declRef) && contextIsolation.isActorIsolated() &&
         !declRef.getDecl()->getAttrs()
             .hasAttribute<UnsafeInheritExecutorAttr>())
-      return forExitsActorToNonisolated(contextIsolation);
+      return forExitsActorToNonisolated(contextIsolation, options);
 
     // Otherwise, we stay in the same concurrency domain, whether on an actor
     // or in a task.
-    return forSameConcurrencyDomain(declIsolation);
+    return forSameConcurrencyDomain(declIsolation, options);
   }
 
   // The declaration we are accessing is actor-isolated. First, check whether
@@ -5941,11 +5977,11 @@ ActorReferenceResult ActorReferenceResult::forReference(
       declIsolation.getActorInstanceParameter() == 0) {
     // If this instance is isolated, we're in the same concurrency domain.
     if (actorInstance->isIsolated())
-      return forSameConcurrencyDomain(declIsolation);
+      return forSameConcurrencyDomain(declIsolation, options);
   } else if (equivalentIsolationContexts(declIsolation, contextIsolation)) {
     // The context isolation matches, so we are in the same concurrency
     // domain.
-    return forSameConcurrencyDomain(declIsolation);
+    return forSameConcurrencyDomain(declIsolation, options);
   }
 
   // Initializing an actor isolated stored property with a value effectively
@@ -5965,7 +6001,8 @@ ActorReferenceResult ActorReferenceResult::forReference(
         // Treat the decl isolation as 'preconcurrency' to downgrade violations
         // to warnings, because violating Sendable here is accepted by the
         // Swift 5.9 compiler.
-        return forEntersActor(declIsolation, Flags::Preconcurrency);
+        options |= Flags::Preconcurrency;
+        return forEntersActor(declIsolation, options);
       }
     }
   }
@@ -5975,7 +6012,7 @@ ActorReferenceResult ActorReferenceResult::forReference(
   if (actorInstance &&
       checkedByFlowIsolation(
           fromDC, *actorInstance, declRef.getDecl(), declRefLoc, useKind))
-    return forSameConcurrencyDomain(declIsolation);
+    return forSameConcurrencyDomain(declIsolation, options);
 
   // If we are delegating to another initializer, treat them as being in the
   // same concurrency domain.
@@ -5984,7 +6021,7 @@ ActorReferenceResult ActorReferenceResult::forReference(
   if (actorInstance && actorInstance->isSelf() &&
       isa<ConstructorDecl>(declRef.getDecl()) &&
       isa<ConstructorDecl>(fromDC))
-    return forSameConcurrencyDomain(declIsolation);
+    return forSameConcurrencyDomain(declIsolation, options);
 
   // If there is an instance that corresponds to 'self',
   // we are in a constructor or destructor, and we have a stored property of
@@ -5994,18 +6031,14 @@ ActorReferenceResult ActorReferenceResult::forReference(
       isNonInheritedStorage(declRef.getDecl(), fromDC) &&
       declIsolation.isGlobalActor() &&
       (isa<ConstructorDecl>(fromDC) || isa<DestructorDecl>(fromDC)))
-    return forSameConcurrencyDomain(declIsolation);
-
-  // Determine what adjustments we need to perform for cross-actor
-  // references.
-  Options options = llvm::None;
+    return forSameConcurrencyDomain(declIsolation, options);
 
   // At this point, we are accessing the target from outside the actor.
   // First, check whether it is something that can be accessed directly,
   // without any kind of promotion.
   if (isAccessibleAcrossActors(
           declRef.getDecl(), declIsolation, fromDC, options, actorInstance))
-    return forEntersActor(declIsolation, llvm::None);
+    return forEntersActor(declIsolation, options);
 
   // This is a cross-actor reference.
 
diff --git a/lib/Sema/TypeCheckConcurrency.h b/lib/Sema/TypeCheckConcurrency.h
@@ -202,6 +202,10 @@ struct ActorReferenceResult {
 
     /// The declaration is being accessed from a @preconcurrency context.
     Preconcurrency = 1 << 3,
+
+    /// Only arguments cross an isolation boundary, e.g. because they
+    /// escape into an actor in a nonisolated actor initializer.
+    OnlyArgsCrossIsolation = 1 << 4,
   };
 
   using Options = OptionSet<Flags>;
@@ -212,13 +216,13 @@ struct ActorReferenceResult {
 
 private:
   static ActorReferenceResult forSameConcurrencyDomain(
-      ActorIsolation isolation);
+      ActorIsolation isolation, Options options = llvm::None);
 
   static ActorReferenceResult forEntersActor(
       ActorIsolation isolation, Options options);
 
   static ActorReferenceResult forExitsActorToNonisolated(
-      ActorIsolation isolation);
+      ActorIsolation isolation, Options options = llvm::None);
 
 public:
   /// Determine what happens when referencing the given declaration from the
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -1055,15 +1055,7 @@ static void checkDefaultArguments(ParameterList *params) {
 
     // If the default argument has isolation, it must match the
     // isolation of the decl context.
-    auto defaultArgIsolation = param->getInitializerIsolation();
-    if (defaultArgIsolation.isActorIsolated()) {
-      auto *dc = param->getDeclContext();
-      auto enclosingIsolation = getActorIsolationOfContext(dc);
-      if (enclosingIsolation != defaultArgIsolation) {
-        param->diagnose(diag::isolated_default_argument_context,
-            defaultArgIsolation, enclosingIsolation);
-      }
-    }
+    (void)param->getInitializerIsolation();
 
     if (!ifacety->hasPlaceholder()) {
       continue;
diff --git a/test/Concurrency/isolated_default_arguments.swift b/test/Concurrency/isolated_default_arguments.swift
diff --git a/test/Concurrency/isolated_default_property_inits.swift b/test/Concurrency/isolated_default_property_inits.swift
