[Sema] Improve SerialExecutor diagnostics

Author: kabiroberai

include/swift/AST/DiagnosticsSema.def

@@ -6676,6 +6676,10 @@ WARNING(executor_enqueue_deprecated_owned_job_implementation,Deprecation,
         "'Executor.enqueue(Job)' is deprecated as a protocol requirement; "
         "conform type %0 to 'Executor' by implementing 'func enqueue(ExecutorJob)' instead",
         (Type))
+WARNING(executor_enqueue_unused_implementation, none,
+        "'Executor.enqueue(ExecutorJob)' will never be used, due to the presence of "
+        "'enqueue(UnownedJob)'",
+        (Type))
 
 //------------------------------------------------------------------------------
 // MARK: property wrapper diagnostics

lib/Sema/TypeCheckConcurrency.cpp

@@ -1334,10 +1334,18 @@ void swift::tryDiagnoseExecutorConformance(ASTContext &C,
   ValueDecl *unownedEnqueueWitnessDecl = unownedEnqueueWitness.getDecl();
   ValueDecl *moveOnlyEnqueueWitnessDecl = nullptr;
   ValueDecl *legacyMoveOnlyEnqueueWitnessDecl = nullptr;
+  // true iff the nominal type's availability allows the legacy requirement
+  // to be omitted in favor of moveOnlyEnqueueRequirement
+  bool canRemoveOldDecls = false;
 
   if (moveOnlyEnqueueRequirement) {
     moveOnlyEnqueueWitnessDecl = concreteConformance->getWitnessDeclRef(
         moveOnlyEnqueueRequirement).getDecl();
+    AvailabilityContext requirementInfo
+        = AvailabilityInference::availableRange(moveOnlyEnqueueRequirement, C);
+    AvailabilityContext declInfo =
+        TypeChecker::overApproximateAvailabilityAtLocation(nominal->getLoc(), dyn_cast<DeclContext>(nominal));
+    canRemoveOldDecls = declInfo.isContainedIn(requirementInfo);
   }
   if (legacyMoveOnlyEnqueueRequirement) {
     legacyMoveOnlyEnqueueWitnessDecl = concreteConformance->getWitnessDeclRef(
@@ -1346,8 +1354,14 @@ void swift::tryDiagnoseExecutorConformance(ASTContext &C,
 
   // --- Diagnose warnings and errors
 
+  if (!canRemoveOldDecls &&
+      unownedEnqueueWitnessDecl && unownedEnqueueWitnessDecl->getLoc().isValid() &&
+      moveOnlyEnqueueWitnessDecl && moveOnlyEnqueueWitnessDecl->getLoc().isValid()) {
+    diags.diagnose(moveOnlyEnqueueWitnessDecl->getLoc(), diag::executor_enqueue_unused_implementation, nominalTy);
+  }
+
   // Old UnownedJob based impl is present, warn about it suggesting the new protocol requirement.
-  if (unownedEnqueueWitnessDecl && unownedEnqueueWitnessDecl->getLoc().isValid()) {
+  if (canRemoveOldDecls && unownedEnqueueWitnessDecl && unownedEnqueueWitnessDecl->getLoc().isValid()) {
     diags.diagnose(unownedEnqueueWitnessDecl->getLoc(), diag::executor_enqueue_unowned_implementation, nominalTy);
   }
   // Old Job based impl is present, warn about it suggesting the new protocol requirement.

test/Concurrency/custom_executor_enqueue_impls.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -enable-experimental-move-only -disable-availability-checking
+// RUN: %target-typecheck-verify-swift -enable-experimental-move-only
 // REQUIRES: concurrency
 
 // rdar://106849189 move-only types should be supported in freestanding mode
@@ -7,10 +7,8 @@
 // FIXME: rdar://107112715 test failing on iOS simulator, investigating
 // UNSUPPORTED: OS=ios
 
-// Such type may be encountered since Swift 5.5 (5.1 backdeployed) if someone implemented the
-// not documented, but public Executor types back then already.
-//
-// We keep support for them, but also log a deprecation warning that they should move to the new signature.
+// If the availability is recent enough, log a deprecation warning to move to the new signature.
+@available(SwiftStdlib 5.9, *)
 final class OldExecutor: SerialExecutor {
   func enqueue(_ job: UnownedJob) {} // expected-warning{{'Executor.enqueue(UnownedJob)' is deprecated as a protocol requirement; conform type 'OldExecutor' to 'Executor' by implementing 'func enqueue(ExecutorJob)' instead}}
 
@@ -19,10 +17,23 @@ final class OldExecutor: SerialExecutor {
   }
 }
 
+/// Such a type may be encountered since Swift 5.5 (5.1 backdeployed) if someone implemented the
+/// not documented, but public Executor types back then already. Allow these to be implemented
+/// without warnings.
+@available(SwiftStdlib 5.1, *)
+final class OldExecutorOldStdlib: SerialExecutor {
+  func enqueue(_ job: UnownedJob) {}
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}
+
 /// Implementing both enqueue methods is legal, but somewhat useless --
 /// we call into the "old one"; so the Owned version is not used in such impl.
 ///
 /// That's why we do log the deprecation warning, people should use the move-only version.
+@available(SwiftStdlib 5.9, *)
 final class BothExecutor: SerialExecutor {
   func enqueue(_ job: UnownedJob) {} // expected-warning{{'Executor.enqueue(UnownedJob)' is deprecated as a protocol requirement; conform type 'BothExecutor' to 'Executor' by implementing 'func enqueue(ExecutorJob)' instead}}
 
@@ -33,7 +44,22 @@ final class BothExecutor: SerialExecutor {
   }
 }
 
+/// Meanwhile, we warn on the ExecutorJob witness if the type has a broader
+/// availability, since in this case the UnownedJob version needs to exist.
+@available(SwiftStdlib 5.1, *)
+final class BothExecutorOld: SerialExecutor {
+  func enqueue(_ job: UnownedJob) {}
+
+  @available(SwiftStdlib 5.9, *)
+  func enqueue(_ job: __owned ExecutorJob) {} // expected-warning{{'Executor.enqueue(ExecutorJob)' will never be used, due to the presence of 'enqueue(UnownedJob)'}}
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}
+
 /// For now we must keep all 3 implementation kinds and warn about deprecated ones
+@available(SwiftStdlib 5.9, *)
 final class TripleExecutor: SerialExecutor {
   func enqueue(_ job: UnownedJob) {} // expected-warning{{'Executor.enqueue(UnownedJob)' is deprecated as a protocol requirement; conform type 'TripleExecutor' to 'Executor' by implementing 'func enqueue(ExecutorJob)' instead}}
 
@@ -52,6 +78,7 @@ final class TripleExecutor: SerialExecutor {
 /// we manually detect and emit an error if neither of them is implemented.
 ///
 /// We do so because we implement them recursively, so one of them must be implemented basically.
+@available(SwiftStdlib 5.9, *)
 final class NoneExecutor: SerialExecutor { // expected-error{{type 'NoneExecutor' does not conform to protocol 'Executor'}}
 
   func asUnownedSerialExecutor() -> UnownedSerialExecutor {
@@ -60,6 +87,7 @@ final class NoneExecutor: SerialExecutor { // expected-error{{type 'NoneExecutor
 }
 
 /// Job still is deprecated
+@available(SwiftStdlib 5.9, *)
 final class StillDeprecated: SerialExecutor {
   // expected-warning@+2{{'Job' is deprecated: renamed to 'ExecutorJob'}}
   // expected-note@+1{{use 'ExecutorJob' instead}}
@@ -71,6 +99,7 @@ final class StillDeprecated: SerialExecutor {
 }
 
 /// Just implementing the new signature causes no warnings, good.
+@available(SwiftStdlib 5.9, *)
 final class NewExecutor: SerialExecutor {
   func enqueue(_ job: consuming ExecutorJob) {} // no warnings
 
@@ -80,6 +109,7 @@ final class NewExecutor: SerialExecutor {
 }
 
 // Good impl, but missing the ownership keyword
+@available(SwiftStdlib 5.9, *)
 final class MissingOwnership: SerialExecutor {
   func enqueue(_ job: ExecutorJob) {} // expected-error{{noncopyable parameter must specify its ownership}}
   // expected-note@-1{{add 'borrowing' for an immutable reference}}