Merge pull request #67231 from ktoso/pick-serial-executor-warning

🍒[5.9][Sema] Improve SerialExecutor diagnostics

Author: ktoso

include/swift/AST/DiagnosticsSema.def

@@ -6650,6 +6650,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)'",
+        ())
 
 //------------------------------------------------------------------------------
 // MARK: property wrapper diagnostics

lib/Sema/TypeCheckConcurrency.cpp

@@ -1347,8 +1347,34 @@ void swift::tryDiagnoseExecutorConformance(ASTContext &C,
 
   // --- Diagnose warnings and errors
 
+  // true iff the nominal type's availability allows the legacy requirement
+  // to be omitted in favor of moveOnlyEnqueueRequirement
+  bool canRemoveOldDecls;
+  if (!moveOnlyEnqueueRequirement) {
+    // The move only enqueue does not exist in this lib version, we must keep relying on the UnownedJob version
+    canRemoveOldDecls = false;
+  } else if (C.LangOpts.DisableAvailabilityChecking) {
+    // Assume we have all APIs available, and thus can use the ExecutorJob
+    canRemoveOldDecls = true;
+  } else {
+    // Check if the availability of nominal is high enough to be using the ExecutorJob version
+    AvailabilityContext requirementInfo
+        = AvailabilityInference::availableRange(moveOnlyEnqueueRequirement, C);
+    AvailabilityContext declInfo =
+        TypeChecker::overApproximateAvailabilityAtLocation(nominal->getLoc(), dyn_cast<DeclContext>(nominal));
+    canRemoveOldDecls = declInfo.isContainedIn(requirementInfo);
+  }
+
+  // If both old and new enqueue are implemented, but the old one cannot be removed,
+  // emit a warning that the new enqueue is unused.
+  if (!canRemoveOldDecls &&
+      unownedEnqueueWitnessDecl && unownedEnqueueWitnessDecl->getLoc().isValid() &&
+      moveOnlyEnqueueWitnessDecl && moveOnlyEnqueueWitnessDecl->getLoc().isValid()) {
+    diags.diagnose(moveOnlyEnqueueWitnessDecl->getLoc(), diag::executor_enqueue_unused_implementation);
+  }
+
   // 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_availability.swift

@@ -0,0 +1,70 @@
+// RUN: %target-typecheck-verify-swift -enable-experimental-move-only
+// REQUIRES: concurrency
+// REQUIRES: OS=macosx
+
+// rdar://106849189 move-only types should be supported in freestanding mode
+// UNSUPPORTED: freestanding
+
+/// 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)
+  }
+}
+
+/// 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 BothExecutorOldStdlib: 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)
+  }
+}
+
+/// Meanwhile, we warn on the UnownedJob overload if the availability is new enough
+/// that it can be dropped.
+@available(SwiftStdlib 5.9, *)
+final class BothExecutorNewStdlib: SerialExecutor {
+  func enqueue(_ job: UnownedJob) {} // expected-warning{{'Executor.enqueue(UnownedJob)' is deprecated as a protocol requirement; conform type 'BothExecutorNewStdlib' to 'Executor' by implementing 'func enqueue(ExecutorJob)' instead}}
+
+  func enqueue(_ job: __owned ExecutorJob) {}
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}
+
+@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}}
+
+  // expected-warning@+2{{'Job' is deprecated: renamed to 'ExecutorJob'}}
+  // expected-note@+1{{use 'ExecutorJob' instead}}
+  func enqueue(_ job: __owned Job) {} // expected-warning{{'Executor.enqueue(Job)' is deprecated as a protocol requirement; conform type 'TripleExecutor' to 'Executor' by implementing 'func enqueue(ExecutorJob)' instead}}
+
+  func enqueue(_ job: consuming ExecutorJob) {}
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}
+
+/// Implementing the new signature on 5.9 platforms is good, no warnings
+@available(SwiftStdlib 5.9, *)
+final class NewExecutorNewStdlib: SerialExecutor {
+  func enqueue(_ job: __owned ExecutorJob) {}
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}