[concurrency] Allow calls to sync actor functions to be interpreted as async

[kavon]

In the current proposal for Actors, we have the following restrictions:

1. "[...] synchronous instance methods of actor classes are actor-isolated and, therefore, not available from outside the actor instance."

2. "[...] synchronous functions may only be invoked by the specific actor instance itself, and not even by any other instance of the same actor class."

Some discussion about this proposal [1] led to the suggestion that loosens these restrictions:

Synchronous methods of an actor class are available to code outside of an actor instance (i.e., can be accessed outside of self). But, such synchronous calls will be automatically treated as an async call, requiring the use of await, etc.

There is tentative support for this loosened restriction right now. This PR implements the suggestion in the type checker.

[1] https://forums.swift.org/t/concurrency-actors-actor-isolation/41613/75

Resolves rdar://71253835

TODO:

• basic implementation
• regression tests
• Fix crash when binding an actor-isolated sync global actor function in a let.
• Check on & possibly fix interaction of implicitly async & async let

[kavon]

Right now, this PR doesn't include the appropriate expansion of the AST into SIL. I can add that part onto this PR if it's looking good so far in terms of type checking.

@swift-ci please smoke test

[kavon]

@swift-ci please smoke test

[ktoso]

Small side question on API/ABI evolution I wanted to sanity check here:

• if I have an actor func that was sync, but public; so others call it as async
• is it safe, compatibility wise, to make the function now async? (seems that no!, and it's quite tricky)

On one hand... it definitely seems safe from outside perspective: they only could ever call it as async, so if I make it "really async now" for outside callers nothing changes. (Is this an ABI safe refactoring then, I would hope so?)

But... if someone makes an extension MyActor and then calls that function -- if I'd make it async, would I break code? I think so, since now we'd require the await...

It's kind of weird because "inside actor" even though the async function is a suspension point, it will never really suspend at that call it may suspend inside that function that is now async...

The concern here is:

• a bunch of actors are lazy and rely on "yay, everything is async by default basically": func hi() -> String { "hi" }
• then they notice, oh, actually I need to call Task.checkCancellation or some tracing API: func hi() async -> String { await Task.something(); "hi" }
• can we make such refactorings or have we just led people into a trap, and they can't nicely adopt more features from the model?

I guess I'm just realizing that this makes it easy to get into a specific case of source breaking... (only "inside the actor") but still; I guess we should call this tricky bit out when we document this automatic ability?

[kavon]

Thanks for asking about this @ktoso!

• if I have an actor func that was sync, but public; so others call it as async
• is it safe, compatibility wise, to make the function now async? (seems that no!, and it's quite tricky)

It's probably confusing from my naming scheme of "implicitly async" in the code and also the title of this PR, but synchronous actor functions remain synchronous, i.e., their calling convention does not change. So, no new ABI/API issues should arise as a result of these changes.

At a high-level, what this PR does is push the responsibility of creating an async version of a sync function into the compiler, instead of onto the programmer. For example, consider this code:

actor class A {
  func foo() { /*...*/ }
  func bar() { foo() }
}

func f() async {
  let a = A()
  a.foo()  // currently, illegal!
}

We can't invoke the synchronous a.foo because it may require a suspension. However, if the programmer had added a straightforward async entrypoint to access foo, called fooAsync:

actor class A2 {
  func foo() { /*...*/ }
  func bar() { foo() }
  func fooAsync() async { foo() } // New entry-point added by the programmer.
}

Then the programmer would simply write await a.fooAsync() in f and would be just fine. They'll still be able to do this with the proposed changes. But of course, to make life easier, the compiler could interpret that previously illegal call-site a.foo() as being implicitly invoked inside of an async closure. More generally, for calls to sync actor instance methods (and sync global actor functions), we could interpret the call-site like so:

a.foo(a, b, c)  -->  { (a1, b1, c1) async in a.foo(a1, b1, c1) }(a, b, c)

So, a.foo remains synchronous (there's no await in the closure!), but we implicitly introduced an async closure at each of its call-sites only as-needed to make the calls legal. Thus in the definition of bar, the call to foo remains the same and is not "implicitly async". But the interpretation of the a.foo() call-site in f is an invocation of an async closure, so an await is now required there:

actor class A {
  func foo() { /*...*/ }
  func bar() { foo() } // this remains unchanged, no await!
}

func f() async {
  let a = A()
  await a.foo()  // with this PR, this is now OK!
}

Now, we could do better when actually implementing this interpretation a call-site in the compiler to look more like A2 (and avoid all the extra wrappers). So far, this PR just marks the ApplyExpr as being in need of this special interpretation later in the pipeline.

[ktoso]

Thanks Kavon, yeah that's all fine; From the outside it's all fine I believe -- I was pondering the compatibility story for the following (why I mention extensions up there):

public actor class A {
  public func foo() { /*...*/ }
}

extension A { // someone does this in their code
  func hello() { foo() } // no await needed
}

func f() async {
  let a = A()
  await a.foo()  // with this PR, this is now OK!
}

which needs to foo() become because I need to e.g. use instrumentation or deadlines:

actor class A {
  func foo() async { if await Task.currentDeadline() ... }
}

extension A { // someone does this
  func hello() { await foo() } // change required; someone who had such extension, must now await
}

func f() async {
  let a = A()
  await a.foo()  // outside of actor no changes, yay!
}

I was worrying about "what if people put extensions on my actor and use foo() -> but after sleeping on it... I guess that's true always: it is not compatible to change sync -> async, and the "outside the actor case" just (almost accidentally?) becomes compatible to make such change given this PR -- since for the outside it always is async. But if people wrote an extension to A, and called foo() (it was public etc), then I can't make it async without breaking people.

But that's the same with any public API I suppose... Just means that this PR does not solve this and that's fine and expected.

For the "outside" of my actor ABI/API it is compatible to make foo() async, because they always had to invoke it as async to begin with?

[kavon]

I was worrying about "what if people put extensions on my actor and use foo() -> but after sleeping on it... I guess that's true always: it is not compatible to change sync -> async, and the "outside the actor case" just (almost accidentally?) becomes compatible to make such change given this PR -- since for the outside it always is async. But if people wrote an extension to A, and called foo() (it was public etc), then I can't make it async without breaking people.

Ahh, I see what you mean. So, this PR adds some forwards compatibility for clients when an API changes between sync & async, but not in all cases. Also, the forwards compatibility is only at the source level, not at all in the ABI level: we cannot link the previously compiled client code with the newly changed library code at all... it requires a recompile.

While I view this minor forwards compatibility as an accidental feature, perhaps if we're not clear that sync/async changes are still breaking, API designers might accidentally break clients they forgot about client-side extensions of the actor class.

[kavon]

@swift-ci please smoke test

[kavon]

@swift-ci please smoke test

[DougGregor]

This is looking really good!

[kavon]

@swift-ci please smoke test and merge

[ktoso]

Ahh, I see what you mean. So, this PR adds some forwards compatibility for clients when an API changes between sync & async, but not in all cases. Also, the forwards compatibility is only at the source level, not at all in the ABI level: we cannot link the previously compiled client code with the newly changed library code at all... it requires a recompile.

Thanks for clarifying the ABI bit, it's a bit surprising to be honest... So if I have an actor which needs to start checking for cancellation and I forgot to make the function async because I mostly looked at it from the outside I'd be in trouble and need a new function... 🤔 You folks know better if that's a real ABI concern or not really I guess. One can catch this in review I guess to "future proof" APIs hm.

Just raising the concern about those "forgot that I didn't declare it async", otherwise the feature is cool 👍

[kavon]

@swift-ci please smoke test and merge