sideeffect-io / AsyncExtensions

AsyncExtensions aims to mimic Swift Combine operators for async sequences.
MIT License
308 stars 26 forks source link

Swift Concurrency warning flags #16

Closed rjchatfield closed 1 year ago

rjchatfield commented 2 years ago

Q: Is there a reason you're not yet using the warnings to help ensure accuracy?

.target(
  ...,
  swiftSettings: [
    .unsafeFlags([
      "-Xfrontend", "-warn-concurrency",
      "-Xfrontend", "-enable-actor-data-race-checks",
    ])
  ]
)

Perhaps it shouldn't be shipped that way, perhaps adding a new target just for development?

(Fantastic library, btw)

twittemb commented 2 years ago

Hi

No reason. I was not aware of those flags. I will add them.

Thanks.

Sajjon commented 1 year ago

@twittemb Your library is GREAT! Thanks a lot for this, specifically the AnyAsyncSequence just fits right, feels like a missing piece in Swift Structured Concurrency.

However, as @rjchatfield points out, this library ought to be compiled with these two flags. When enabled, 45 warnings are emitted in Xcode 14.0 beta 6 (14A5294g) (macOS 12.5.1 (21G83)).

While I do not know how serious these warnings really are, I think we should try to prioritise getting rid of them all - until they are gone I'm reluctant using this awesome library.

I raise in issue in Apple's AsyncAlgorithms about concurrency warnings and Franz started fixing some in PR https://github.com/apple/swift-async-algorithms/pull/198, maybe that can act as source of inspiration?

I would also LOVE a Sendable version of your great AsyncSequences and Operators. Especially a Sendable version of AnyAsyncSequence. I've begun pondering one would best support that. I think we have to choices:

  1. Make AnyAsyncSequence itself Sendable, the implications of this is that (if we want to be proper, and have zero concurrency warnings) both Element and the BaseAsyncSequence itself would have to be Sendable, which probably would "propagate" into all AsyncSequences introduced in this library, and the operatros, to be Sendable too. Maybe this is OK, because we could try the extension SomeAsyncSequence: Sendable where Element: Sendable, Upstream: Sendable approach that AsyncAlgorithm uses (see AsyncChain2Sequence for example)
  2. Or introduce a twin version of each with Sendable in its name, e.g.AnyAsyncSendableSequence`, which might be a lot of code duplication, depending on how hard it is. It seems non trivial, so one might not be able to employ GYB source generation

I began the extension SomeAsyncSequence: Sendable where Element: Sendable, Upstream: Sendable approach here in this draft PR, but that was before I enabled strict concurrency check and got too many warnings, so I think one wanna fix the warnings first - which is a non trivial task (pun not intended).

What do you think @twittemb ?

Here are some of the warnings:

Screenshot 2022-09-12 at 09 54 58
twittemb commented 1 year ago

Hi there,

Thanks for your comments.

I'm working on a huge refactoring to transform this lib in a companion to swift-async-algorithms. I intend to provide the same kind of operators as currently (+others) in a way that I will be able to remove them in favour of swift-async-algorithms once it is stable.

The refactor is on a branch called refactor and there is no more warnings on this branch. I still have a few things to do to make it a nice companion to swift-async-algorithms, especially in the AsyncChannel/AsyncSubject side where I want to provide a more unified version (perhaps something like Kotlin with Channels/SharedFlows)