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

Conversation

kabiroberai
Copy link
Contributor

@kabiroberai kabiroberai commented Jun 26, 2023

This PR tackles the case of SerialExecutor conformances with pre-SwiftStdlib 5.9 deployment targets. Previously, such a conformance would produce an unfixable warning about the enqueue(UnownedJob) requirement being deprecated — even though it's the only requirement that can be implemented without restricting the availability to SwiftStdlib 5.9. Fixing this involves two main changes:

  • The aforementioned warning will now be emitted iff the witness' nominal type has a sufficiently recent availability, under which the ExecutorJob requirement can be used.
  • There is now a new warning if both ExecutorJob and UnownedJob requirements are implemented in a pre-5.9 availability context, pointing out that the ExecutorJob overload will never be selected.

For one motivating example behind this change, consider the custom ClockExecutor in swift-async-algorithms. This type currently has an unfixable warning about enqueue(UnownedJob). Moreover, it appears that there was an attempted fix via apple/swift-async-algorithms#274 in order to address this warning — but in fact the new overload is never selected. With this PR, the old warning disappears and typechecking should now diagnose the fact that the ExecutorJob overload does not work as expected. If/when swift-async-algorithms drops support for pre-5.9 targets, the original deprecation warning will correctly appear again.

@theblixguy
Copy link
Collaborator

@swift-ci please test

@kabiroberai
Copy link
Contributor Author

GH won't let me request reviewers but @ktoso @kavon thought I'd bring this to your attention, would appreciate a review!

@kavon kavon requested review from ktoso and kavon July 3, 2023 20:55
Copy link
Member

@kavon kavon left a comment

Choose a reason for hiding this comment

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

Is it possible to fix the overall issue of the deprecation warning by specifying which version of Swift the old enqueue got deprecated in? Right now it appears to say in all versions:

@available(*, deprecated, message: "Implement 'enqueue(_: __owned ExecutorJob)' instead")

but perhaps we could just change this to say it was deprecated in 5.9. Thoughts @ktoso?

include/swift/AST/DiagnosticsSema.def Outdated Show resolved Hide resolved
@kabiroberai
Copy link
Contributor Author

Is it possible to fix the overall issue of the deprecation warning by specifying which version of Swift the old enqueue got deprecated in?

The problematic warning is triggered by a custom check in sema, which is what this PR modifies. Meanwhile, the warning due to @available that you mention isn't triggered at all, since that would only come into play if enqueue(UnownedJob) were manually invoked by a caller. That said, it might be a good idea to make the change you suggest anyway, since it'd be clearer from a semantic perspective for anyone reading the interface.

Thanks for the review @kavon, it's a bit late rn but I'll try to address these changes some time over the coming week 🙌

@ktoso
Copy link
Contributor

ktoso commented Jul 4, 2023

Is it possible to fix the overall issue of the deprecation warning by specifying which version of Swift the old enqueue got deprecated in? Right now it appears to say in all versions:

@available(*, deprecated, message: "Implement 'enqueue(_: __owned ExecutorJob)' instead")
but perhaps we could just change this to say it was deprecated in 5.9. Thoughts @ktoso?

There is no such capability AFAIK @tshortli should comment on general tricks with availability though, we worked together and arrived at this combination of deprecation annotations -- these changes carried a high risk and caused a lot of mayhem along the way, so we better be careful when introducing changes here.

// 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?

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

AFAICS this looks correct, the new diagnostic also makes sense.

Note that the warning is not from the deprecated annotation but directly from Sema, as @kabiroberai mentioned already.

Let's give this a full test run, and I expect this must be disabled on some platforms AFAIR

@ktoso
Copy link
Contributor

ktoso commented Jul 4, 2023

@swift-ci please test

@ktoso
Copy link
Contributor

ktoso commented Jul 5, 2023

@swift-ci please test

@ktoso ktoso added the concurrency Feature: umbrella label for concurrency language features label Jul 5, 2023
@ktoso ktoso self-assigned this Jul 5, 2023
@kabiroberai
Copy link
Contributor Author

Hey @ktoso, is there anything blocking this from being merged now?

@ktoso
Copy link
Contributor

ktoso commented Jul 11, 2023

Thanks looking into final checks here, I think it looks good. Let's confirm freestanding

@ktoso
Copy link
Contributor

ktoso commented Jul 11, 2023

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please clean test with toolchain and preset

@ktoso
Copy link
Contributor

ktoso commented Jul 11, 2023

Radar tracking this rdar://110329131

@ktoso ktoso merged commit 2044cb7 into swiftlang:main Jul 12, 2023
@ktoso
Copy link
Contributor

ktoso commented Jul 12, 2023

Thanks for the good work here @kabiroberai !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Feature: umbrella label for concurrency language features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants