pointfreeco / swift-concurrency-extras

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

Annotate `Result.init(catching:)` closure parameter as `@Sendable` #16

Closed jpsim closed 12 months ago

jpsim commented 12 months ago

The following example previously produced some diagnostics:

func makeInt(closure: @escaping () async throws -> Int) async throws -> Int {
  try await closure()
}

actor IntFactory {
  func zero() async -> Result<Int, Error> {
    await Result {
                 ^ warning: passing argument of non-sendable type '() async throws -> Int' outside of actor-isolated context may introduce data races
                 ^ note: a function type must be marked '@Sendable' to conform to 'Sendable'
      try await makeInt { 0 }
    }
  }
}
stephencelis commented 12 months ago

@jpsim After thinking about the problem more and chatting with Brandon, we're actually not sure making this closure @Sendable makes sense after all. The initializer's async function is not escaping and we don't expect it to cross concurrency boundaries. And the warning you were seeing when @Sendable isn't applied better reveals that you are invoking a non-sendable makeInt function inside the actor. Adding @Sendable seems to hide that by making the initializer sendable when it really doesn't need to be.

Following Apple's lead, what we would like to do is mark the initializer with @_unsafeInheritExecutor, but sadly that attribute only works on funcs. Using @_unsafeInheritExecutor would move the warning to the makeInt line instead and force you to contend with things more explicitly. For example you could always hop out to a @Sendable closure:

actor IntFactory {
  func zero() async -> Result<Int, Error> {
    return await { @Sendable in
      await Result {
        try await makeInt { 0 }
      }
    }()
  }
}

I think what we would like to do is expose a new Result.catching or Result.catch static function for this so that we can annotate it with @_unsafeInheritExecutor, and then we would mark this initializer as @available(*, unavailable). I'll push a branch and then can you make sure the unavailable initializer doesn't clash with the one in your project?

stephencelis commented 12 months ago

Pushed result-catching.

jpsim commented 12 months ago

I see. Yeah that all makes sense to me. It's unfortunate that the underscored unsafe attribute is the best way to handle this situation though.

jpsim commented 12 months ago

Your branch works for me. Though presumably you won’t want to fatalError in the deprecated func.

Closing this in favor of your approach.

stephencelis commented 12 months ago

@jpsim It should be fully unavailable and uncallable. I just wanted to make sure it didn't conflict with your overload, thanks!