Audit pass on Sendable conformances and requirements #272
Conversation
Package.swift
Outdated
| swiftSettings: [ | ||
| .unsafeFlags([ | ||
| "-strict-concurrency=complete" | ||
| ], .when(configuration: .debug)) |
There was a problem hiding this comment.
I don't think we add this. AFAIK any package with unsafeFlags can't be depended on.
There was a problem hiding this comment.
ugh that thing is still present eh... that is unfortunate.
There was a problem hiding this comment.
FWIW - I've been shimming around this by only setting it when there's an environment variable ('CI') set so that I can still get the benefit in my local CI without impacting downstream packages.
There was a problem hiding this comment.
@neonichu is there a sanctioned way to have strict concurrency warnings on without negatively impacting downstream consumers?
There was a problem hiding this comment.
We have a discussion about this in the Swift repo swiftlang/swift#65993. It would be great if we can find a solution to safely opt-in to strict checking . cc @tomerd @hborla
There was a problem hiding this comment.
It seems to me that it is impossible to safely opt-in to a flag that is considered to be unstable.
I would suggest working around this by conditionally passing the unsafe flag based on some environment variable so that it can be enabled for local development / CI, but not for clients of the package.
| } | ||
| } | ||
|
|
||
| extension OrderedSetContainer: @unchecked Sendable where Element: Sendable { } |
There was a problem hiding this comment.
Why do we need @unchecked here? If OrderedSet is conditionally Sendable than we should be able to do the same here
There was a problem hiding this comment.
because OrderedSet itself is not marked w/ Sendable.
There was a problem hiding this comment.
That sounds like an oversight right?
There was a problem hiding this comment.
it is there at an untagged version; 1.1.0
phausler
force-pushed
the
pr/sendable_audit
branch
from
3510f36 to
37fd4d5
Compare
| struct ChannelStateMachine<Element, Failure: Error>: Sendable { | ||
| private struct SuspendedProducer: Hashable { | ||
| // NOTE: this is only marked as unchecked since the swift-collections tag is before auditing for Sendable | ||
| extension OrderedSet: @unchecked Sendable where Element: Sendable { } |
There was a problem hiding this comment.
@lorentey This is the use in question
There was a problem hiding this comment.
This is alright to do -- the type is already conditionally sendable, it just isn't marked as such in a shipping release yet.
My only worry is what happens when swift-collection ships the official conformance. Is the duplicate conformance going to become a compiler error?
There was a problem hiding this comment.
we are limiting usage to 1.0.4 so as soon as we upgrade we can remove this
plus iirc the duplicate conformance should be a no-op
This brings the main API surface up to Sendable clean in complete mode.
The primary point of change is Async(Throwing)Channel. Which previously permitted a potential Sendability soundness hole. It now requires that AsyncChannel's Element type MUST be Sendable. The reasoning is that this type's primary use case is to send elements from one isolation to another. This means that the element must be Sendable to achieve this.