Skip to content

Commit f24ff98

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 (cherry picked from commit c12c99f)
1 parent 0cd265d commit f24ff98

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
@@ -447,6 +447,21 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
447447
return info;
448448
}
449449

450+
// See if our function apply site has an implicit isolated parameter. In
451+
// such a case, we know that we have a caller inheriting isolated
452+
// function. Return that this has disconnected isolation.
453+
//
454+
// DISCUSSION: The reason why we are doing this is that we already know that
455+
// the AST is not going to label this as an isolation crossing point so we
456+
// will only perform a merge. We want to just perform an isolation merge
457+
// without adding additional isolation info. Otherwise, a nonisolated
458+
// function that
459+
if (auto paramInfo = fas.getSubstCalleeType()->maybeGetIsolatedParameter();
460+
paramInfo && paramInfo->hasOption(SILParameterInfo::ImplicitLeading) &&
461+
paramInfo->hasOption(SILParameterInfo::Isolated)) {
462+
return SILIsolationInfo::getDisconnected(false /*unsafe nonisolated*/);
463+
}
464+
450465
if (auto *isolatedOp = fas.getIsolatedArgumentOperandOrNullPtr()) {
451466
// First look through ActorInstance agnostic values so we can find the
452467
// type of the actual underlying actor (e.x.: copy_value,
@@ -539,9 +554,8 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
539554
if (actorInstance) {
540555
if (auto actualIsolatedValue =
541556
ActorInstance::getForValue(actorInstance)) {
542-
// See if we have a function parameter. In that case, we want to see
543-
// if we have a function argument. In such a case, we need to use
544-
// the right parameter and the var decl.
557+
// See if we have a function parameter. In such a case, we need to
558+
// use the right parameter and the var decl.
545559
if (auto *fArg = dyn_cast<SILFunctionArgument>(
546560
actualIsolatedValue.getValue())) {
547561
if (auto info =
@@ -985,6 +999,13 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) {
985999
// handles isolated self and specifically marked isolated.
9861000
if (auto *isolatedArg = llvm::cast_or_null<SILFunctionArgument>(
9871001
fArg->getFunction()->maybeGetIsolatedArgument())) {
1002+
// See if the function is nonisolated(nonsending). In such a case, return
1003+
// task isolated.
1004+
if (auto funcIsolation = fArg->getFunction()->getActorIsolation();
1005+
funcIsolation && funcIsolation->isCallerIsolationInheriting()) {
1006+
return SILIsolationInfo::getTaskIsolated(fArg);
1007+
}
1008+
9881009
auto astType = isolatedArg->getType().getASTType();
9891010
if (astType->lookThroughAllOptionalTypes()->getAnyActor()) {
9901011
return SILIsolationInfo::getActorInstanceIsolated(fArg, isolatedArg);

lib/Sema/TypeCheckConcurrency.cpp

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

4070+
if (calleeDecl &&
4071+
calleeDecl->getAttrs().hasAttribute<UnsafeInheritExecutorAttr>())
4072+
return false;
4073+
40704074
auto fnTypeIsolation = fnType->getIsolation();
40714075
if (fnTypeIsolation.isGlobalActor()) {
40724076
// If the function type is global-actor-qualified, determine whether
@@ -4112,10 +4116,6 @@ namespace {
41124116
calleeDecl = memberRef->first.getDecl();
41134117
argForIsolatedParam = selfApplyFn->getBase();
41144118
}
4115-
} else if (calleeDecl &&
4116-
calleeDecl->getAttrs()
4117-
.hasAttribute<UnsafeInheritExecutorAttr>()) {
4118-
return false;
41194119
}
41204120

41214121
// Check for isolated parameters.
@@ -4166,11 +4166,25 @@ namespace {
41664166
break;
41674167
}
41684168

4169-
// If we're calling an async function that's nonisolated, and we're in
4170-
// an isolated context, then we're exiting the actor context.
4171-
if (mayExitToNonisolated && fnType->isAsync() &&
4172-
getContextIsolation().isActorIsolated())
4173-
unsatisfiedIsolation = ActorIsolation::forNonisolated(/*unsafe=*/false);
4169+
// If we're calling an async function that's nonisolated, and we're in an
4170+
// isolated context, then we're exiting the actor context unless we have
4171+
// nonisolated(nonsending) isolation.
4172+
//
4173+
// NOTE: We do not check fnTypeIsolation since that is the AST level
4174+
// actual isolation which does not have nonisolated(nonsending) added to
4175+
// it yet. Instead, we want the direct callee including casts since those
4176+
// casts is what would add the nonisolated(nonsending) bit to the function
4177+
// type isolation.
4178+
if (mayExitToNonisolated && fnType->isAsync()) {
4179+
if (getContextIsolation().isActorIsolated() &&
4180+
!fnTypeIsolation.isNonIsolatedCaller())
4181+
unsatisfiedIsolation =
4182+
ActorIsolation::forNonisolated(/*unsafe=*/false);
4183+
else if (getContextIsolation().isCallerIsolationInheriting() &&
4184+
fnTypeIsolation.isNonIsolated())
4185+
unsatisfiedIsolation =
4186+
ActorIsolation::forNonisolated(/*unsafe=*/false);
4187+
}
41744188

41754189
// If there was no unsatisfied actor isolation, we're done.
41764190
if (!unsatisfiedIsolation)
@@ -8129,12 +8143,14 @@ ActorReferenceResult ActorReferenceResult::forReference(
81298143
// When the declaration is not actor-isolated, it can always be accessed
81308144
// directly.
81318145
if (!declIsolation.isActorIsolated()) {
8146+
if (declRef.getDecl()->getAttrs().hasAttribute<UnsafeInheritExecutorAttr>())
8147+
return forSameConcurrencyDomain(declIsolation, options);
8148+
81328149
// If the declaration is asynchronous and we are in an actor-isolated
8133-
// context (of any kind), then we exit the actor to the nonisolated context.
8134-
if (decl->isAsync() && contextIsolation.isActorIsolated() &&
8135-
!declRef.getDecl()
8136-
->getAttrs()
8137-
.hasAttribute<UnsafeInheritExecutorAttr>())
8150+
// context (of any kind) or it is a caller isolation inheriting, then
8151+
// we exit the actor to the nonisolated context.
8152+
if (decl->isAsync() && (contextIsolation.isActorIsolated() ||
8153+
contextIsolation.isCallerIsolationInheriting()))
81388154
return forExitsActorToNonisolated(contextIsolation, options);
81398155

81408156
// 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)