Merge pull request #66640 from JTurcotti/protocol-variance

Super-level sendable checking for function parameters in overrides and conformances

Author: Joshua Turcotti

include/swift/AST/DiagnosticsSema.def

@@ -5229,8 +5229,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

@@ -1004,8 +1004,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 = [&] {
@@ -1018,23 +1019,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;
@@ -1045,33 +1054,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;
   }
 
@@ -3893,7 +3909,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(
@@ -4369,9 +4385,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

@@ -3009,16 +3009,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);
@@ -3034,7 +3024,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) {
@@ -3051,7 +3041,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:
@@ -3133,8 +3123,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}}
 }
 
 // ----------------------------------------------------------------------