Implement async defer

[Jumhyn]

Tests etc tbc

[slavapestov]

You can remove FuncDecl::isDeferBody() too then

[ktoso]

looks pretty promising so far. The escaping shouldn't really break anything concurrency checking wise as long as the closure is not sending/sendable; that's definitely worth adding a bunch of tests though

[ktoso]

Perhaps we could relax this assertion that if the defer body is not async then we keep the check that it is not escaping, but if it is it can be? Not sure if strictly necessary but we'd err on the side of caution this way 🤔

[ktoso]

Yeah the indiscriminate change to escaping won't fly because we break existing semantics, e.g.

OutputSpan.swift:428:11: error: escaping closure captures mutating 'self' parameter
426 | #endif
427 |     var initializedCount = self._count
428 |     defer {
    |           `- error: escaping closure captures mutating 'self' parameter
429 |       _precondition(
430 |         0 <= initializedCount && initializedCount <= capacity,
    |         |                        |                   `- note: captured here
    |         |                        `- note: captured indirectly by this call
    |         `- note: captured indirectly by this call
431 |         "OutputSpan capacity overflow"
432 |       )
433 |       self._count = initializedCount
    |                   `- note: captured here

So I'd look into if we're able to only be escaping when necessary due to the async-nature of the closure

[jumhyn-browser]

@ktoso welp, yeah, that's not an option. There's no reason this closure needs to be escaping except that IIUC we don't do any real escape analysis today for closures assigned to local bindings, we just assume they're always escaping.

I don't think we'll want to apply this only to async defer bodies either, both because I think the model should stay consistent between the async/non-async case, and because I don't think the limitations like "can't capture mutating self" should apply to async defer bodies just because of weird implementation limitations... so seems like there's a couple paths:

• Keep the tempDecl as a FuncDecl but add the necessary hooks elsewhere to allow the body to participate in type checking and have its async-ness updated
• Keep as a FuncDecl but eagerly look at the body to find any awaits and use that to set the isAsync bit
• Use a PBD as in this PR but try to wire up the minimal escape analysis needed to satisfy the type checker and properly mark this closure as non-escaping
• Use a PBD but artificially force the closure to be non-escaping because we 'know' it won't escape by virtue of the fact that it's a defer body.

Will think a bit...

[Jumhyn]

@ktoso Ah I commented from my work account by seems like you figured it out 😉

As a side note, if you have a gut sense about which of those avenues might be most fruitful I am all ears!

[hborla]

I'm inclined to keep the body represented as a FuncDecl and go with either option 1 or 2. I think ClosureEffectsRequest already factors out scanning the brace statement for AwaitExpr so that can be easily applied to a FuncDecl body as well.

I think ideally we wouldn't actually create a function decl or a closure expression in the type checker for a defer body at all, and we'd just let the type checker look at the statements in the defer body and diagnose issues like missing await without considering it to be in a different local context than the enclosing function. But I don't think we should do that sort of refactoring here, and I think keeping the FuncDecl is the most straightforward path to a working async defer implementation!

Relatedly, we probably also need to be careful of the actor isolation for the closure / function that represents the async defer body to make sure that they always have the nonisolated(nonsending) semantics. We've seen some very weird actor isolation bugs with defer, including an issue in main actor mode where the function created for a defer body is inferred as @MainActor, even when the defer statement is written in a nonisolated function!

[Jumhyn]

Thanks for taking a look @hborla!

I'm inclined to keep the body represented as a FuncDecl and go with either option 1 or 2. I think ClosureEffectsRequest already factors out scanning the brace statement for AwaitExpr so that can be easily applied to a FuncDecl body as well.

Sounds good, it seems like eagerly looking ahead is probably the best route since it seems like there's a lot of downstream reliance on FuncDecl having the proper signature from the get-go. And I think we'll still end up with reasonable diagnostics in the case where someone writes an async call but forgets await.

I think ideally we wouldn't actually create a function decl or a closure expression in the type checker for a defer body at all

Yeah, the number of times we have to explicitly check isDeferBody definitely makes me a bit uneasy.

Relatedly, we probably also need to be careful of the actor isolation for the closure / function that represents the async defer body to make sure that they always have the nonisolated(nonsending) semantics.

Will add some tests for these cases to make sure they're handled correctly!

[Jumhyn]

@swift-ci please test

[Jumhyn]

@swift-ci please test

[Jumhyn]

@swift-ci please test

[Jumhyn]

@swift-ci build toolchain

[Jumhyn]

@swift-ci please test macOS

[Jumhyn]

@swift-ci please test

[Jumhyn]

@swift-ci please build toolchain

[jamieQ]

what behavior do we get if we have no source-level 'await' in the defer?

func f() {
  defer { async let _: () = voidFunc() }
  voidFunc()
}