-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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.
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
Conversation
@swift-ci Please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for following up on all those John!
take(iso: #isolation) | ||
} | ||
do {} | ||
} |
There was a problem hiding this comment.
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 {}
}
There was a problem hiding this comment.
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.
implicitly uses the isolation.
8c9f0fd
to
d780b62
Compare
@jamieQ I've added a fix for your test case with default arguments. Thanks for being on the ball about this! I should've anticipated that possibly being a problem for capture analysis. I can verify your crasher. What seems to be happening is that the SIL optimizer is removing the actor hop in |
@swift-ci Please test |
@rjmccall i double checked, and i don't think your patch affected the behavior of the executor assertions – but, as hoped, the patches do make the isolation macro work as expected (yay!). the behavior where an isolation assertion will pass but the |
Okay, thanks. So it sounds like:
This is consistent with the |
Follow-up to #84181.