[6.2] [nonisolated-nonsending] Make the AST not consider nonisolated(nonsending) to be an actor isolation crossing point. #82776

gottesmm · 2025-07-03T17:11:20Z

Explanation: Big picture this fixes a large hole in the implementation of
SE-0461 and then fixes a few additional issues that this exposed.

Previously Sema was allowing for nonisolated(nonsending) callees to be potential
sending points. This violated the semantics of SE-0461. We previously got away
with this since Sema was delegating checking of function arguments to
SendNonSendable and SendNonSendable was treating them as isolated functions due
to their isolated parameter and thus squelched the error.

Our luck ran out when it came to imported objc async methods. These do not
have an isolated parameter, so we would consider them to be nonisolated at the
SendNonSendable level. So we did not squelch the error. So for instance, the
following function would emit an error when it clearly should not:

@MainActor
func defeatRegion(baz: Baz) async throws -> Bar? {
  let foo = ObjectiveCType()
  return try await foo.findCompatibleBar(for: baz) // ERROR: Sending 'baz' risks causing data races
}

In this change, we fix that problem by treating nonisolated(nonsending) callees
as not being a send since we are going to be inheriting isolation from our
caller.

In the process of doing this, the author looked through this code to see if
there were any other issues that violated SE-0461 and found that we were
allowing for parameters of nonisolated(nonsending) parameters to be passed to
@Concurrent functions which should not be allowed. The reason why this is not
allowed is that the parameter of the nonisolated(nonsending) function /could/ be
isolated to an actor meaning that by passing it to the @Concurrent function we
would be escaping the value to another isolation domain. We fixed this as well
at the Sema level.

In order to be careful with this change, the authors massively expanded the test
coverage for this feature by porting all of the SendNonSendable tests to run
both with and without NonisolatedNonsendingByDefault.

When I enabled those tests, we discovered additional issues exposed by inferring nonisolated(nonsending) on expressions (#82731) . Specifically:

We did not handle cases where a closure would be global actor isolated or isolated to an actor instance due to a capture. I fixed this in 78bc443
We were introducing round trip thunks that went from nonisolated(nonsending) -> @concurrent -> nonisolated(nonsending) which results in a nonisolated(nonsending) function that always has concurrent isolation (which isn't what we want). The peephole eliminates that issues.
We were not respecting the isolation of inheritActorIsolation. Instead we always did nonisolated(nonsending) in such cases resulting in the incorrect isolation.

With all of those together, we now pass all of the transfernonsendable tests with NonisolatedNonsendingByDefault enabled.

When doing that, the authors noticed that when NonisolatedNonsendingByDefault
was enabled, we were emitting diagnostics saying caller inheriting
isolation. This violates the spirit of SE-0461, so we threaded through a
SILFunction into the diagnostic printing code and used that to tweak the
diagnostics depending on whether or not NonisolatedNonsendingByDefault is
enabled. In the case where it is disabled:

Functions with @Concurrent isolation are called "nonisolated" and regions
containing parameters of those functions are called "task-isolated".
Functions with nonisolated(nonsending) isolation are called
"nonisolated(nonsending)" and regions containing parameters of those functions
are called "nonisolated(nonsending) task-isolated".

When it is enabled:

Functions with @Concurrent isolation are called "@Concurrent" and regions
containing parameters of those functions are called "@Concurrent task-isolated".
Functions with nonisolated(nonsending) isolation are called
"nonisolated" and regions containing parameters of those functions
are called "task-isolated".

This ensures that in cases that do not involve explicit @Concurrent or
nonisolated(nonsending) parameters, one just sees task-isolated and nonisolated
in the SendNonSendable diagnostics when one enables or disables
NonisolatedNonsendingByDefault due to the way the upcoming feature changes how
we infer isolation.

Scope: This impacts the diagnostics that we emit when calling
nonisolated(nonsending) functions and when passing a parameter of an
nonisolated(nonsending) function to an @Concurrent function.

Issues:

rdar://154139237

Original PRs:

Risk: Medium. The main risk is that we would start emitting less diagnostics
that we did not perviously when calling nonisolated(nonsending) function or
calling nonisolated functions from nonisolated(nonsending) functions. That being
said this closes a few major holes in the model and ensures that we actually
follow the semantics of SE-0461 so the authors believe it is worth it. The
authors also believe that by massively expanding the amount of tests that run
with this feature that we have mitigated the risk of this fix.

Testing: Massively expanded the amount of tests that run with
NonisolatedNonsendingByDefault enabled by converting all SendNonSendable tests
(and a few others) to run with both. Also added specific tests to test out this
behavior both with and without objc.

Reviewer: @xedin

gottesmm · 2025-07-03T17:11:27Z

@swift-ci test

gottesmm · 2025-07-03T17:26:39Z

NOTE: Most of the changes in this PR are actually in the tests since I also massively expanded the amount of test coverage we have for this feature by changing most SendNonSendable tests to run both with and without NonisolatedNonsendingByDefault.

gottesmm · 2025-07-03T17:27:00Z

So it is best to review the change commit by commit to see the actual amount of changes.

gottesmm · 2025-07-08T18:14:30Z

@swift-ci test

gottesmm · 2025-07-08T18:30:55Z

@swift-ci test

gottesmm · 2025-07-08T20:56:07Z

@swift-ci test

gottesmm · 2025-07-09T19:22:43Z

Just as an FYI, this passed all tests. There is just a merge conflict.

@mainactor

…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 (cherry picked from commit c12c99f)

…ault. Going to update the tests in the next commit. This just makes it easier to review. (cherry picked from commit a6edf4f)

(cherry picked from commit 5c190b9)

…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. (cherry picked from commit 4ce4fc4)

…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. (cherry picked from commit 4433ab8)

…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. (cherry picked from commit 14634b6)

…64>. This makes the code easier to write and also prevents any lifetime issues from a diagnostic outliving the SmallString due to diagnostic transactions. (cherry picked from commit 010fa39)

I cherry-picked 4c1440735b8066d061c668d3b655955c1831f1dd to prevent merge conflicts. I think these tests were updated by a subsequent PR. Rather than cherry-picking more work, I just updated these tests since the change seemed benign and that this kept the overall amount of cherry-picked commits small (converting %$NUMBER to %kind$NUMBER for some ValueDecls).

gottesmm · 2025-07-09T19:25:25Z

@swift-ci test

gottesmm · 2025-07-09T19:55:51Z

@swift-ci test linux platform

gottesmm · 2025-07-09T21:15:49Z

@swift-ci test windows platform

gottesmm · 2025-07-09T21:16:53Z

windows failed due to:

LINK : fatal error LNK1104: cannot open file 'libswiftSynchronization.lib'

gottesmm · 2025-07-09T23:31:25Z

Windows failure was due to: swiftlang/swift-foundation#1417

gottesmm · 2025-07-09T23:31:32Z

@swift-ci test windows platform

hborla

I'm still in the process of reviewing this change but I had a few comments in the first commit that I figured I'd post now.

hborla · 2025-07-09T21:21:33Z

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

+    // the AST is not going to label this as an isolation crossing point so we
+    // will only perform a merge. We want to just perform an isolation merge
+    // without adding additional isolation info. Otherwise, a nonisolated
+    // function that


I don't understand why this is necessary. Isn't a region merge the default rule for function calls? It should behave the same way as a nonisolated synchronous function. Or are you trying to prevent a different code path that looks for isolated parameters -- which would always merge into the actor's region -- from being taken here? Perhaps this is answered by the omitted part of the last sentence in the comment :)

hborla · 2025-07-10T01:33:30Z

lib/Sema/TypeCheckConcurrency.cpp

+      // actual isolation which does not have nonisolated(nonsending) added to
+      // it yet. Instead, we want the direct callee including casts since those
+      // casts is what would add the nonisolated(nonsending) bit to the function
+      // type isolation.


The check below is confusing, in part because @concurrent is represented by ActorIsolation::Nonisolated + async. I also do not understand the second part of the comment about not checking fnTypeIsolation because the conditions do look at fnTypeIsolation.

The conditions where we want to consider the call have an unsatisfied isolation (i.e. the call crosses an isolation boundary) are:

The context isolation is actor isolated or nonisolated(nonsending), and

The callee is @concurrent.

Can't this be simplified to:

if (mayExitToNonisolated && fnType->isAsync() && fnTypeIsolation.isNonisolated()) { auto contextIsolation = getContextIsolation(); if (contextIsolation.isActorIsolated() || getContextIsolation().isCallerIsolationInheriting()) unsatisfiedIsolation = ActorIsolation::forNonisolated(/*unsafe=*/false); }

I don't understand why the condition about fnTypeIsolation is different between the two branches.

hborla · 2025-07-10T01:37:47Z

test/ClangImporter/regionbasedisolation.swift

+  let x = ObjCObject()
+  await x.useValue(y)
+  await useValueConcurrently(x)  // expected-error {{sending 'x' risks causing data races}}
+  // expected-note @-1 {{sending main actor-isolated 'x' to nonisolated global function 'useValueConcurrently' risks causing data races between nonisolated and main actor-isolated uses}}


Please don't add more to this change, but this diagnostic really should mention @concurrent in the error. The intention is not to hide @concurrent from people if they don't have the upcoming feature enabled.

hborla · 2025-07-10T01:51:00Z

test/Concurrency/nonisolated_inherits_isolation.swift

+func testUnrolledLoopWithAsyncLet(_ a: MainActorKlass) async {
+  let k = NonSendableKlass()
+  await unspecifiedAsyncUse(k)
+  // This is valid since our valid is disconnected here.


Typo? Should it say "our value is disconnected here"?

Also nit: you don't need await on the initializer expression for an async let.

hborla · 2025-07-10T03:43:54Z

test/Concurrency/nonisolated_inherits_isolation.swift

 nonisolated(nonsending) func nonisolatedNonSendingCallingVariousNonisolated(_ x: NonSendableKlass) async {
  await x.unspecifiedCaller() // expected-disabled-error {{sending 'x' risks causing data races}}
-  // expected-disabled-note @-1 {{sending task-isolated 'x' to nonisolated instance method 'unspecifiedCaller()' risks causing data races between nonisolated and task-isolated uses}}
+  // expected-disabled-note @-1 {{sending nonisolated(nonsending) task-isolated 'x' to nonisolated instance method 'unspecifiedCaller()' risks causing data races between nonisolated and nonisolated(nonsending) task-isolated uses}}


I don't think the nonisolated(nonsending) adds any clarity here. To me, task-isolated implies that the value is tied to the concurrent task and whatever actor the task is running on.

hborla · 2025-07-10T03:46:42Z

test/Concurrency/transfernonsendable_asynclet.swift

  async let y = transferToMainInt(x) // expected-warning {{sending 'x' risks causing data races}}
-  // expected-note @-1 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local nonisolated uses}}
+  // expected-ni-note @-1 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local nonisolated uses}}
+  // expected-ni-ns-note @-2 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local @concurrent uses}}


Where did the @concurrent come from here? If I understand correctly, this diagnostic only happens under NonisolatedNonsendingByDefault, and in that case, the local context is not @concurrent. Is it mistakenly pulled from the async let initializer isolation?

This is actually something I wrote down that I forgot to fix. Let me see what I can do.

lib/Sema/TypeCheckType.cpp

test/Concurrency/attr_execution/migration_mode.swift

@xedin

…iterals_isolationinference.swift now passes. The reason why this failed is that concurrently to @xedin landing 79af04c, I enabled NonisolatedNonsendingByDefault on a bunch of other tests. That change broke the test and so we needed to fix it. This commit fixes a few issues that were exposed: 1. We do not propagate nonisolated(nonsending) into a closure if its inferred context isolation is global actor isolated or if the closure captures an isolated parameter. We previously just always inferred nonisolated(nonsending). Unfortunately since we do not yet have capture information in CSApply, this required us to put the isolation change into TypeCheckConcurrency.cpp and basically have function conversions of the form: ``` (function_conversion_expr type="nonisolated(nonsending) () async -> Void" (closure_expr type="() async -> ()" isolated_to_caller_isolation)) ``` Notice how we have a function conversion to nonisolated(nonsending) from a closure expr that has an isolation that is isolated_to_caller. 2. With this in hand, we found that this pattern caused us to first thunk a nonisolated(nonsending) function to an @Concurrent function and then thunk that back to nonisolated(nonsending), causing the final function to always be concurrent. I put into SILGen a peephole that recognizes this pattern and emits the correct code. 3. With that in hand, we found that we were emitting nonisolated(nonsending) parameters for inheritActorContext functions. This was then fixed by @xedin in With all this in hand, closure literal isolation and all of the other RBI tests with nonisolated(nonsending) enabled pass. rdar://154969621 (cherry picked from commit 648bb8f)

… errors around obfuscated Sendable functions Specifically the type checker to work around interface types not having isolation introduces casts into the AST that enrich the AST with isolation information. Part of that information is Sendable. This means that we can sometimes lose due to conversions that a function is actually Sendable. To work around this, we today suppress those errors when they are emitted (post 6.2, we should just change their classification as being Sendable... but I don't want to make that change now). This change just makes the pattern matching for these conversions handle more cases so that transfernonsendable_closureliterals_isolationinference.swift now passes.

xedin · 2025-07-11T01:07:06Z

@swift-ci please test

xedin · 2025-07-11T04:59:02Z

@swift-ci please test Linux platform

gottesmm marked this pull request as ready for review July 3, 2025 18:25

gottesmm requested a review from a team as a code owner July 3, 2025 18:25

gottesmm force-pushed the release/6.2-154139237 branch from 1721b3e to 78bc443 Compare July 8, 2025 18:14

gottesmm force-pushed the release/6.2-154139237 branch from 38ad731 to 90b894e Compare July 8, 2025 18:30

gottesmm enabled auto-merge July 9, 2025 16:52

gottesmm and others added 8 commits July 9, 2025 12:24

[rbi] Convert all rbi tests to run also in NonisolatedNonsendingByDef…

065ffa3

…ault. Going to update the tests in the next commit. This just makes it easier to review. (cherry picked from commit a6edf4f)

AST: Cut down on DescriptiveDeclKind usage in DiagnosticsSIL.def

f5d547c

(cherry picked from commit 5c190b9)

[rbi] Use interned StringRefs for diagnostics instead of SmallString<…

f920aba

…64>. This makes the code easier to write and also prevents any lifetime issues from a diagnostic outliving the SmallString due to diagnostic transactions. (cherry picked from commit 010fa39)

gottesmm force-pushed the release/6.2-154139237 branch from 5a28b3d to 3a5e778 Compare July 9, 2025 19:25

hborla reviewed Jul 10, 2025

View reviewed changes

xedin force-pushed the release/6.2-154139237 branch from 3a5e778 to 1f9e302 Compare July 11, 2025 01:05

hborla approved these changes Jul 14, 2025

View reviewed changes

gottesmm merged commit 187b1ee into swiftlang:release/6.2 Jul 14, 2025
5 checks passed

[6.2] [nonisolated-nonsending] Make the AST not consider nonisolated(nonsending) to be an actor isolation crossing point. #82776

[6.2] [nonisolated-nonsending] Make the AST not consider nonisolated(nonsending) to be an actor isolation crossing point. #82776

Uh oh!

Conversation

gottesmm commented Jul 3, 2025 • edited by xedin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm commented Jul 3, 2025

Uh oh!

gottesmm commented Jul 3, 2025

Uh oh!

gottesmm commented Jul 3, 2025

Uh oh!

gottesmm commented Jul 8, 2025

Uh oh!

gottesmm commented Jul 8, 2025

Uh oh!

gottesmm commented Jul 8, 2025

Uh oh!

gottesmm commented Jul 9, 2025

Uh oh!

gottesmm commented Jul 9, 2025

Uh oh!

gottesmm commented Jul 9, 2025

Uh oh!

gottesmm commented Jul 9, 2025

Uh oh!

gottesmm commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm commented Jul 9, 2025

Uh oh!

gottesmm commented Jul 9, 2025

Uh oh!

hborla left a comment

Choose a reason for hiding this comment

Uh oh!

hborla Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

hborla Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

gottesmm Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xedin commented Jul 11, 2025

Uh oh!

xedin commented Jul 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gottesmm commented Jul 3, 2025 •

edited by xedin

Loading

gottesmm commented Jul 9, 2025 •

edited

Loading