[nonisolated-nonsending] Make the AST not consider nonisolated(nonsending) to be an actor isolation crossing point. #82555
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gottesmm
requested review from
eeckstein,
hborla,
ktoso,
slavapestov and
xedin
as code owners
ricky-sharp
reviewed
Jun 27, 2025
There was a problem hiding this comment.
Ending of this discussion got omitted
ktoso
approved these changes
Jun 27, 2025
xedin
reviewed
Jun 27, 2025
…ding) to be an actor isolation crossing point.
We were effectively working around this previously at the SIL level. This caused
us not to obey the semantics of the actual evolution proposal. As an example of
this, in the following, x should not be considered main actor isolated:
```swift
nonisolated(nonsending) func useValue<T>(_ t: T) async {}
@mainactor func test() async {
let x = NS()
await useValue(x)
print(x)
}
```
we should just consider this to be a merge and since useValue does not have any
MainActor isolated parameters, x should not be main actor isolated and we should
not emit an error here.
I also fixed a separate issue where we were allowing for parameters of
nonisolated(nonsending) functions to be passed to @Concurrent functions. We
cannot allow for this to happen since the nonisolated(nonsending) parameters
/could/ be actor isolated. Of course, we have lost that static information at
this point so we cannot allow for it. Given that we have the actual dynamic
actor isolation information, we could dynamically allow for the parameters to be
passed... but that is something that is speculative and is definitely outside of
the scope of this patch.
rdar://154139237
…ault. Going to update the tests in the next commit. This just makes it easier to review.
…olationInfo::printActorIsolationForDiagnostics. I am doing this so that I can change how we emit the diagnostics just for SendNonSendable depending on if NonisolatedNonsendingByDefault is enabled without touching the rest of the compiler. This does not actually change any of the actual output though.
…ss LangOpts.Features so we can change how we print based off of NonisolatedNonsendingByDefault. We do not actually use this information yet though... This is just to ease review.
…sending) parameter and adjust printing as appropriate. Specifically in terms of printing, if NonisolatedNonsendingByDefault is enabled, we print out things as nonisolated/task-isolated and @concurrent/@Concurrent task-isolated. If said feature is disabled, we print out things as nonisolated(nonsending)/nonisolated(nonsending) task-isolated and nonisolated/task-isolated. This ensures in the default case, diagnostics do not change and we always print out things to match the expected meaning of nonisolated depending on the mode. I also updated the tests as appropriate/added some more tests/added to the SendNonSendable education notes information about this.
gottesmm
force-pushed
the
pr-b4de2f5d58d852cd3e9e606101e1b0ff42ce6092
branch
from
96ab824 to
c1bfc32
Compare
Contributor
Author
|
@swift-ci smoke test |
gottesmm
force-pushed
the
pr-b4de2f5d58d852cd3e9e606101e1b0ff42ce6092
branch
from
c1bfc32 to
14634b6
Compare
xedin
approved these changes
Jul 2, 2025
Contributor
xedin
left a comment
xedin
left a comment
There was a problem hiding this comment.
LGTM! We should be careful about using StringRef in the diagnostic though, the arguments need to be owned by the ASTContext to make that the diagnostic doesn't outlive the argument.
Contributor
Author
|
I can fix that real quick. Just rerunning tests locally. |
Contributor
|
Thank you! |
gottesmm
force-pushed
the
pr-b4de2f5d58d852cd3e9e606101e1b0ff42ce6092
branch
2 times, most recently
from
d61f31e to
0c8b629
Compare
…64>. This makes the code easier to write and also prevents any lifetime issues from a diagnostic outliving the SmallString due to diagnostic transactions.
gottesmm
force-pushed
the
pr-b4de2f5d58d852cd3e9e606101e1b0ff42ce6092
branch
from
0c8b629 to
010fa39
Compare
Contributor
Author
|
@swift-ci smoke test |
Contributor
Author
|
Failed in lldb |
Contributor
Author
|
@swift-ci smoke test macOS platform |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were effectively working around this previously at the SIL level. This caused
us not to obey the semantics of the actual evolution proposal. As an example of
this, in the following, x should not be considered main actor isolated:
we should just consider this to be a merge and since useValue does not have any
MainActor isolated parameters, x should not be main actor isolated and we should
not emit an error here.
I also fixed a separate issue where we were allowing for parameters of
nonisolated(nonsending) functions to be passed to
@concurrentfunctions. Wecannot allow for this to happen since the nonisolated(nonsending) parameters
/could/ be actor isolated. Of course, we have lost that static information at
this point so we cannot allow for it. Given that we have the actual dynamic
actor isolation information, we could dynamically allow for the parameters to be
passed... but that is something that is speculative and is definitely outside of
the scope of this patch.
rdar://154139237