Hi all,
My apologies for not getting to review this sooner.
I'm thrilled to see this proposal. It's very carefully thought out and implemented, and extremely important for the usability for Swift's actor isolation model. Thank you!
My impression follows along pretty closely with @ktoso 's review. In particular, this would have been my summary:
I agree with Konrad that copying task-locals to deinits is potentially a lot of expense without a lot of use cases supporting it. I think I'll take the argument further, that a deinit that depends on task locals is already semantically broken, because merely capturing the value in a different task would make it observe different task locals, and would be very likely be susceptible to higher-level race conditions where the deinit can happen in different tasks.
To this comment:
My concern is that there's a performance cliff here: if you happen to trigger deallocation from a task-local-heavy task, deinitialization suddenly becomes expensive, and it would be hard to work around. And again, I feel like we're adding a cost throughout the system to serve a narrow use case that's more like an anti-pattern.
I share this concern: it has so far been the case that the implicit deinitializer can be written explicitly as deinit { }, and it seems unfortunate to introduce a subtle semantic difference here.
This is part of a more general concern that defaulting to an isolated deinit feels off: there's a binary size and runtime cost. I expect it to be small for default actors, and that actors with custom executors are fairly rare... but in the app space I expect there to be a lot of @MainActor class types, and if we're unnecessarily hopping over to the main thread for deinitialization that could become a problem.
I think I'd like to flip the default here, and say that deinit is nonisolated by default, just like init is nonisolated by default. Most deinits will stay nonisolated (which is good for code size/performance), and developers can opt in to making the deinitializer isolated in response to some need---say, accessing a non-Sendable property or using some isolated operation. We can make this discoverable through improved diagnostics.
I'm flip-flopping on this:
Originally, I considered deinit async to be a separate feature, but I find this interesting. If, as I'm arguing above, we need to opt in to isolated deinitializers, then we have a choice between adding new syntax (isolated deinit) or extending the scope of this feature to mirror that of initializers (deinit async). The latter is really interesting: an isolated deinitializer is effectively an async function already, and marking it as such (1) highlights that there is a potential suspension point when performing deinitialization, and (2) would enable use of other async functions, which is a common feature request.
I'm going to look into this further. Something is tripping up our toolchain builds. Sorry about that!
Doug