-
Notifications
You must be signed in to change notification settings - Fork 10
Marked objects as Sendable or explicitly not Sendable #8
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
base: main
Are you sure you want to change the base?
Conversation
Package.swift
Outdated
@@ -65,6 +65,7 @@ let defaultSwiftSettings: [SwiftSetting] = | |||
.enableUpcomingFeature("ExistentialAny"), | |||
.enableUpcomingFeature("InternalImportsByDefault"), | |||
.enableUpcomingFeature("MemberImportVisibility"), | |||
.unsafeFlags(["-Xfrontend", "-require-explicit-sendable"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't unconditionally add this flag here; these flags are always enabled. This one should only be added for development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove it! Thanks for the reminder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can still have it so we can enable it during development though - here's an example: https://github.com/apple/swift-nio-ssl/blob/main/Package.swift#L59-L73
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let me add this then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to add it back behind a flag. It was helpful when we were doing lots of strict concurrency work, but we're very unlikely to flip that flag back on now that the work has been done.
@@ -49,7 +49,7 @@ struct AsyncSequenceOfOne<Element: Sendable, Failure: Error>: AsyncSequence, Sen | |||
} | |||
|
|||
@usableFromInline | |||
struct AsyncIterator: AsyncIteratorProtocol { | |||
struct AsyncIterator: AsyncIteratorProtocol, Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, iterators aren't Sendable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I mark it as not Sendable even though it currently conforms to Sendable then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact it implicitly conforms to Sendable
right now because its implementation ticks all the boxes doesn't mean it should be Sendable
when you consider the semantics. In general, all iterators keep some internal state to know what the next item should be. If an iterator is shared between different tasks, then it becomes undeterministic which task gets which element, which isn't good.
Also, I left the comment on this iterator but this should be true for all other iterators marked as Sendable
on this PR.
Performed the necessary changes to enable compiler flag
-require-explicit-sendable
.