[Concurrency] Warn about "old" Executor.enqueue impl and prefer new

Author: ktoso

include/swift/AST/Decl.h

@@ -3816,6 +3816,9 @@ class NominalTypeDecl : public GenericTypeDecl, public IterableDeclContext {
   /// Find the 'RemoteCallArgument(label:name:value:)' initializer function.
   ConstructorDecl *getDistributedRemoteCallArgumentInitFunction() const;
 
+  /// Get the move-only `enqueue(Job)` protocol requirement function on the `Executor` protocol.
+  AbstractFunctionDecl *getExecutorOwnedEnqueueFunction() const;
+
   /// Collect the set of protocols to which this type should implicitly
   /// conform, such as AnyObject (for classes).
   void getImplicitProtocols(SmallVectorImpl<ProtocolDecl *> &protocols);

include/swift/AST/DiagnosticsSema.def

@@ -6324,6 +6324,11 @@ WARNING(hashvalue_implementation,Deprecation,
         "conform type %0 to 'Hashable' by implementing 'hash(into:)' instead",
         (Type))
 
+WARNING(executor_enqueue_unowned_implementation,Deprecation,
+        "'Executor.enqueue(UnownedJob)' is deprecated as a protocol requirement; "
+        "conform type %0 to 'Executor' by implementing 'enqueueOwned(Job)' instead",
+        (Type))
+
 //------------------------------------------------------------------------------
 // MARK: property wrapper diagnostics
 //------------------------------------------------------------------------------

include/swift/AST/KnownIdentifiers.def

@@ -84,6 +84,7 @@ IDENTIFIER(encodeIfPresent)
 IDENTIFIER(Encoder)
 IDENTIFIER(encoder)
 IDENTIFIER(enqueue)
+IDENTIFIER(enqueueOwned)
 IDENTIFIER(erasing)
 IDENTIFIER(error)
 IDENTIFIER(errorDomain)

include/swift/AST/KnownProtocols.def

@@ -75,6 +75,7 @@ PROTOCOL(SIMD)
 PROTOCOL(SIMDScalar)
 PROTOCOL(BinaryInteger)
 PROTOCOL(RangeReplaceableCollection)
+PROTOCOL(Executor)
 PROTOCOL(SerialExecutor)
 PROTOCOL(GlobalActor)
 

include/swift/AST/KnownSDKTypes.def

@@ -39,6 +39,8 @@ KNOWN_SDK_TYPE_DECL(ObjectiveC, ObjCBool, StructDecl, 0)
 // standardized
 KNOWN_SDK_TYPE_DECL(Concurrency, UnsafeContinuation, NominalTypeDecl, 2)
 KNOWN_SDK_TYPE_DECL(Concurrency, MainActor, NominalTypeDecl, 0)
+KNOWN_SDK_TYPE_DECL(Concurrency, Executor, NominalTypeDecl, 0)
+KNOWN_SDK_TYPE_DECL(Concurrency, SerialExecutor, NominalTypeDecl, 0)
 KNOWN_SDK_TYPE_DECL(Concurrency, UnownedSerialExecutor, NominalTypeDecl, 0)
 
 KNOWN_SDK_TYPE_DECL(Concurrency, TaskLocal, ClassDecl, 1)

lib/AST/ASTContext.cpp

@@ -1114,6 +1114,7 @@ ProtocolDecl *ASTContext::getProtocol(KnownProtocolKind kind) const {
   case KnownProtocolKind::GlobalActor:
   case KnownProtocolKind::AsyncSequence:
   case KnownProtocolKind::AsyncIteratorProtocol:
+  case KnownProtocolKind::Executor:
   case KnownProtocolKind::SerialExecutor:
     M = getLoadedModule(Id_Concurrency);
     break;

lib/AST/Decl.cpp

@@ -5085,6 +5085,39 @@ VarDecl *NominalTypeDecl::getGlobalActorInstance() const {
                            nullptr);
 }
 
+AbstractFunctionDecl *
+NominalTypeDecl::getExecutorOwnedEnqueueFunction() const {
+  auto &C = getASTContext();
+
+  auto proto = dyn_cast<ProtocolDecl>(this);
+  if (!proto)
+    return nullptr;
+
+  llvm::SmallVector<ValueDecl *, 2> results;
+  lookupQualified(getSelfNominalTypeDecl(),
+                  DeclNameRef(C.Id_enqueueOwned), // FIXME: make it enqueue
+                  NL_ProtocolMembers,
+                  results);
+
+  for (auto candidate: results) {
+    // we're specifically looking for the Executor protocol requirement
+    if (!isa<ProtocolDecl>(candidate->getDeclContext()))
+      continue;
+
+    if (auto *funcDecl = dyn_cast<AbstractFunctionDecl>(candidate)) {
+      if (funcDecl->getParameters()->size() != 1)
+        continue;
+
+      auto params = funcDecl->getParameters();
+      if (params->get(0)->getSpecifier() == ParamSpecifier::Owned) {
+        return funcDecl;
+      }
+    }
+  }
+
+  return nullptr;
+}
+
 ClassDecl::ClassDecl(SourceLoc ClassLoc, Identifier Name, SourceLoc NameLoc,
                      ArrayRef<InheritedEntry> Inherited,
                      GenericParamList *GenericParams, DeclContext *Parent,

lib/IRGen/GenMeta.cpp

@@ -5880,6 +5880,7 @@ SpecialProtocol irgen::getSpecialProtocolID(ProtocolDecl *P) {
   case KnownProtocolKind::CxxSequence:
   case KnownProtocolKind::UnsafeCxxInputIterator:
   case KnownProtocolKind::UnsafeCxxRandomAccessIterator:
+  case KnownProtocolKind::Executor:
   case KnownProtocolKind::SerialExecutor:
   case KnownProtocolKind::Sendable:
   case KnownProtocolKind::UnsafeSendable:

lib/Sema/TypeCheckConcurrency.cpp

@@ -1191,6 +1191,45 @@ void swift::diagnoseMissingExplicitSendable(NominalTypeDecl *nominal) {
   }
 }
 
+void swift::tryDiagnoseExecutorConformance(ASTContext &C,
+                                           const NominalTypeDecl *nominal,
+                                           ProtocolDecl *proto) {
+  assert(proto->isSpecificProtocol(KnownProtocolKind::Executor) ||
+         proto->isSpecificProtocol(KnownProtocolKind::SerialExecutor));
+
+  auto &diags = C.Diags;
+  auto module = nominal->getParentModule();
+  Type nominalTy = nominal->getDeclaredInterfaceType();
+
+  // enqueue(_: UnownedJob)
+  auto unownedEnqueueDeclName = DeclName(C, DeclBaseName(C.Id_enqueue), { Identifier() });
+  // enqueueOwned(_: Job)
+  auto moveOnlyEnqueueDeclName = DeclName(C, DeclBaseName(C.Id_enqueueOwned), { Identifier() });
+
+  auto conformance = module->lookupConformance(nominalTy, proto);
+  auto unownedEnqueueWitness = conformance.getWitnessByName(nominalTy, unownedEnqueueDeclName);
+  auto moveOnlyEnqueueWitness = conformance.getWitnessByName(nominalTy, moveOnlyEnqueueDeclName);
+
+  if (auto enqueueUnownedDecl = unownedEnqueueWitness.getDecl()) {
+    // Old UnownedJob based impl is present, warn about it suggesting the new protocol requirement.
+    if (enqueueUnownedDecl->getLoc().isValid())
+      diags.diagnose(enqueueUnownedDecl->getLoc(), diag::executor_enqueue_unowned_implementation, nominalTy);
+  }
+
+  if (unownedEnqueueWitness.getDecl()->getLoc().isInvalid() &&
+      moveOnlyEnqueueWitness.getDecl()->getLoc().isInvalid()) {
+    // Neither old or new implementation have been found, but we provide default impls for them
+    // that are mutually recursive, so we must error and suggest implementing the right requirement.
+    auto ownedRequirement = C.getExecutorDecl()->getExecutorOwnedEnqueueFunction();
+    nominal->diagnose(diag::type_does_not_conform, nominalTy, proto->getDeclaredInterfaceType());
+    ownedRequirement->diagnose(diag::no_witnesses,
+                               getProtocolRequirementKind(ownedRequirement),
+                               ownedRequirement->getName(),
+                               proto->getDeclaredInterfaceType(),
+                               /*AddFixIt=*/true);
+  }
+}
+
 /// Determine whether this is the main actor type.
 /// FIXME: the diagnostics engine has a copy of this.
 static bool isMainActor(Type type) {

lib/Sema/TypeCheckConcurrency.h

@@ -280,6 +280,9 @@ void diagnoseMissingSendableConformance(
 /// state whether it conforms to Sendable, provide a diagnostic.
 void diagnoseMissingExplicitSendable(NominalTypeDecl *nominal);
 
+/// Warn about deprecated `Executor.enqueue` implementations.
+void tryDiagnoseExecutorConformance(ASTContext &C, const NominalTypeDecl *nominal, ProtocolDecl *proto);
+
 /// How the Sendable check should be performed.
 enum class SendableCheck {
   /// Sendable conformance was explicitly stated and should be

lib/Sema/TypeCheckProtocol.cpp

@@ -6478,6 +6478,8 @@ void TypeChecker::checkConformancesInContext(IterableDeclContext *idc) {
     } else if (proto->isSpecificProtocol(
                    KnownProtocolKind::UnsafeSendable)) {
       sendableConformanceIsUnchecked = true;
+    } else if (proto->isSpecificProtocol(KnownProtocolKind::Executor)) {
+      tryDiagnoseExecutorConformance(Context, nominal, proto);
     }
   }
 

stdlib/public/BackDeployConcurrency/PartialAsyncTask.swift

@@ -30,6 +30,7 @@ public struct UnownedJob: Sendable {
 
   @_alwaysEmitIntoClient
   @inlinable
+  @available(*, deprecated, message: "Use SerialExecutor.runJobSynchronously(_:) instead.")
   public func _runSynchronously(on executor: UnownedSerialExecutor) {
       _swiftJobRun(self, executor)
   }

stdlib/public/Concurrency/Executor.swift

@@ -15,28 +15,31 @@ import Swift
 /// A service that can execute jobs.
 @available(SwiftStdlib 5.1, *)
 public protocol Executor: AnyObject, Sendable {
-  // This requirement is repeated here as a non-override so that we
-  // get a redundant witness-table entry for it.  This allows us to
-  // avoid drilling down to the base conformance just for the basic
-  // work-scheduling operation.
   @available(*, deprecated, message: "Implement enqueueJob instead")
   func enqueue(_ job: UnownedJob)
 
   @available(SwiftStdlib 5.9, *)
-  func enqueueJob(_ job: __owned Job) // FIXME: figure out how to introduce in compatible way
+  func enqueueOwned(_ job: __owned Job) // FIXME: figure out how to introduce in compatible way
 }
 
 /// A service that executes jobs.
 @available(SwiftStdlib 5.1, *)
 public protocol SerialExecutor: Executor {
-
+  // This requirement is repeated here as a non-override so that we
+  // get a redundant witness-table entry for it.  This allows us to
+  // avoid drilling down to the base conformance just for the basic
+  // work-scheduling operation.
   @_nonoverride
   @available(*, deprecated, message: "Implement enqueueJob instead")
   func enqueue(_ job: UnownedJob)
 
+  // This requirement is repeated here as a non-override so that we
+  // get a redundant witness-table entry for it.  This allows us to
+  // avoid drilling down to the base conformance just for the basic
+  // work-scheduling operation.
   @_nonoverride
   @available(SwiftStdlib 5.9, *)
-  func enqueueJob(_ job: __owned Job) // FIXME: figure out how to introduce in compatible way
+  func enqueueOwned(_ job: __owned Job) // FIXME: figure out how to introduce in compatible way
 
   /// Convert this executor value to the optimized form of borrowed
   /// executor references.
@@ -48,7 +51,8 @@ extension Executor {
   public func enqueue(_ job: UnownedJob) { // FIXME: this is bad; how could we deploy this nicely
     fatalError("Please implement \(Self.self).enqueueJob(_:)")
   }
-  public func enqueueJob(_ job: __owned Job) {
+
+  public func enqueueOwned(_ job: __owned Job) {
     self.enqueue(UnownedJob(job))
   }
 }
@@ -117,7 +121,7 @@ internal func _getJobTaskId(_ job: UnownedJob) -> UInt64
 internal func _enqueueOnExecutor<E>(job unownedJob: UnownedJob, executor: E)
 where E: SerialExecutor {
   if #available(SwiftStdlib 5.9, *) {
-    executor.enqueueJob(Job(context: unownedJob.context))
+    executor.enqueueOwned(Job(context: unownedJob.context))
   } else {
     executor.enqueue(unownedJob)
   }

stdlib/public/Concurrency/PartialAsyncTask.swift

@@ -102,7 +102,13 @@ extension UnownedSerialExecutor {
   @_alwaysEmitIntoClient
   @inlinable
   public func runJobSynchronously(_ job: __owned Job) {
-    UnownedJob(job)._runSynchronously(on: self)
+    _swiftJobRun(UnownedJob(job), self)
+  }
+
+  @_alwaysEmitIntoClient
+  @inlinable
+  public func runUnownedJobSynchronously(_ job: UnownedJob) {
+    _swiftJobRun(job, self)
   }
 }
 
@@ -114,7 +120,13 @@ extension SerialExecutor {
   @_alwaysEmitIntoClient
   @inlinable
   public func runJobSynchronously(_ job: __owned Job) {
-    UnownedJob(job)._runSynchronously(on: self.asUnownedSerialExecutor())
+    _swiftJobRun(UnownedJob(job), self.asUnownedSerialExecutor())
+  }
+
+  @_alwaysEmitIntoClient
+  @inlinable
+  public func runUnownedJobSynchronously(_ job: UnownedJob) {
+    _swiftJobRun(job, self.asUnownedSerialExecutor())
   }
 }
 

stdlib/public/Concurrency/Task.swift

@@ -843,7 +843,7 @@ public struct UnsafeCurrentTask {
   /// The current task's base priority.
   ///
   /// - SeeAlso: `TaskPriority`
-  /// - SeeAlso: `Task.currentBasePriority`
+  /// - SeeAlso: `Task.basePriority`
   @available(SwiftStdlib 5.9, *)
   public var basePriority: TaskPriority {
     TaskPriority(rawValue: _taskBasePriority(_task))

test/Concurrency/Runtime/custom_executors_default.swift

@@ -1,4 +1,4 @@
-// RUN: %target-run-simple-swift( -Xfrontend -disable-availability-checking %import-libdispatch -parse-as-library) | %FileCheck %s
+// RUN: %target-run-simple-swift( -Xfrontend enable-experimental-move-only -Xfrontend -disable-availability-checking %import-libdispatch -parse-as-library) | %FileCheck %s
 
 // REQUIRES: concurrency
 // REQUIRES: executable_test

test/Concurrency/Runtime/custom_executors_moveOnly_job.swift

@@ -9,10 +9,8 @@
 
 final class InlineExecutor: SerialExecutor, CustomStringConvertible {
 
-  public func enqueueJob(_ job: __owned Job) {
-    print("\(self): enqueue (job: \(job.description))")
+  public func enqueueOwned(_ job: __owned Job) {
     runJobSynchronously(job)
-    print("\(self): after run")
   }
 
   public func asUnownedSerialExecutor() -> UnownedSerialExecutor {

test/Concurrency/Runtime/custom_executors_priority.swift

@@ -47,7 +47,7 @@ actor Custom {
     }.value
     await Task(priority: .low) {
       print("P: \(Task.currentPriority)")
-      print("B: \(Task.currentBasePriority)")
+      print("B: \(Task.basePriority)")
       await actor.report()
     }.value
     print("end")

test/Concurrency/Runtime/custom_executors_protocol.swift

@@ -10,9 +10,9 @@
 
 import Dispatch
 
-func checkIfMainQueue(expectedAnswer expected: Bool) {
-  dispatchPrecondition(condition: expected ? .onQueue(DispatchQueue.main)
-      : .notOnQueue(DispatchQueue.main))
+func checkIfMainQueue(expectedAnswer expected: Bool, expectedQueue: DispatchQueue) {
+  dispatchPrecondition(condition: expected ? .onQueue(expectedQueue)
+      : .notOnQueue(expectedQueue))
 }
 
 protocol WithSpecifiedExecutor: Actor {
@@ -29,7 +29,7 @@ extension WithSpecifiedExecutor {
   }
 }
 
-final class InlineExecutor: SpecifiedExecutor, Swift.CustomStringConvertible {
+final class InlineExecutor: SpecifiedExecutor, CustomStringConvertible {
   let name: String
 
   init(_ name: String) {
@@ -42,7 +42,7 @@ final class InlineExecutor: SpecifiedExecutor, Swift.CustomStringConvertible {
     print("\(self): after run")
   }
 
-  public func enqueueJob(_ job: __owned Job) {
+  public func enqueueOwned(_ job: __owned Job) {
     print("\(self): enqueue")
     runJobSynchronously(job)
     print("\(self): after run")
@@ -78,11 +78,12 @@ actor MyActor: WithSpecifiedExecutor {
 @main struct Main {
   static func main() async {
     print("begin")
-    let one = InlineExecutor("one")
+    let queue = DispatchQueue(label: "CustomQueue")
+    let one = NaiveQueueExecutor(queue)
     let actor = MyActor(executor: one)
-    await actor.test(expectedExecutor: one)
-    await actor.test(expectedExecutor: one)
-    await actor.test(expectedExecutor: one)
+    await actor.test(expectedExecutor: one, expectedQueue: queue)
+    await actor.test(expectedExecutor: one, expectedQueue: queue)
+    await actor.test(expectedExecutor: one, expectedQueue: queue)
     print("end")
   }
 }

test/Concurrency/custom_executor_enqueue_impls.swift

@@ -0,0 +1,47 @@
+// RUN: %target-typecheck-verify-swift -enable-experimental-move-only -disable-availability-checking
+// REQUIRES: concurrency
+
+// 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.
+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 'enqueueOwned(Job)' instead}}
+
+  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.
+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 'enqueueOwned(Job)' instead}}
+  func enqueueOwned(_ job: __owned Job) {}
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}
+
+/// Even though we do provide default impls for both enqueue requirements,
+/// 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.
+final class NoneExecutor: SerialExecutor { // expected-error{{type 'NoneExecutor' does not conform to protocol 'Executor'}}
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}
+
+/// Just implementing the new signature causes no warnings, good.
+final class NewExecutor: SerialExecutor {
+  func enqueueOwned(_ job: __owned Job) {} // no warnings
+
+  func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+}