SE-0423: Dynamic actor isolation enforcement from non-strict-concurrency contexts

Jon_Shier · 2024-02-20T22:15:53.525Z

FYI, title says SE-0243. Was confused for a second.

cal · 2024-02-20T23:18:28.009Z

This looks like it will be really valuable.

When we tried to adopt concurrency in some existing feature code in Airbnb's app codebase, this is one of the first big obstacles we ran up against. The easiest way for us to make the type concurrency-safe was with a @MainActor annotation, but this prevented us from easily implementing conformances to our existing protocols. Allowing seamless integration with preconcurrency protocols should make incremental adoption of concurrency much easier.

hisekaldma · 2024-02-21T10:32:13.801Z

I thought “witnesses” and “thunks” were implementation terms, and not part of the user model of the language? At any rate, this was rather hard to understand as a user. (The “should” in the third point especially tripped me up. Should be handled defensively by whom – the user or the compiler? If this is describing a change in the compiler, shouldn’t it be “would”?)

hborla · 2024-02-22T03:45:55.457Z

hisekaldma:

I thought “witnesses” and “thunks” were implementation terms, and not part of the user model of the language?

A protocol witness is part of the user facing language semantics. It's just a formal term to describe the implementation of a protocol requirement.

"Thunk" is definitely closer to an implementation detail of the compiler, but it's difficult to describe the semantics in this proposal without it. We can definitely define the term in the proposal at the point when it's used to describe the behavior of synchronous isolated @objc methods. An "@objc thunk" is effectively just a wrapper function that calls the native Swift function. For @objc methods, the rules in this proposal mean that you only ever get the dynamic checks when calling through Objective-C. When you call the method directly from Swift, you will not pay the cost of the dynamic checks.

hisekaldma:

(The “should” in the third point especially tripped me up. Should be handled defensively by whom – the user or the compiler? If this is describing a change in the compiler, shouldn’t it be “would”?)

This part of the proposal text is providing justification for the proposed semantic change. We believe dynamic checks should guard code that erases actor isolation when crossing a module boundary into not-concurrency-checked code, because that module may violate actor isolation when using that function value due to the lack of strict concurrency checking.

John_McCall · 2024-02-23T01:33:09.313Z

John_McCall · 2024-03-04T17:41:11.384Z

Swift has an explicit coercion on functions that erases static actor isolation. For example:

func test(operation: @MainActor () -> Int) {
  let fn = operation as () -> Int // erases our knowledge that `operation` is isolated to `MainActor`
  fn()
}

This coercion is dynamically safe, inasmuch as the resulting function does a dynamic isolation check. But I'm not sure we've ever documented its existence, and since it's doing a dynamic isolation check, this proposal might be a good vehicle for it.

hborla · 2024-03-05T05:49:57.184Z

The compiler does not insert a dynamic check for that code today. Under -swift-version 6, the actor isolation checker diagnoses the conversion as invalid:

15 │ func test(operation: @escaping @MainActor () -> Int) {
16 │   let fn = operation as () -> Int // erases our knowledge that `operation` is isolated to `MainActor`
   │            ╰─ error: converting function value of type '@MainActor () -> Int' to '() -> Int' loses global actor 'MainActor'
17 │   fn()
18 │ }

However, we certainly could consider allowing this and emitting the dynamic checks as part of this proposal.

wadetregaskis · 2024-03-05T06:37:30.796Z

Should it require as!? It seems dangerous to silently (without the warning afforded by an exclamation mark) drop compile-time safety and introduce a potentially crash point.

John_McCall · 2024-03-05T09:45:04.030Z

Hmm, alright. I need to check what I was thinking of.

tgoyne · 2024-03-05T16:10:59.489Z

-enable-actor-data-race-checks plus unsafeBitCast(operation, to: (() -> Int).self) gives you a statically unisolated closure with dynamic runtime checks.

John_McCall · 2024-03-06T01:08:40.246Z

Okay. If we don't allow that conversion in Swift 6, I think I wouldn't want to spell it with as — it seems like something that should require an explicit assumeIsolated.

John_McCall · 2024-03-06T03:43:36.723Z

I'd like to understand the situations where we're actually inserting checks under this proposal. Let me lay out some structure for that discussion.

Let's just call everything a function and ignore all the surface-level details like property accessors vs. methods. We've got code that requires isolation, it's getting called somehow, and that's all that matters.
We only care about synchronous functions because async functions that require isolation will just establish it internally. And synchronous functions preserve isolation, so really we just care about establishing a precondition of isolation in the function. So we're talking about having a check on one side or the other of a call.
We've got three basic options for how to do checking:
- We emit an unconditional check, which has code-size and execution-time costs in all cases.
- We create two paths for the call, one of which does a check, and then use the unchecked path when we know that we've safely established isolation in the caller. This mostly avoids execution-time costs but probably has significantly larger code-size costs (unless we can statically eliminate the checking path).
- We don't do the check and just accept dynamic unsafety.

The key question is how the call is done and what each side of the call knows about the other. Since we're protecting callees, the information question comes down to whether we know the caller can be trusted to isolate the function correctly. We can then consider that question for each of the basic ways we support for calling Swift functions:

calls to known functions (direct dispatch)
calls to opaque function values (arbitrary values of function type)
calls through Objective-C dispatch (@objc)
calls to protocol requirements
calls to overridable class methods

Objective-C dispatch is highly dynamic by design. Moreover, we can only provide a single entry point in the method table for any particular method selector. Moreover, callers will very often be cross-language, and in particular they will be Objective-C, which we cannot easily make cooperate with Swift concurrency. Moreover, there's a significant baseline cost to being called through Objective-C dispatch, and it's not unreasonable to say that code sensitive to micro-level performance should try to avoid it anyway. So I think we should be doing this check in any Objective-C entry point we emit for an isolated method. And the proposal is clear that we should do that; so far, so good.

With function values, we again don't have the flexibility to emit different entry points — we're not going to carry around multiple function pointers in every function value. We do, however, have some flexibility to add logic around an existing function value. If the function value comes from a closure, we can usually just add that logic directly to the closure function. If the function value is opaque, or it's something like a direct function reference, we can add the logic to a "thunk" that wraps the old function value. So I think we should be adding this logic to any function value that we can't prove (to some reasonable extent) stays within trusted code. It sounds like the proposal is aiming to do that, but it'd be good to understand and document the limitations of that.^[1]

Known functions, protocol requirements, and overridable class methods are surprisingly similar to each other. First, we do have the ability to emit multiple entry points per source function by adding new public symbols, protocol requirements, and/or class v-table entries. Doing that implicitly and everywhere would be a large code-size hit. If we were designing everything from scratch, we could avoid duplicating entry points by just performing the checks on the caller side. Unfortunately, that requires cooperation from the caller side. If the caller is recompiled by a compiler aware of this feature, but in a language mode that doesn't generally enforce isolation, we could add checks to the caller; we'd then be able to eliminate those checks if/when the caller is upgraded to Swift 6. However, if the caller isn't recompiled, those checks just won't happen. Regardless, it sounds like there's nothing like this currently in the proposal, and we simply don't perform any checks for these kinds of dispatch except for the specific case of isolated witnesses to non-isolated protocol requirements.

Do I understand the proposal correctly?

For example, there's no place in @tgoyne's example that formally erases the isolation from the original function; the erasure just happens magically when we reinterpret the bits of the function value. We would naturally not emit any checks for his example under this proposal; but of course this leaves the safety hole open. ↩︎

cal · 2024-03-08T23:01:32.363Z

Would this also allow isolated types to subclass preconcurrency nonisolated types?

An issue we just hit when updating to Swift 5.10 is that this is no longer allowed when strict concurrency checks are enabled:

// warning: main actor-isolated class 'MyTests' has different actor isolation 
// from nonisolated superclass 'XCTestCase'; this is an error in Swift 6
@MainActor
final class MyTests: XCTestCase {
  // ...
}

In Swift 6.0 will this sort of thing be possible with @preconcurrency, like for protocol conformances?

@MainActor
final class MyTests: @preconcurrency XCTestCase {
  // ...
}

hborla · 2024-03-11T16:55:38.581Z

John_McCall:

So I think we should be adding this logic to any function value that we can't prove (to some reasonable extent) stays within trusted code. It sounds like the proposal is aiming to do that, but it'd be good to understand and document the limitations of that.

Yes, that's what the proposal is aiming to do, but it's very possible there are cases we've missed in how it actually determines when to insert the dynamic checks around function values. The current proposal will only insert the dynamic checks if the function value is an argument and the callee is a function that is from an imported library where the library was not built with the Swift 6 language mode.

John_McCall:

Doing that implicitly and everywhere would be a large code-size hit. If we were designing everything from scratch, we could avoid duplicating entry points by just performing the checks on the caller side. Unfortunately, that requires cooperation from the caller side. If the caller is recompiled by a compiler aware of this feature, but in a language mode that doesn't generally enforce isolation, we could add checks to the caller; we'd then be able to eliminate those checks if/when the caller is upgraded to Swift 6. However, if the caller isn't recompiled, those checks just won't happen.

I think this idea is actually pretty compelling, because it enables the dynamic checks for direct calls to synchronous isolated functions without incurring the performance cost in the code that has strict checking enabled. I don't think it's necessarily a bad thing that code that isn't recompiled will continue to behave the way that it does today.

Would we only need to do this for isolated functions that are @preconcurrency, because it's a hard error to call a plain actor-isolated function from outside the isolation domain even under -strict-concurrency=minimal?

hborla · 2024-03-11T16:58:08.945Z

cal:

Would this also allow isolated types to subclass preconcurrency nonisolated types?

No, adding isolation in subclasses is more complicated because of the implicit Sendable conformance that comes with global-actor isolation on classes. It allows violating Sendable rules by smuggling non-Sendable state from the superclass over isolation boundaries, which can lead to concurrent access. Though this isn't covered by this proposal, I'm working on a separate proposal to lift some of these restrictions (at the cost of other restrictions, but I believe the other restrictions are a little easier to work with).

cal:

An issue we just hit when updating to Swift 5.10 is that this is no longer allowed when strict concurrency checks are enabled:

In case you didn't see it, there's a thread over in Using Swift specifically about this pattern of isolating an XCTestCase subclass to the main actor and strategies for resolving the warnings in 5.10: Swift 5.10 Concurrency and XCTest

John_McCall · 2024-03-11T16:58:35.942Z

hborla:

Would we only need to do this for isolated functions that are @preconcurrency, because it's a hard error to call a plain actor-isolated function from outside the isolation domain even under -strict-concurrency=minimal?

This is a kind of philosophical question. What are the semantics of a function that doesn't compile? :)

hborla · 2024-03-11T17:01:11.477Z

John_McCall:

This is a kind of philosophical question. What are the semantics of a function that doesn't compile? :)

Heh, what I mean is: can we eliminate the checks at a call-site that does compile? But the answer is probably no because you could have some transitive caller that is marked @preconcurrency in the unchecked code, which could then be accidentally called from off of the actor without inserting the checks, because that @preconcurrency function itself doesn't have strict checking.

EDIT: here's an example because this was probably confusing

// ModuleA which has -swift-version 6
@MainActor public func global() { /* mutate main actor state here */ }

// Module B which has -swift-version 5 -strict-concurrency=minimal
import ModuleA

@MainActor func call() {
  global()
}

@preconcurrency @MainActor func transitiveCaller() {
  call()
}

func notIsolated() {
  transitiveCaller() // not even a warning due to minimal checking!
}

John_McCall · 2024-03-11T18:11:24.111Z

Thanks, that example clarifies things a lot.

The logic is to introduce the check at places where we're no longer able to strictly enforce caller isolation, right? So if putting @MainActor without @preconcurrency on a function makes us strictly enforce isolation on calls to it, even in a module that otherwise has minimal checking, I feel like that function shouldn't have internal checks. In your example, I guess that would mean the check would be in transitiveCaller, although I think you could argue that it should be at the beginning of transitiveCaller, or maybe even in its call sites. Call sites would still be possible, right? We'd just have to recognize things like "okay, using this function as a value erases @MainActor because it's @preconcurrency, so we need to check that in a thunk".

Ben_Cohen · 2024-04-10T01:59:49.341Z

Review Update

The proposal has been revised by the proposal author in response to review feedback. You can find the new review here.

Ben_Cohen · 2024-04-10T02:00:00.369Z