Skip to content

Commit c12c99f

Browse files
committed
[nonisolated-nonsending] Make the AST not consider nonisolated(nonsending) 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
1 parent ce7a3b3 commit c12c99f

File tree

5 files changed

+283
-31
lines changed

5 files changed

+283
-31
lines changed

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,21 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
442442
return info;
443443
}
444444

445+
// See if our function apply site has an implicit isolated parameter. In
446+
// such a case, we know that we have a caller inheriting isolated
447+
// function. Return that this has disconnected isolation.
448+
//
449+
// DISCUSSION: The reason why we are doing this is that we already know that
450+
// the AST is not going to label this as an isolation crossing point so we
451+
// will only perform a merge. We want to just perform an isolation merge
452+
// without adding additional isolation info. Otherwise, a nonisolated
453+
// function that
454+
if (auto paramInfo = fas.getSubstCalleeType()->maybeGetIsolatedParameter();
455+
paramInfo && paramInfo->hasOption(SILParameterInfo::ImplicitLeading) &&
456+
paramInfo->hasOption(SILParameterInfo::Isolated)) {
457+
return SILIsolationInfo::getDisconnected(false /*unsafe nonisolated*/);
458+
}
459+
445460
if (auto *isolatedOp = fas.getIsolatedArgumentOperandOrNullPtr()) {
446461
// First look through ActorInstance agnostic values so we can find the
447462
// type of the actual underlying actor (e.x.: copy_value,
@@ -534,9 +549,8 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
534549
if (actorInstance) {
535550
if (auto actualIsolatedValue =
536551
ActorInstance::getForValue(actorInstance)) {
537-
// See if we have a function parameter. In that case, we want to see
538-
// if we have a function argument. In such a case, we need to use
539-
// the right parameter and the var decl.
552+
// See if we have a function parameter. In such a case, we need to
553+
// use the right parameter and the var decl.
540554
if (auto *fArg = dyn_cast<SILFunctionArgument>(
541555
actualIsolatedValue.getValue())) {
542556
if (auto info =
@@ -980,6 +994,13 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) {
980994
// handles isolated self and specifically marked isolated.
981995
if (auto *isolatedArg = llvm::cast_or_null<SILFunctionArgument>(
982996
fArg->getFunction()->maybeGetIsolatedArgument())) {
997+
// See if the function is nonisolated(nonsending). In such a case, return
998+
// task isolated.
999+
if (auto funcIsolation = fArg->getFunction()->getActorIsolation();
1000+
funcIsolation && funcIsolation->isCallerIsolationInheriting()) {
1001+
return SILIsolationInfo::getTaskIsolated(fArg);
1002+
}
1003+
9831004
auto astType = isolatedArg->getType().getASTType();
9841005
if (astType->lookThroughAllOptionalTypes()->getAnyActor()) {
9851006
return SILIsolationInfo::getActorInstanceIsolated(fArg, isolatedArg);

lib/Sema/TypeCheckConcurrency.cpp

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4034,6 +4034,10 @@ namespace {
40344034
Expr *argForIsolatedParam = nullptr;
40354035
auto calleeDecl = apply->getCalledValue(/*skipFunctionConversions=*/true);
40364036

4037+
if (calleeDecl &&
4038+
calleeDecl->getAttrs().hasAttribute<UnsafeInheritExecutorAttr>())
4039+
return false;
4040+
40374041
auto fnTypeIsolation = fnType->getIsolation();
40384042
if (fnTypeIsolation.isGlobalActor()) {
40394043
// If the function type is global-actor-qualified, determine whether
@@ -4079,10 +4083,6 @@ namespace {
40794083
calleeDecl = memberRef->first.getDecl();
40804084
argForIsolatedParam = selfApplyFn->getBase();
40814085
}
4082-
} else if (calleeDecl &&
4083-
calleeDecl->getAttrs()
4084-
.hasAttribute<UnsafeInheritExecutorAttr>()) {
4085-
return false;
40864086
}
40874087

40884088
// Check for isolated parameters.
@@ -4133,11 +4133,25 @@ namespace {
41334133
break;
41344134
}
41354135

4136-
// If we're calling an async function that's nonisolated, and we're in
4137-
// an isolated context, then we're exiting the actor context.
4138-
if (mayExitToNonisolated && fnType->isAsync() &&
4139-
getContextIsolation().isActorIsolated())
4140-
unsatisfiedIsolation = ActorIsolation::forNonisolated(/*unsafe=*/false);
4136+
// If we're calling an async function that's nonisolated, and we're in an
4137+
// isolated context, then we're exiting the actor context unless we have
4138+
// nonisolated(nonsending) isolation.
4139+
//
4140+
// NOTE: We do not check fnTypeIsolation since that is the AST level
4141+
// actual isolation which does not have nonisolated(nonsending) added to
4142+
// it yet. Instead, we want the direct callee including casts since those
4143+
// casts is what would add the nonisolated(nonsending) bit to the function
4144+
// type isolation.
4145+
if (mayExitToNonisolated && fnType->isAsync()) {
4146+
if (getContextIsolation().isActorIsolated() &&
4147+
!fnTypeIsolation.isNonIsolatedCaller())
4148+
unsatisfiedIsolation =
4149+
ActorIsolation::forNonisolated(/*unsafe=*/false);
4150+
else if (getContextIsolation().isCallerIsolationInheriting() &&
4151+
fnTypeIsolation.isNonIsolated())
4152+
unsatisfiedIsolation =
4153+
ActorIsolation::forNonisolated(/*unsafe=*/false);
4154+
}
41414155

41424156
// If there was no unsatisfied actor isolation, we're done.
41434157
if (!unsatisfiedIsolation)
@@ -8005,12 +8019,14 @@ ActorReferenceResult ActorReferenceResult::forReference(
80058019
// When the declaration is not actor-isolated, it can always be accessed
80068020
// directly.
80078021
if (!declIsolation.isActorIsolated()) {
8022+
if (declRef.getDecl()->getAttrs().hasAttribute<UnsafeInheritExecutorAttr>())
8023+
return forSameConcurrencyDomain(declIsolation, options);
8024+
80088025
// If the declaration is asynchronous and we are in an actor-isolated
8009-
// context (of any kind), then we exit the actor to the nonisolated context.
8010-
if (decl->isAsync() && contextIsolation.isActorIsolated() &&
8011-
!declRef.getDecl()
8012-
->getAttrs()
8013-
.hasAttribute<UnsafeInheritExecutorAttr>())
8026+
// context (of any kind) or it is a caller isolation inheriting, then
8027+
// we exit the actor to the nonisolated context.
8028+
if (decl->isAsync() && (contextIsolation.isActorIsolated() ||
8029+
contextIsolation.isCallerIsolationInheriting()))
80148030
return forExitsActorToNonisolated(contextIsolation, options);
80158031

80168032
// Otherwise, we stay in the same concurrency domain, whether on an actor

test/ClangImporter/Inputs/regionbasedisolation.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@ NS_ASSUME_NONNULL_BEGIN
1313
(void (^)(NSArray<NSObject *> *_Nullable,
1414
NSError *_Nullable))completionHandler;
1515

16+
- (void)useValue:(id)object
17+
withCompletionHandler:(void (^)(NSObject *_Nullable))completionHandler;
18+
1619
@end
1720

1821
NS_ASSUME_NONNULL_END

test/ClangImporter/regionbasedisolation.swift

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
// RUN: %target-swift-frontend %s -import-objc-header %S/Inputs/regionbasedisolation.h -verify -c -swift-version 6
1+
// RUN: %target-swift-frontend %s -import-objc-header %S/Inputs/regionbasedisolation.h -verify -verify-additional-prefix ni- -c -swift-version 6
2+
// RUN: %target-swift-frontend %s -import-objc-header %S/Inputs/regionbasedisolation.h -verify -verify-additional-prefix ni-ns- -c -swift-version 6 -enable-upcoming-feature NonisolatedNonsendingByDefault
23
// RUN: %target-swift-frontend %s -import-objc-header %S/Inputs/regionbasedisolation.h -emit-silgen -swift-version 6 | %FileCheck %s
34

45
// REQUIRES: objc_interop
@@ -123,3 +124,39 @@ extension ObjCObject {
123124
try await loadObjects2()
124125
} // expected-error {{task or actor-isolated value cannot be sent}}
125126
}
127+
128+
@concurrent func useValueConcurrently<T>(_ t: T) async {}
129+
@MainActor func useValueMainActor<T>(_ t: T) async {}
130+
nonisolated(nonsending) func useValueNonIsolatedNonSending<T>(_ t: T) async {}
131+
132+
@MainActor func testMainActorMerging(_ y: NSObject) async {
133+
let x = ObjCObject()
134+
await x.useValue(y)
135+
await useValueConcurrently(x) // expected-error {{sending 'x' risks causing data races}}
136+
// expected-note @-1 {{sending main actor-isolated 'x' to nonisolated global function 'useValueConcurrently' risks causing data races between nonisolated and main actor-isolated uses}}
137+
}
138+
139+
func testTaskLocal(_ y: NSObject) async {
140+
let x = ObjCObject()
141+
await x.useValue(y)
142+
await useValueConcurrently(x) // expected-ni-ns-error {{sending 'x' risks causing data races}}
143+
// expected-ni-ns-note @-1 {{sending task-isolated 'x' to nonisolated global function 'useValueConcurrently' risks causing data races between nonisolated and task-isolated uses}}
144+
145+
// This is not safe since we merge x into y's region making x task
146+
// isolated. We then try to send it to a main actor function.
147+
await useValueMainActor(x) // expected-error {{sending 'x' risks causing data races}}
148+
// expected-note @-1 {{sending task-isolated 'x' to main actor-isolated global function 'useValueMainActor' risks causing data races between main actor-isolated and task-isolated uses}}
149+
}
150+
151+
actor MyActor {
152+
var field = NSObject()
153+
154+
// This is safe since x.useValue is going to be nonisolated(nonsending) so we
155+
// are going to merge x into the actor. And useValueNonIsolatedNonSending
156+
// inherits from the context.
157+
func test() async {
158+
let x = ObjCObject()
159+
await x.useValue(field)
160+
await useValueNonIsolatedNonSending(x)
161+
}
162+
}

0 commit comments

Comments
 (0)