Skip to content

[Concurrency] Fix preconcurrency downgrade behavior for Sendable closures and generic requirements. #77459

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 5 commits into from
Nov 9, 2024

Conversation

hborla
Copy link
Member

@hborla hborla commented Nov 7, 2024

Sendable violations inside @preconcurrency @Sendable closures should be suppressed in minimal checking, and diagnosed as warnings under complete checking, including the Swift 6 language mode. This change fixes an issue where warnings were still produced in minimal checking, and errors were still produced under -swift-version 6, which means that libraries cannot stage in new Sendable annotations if their clients have already migrated to Swift 6.

The preconcurrency downgrade infrastructure is currently pretty difficult to follow, because the information is passed through SendableCheckContext throughout the diagnoseNonSendableTypes APIs, and there are different code paths for @preconcurrency on nominal declarations versus import statements. This PR does not attempt to clean any of this code up, because I'd like to cherry pick the bug fix to release/6.0. I'll look at clarifying this code in a follow up change.

This change also downgrades Sendable requirement failures when the requirement is on a @preconcurrency declaration. However, the change in this PR is not sufficient for requirement inference, e.g.

// module A
@preconcurrency
struct RequireSendable<T: Sendable> {}

// module B

class NotSendable {}

func f<T, U: Sendable>(_: T, _: U) -> RequireSendable<T>? { nil }

func call(ns: NotSendable) {
  f(ns, ns)
}

A case like the above requires more careful handling. @preconcurrency can't simply be inferred on f, because that would break ABI by stripping U: Sendable from the generic signature. @preconcurrency may need to apply per requirement in order to maintain source compatibility without breaking ABI.

Resolves: rdar://138535438, rdar://139234188, #76652

@hborla hborla marked this pull request as ready for review November 7, 2024 21:09
@hborla hborla changed the title [Concurrency] Fix preconcurrency downgrade behavior for Sendable closures. [Concurrency] Fix preconcurrency downgrade behavior for @Sendable closures. Nov 7, 2024
@hborla
Copy link
Member Author

hborla commented Nov 7, 2024

@swift-ci please smoke test

@hborla hborla force-pushed the preconcurrency-downgrade branch from ed4e7d5 to 47089ed Compare November 7, 2024 23:55
…ures.

Sendable violations inside `@preconcurrency @Sendable` closures should be
suppressed in minimal checking, and diagnosed as warnings under complete
checking, including the Swift 6 language mode.
@hborla hborla force-pushed the preconcurrency-downgrade branch from 47089ed to a31a9d3 Compare November 8, 2024 00:02
@hborla
Copy link
Member Author

hborla commented Nov 8, 2024

@swift-ci please smoke test

@hborla hborla requested a review from tshortli as a code owner November 8, 2024 01:35
@hborla
Copy link
Member Author

hborla commented Nov 8, 2024

@swift-ci please smoke test

@hborla hborla requested a review from DougGregor November 8, 2024 01:35
@hborla
Copy link
Member Author

hborla commented Nov 8, 2024

@swift-ci please test source compatibility

@hborla hborla changed the title [Concurrency] Fix preconcurrency downgrade behavior for @Sendable closures. [Concurrency] Fix preconcurrency downgrade behavior for Sendable closures and generic requirements. Nov 8, 2024
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This all looks safe, and will downgrade a number of errors to warnings based on @preconcurrency. Thank you!

Comment on lines +6243 to 6249
// FIXME: contextRequiresStrictConcurrencyChecking is called from
// within the constraint system, but closures are only set to be isolated
// by preconcurrency in solution application because it's dependent on
// overload resolution. The constraint system either needs to check its
// own state on the current path, or not make type inference decisions based
// on concurrency checking level.
if (isolatedByPreconcurrency(explicitClosure)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is why isolatedByPreconcurrency is a closure that's passed in, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and it looks like ClosureIsolatedByPreconcurrency (used by constraint system calls) does check the constraint system state, but I ran into a case while debugging where this line of code returned an unexpected result mid constraint solving. It didn't seem to be causing any issues for the preconcurrency downgrade behavior, so I left this comment for myself to figure out what happened there after I land this.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 8, 2024

@swift-ci build toolchain

@Lukasa
Copy link
Contributor

Lukasa commented Nov 8, 2024

@swift-ci build toolchain macOS platform

@Lukasa
Copy link
Contributor

Lukasa commented Nov 8, 2024

Hmm, this toolchain did not appear to work. Building the current main of Hummingbird using the current main of swift-nio (by putting swift-nio in edit mode) produces the same errors using the downloaded Ubuntu 20.04 toolchain as we see with the current 6.0 toolchain:

/hummingbird/Sources/HummingbirdTLS/TLSChannel.swift:40:44: error: conformance of 'NIOSSLHandler' to 'Sendable' is unavailable
38 |     @inlinable
39 |     public func setup(channel: Channel, logger: Logger) -> EventLoopFuture<Value> {
40 |         return channel.pipeline.addHandler(NIOSSLServerHandler(context: self.sslContext)).flatMap {
   |                                            `- error: conformance of 'NIOSSLHandler' to 'Sendable' is unavailable
41 |             self.baseChannel.setup(channel: channel, logger: logger)
42 |         }

/hummingbird/.build/checkouts/swift-nio-ssl/Sources/NIOSSL/NIOSSLHandler.swift:791:1: note: conformance of 'NIOSSLHandler' to 'Sendable' has been explicitly marked unavailable here
 789 | 
 790 | @available(*, unavailable)
 791 | extension NIOSSLHandler: Sendable {}
     | `- note: conformance of 'NIOSSLHandler' to 'Sendable' has been explicitly marked unavailable here
 792 | 
 793 | extension NIOSSLHandler {
/hummingbird/Sources/HummingbirdTLS/TLSChannel.swift:40:44: error: conformance of 'NIOSSLHandler' to 'Sendable' is unavailable
38 |     @inlinable
39 |     public func setup(channel: Channel, logger: Logger) -> EventLoopFuture<Value> {
40 |         return channel.pipeline.addHandler(NIOSSLServerHandler(context: self.sslContext)).flatMap {
   |                                            `- error: conformance of 'NIOSSLHandler' to 'Sendable' is unavailable
41 |             self.baseChannel.setup(channel: channel, logger: logger)
42 |         }

/hummingbird/.build/checkouts/swift-nio-ssl/Sources/NIOSSL/NIOSSLHandler.swift:791:1: note: conformance of 'NIOSSLHandler' to 'Sendable' has been explicitly marked unavailable here
 789 | 
 790 | @available(*, unavailable)
 791 | extension NIOSSLHandler: Sendable {}
     | `- note: conformance of 'NIOSSLHandler' to 'Sendable' has been explicitly marked unavailable here
 792 | 
 793 | extension NIOSSLHandler {

This is the addHandler method in question, with the @preconcurrency annotation on it.

erasure expressions for arguments to `@preconcurrency` functions in Swift
6 mode.
@hborla
Copy link
Member Author

hborla commented Nov 8, 2024

@swift-ci build toolchain

@hborla
Copy link
Member Author

hborla commented Nov 8, 2024

@swift-ci please smoke test

@hborla
Copy link
Member Author

hborla commented Nov 9, 2024

@swift-ci build toolchain

@hborla
Copy link
Member Author

hborla commented Nov 9, 2024

@swift-ci please smoke test

@hborla hborla merged commit dc01137 into swiftlang:main Nov 9, 2024
6 checks passed
@hborla hborla deleted the preconcurrency-downgrade branch November 9, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants