Merge pull request #59086 from DougGregor/se-0338-sendable

Author: DougGregor

CHANGELOG.md

@@ -5,6 +5,39 @@ _**Note:** This is in reverse chronological order, so newer entries are added to
 
 ## Swift 5.7
 
+* [SE-0338][]:
+
+  Non-isolated async functions now always execute on the global concurrent pool,
+  so calling a non-isolated async function from actor-isolated code will leave
+  the actor. For example:
+
+  ```swift
+  class C { }
+
+  func f(_: C) async { /* always executes on the global concurrent pool */ }
+
+  actor A {
+    func g(c: C) async {
+      /* always executes on the actor */
+      print("on the actor")
+
+      await f(c)
+    }
+  }
+  ```
+
+  Prior to this change, the call from `f` to `g` might have started execution of
+  `g` on the actor, which could lead to actors being busy longer than strictly
+  necessary. Now, the non-isolated async function will always hop to the global
+  cooperative pool, not run on the actor. This can result in a behavior change
+  for programs that assumed that a non-isolated async function called from a
+  `@MainActor` context will be executed on the main actor, although such
+  programs were already technically incorrect.
+
+  Additionally, when leaving an actor to execution on the global cooperative
+  pool, `Sendable` checking will be performed, so the compiler will emit a
+  diagnostic in the call to `f` if `c` is not of `Sendable` type.
+
 * [SE-0350][]:
 
   The standard library has a new `Regex<Output>` type.
@@ -9421,6 +9454,7 @@ Swift 1.0
 [SE-0335]: <https://github.com/apple/swift-evolution/blob/main/proposals/0335-existential-any.md>
 [SE-0336]: <https://github.com/apple/swift-evolution/blob/main/proposals/0336-distributed-actor-isolation.md>
 [SE-0337]: <https://github.com/apple/swift-evolution/blob/main/proposals/0337-support-incremental-migration-to-concurrency-checking.md>
+[SE-0338]: <https://github.com/apple/swift-evolution/blob/main/proposals/0338-clarify-execution-non-actor-async.md>
 [SE-0340]: <https://github.com/apple/swift-evolution/blob/main/proposals/0340-swift-noasync.md>
 [SE-0341]: <https://github.com/apple/swift-evolution/blob/main/proposals/0341-opaque-parameters.md>
 [SE-0343]: <https://github.com/apple/swift-evolution/blob/main/proposals/0343-top-level-concurrency.md>

include/swift/AST/DiagnosticsSema.def

@@ -4657,9 +4657,10 @@ ERROR(isolated_parameter_not_actor,none,
 
 WARNING(non_sendable_param_type,none,
         "non-sendable type %0 %select{passed in call to %4 %2 %3|"
+        "exiting %4 context in call to non-isolated %2 %3|"
         "passed in implicitly asynchronous call to %4 %2 %3|"
-        "in parameter of %4 %2 %3 satisfying non-isolated protocol "
-        "requirement|"
+        "in parameter of %4 %2 %3 satisfying protocol requirement|"
+        "in parameter of %4 overriding %2 %3|"
         "in parameter of %4 '@objc' %2 %3}1 cannot cross actor boundary",
         (Type, unsigned, DescriptiveDeclKind, DeclName, ActorIsolation))
 WARNING(non_sendable_call_param_type,none,
@@ -4668,8 +4669,10 @@ WARNING(non_sendable_call_param_type,none,
         (Type, bool, ActorIsolation))
 WARNING(non_sendable_result_type,none,
         "non-sendable type %0 returned by %select{call to %4 %2 %3|"
+        "call from %4 context to non-isolated %2 %3|"
         "implicitly asynchronous call to %4 %2 %3|"
-        "%4 %2 %3 satisfying non-isolated protocol requirement|"
+        "%4 %2 %3 satisfying protocol requirement|"
+        "%4 overriding %2 %3|"
         "%4 '@objc' %2 %3}1 cannot cross actor boundary",
         (Type, unsigned, DescriptiveDeclKind, DeclName, ActorIsolation))
 WARNING(non_sendable_call_result_type,none,
@@ -4679,8 +4682,10 @@ WARNING(non_sendable_call_result_type,none,
 WARNING(non_sendable_property_type,none,
         "non-sendable type %0 in %select{"
         "%select{asynchronous access to %5 %1 %2|"
+        "asynchronous access from %5 context to non-isolated %1 %2|"
         "implicitly asynchronous access to %5 %1 %2|"
-        "conformance of %5 %1 %2 to non-isolated protocol requirement|"
+        "conformance of %5 %1 %2 to protocol requirement|"
+        "%5 overriding %1 %2|"
         "%5 '@objc' %1 %2}4|captured local %1 %2}3 cannot "
         "cross %select{actor|task}3 boundary",
         (Type, DescriptiveDeclKind, DeclName, bool, unsigned, ActorIsolation))

lib/Sema/TypeCheckConcurrency.cpp

@@ -934,7 +934,16 @@ bool swift::diagnoseNonSendableTypes(
 
 bool swift::diagnoseNonSendableTypesInReference(
     ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc loc,
-    SendableCheckReason reason) {
+    SendableCheckReason reason, Optional<ActorIsolation> knownIsolation) {
+
+  // Retrieve the actor isolation to use in diagnostics.
+  auto getActorIsolation = [&] {
+    if (knownIsolation)
+      return *knownIsolation;
+
+    return swift::getActorIsolation(declRef.getDecl());
+  };
+
   // For functions, check the parameter and result types.
   SubstitutionMap subs = declRef.getSubstitutions();
   if (auto function = dyn_cast<AbstractFunctionDecl>(declRef.getDecl())) {
@@ -943,7 +952,7 @@ bool swift::diagnoseNonSendableTypesInReference(
       if (diagnoseNonSendableTypes(
               paramType, fromDC, loc, diag::non_sendable_param_type,
               (unsigned)reason, function->getDescriptiveKind(),
-              function->getName(), getActorIsolation(function)))
+              function->getName(), getActorIsolation()))
         return true;
     }
 
@@ -953,7 +962,7 @@ bool swift::diagnoseNonSendableTypesInReference(
       if (diagnoseNonSendableTypes(
               resultType, fromDC, loc, diag::non_sendable_result_type,
               (unsigned)reason, func->getDescriptiveKind(), func->getName(),
-              getActorIsolation(func)))
+              getActorIsolation()))
         return true;
     }
 
@@ -970,7 +979,7 @@ bool swift::diagnoseNonSendableTypesInReference(
             var->getDescriptiveKind(), var->getName(),
             var->isLocalCapture(),
             (unsigned)reason,
-            getActorIsolation(var)))
+            getActorIsolation()))
       return true;
   }
 
@@ -980,7 +989,7 @@ bool swift::diagnoseNonSendableTypesInReference(
       if (diagnoseNonSendableTypes(
               paramType, fromDC, loc, diag::non_sendable_param_type,
               (unsigned)reason, subscript->getDescriptiveKind(),
-              subscript->getName(), getActorIsolation(subscript)))
+              subscript->getName(), getActorIsolation()))
         return true;
     }
 
@@ -989,7 +998,7 @@ bool swift::diagnoseNonSendableTypesInReference(
     if (diagnoseNonSendableTypes(
             resultType, fromDC, loc, diag::non_sendable_result_type,
             (unsigned)reason, subscript->getDescriptiveKind(),
-            subscript->getName(), getActorIsolation(subscript)))
+            subscript->getName(), getActorIsolation()))
       return true;
 
     return false;
@@ -2772,8 +2781,10 @@ namespace {
         if (diagnoseReferenceToUnsafeGlobal(decl, loc))
           return true;
 
-        // FIXME: SE-0338 would trigger Sendable checks here.
-        return false;
+        return diagnoseNonSendableTypesInReference(
+                   declRef, getDeclContext(), loc,
+                   SendableCheckReason::ExitingActor,
+                   result.isolation);
 
       case ActorReferenceResult::EntersActor:
         // Handle all of the checking below.
@@ -3539,19 +3550,25 @@ static ActorIsolation getOverriddenIsolationFor(ValueDecl *value) {
   return isolation.subst(subs);
 }
 
+static ConcreteDeclRef getDeclRefInContext(ValueDecl *value) {
+  auto declContext = value->getInnermostDeclContext();
+  if (auto genericEnv = declContext->getGenericEnvironmentOfContext()) {
+    return ConcreteDeclRef(
+        value, genericEnv->getForwardingSubstitutionMap());
+  }
+
+  return ConcreteDeclRef(value);
+}
+
 /// Generally speaking, the isolation of the decl that overrides
 /// must match the overridden decl. But there are a number of exceptions,
 /// e.g., the decl that overrides can be nonisolated.
 /// \param isolation the isolation of the overriding declaration.
 static OverrideIsolationResult validOverrideIsolation(
     ValueDecl *value, ActorIsolation isolation,
     ValueDecl *overridden, ActorIsolation overriddenIsolation) {
-  ConcreteDeclRef valueRef(value);
+  ConcreteDeclRef valueRef = getDeclRefInContext(value);
   auto declContext = value->getInnermostDeclContext();
-  if (auto genericEnv = declContext->getGenericEnvironmentOfContext()) {
-    valueRef = ConcreteDeclRef(
-        value, genericEnv->getForwardingSubstitutionMap());
-  }
 
   auto refResult = ActorReferenceResult::forReference(
       valueRef, SourceLoc(), declContext, None, None,
@@ -3570,9 +3587,7 @@ static OverrideIsolationResult validOverrideIsolation(
     if (isAsyncDecl(overridden) ||
         isAccessibleAcrossActors(
             overridden, refResult.isolation, declContext)) {
-      // FIXME: Perform Sendable checking here because we're entering an
-      // actor.
-      return OverrideIsolationResult::Allowed;
+      return OverrideIsolationResult::Sendable;
     }
 
     // If the overridden declaration is from Objective-C with no actor
@@ -3948,7 +3963,9 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
     return;
 
   case OverrideIsolationResult::Sendable:
-    // FIXME: Do the Sendable check.
+    diagnoseNonSendableTypesInReference(
+        getDeclRefInContext(value), value->getInnermostDeclContext(),
+        value->getLoc(), SendableCheckReason::Override);
     return;
 
   case OverrideIsolationResult::Disallowed:
@@ -4886,9 +4903,8 @@ bool swift::isThrowsDecl(ConcreteDeclRef declRef) {
   return false;
 }
 
-bool swift::isAccessibleAcrossActors(
-    ValueDecl *value, const ActorIsolation &isolation,
-    const DeclContext *fromDC, Optional<ReferencedActor> actorInstance) {
+/// Determine whether a reference to this value isn't actually a value.
+static bool isNonValueReference(const ValueDecl *value) {
   switch (value->getKind()) {
   case DeclKind::AssociatedType:
   case DeclKind::Class:
@@ -4899,13 +4915,7 @@ bool swift::isAccessibleAcrossActors(
   case DeclKind::Protocol:
   case DeclKind::Struct:
   case DeclKind::TypeAlias:
-    return true;
-
   case DeclKind::EnumCase:
-  case DeclKind::EnumElement:
-    // Type-level entities are always accessible across actors.
-    return true;
-
   case DeclKind::IfConfig:
   case DeclKind::Import:
   case DeclKind::InfixOperator:
@@ -4917,16 +4927,26 @@ bool swift::isAccessibleAcrossActors(
   case DeclKind::PrecedenceGroup:
   case DeclKind::PrefixOperator:
   case DeclKind::TopLevelCode:
-    // Non-value entities are always accessible across actors.
-    return true;
-
   case DeclKind::Destructor:
-    // Destructors are always accessible across actors.
     return true;
 
+  case DeclKind::EnumElement:
   case DeclKind::Constructor:
-    // Initializers are accessible across actors unless they are global-actor
-    // qualified.
+  case DeclKind::Param:
+  case DeclKind::Var:
+  case DeclKind::Accessor:
+  case DeclKind::Func:
+  case DeclKind::Subscript:
+    return false;
+  }
+}
+
+bool swift::isAccessibleAcrossActors(
+    ValueDecl *value, const ActorIsolation &isolation,
+    const DeclContext *fromDC, Optional<ReferencedActor> actorInstance) {
+  // Initializers and enum elements are accessible across actors unless they
+  // are global-actor qualified.
+  if (isa<ConstructorDecl>(value) || isa<EnumElementDecl>(value)) {
     switch (isolation) {
     case ActorIsolation::ActorInstance:
     case ActorIsolation::Independent:
@@ -4937,19 +4957,15 @@ bool swift::isAccessibleAcrossActors(
     case ActorIsolation::GlobalActor:
       return false;
     }
+  }
 
-  case DeclKind::Param:
-  case DeclKind::Var:
-    // 'let' declarations are immutable, so some of them can be accessed across
-    // actors.
-    return varIsSafeAcrossActors(
-        fromDC->getParentModule(), cast<VarDecl>(value), isolation);
-
-  case DeclKind::Accessor:
-  case DeclKind::Func:
-  case DeclKind::Subscript:
-    return false;
+  // 'let' declarations are immutable, so some of them can be accessed across
+  // actors.
+  if (auto var = dyn_cast<VarDecl>(value)) {
+    return varIsSafeAcrossActors(fromDC->getParentModule(), var, isolation);
   }
+
+  return false;
 }
 
 ActorReferenceResult ActorReferenceResult::forSameConcurrencyDomain(
@@ -4998,6 +5014,11 @@ ActorReferenceResult ActorReferenceResult::forReference(
       declIsolation = declIsolation.subst(declRef.getSubstitutions());
   }
 
+  // If the entity we are referencing is not a value, we're in thesame
+  // concurrency domain.
+  if (isNonValueReference(declRef.getDecl()))
+    return forSameConcurrencyDomain(declIsolation);
+
   // Compute the isolation of the context, if not provided.
   ActorIsolation contextIsolation = ActorIsolation::forUnspecified();
   if (knownContextIsolation) {

lib/Sema/TypeCheckConcurrency.h

@@ -75,6 +75,9 @@ enum class SendableCheckReason {
   /// A reference to an actor from outside that actor.
   CrossActor,
 
+  /// Exiting an actor to non-isolated async code.
+  ExitingActor,
+
   /// A synchronous operation that was "promoted" to an asynchronous one
   /// because it was out of the actor's domain.
   SynchronousAsAsync,
@@ -83,6 +86,9 @@ enum class SendableCheckReason {
   /// actor isolation.
   Conformance,
 
+  /// An override of a function.
+  Override,
+
   /// The declaration is being exposed to Objective-C.
   ObjC,
 };
@@ -268,7 +274,8 @@ struct ActorReferenceResult {
 /// \returns true if an problem was detected, false otherwise.
 bool diagnoseNonSendableTypesInReference(
     ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc loc,
-    SendableCheckReason refKind);
+    SendableCheckReason refKind,
+    Optional<ActorIsolation> knownIsolation = None);
 
 /// Produce a diagnostic for a missing conformance to Sendable.
 void diagnoseMissingSendableConformance(

lib/Sema/TypeCheckProtocol.cpp

@@ -2955,6 +2955,10 @@ bool ConformanceChecker::checkActorIsolation(
     requirementIsolation = requirementIsolation.subst(subs);
   }
 
+  SourceLoc loc = witness->getLoc();
+  if (loc.isInvalid())
+    loc = Conformance->getLoc();
+
   auto refResult = ActorReferenceResult::forReference(
       getConcreteWitness(), witness->getLoc(), DC, None, None,
       None, requirementIsolation);
@@ -2972,7 +2976,8 @@ bool ConformanceChecker::checkActorIsolation(
     return false;
 
   case ActorReferenceResult::ExitsActorToNonisolated:
-    // FIXME: SE-0338 would diagnose this.
+    diagnoseNonSendableTypesInReference(
+        getConcreteWitness(), DC, loc, SendableCheckReason::Conformance);
     return false;
 
   case ActorReferenceResult::EntersActor:
@@ -3016,10 +3021,6 @@ bool ConformanceChecker::checkActorIsolation(
 
   // If we aren't missing anything, do a Sendable check and move on.
   if (!missingOptions) {
-    SourceLoc loc = witness->getLoc();
-    if (loc.isInvalid())
-      loc = Conformance->getLoc();
-
     // FIXME: Disable Sendable checking when the witness is an initializer
     // that is explicitly marked nonisolated.
     if (isa<ConstructorDecl>(witness) &&

test/Concurrency/concurrent_value_checking.swift

@@ -223,7 +223,7 @@ protocol AsyncProto {
 }
 
 extension A1: AsyncProto {
-  func asyncMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of actor-isolated instance method 'asyncMethod' satisfying non-isolated protocol requirement cannot cross actor boundary}}
+  func asyncMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of actor-isolated instance method 'asyncMethod' satisfying protocol requirement cannot cross actor boundary}}
 }
 
 protocol MainActorProto {
@@ -232,7 +232,7 @@ protocol MainActorProto {
 
 class SomeClass: MainActorProto {
   @SomeGlobalActor
-  func asyncMainMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of global actor 'SomeGlobalActor'-isolated instance method 'asyncMainMethod' satisfying non-isolated protocol requirement cannot cross actor boundary}}
+  func asyncMainMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of global actor 'SomeGlobalActor'-isolated instance method 'asyncMainMethod' satisfying protocol requirement cannot cross actor boundary}}
 }
 
 // ----------------------------------------------------------------------