Skip to content

[TypeChecker] Allow closures to assume nonisolated(nonsending) #80872

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

Merged
merged 3 commits into from
Apr 17, 2025

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Apr 17, 2025

Always infer nonisolated(nonsending) from context directly on a closure unless the closure is marked as @concurrent, otherwise the closure is not going to get correct isolation and going to hop to the wrong executor in its preamble.

Resolves: rdar://149107104

Always infer `nonisolated(nonsending)` from context directly on
a closure unless the closure is marked as `@concurrent`, otherwise
the closure is not going to get correct isolation and going to hop
to the wrong executor in its preamble.

Resolves: rdar://149107104
…t actor

Closures that are caller isolated used to still hop to the generic
executor, which is incorrect.
@xedin xedin requested a review from jckarter as a code owner April 17, 2025 01:22
@xedin
Copy link
Contributor Author

xedin commented Apr 17, 2025

@swift-ci please test

func testThatClosuresAssumeIsolation(fn: inout nonisolated(nonsending) (Int) async -> Void) {
// CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCcfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
// CHECK: bb0([[EXECUTOR:%.*]] : @guaranteed $Optional<any Actor>):
// CHECK: hop_to_executor [[EXECUTOR]]
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, thank you!

@ktoso
Copy link
Contributor

ktoso commented Apr 17, 2025

Hmm the callActorFromStartSynchronousTask case failed in startSynchronously.swift, but that's likely a wrong test since we're correcting behavior here tbh... I'll see what to do about that test, we will be changing it a lot anyway to account for SE evolution

@ktoso
Copy link
Contributor

ktoso commented Apr 17, 2025

Hmm the test is actually a bit racy, I attempted to improve it by making sure the called func suspends, which should give us the expected result consistently.

@ktoso
Copy link
Contributor

ktoso commented Apr 17, 2025

@swift-ci please smoke test

@xedin xedin merged commit f0bdecd into swiftlang:main Apr 17, 2025
3 checks passed
@ktoso
Copy link
Contributor

ktoso commented Apr 25, 2025

This has unblocked #80753

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants