modify Sendable checking of overrides and protocol conformances. In the past, both the results and parameters of overriding (resp. conforming) functions were checked for Sendability. This is overly restrictive. For safety, the parameters of the overridden (resp. requiring) function should be checked for Sendability and the results of the overriding (resp. conforming) should be checked. This commit implements that change.

Author: jturcotti

include/swift/AST/DiagnosticsSema.def

@@ -5176,8 +5176,8 @@ 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 protocol requirement|"
-        "in parameter of %4 overriding %2 %3|"
+        "in parameter of the protocol requirement satisfied by %4 %2 %3|"
+        "in parameter of superclass method overridden by %4 %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,

lib/Sema/TypeCheckConcurrency.cpp

@@ -1003,8 +1003,9 @@ bool swift::diagnoseNonSendableTypes(
 }
 
 bool swift::diagnoseNonSendableTypesInReference(
-    ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc loc,
-    SendableCheckReason reason, Optional<ActorIsolation> knownIsolation) {
+    ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc refLoc,
+    SendableCheckReason refKind, Optional<ActorIsolation> knownIsolation,
+    FunctionCheckKind funcCheckKind, SourceLoc diagnoseLoc) {
 
   // Retrieve the actor isolation to use in diagnostics.
   auto getActorIsolation = [&] {
@@ -1017,23 +1018,31 @@ bool swift::diagnoseNonSendableTypesInReference(
   // For functions, check the parameter and result types.
   SubstitutionMap subs = declRef.getSubstitutions();
   if (auto function = dyn_cast<AbstractFunctionDecl>(declRef.getDecl())) {
-    for (auto param : *function->getParameters()) {
-      Type paramType = param->getInterfaceType().subst(subs);
-      if (diagnoseNonSendableTypes(
-              paramType, fromDC, loc, diag::non_sendable_param_type,
-              (unsigned)reason, function->getDescriptiveKind(),
-              function->getName(), getActorIsolation()))
-        return true;
+    if (funcCheckKind != FunctionCheckKind::Results) {
+      // only check params if funcCheckKind specifies so
+      for (auto param : *function->getParameters()) {
+        Type paramType = param->getInterfaceType().subst(subs);
+        if (diagnoseNonSendableTypes(
+            paramType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
+                diag::non_sendable_param_type,
+            (unsigned)refKind, function->getDescriptiveKind(),
+            function->getName(), getActorIsolation()))
+          return true;
+      }
     }
 
     // Check the result type of a function.
     if (auto func = dyn_cast<FuncDecl>(function)) {
-      Type resultType = func->getResultInterfaceType().subst(subs);
-      if (diagnoseNonSendableTypes(
-              resultType, fromDC, loc, diag::non_sendable_result_type,
-              (unsigned)reason, func->getDescriptiveKind(), func->getName(),
-              getActorIsolation()))
-        return true;
+      if (funcCheckKind != FunctionCheckKind::Params) {
+        // only check results if funcCheckKind specifies so
+        Type resultType = func->getResultInterfaceType().subst(subs);
+        if (diagnoseNonSendableTypes(
+            resultType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
+                diag::non_sendable_result_type,
+            (unsigned)refKind, func->getDescriptiveKind(), func->getName(),
+            getActorIsolation()))
+          return true;
+      }
     }
 
     return false;
@@ -1044,33 +1053,40 @@ bool swift::diagnoseNonSendableTypesInReference(
         ? var->getType()
         : var->getValueInterfaceType().subst(subs);
     if (diagnoseNonSendableTypes(
-            propertyType, fromDC, loc,
+            propertyType, fromDC, refLoc,
             diag::non_sendable_property_type,
             var->getDescriptiveKind(), var->getName(),
             var->isLocalCapture(),
-            (unsigned)reason,
+            (unsigned)refKind,
             getActorIsolation()))
       return true;
   }
 
   if (auto subscript = dyn_cast<SubscriptDecl>(declRef.getDecl())) {
     for (auto param : *subscript->getIndices()) {
-      Type paramType = param->getInterfaceType().subst(subs);
+      if (funcCheckKind != FunctionCheckKind::Results) {
+        // Check params of this subscript override for sendability
+        Type paramType = param->getInterfaceType().subst(subs);
+        if (diagnoseNonSendableTypes(
+                paramType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
+                diag::non_sendable_param_type,
+                (unsigned)refKind, subscript->getDescriptiveKind(),
+                subscript->getName(), getActorIsolation()))
+          return true;
+      }
+    }
+
+    if (funcCheckKind != FunctionCheckKind::Results) {
+      // Check the element type of a subscript.
+      Type resultType = subscript->getElementInterfaceType().subst(subs);
       if (diagnoseNonSendableTypes(
-              paramType, fromDC, loc, diag::non_sendable_param_type,
-              (unsigned)reason, subscript->getDescriptiveKind(),
-              subscript->getName(), getActorIsolation()))
+          resultType, fromDC, refLoc, diagnoseLoc.isInvalid() ? refLoc : diagnoseLoc,
+              diag::non_sendable_result_type,
+          (unsigned)refKind, subscript->getDescriptiveKind(),
+          subscript->getName(), getActorIsolation()))
         return true;
     }
 
-    // Check the element type of a subscript.
-    Type resultType = subscript->getElementInterfaceType().subst(subs);
-    if (diagnoseNonSendableTypes(
-            resultType, fromDC, loc, diag::non_sendable_result_type,
-            (unsigned)reason, subscript->getDescriptiveKind(),
-            subscript->getName(), getActorIsolation()))
-      return true;
-
     return false;
   }
 
@@ -3892,7 +3908,7 @@ static ActorIsolation getOverriddenIsolationFor(ValueDecl *value) {
   return isolation.subst(subs);
 }
 
-static ConcreteDeclRef getDeclRefInContext(ValueDecl *value) {
+ConcreteDeclRef swift::getDeclRefInContext(ValueDecl *value) {
   auto declContext = value->getInnermostDeclContext();
   if (auto genericEnv = declContext->getGenericEnvironmentOfContext()) {
     return ConcreteDeclRef(
@@ -4368,9 +4384,18 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
     return;
 
   case OverrideIsolationResult::Sendable:
+    // Check that the results of the overriding method are sendable
     diagnoseNonSendableTypesInReference(
         getDeclRefInContext(value), value->getInnermostDeclContext(),
-        value->getLoc(), SendableCheckReason::Override);
+        value->getLoc(), SendableCheckReason::Override,
+        getActorIsolation(value), FunctionCheckKind::Results);
+
+    // Check that the parameters of the overridden method are sendable
+    diagnoseNonSendableTypesInReference(
+        getDeclRefInContext(overridden), overridden->getInnermostDeclContext(),
+        overridden->getLoc(), SendableCheckReason::Override,
+        getActorIsolation(value), FunctionCheckKind::Params,
+        value->getLoc());
     return;
 
   case OverrideIsolationResult::Disallowed:

lib/Sema/TypeCheckConcurrency.h

@@ -246,6 +246,17 @@ struct ActorReferenceResult {
   operator Kind() const { return kind; }
 };
 
+/// Specifies whether checks applied to function types should
+/// apply to their params, results, or both
+enum class FunctionCheckKind {
+  /// Check params and results
+  ParamsResults,
+  /// Check params only
+  Params,
+  /// Check results only
+  Results,
+};
+
 /// Diagnose the presence of any non-sendable types when referencing a
 /// given declaration from a particular declaration context.
 ///
@@ -260,17 +271,26 @@ struct ActorReferenceResult {
 ///
 /// \param fromDC The context from which the reference occurs.
 ///
-/// \param loc The location at which the reference occurs, which will be
+/// \param refLoc The location at which the reference occurs, which will be
 /// used when emitting diagnostics.
 ///
 /// \param refKind Describes what kind of reference is being made, which is
 /// used to tailor the diagnostic.
 ///
+/// \param funcCheckKind Describes whether function types in this reference
+/// should be checked for sendability of their results, params, or both
+///
+/// \param diagnoseLoc Provides an alternative source location to `refLoc`
+/// to be used for reporting the top level diagnostic while auxiliary
+/// warnings and diagnostics are reported at `refLoc`.
+///
 /// \returns true if an problem was detected, false otherwise.
 bool diagnoseNonSendableTypesInReference(
-    ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc loc,
+    ConcreteDeclRef declRef, const DeclContext *fromDC, SourceLoc refLoc,
     SendableCheckReason refKind,
-    Optional<ActorIsolation> knownIsolation = None);
+    Optional<ActorIsolation> knownIsolation = None,
+    FunctionCheckKind funcCheckKind = FunctionCheckKind::ParamsResults,
+    SourceLoc diagnoseLoc = SourceLoc());
 
 /// Produce a diagnostic for a missing conformance to Sendable.
 void diagnoseMissingSendableConformance(
@@ -283,6 +303,9 @@ void diagnoseMissingExplicitSendable(NominalTypeDecl *nominal);
 /// Warn about deprecated `Executor.enqueue` implementations.
 void tryDiagnoseExecutorConformance(ASTContext &C, const NominalTypeDecl *nominal, ProtocolDecl *proto);
 
+// Get a concrete reference to a declaration
+ConcreteDeclRef getDeclRefInContext(ValueDecl *value);
+
 /// How the Sendable check should be performed.
 enum class SendableCheck {
   /// Sendable conformance was explicitly stated and should be
@@ -359,6 +382,37 @@ namespace detail {
   };
 }
 
+/// Diagnose any non-Sendable types that occur within the given type, using
+/// the given diagnostic.
+///
+/// \param typeLoc is the source location of the type being diagnosed
+///
+/// \param diagnoseLoc is the source location at which the main diagnostic should
+/// be reported, which can differ from typeLoc
+///
+/// \returns \c true if any errors were produced, \c false if no diagnostics or
+/// only warnings and notes were produced.
+template<typename ...DiagArgs>
+bool diagnoseNonSendableTypes(
+    Type type, SendableCheckContext fromContext,
+    SourceLoc typeLoc, SourceLoc diagnoseLoc,
+    Diag<Type, DiagArgs...> diag,
+    typename detail::Identity<DiagArgs>::type ...diagArgs) {
+
+    ASTContext &ctx = fromContext.fromDC->getASTContext();
+    return diagnoseNonSendableTypes(
+        type, fromContext, typeLoc, [&](Type specificType,
+                                        DiagnosticBehavior behavior) {
+
+          if (behavior != DiagnosticBehavior::Ignore) {
+            ctx.Diags.diagnose(diagnoseLoc, diag, type, diagArgs...)
+                .limitBehavior(behavior);
+          }
+
+          return false;
+        });
+}
+
 /// Diagnose any non-Sendable types that occur within the given type, using
 /// the given diagnostic.
 ///
@@ -369,17 +423,9 @@ bool diagnoseNonSendableTypes(
     Type type, SendableCheckContext fromContext, SourceLoc loc,
     Diag<Type, DiagArgs...> diag,
     typename detail::Identity<DiagArgs>::type ...diagArgs) {
-  ASTContext &ctx = fromContext.fromDC->getASTContext();
-  return diagnoseNonSendableTypes(
-      type, fromContext, loc, [&](Type specificType,
-      DiagnosticBehavior behavior) {
-    if (behavior != DiagnosticBehavior::Ignore) {
-      ctx.Diags.diagnose(loc, diag, type, diagArgs...)
-        .limitBehavior(behavior);
-    }
 
-    return false;
-  });
+    return diagnoseNonSendableTypes(type, fromContext, loc, loc, diag,
+                             std::forward<decltype(diagArgs)>(diagArgs)...);
 }
 
 /// Diagnose this sendability error with behavior based on the import of

lib/Sema/TypeCheckProtocol.cpp

@@ -3006,16 +3006,6 @@ static bool hasExplicitGlobalActorAttr(ValueDecl *decl) {
 
 Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
     ValueDecl *requirement, ValueDecl *witness) {
-  /// Retrieve a concrete witness for Sendable checking.
-  auto getConcreteWitness = [&] {
-    if (auto genericEnv = witness->getInnermostDeclContext()
-            ->getGenericEnvironmentOfContext()) {
-      return ConcreteDeclRef(
-          witness, genericEnv->getForwardingSubstitutionMap());
-    }
-
-    return ConcreteDeclRef(witness);
-  };
 
   // Determine the isolation of the requirement itself.
   auto requirementIsolation = getActorIsolation(requirement);
@@ -3031,7 +3021,7 @@ Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
     loc = Conformance->getLoc();
 
   auto refResult = ActorReferenceResult::forReference(
-      getConcreteWitness(), witness->getLoc(), DC, None, None,
+      getDeclRefInContext(witness), witness->getLoc(), DC, None, None,
       None, requirementIsolation);
   bool sameConcurrencyDomain = false;
   switch (refResult) {
@@ -3048,7 +3038,7 @@ Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
 
   case ActorReferenceResult::ExitsActorToNonisolated:
     diagnoseNonSendableTypesInReference(
-        getConcreteWitness(), DC, loc, SendableCheckReason::Conformance);
+        getDeclRefInContext(witness), DC, loc, SendableCheckReason::Conformance);
     return None;
 
   case ActorReferenceResult::EntersActor:
@@ -3130,8 +3120,19 @@ Optional<ActorIsolation> ConformanceChecker::checkActorIsolation(
         witness->getAttrs().hasAttribute<NonisolatedAttr>())
       return None;
 
+    // Check that the results of the witnessing method are sendable
     diagnoseNonSendableTypesInReference(
-        getConcreteWitness(), DC, loc, SendableCheckReason::Conformance);
+        getDeclRefInContext(witness), DC, loc, SendableCheckReason::Conformance,
+        getActorIsolation(witness), FunctionCheckKind::Results);
+
+    // If this requirement is a function, check that its parameters are Sendable as well
+    if (isa<AbstractFunctionDecl>(requirement)) {
+      diagnoseNonSendableTypesInReference(
+          getDeclRefInContext(requirement),
+          requirement->getInnermostDeclContext(), requirement->getLoc(),
+          SendableCheckReason::Conformance, getActorIsolation(witness),
+          FunctionCheckKind::Params, loc);
+    }
 
     // If the witness is accessible across actors, we don't need to consider it
     // isolated.

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 protocol requirement cannot cross actor boundary}}
+  func asyncMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of the protocol requirement satisfied by actor-isolated instance method 'asyncMethod' 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 protocol requirement cannot cross actor boundary}}
+  func asyncMainMethod(_: NotConcurrent) async { } // expected-warning{{non-sendable type 'NotConcurrent' in parameter of the protocol requirement satisfied by global actor 'SomeGlobalActor'-isolated instance method 'asyncMainMethod' cannot cross actor boundary}}
 }
 
 // ----------------------------------------------------------------------