Skip to content
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

[Sema] Improve SerialExecutor diagnostics #66934

Merged
merged 3 commits into from
Jul 12, 2023
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
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -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)'",
())

//------------------------------------------------------------------------------
// MARK: property wrapper diagnostics
Expand Down
28 changes: 27 additions & 1 deletion lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1346,8 +1346,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;
kabiroberai marked this conversation as resolved.
Show resolved Hide resolved
} else if (C.LangOpts.DisableAvailabilityChecking) {
// Assume we have all APIs available, and thus can use the ExecutorJob
canRemoveOldDecls = true;
kabiroberai marked this conversation as resolved.
Show resolved Hide resolved
} else {
// Check if the availability of nominal is high enough to be using the ExecutorJob version
AvailabilityContext requirementInfo
kabiroberai marked this conversation as resolved.
Show resolved Hide resolved
= 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.
Expand Down
70 changes: 70 additions & 0 deletions test/Concurrency/custom_executor_enqueue_availability.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// RUN: %target-typecheck-verify-swift -enable-experimental-move-only
// REQUIRES: concurrency
// REQUIRES: OS=macosx

Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably missing a bunch of disables, like:

Suggested change
// UNSUPPORTED: back_deployment_runtime
// REQUIRES: concurrency_runtime
// UNSUPPORTED: freestanding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The requirements here are based on the ones in custom_executor_enqueue_impls.swift, the primary difference being the removal of -disable-availability-checking from the swiftc invocation. Do we need more disables here than in the other test? I see that I missed copying over the UNSUPPORTED: freestanding so I'll add that in — but not quite sure why this wouldn't support a back-deployed runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't backdeploy the ExecutorJob type, so since the test is touching that I don't think it'll work there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the usages of ExecutorJob are annotated with @available(SwiftStdlib 5.9, *) — that seems sufficient to me but correct me if I'm missing something. The CI checks seem to have passed, is there a specific configuration they aren't covering that I can test manually?

// 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)'}}
kavon marked this conversation as resolved.
Show resolved Hide resolved

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}}
kabiroberai marked this conversation as resolved.
Show resolved Hide resolved

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)
}
}