search

pointfreeco / swift-concurrency-extras

Useful, testable Swift concurrency.
MIT License
343 stars 23 forks source link

Rename `Result.init { … }` to `Result.catching { ... }` #17

Closed stephencelis closed 12 months ago

stephencelis commented 12 months ago

Initializers can't have the @_unsafeInheritExecutor attribute, so we'll need to carve out a func instead to get reasonable async behavior.

stephencelis commented 12 months ago

@jpsim Haha, this has been a journey but with a deeper dive I don't think it's appropriate to add @_unsafeInheritExecutor either. That seems to hide some warnings it shouldn't.

I think what we've concluded is that Result.init we ship should stay the way it is, and it should be on the caller to isolate the closure when needed. So if you're in an isolated actor context, you can do:

await Result { @Sendable in
  …
}

And that should remove the actor warning but still reveal issues with sendability that occur in the closure itself.

So we recommend you remove the custom initializer in your app, use the one in this library, and annotate @Sendable in contexts that need them. Please let us know if you think we're missing something, though!

This stuff is all very complicated, and it's probably a contributing factor to reasync not being a thing. When thinking about all the consequences of sendable closures, actor contexts, and more, it's a lot to get right!

jpsim commented 12 months ago

So we recommend you remove the custom initializer in your app, use the one in this library, and annotate @Sendable in contexts that need them. Please let us know if you think we're missing something, though!

This works for our codebase and the mental model for why this is the correct solution fits in my head, but then again I thought the same thing the two previous times, which makes me really question my own understanding and the complexity of the Sendable model in general 😬.