Sendable checking hole when actors call non-mutating async methods on non-Sendable types

[FranzBusch]

Description
The following code compiles without errors under strict concurrency checking but has a maybe not so obvious but extremely dangerous data race.

struct Counter {
    private var counter = 0
    mutating func increment() async {
        self.counter += 1
    }
}

final class Wrapper {
    private var counter = Counter()

    func increment() async {
        await self.counter.increment()
    }
}

actor Bar {
  private var counter = Wrapper()

  func bar() async {
    await self.counter.increment()
  }
}

This is a tricky situation because the intention of the above code is often to make a non-Sendable type thread safe by storing it inside an actor. However, the compiler fails to diagnose that we are sending an instance of Wrapper across an isolation domain by calling the async function. The compiler must check here that calling any async non-mutating method on a non-Sendable type isn't safe.

Expected behavior
The above code should generate a warning under strict Concurrency checking since it is exhibiting a data race.

rdar://108309344

[ktoso]

Thanks for clarifying :)

Hmm yeah this is incredibly subtle... the hopping off the actor because we arguably might have "over-corrected" free/non-actor execution semantics of async functions in https://github.com/apple/swift-evolution/blob/main/proposals/0338-clarify-execution-non-actor-async.md

The rationale was to not "hog" actors, especially the main actor, which would have made such patterns safe and pretty nice; however in face of the "hop to global pool for non actor methods" as we have today, this becomes unsafe again and we don't warn about it.

I wonder if we should holistically take another look at the "where" execution semantics of things.

Perhaps the "hop off" should have been a specialized thing for some executors, such as the main actor..? I don't know if that would work in the model and static checking generally though -- it may be too hard to check/understand statically.

Either way, under the current model a warning should be issued - thanks for reporting!

[ktoso]

Additional context https://forums.swift.org/t/crash-in-safe-swift-with-actors-concurrency/64496/2

[FranzBusch]

The rationale was to not "hog" actors, especially the main actor, which would have made such patterns safe and pretty nice; however in face of the "hop to global pool for non actor methods" as we have today, this becomes unsafe again and we don't warn about it.

Right, the fact that we are eagerly hopping is leading to this but IMO we shouldn't change that but rather offer users some control over what executor to run on a block of code on. This would allow them to use the actors executor and the compiler can reason about the isolation for this. This is just some random idea and needs more thoughts. In the short term, we should make sure this warns.

[KeithBauerANZ]

Copying my comment from the swift forums:

I've been thinking about this, and the more I think about it, the more I find the idea that any await in an actor jumps back to the "global concurrency pool" completely counterintuitive.

If I have an object which is "protected state" of an actor, and I call an async method on that object, "of course" I want to do that method "on the actor".

Obviously that doesn't hold up if the method requires a particular global actor, but I feel like that's an understandable exception to the general concept, that once on an actor, you don't leave except via a @Sendable closure...

I guess the problem is when you are in a @MainActor function you don't want "more" stuff than necessary to use the main actor, but I still feel like the idea that calling any async function can implicitly yoink you off the main actor to be quite unintuitive. And if this behavior absolutely had to be preserved, the idea that methods on protected state of an actor retain the actor is still more intuitive than the current "any await always leaves the declared actor"

Is there any scope here to change the rules for jumping off an actor to make it work more intuitively, rather than simply adding a warning/error to largely forbid the use of async from actors?

[FranzBusch]

In my opinion, one of the fundamental building blocks that we need is to give developers the control what the global executor for a given scope is. This allows them to decide where their non isolated methods are getting executed.

Changing the way how hopping works right now would again lead to massive confusion. IMO, we should first introduce a warning for this specific Sendable checking problem and then see the impact of it. Once we have that, we can evaluate potential solutions.

[hborla]

This is now diagnosed on main with -strict-concurrency=complete

warning: passing argument of non-sendable type 'Wrapper' outside of actor-isolated context may introduce data races
    await self.counter.increment()
          ^
note: class 'Wrapper' does not conform to the 'Sendable' protocol
final class Wrapper {
            ^

I believe this was fixed by #67730