Skip to content

Avoid issuing warning when user lib implements only ExecutorJob enqueue #68617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1394,9 +1394,13 @@ void swift::tryDiagnoseExecutorConformance(ASTContext &C,
// 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 && moveOnlyEnqueueWitnessDecl) {
if (!isStdlibDefaultImplDecl(moveOnlyEnqueueWitnessDecl)) {
if (!isStdlibDefaultImplDecl(moveOnlyEnqueueWitnessDecl) &&
!isStdlibDefaultImplDecl(unownedEnqueueWitnessDecl)) {
diags.diagnose(moveOnlyEnqueueWitnessDecl->getLoc(),
diag::executor_enqueue_unused_implementation);
if (auto decl = unownedEnqueueWitnessDecl) {
decl->diagnose(diag::decl_declared_here, decl);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ final class OldExecutorOldStdlib: SerialExecutor {
/// availability, since in this case the UnownedJob version needs to exist.
@available(SwiftStdlib 5.1, *)
final class BothExecutorOldStdlib: SerialExecutor {
func enqueue(_ job: UnownedJob) {}
func enqueue(_ job: UnownedJob) {} // expected-note{{'enqueue' declared here}}

@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)'}}
Expand Down
66 changes: 66 additions & 0 deletions test/Concurrency/radar_concurrency_nio.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// RUN: %target-swift-frontend -enable-experimental-move-only %s -emit-sil -o /dev/null -verify
// RUN: %target-swift-frontend -enable-experimental-move-only %s -emit-sil -o /dev/null -verify -strict-concurrency=targeted
// RUN: %target-swift-frontend -enable-experimental-move-only %s -emit-sil -o /dev/null -verify -strict-concurrency=complete
// RUN: %target-swift-frontend -enable-experimental-move-only %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -enable-experimental-feature SendNonSendable

// REQUIRES: concurrency

public protocol EventLoop: Sendable {}

#if compiler(>=5.9)
/// A helper protocol that can be mixed in to a NIO ``EventLoop`` to provide an
/// automatic conformance to `SerialExecutor`.
///
/// Implementers of `EventLoop` should consider conforming to this protocol as
/// well on Swift 5.9 and later.
@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
public protocol NIOSerialEventLoopExecutor: EventLoop, SerialExecutor { }

@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
extension NIOSerialEventLoopExecutor {
@inlinable
public func enqueue(_ job: consuming ExecutorJob) {
fatalError("mock impl")
}

@inlinable
public func asUnownedSerialExecutor() -> UnownedSerialExecutor {
UnownedSerialExecutor(ordinary: self)
}

@inlinable
public var executor: any SerialExecutor {
self
}
}

// EARLIER AVAILABILITY
final class NIODefaultSerialEventLoopExecutor {
@usableFromInline
let loop: EventLoop

@inlinable
init(_ loop: EventLoop) {
self.loop = loop
}
}

@available(macOS 14.0, iOS 17.0, watchOS 10.0, tvOS 17.0, *)
extension NIODefaultSerialEventLoopExecutor: SerialExecutor {
@inlinable
public func enqueue(_ job: consuming ExecutorJob) { // do NOT issue a warning here
Copy link
Contributor Author

@ktoso ktoso Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the meantime tbh you can implement the UnownedJob version to avoid warnings; they're effectively the same nowadays. @FranzBusch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we get deprecation warnings when adding the UnownedJob implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the deprecation only is when you'd call the method though right? The test confirms there are no warnings issued here 🤔 The UnownedJob itself is not deprecated.

And you could technically mark your enqueue impl as deprecated as well actually

fatalError("mock impl")
}

@inlinable
public func asUnownedSerialExecutor() -> UnownedSerialExecutor {
UnownedSerialExecutor(complexEquality: self)

}

@inlinable
public func isSameExclusiveExecutionContext(other: NIODefaultSerialEventLoopExecutor) -> Bool {
false
}
}
#endif