Skip to content

Make sure that we capture the isolation of a defer function that implicitly uses the isolation #84206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 11, 2025
Merged

Make sure that we capture the isolation of a defer function that implicitly uses the isolation #84206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions lib/Sema/TypeCheckCaptures.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class FindCapturedVars : public ASTWalker {
DeclContext *CurDC;
bool NoEscape, ObjC;
bool HasGenericParamCaptures;
bool HasUsesOfCurrentIsolation = false;

public:
FindCapturedVars(SourceLoc CaptureLoc,
Expand All @@ -91,6 +92,10 @@ class FindCapturedVars : public ASTWalker {
CapturedTypes);
}

bool hasUsesOfCurrentIsolation() const {
return HasUsesOfCurrentIsolation;
}

bool hasGenericParamCaptures() const {
return HasGenericParamCaptures;
}
Expand Down Expand Up @@ -693,6 +698,31 @@ class FindCapturedVars : public ASTWalker {
checkType(typeValue->getParamType(), E->getLoc());
}

// Record that we saw an #isolation expression that hasn't been filled in.
if (auto currentIsolation = dyn_cast<CurrentContextIsolationExpr>(E)) {
if (!currentIsolation->getActor())
HasUsesOfCurrentIsolation = true;
}

// Record that we saw an apply of a function with caller isolation.
if (auto apply = dyn_cast<ApplyExpr>(E)) {
if (auto type = apply->getFn()->getType()) {
if (auto fnType = type->getAs<AnyFunctionType>();
fnType && fnType->getIsolation().isNonIsolatedCaller()) {
HasUsesOfCurrentIsolation = true;
}
}
}

// Look into caller-side default arguments.
if (auto defArg = dyn_cast<DefaultArgumentExpr>(E)) {
if (defArg->isCallerSide()) {
if (auto callerSideExpr = defArg->getCallerSideDefaultExpr()) {
callerSideExpr->walk(*this);
}
}
}

return Action::Continue(E);
}

Expand Down Expand Up @@ -747,13 +777,18 @@ class FindCapturedVars : public ASTWalker {
/// Given that a local function is isolated to the given var, should we
/// force a capture of the var?
static bool shouldCaptureIsolationInLocalFunc(AbstractFunctionDecl *AFD,
VarDecl *var) {
VarDecl *var,
bool hasUsesOfCurrentIsolation) {
assert(isa<ParamDecl>(var));

// Don't try to capture an isolated parameter of the function itself.
if (var->getDeclContext() == AFD)
return false;

// Force capture if we have uses of the isolation in the function body.
if (hasUsesOfCurrentIsolation)
return true;

// We only *need* to force a capture of the isolation in an async function
// (in which case it's needed for executor switching) or if we're in the
// mode that forces an executor check in all synchronous functions. But
Expand Down Expand Up @@ -792,7 +827,8 @@ CaptureInfo CaptureInfoRequest::evaluate(Evaluator &evaluator,
auto actorIsolation = getActorIsolation(AFD);
if (actorIsolation.getKind() == ActorIsolation::ActorInstance) {
if (auto *var = actorIsolation.getActorInstance()) {
if (shouldCaptureIsolationInLocalFunc(AFD, var))
if (shouldCaptureIsolationInLocalFunc(AFD, var,
finder.hasUsesOfCurrentIsolation()))
finder.addCapture(CapturedValue(var, 0, AFD->getLoc()));
}
}
Expand Down
57 changes: 51 additions & 6 deletions ...ated_nonsending_isolation_macro_sil.swift → test/Concurrency/isolation_macro_sil.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-swift-frontend -parse-as-library -emit-sil %s | %FileCheck %s
// RUN: %target-swift-frontend -parse-as-library -emit-sil %s -module-name test | %FileCheck %s

// REQUIRES: concurrency

Expand All @@ -9,16 +9,18 @@ nonisolated(nonsending) func nonisolatedNonsending() async {

func take(iso: (any Actor)?) {}

func takeDefaulted(iso: isolated (any Actor)? = #isolation) {}

// CHECK-LABEL: // nonisolatedNonsending()
// CHECK-NEXT: // Isolation: caller_isolation_inheriting
// CHECK-NEXT: sil hidden @$s39isolated_nonsending_isolation_macro_sil21nonisolatedNonsendingyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
// CHECK-NEXT: sil hidden @$s4test21nonisolatedNonsendingyyYaF : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
// CHECK: bb0([[ACTOR:%.*]] : $Optional<any Actor>):
// CHECK: retain_value [[ACTOR]]
// CHECK: debug_value [[ACTOR]], let, name "iso"
// CHECK: [[FUNC:%.*]] = function_ref @$s39isolated_nonsending_isolation_macro_sil4take3isoyScA_pSg_tF : $@convention(thin) (@guaranteed Optional<any Actor>) -> ()
// CHECK: [[FUNC:%.*]] = function_ref @$s4test4take3isoyScA_pSg_tF : $@convention(thin) (@guaranteed Optional<any Actor>) -> ()
// CHECK: apply [[FUNC]]([[ACTOR]]) : $@convention(thin) (@guaranteed Optional<any Actor>) -> ()
// CHECK: release_value [[ACTOR]]
// CHECK: } // end sil function '$s39isolated_nonsending_isolation_macro_sil21nonisolatedNonsendingyyYaF'
// CHECK: } // end sil function '$s4test21nonisolatedNonsendingyyYaF'

// Check that we emit #isolation correctly in closures.
// CHECK-LABEL: // closure #1 in containsClosure()
Expand All @@ -33,6 +35,49 @@ func containsClosure() {
}
}

// Check that we capture variables as necessary to emit #isolation
// correctly in defer bodies.
func deferWithIsolatedParam(_ iso: isolated (any Actor)) {
defer {
take(iso: #isolation)
}
do {}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for following up on this! i tested one more flavor that still seems to trip an assertion:

func implicitIsolationInheriting(iso: isolated (any Actor)? = #isolation) {}

func isolated_defer(_ iso: isolated (any Actor)) {
    defer {
        implicitIsolationInheriting()
    }
    do {}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure if it's just me, but i also tried running the following example inspired from this bug report and now am a bit confused:

actor MyActor {
    func test(_ closure: nonisolated(nonsending) @escaping () async -> Void) async {
        await closure()
    }
}

@concurrent
nonisolated func test() async {
    let a = MyActor()

    nonisolated(nonsending)
    func localF() async -> Void {
        print("local func")
        let iso = #isolation
        print(iso as Any)       // Optional(main.MyActor)
        a.assertIsolated()      // 💥
    }

    await a.test(localF)

    let fn: nonisolated(nonsending) () async -> Void = {
        print("local var closure literal")
        let iso = #isolation
        print(iso as Any)       // Optional(main.MyActor)
        a.assertIsolated()      // 💥
    }

    await a.test(fn)

    await a.test {
        print("contextual closure literal")
        let iso = #isolation
        print(iso as Any)       // Optional(main.MyActor)
        a.assertIsolated()      // 💥
    }
}

@main
enum App {
    static func main() async {
        await test()
    }
}

prior to the changes (here & in #84181), the #isolation expansion would return nil, but the isolation assertions would pass. now it seems it's the other way around. i find both of these states confusing.

it's possible this is some artifact of my local build configuration, in which case i apologize for the noise. however, i was wondering if anyone else may be able to reproduce this behavior, have an explanation for it, or know if there are other tests covering it.

// CHECK-LABEL: sil hidden @$s4test22deferWithIsolatedParamyyScA_pYiF :
// CHECK: bb0(%0 : $any Actor)
// CHECK: [[DEFER:%.*]] = function_ref @$s4test22deferWithIsolatedParamyyScA_pYiF6$deferL_yyF :
// CHECK-NEXT: apply [[DEFER]](%0)

// CHECK-LABEL: sil private @$s4test22deferWithIsolatedParamyyScA_pYiF6$deferL_yyF :
// CHECK: bb0(%0 : @closureCapture $any Actor):
// CHECK: [[T0:%.*]] = enum $Optional<any Actor>, #Optional.some!enumelt, %0
// CHECK: [[FN:%.*]] = function_ref @$s4test4take3isoyScA_pSg_tF :
// CHECK-NEXT: apply [[FN]]([[T0]])

// Check that that happens even with uses in caller-side default
// arguments, which capture analysis was not previously walking into.
func deferWithIsolatedParam_defaultedUse(_ iso: isolated (any Actor)) {
defer {
takeDefaulted()
}
do {}
}

// CHECK-LABEL: sil hidden @$s4test35deferWithIsolatedParam_defaultedUseyyScA_pYiF :
// CHECK: bb0(%0 : $any Actor):
// CHECK: [[DEFER:%.*]] = function_ref @$s4test35deferWithIsolatedParam_defaultedUseyyScA_pYiF6$deferL_yyF :
// CHECK-NEXT: apply [[DEFER]](%0)

// CHECK-LABEL: sil private @$s4test35deferWithIsolatedParam_defaultedUseyyScA_pYiF6$deferL_yyF :
// CHECK: bb0(%0 : @closureCapture $any Actor):
// CHECK: [[T0:%.*]] = enum $Optional<any Actor>, #Optional.some!enumelt, %0
// CHECK: [[FN:%.*]] = function_ref @$s4test13takeDefaulted3isoyScA_pSgYi_tF :
// CHECK-NEXT: apply [[FN]]([[T0]])

// TODO: we can't currently call nonisolated(nonsending) functions in
// defer bodies because they have to be async, but that should be
// tested here as well.

// Check that we emit #isolation correctly in defer bodies.
nonisolated(nonsending)
func hasDefer() async {
Expand All @@ -41,7 +86,7 @@ func hasDefer() async {
}
do {}
}
// CHECK-LABEL: sil hidden @$s39isolated_nonsending_isolation_macro_sil8hasDeferyyYaF :
// CHECK-LABEL: sil hidden @$s4test8hasDeferyyYaF :
// CHECK: bb0(%0 : $Optional<any Actor>):
// CHECK: // function_ref $defer
// CHECK-NEXT: [[DEFER:%.*]] = function_ref
Expand All @@ -66,7 +111,7 @@ func hasNestedDefer() async {
do {}
}

// CHECK-LABEL: sil hidden @$s39isolated_nonsending_isolation_macro_sil14hasNestedDeferyyYaF :
// CHECK-LABEL: sil hidden @$s4test14hasNestedDeferyyYaF :
// CHECK: bb0(%0 : $Optional<any Actor>):
// CHECK: // function_ref $defer #1 () in hasNestedDefer()
// CHECK-NEXT: [[DEFER:%.*]] = function_ref
Expand Down