Skip to content

Commit ced96aa

Browse files
committed
[concurrency] Ensure that we treat closures that are nonisolated(nonsending) via their ActorIsolation as nonisolated(nonsending).
Some notes: 1. In most cases, I think we were getting lucky with this by just inferring the closure's isolation from its decl context. In the specific case that we were looking at here, this was not true since we are returning from an @Concurrent async function a nonisolated(nonsending) method that closes over self. This occurs since even when NonisolatedNonsendingByDefault we want to start importing objc async functions as nonisolated(nonsending). 2. I also discovered that in the ActorIsolationChecker we were not visiting the inner autoclosure meaning that we never set the ActorIsolation field on the closure. After some discussion with @xedin about potentially visiting the function in the ActorIsolationChecker, we came to the conclusion that this was likely to result in source stability changes. So we put in a targeted fix just for autoclosures in this specific case by setting their actor isolation in the type checker. 3. Beyond adding tests to objc_async_from_swift to make sure that when NonisolatedNonsendingByDefault is disabled we do the right thing, I noticed that we did not have any tests that actually tested the behavior around objc_async_from_swift when NonisolatedNonsendingByDefault is enabled. So I added the relevant test lines so we can be sure that we get correct behavior in such a case. rdar://150209093
1 parent 294e4cf commit ced96aa

File tree

5 files changed

+420
-79
lines changed

5 files changed

+420
-79
lines changed

lib/SIL/IR/SILFunctionType.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2633,6 +2633,9 @@ static CanSILFunctionType getSILFunctionType(
26332633
} else if (decl->getAttrs().hasAttribute<ConcurrentAttr>()) {
26342634
actorIsolation = ActorIsolation::forNonisolated(false /*unsafe*/);
26352635
}
2636+
} else if (auto *closure = constant->getAbstractClosureExpr()) {
2637+
if (auto isolation = closure->getActorIsolation())
2638+
actorIsolation = isolation;
26362639
}
26372640

26382641
if (!actorIsolation) {

lib/SILGen/SILGen.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,13 @@ static ActorIsolation getActorIsolationForFunction(SILFunction &fn) {
765765
return ActorIsolation::forNonisolated(false);
766766
}
767767

768+
// If we have a closure expr, check if our type is
769+
// nonisolated(nonsending). In that case, we use that instead.
770+
if (auto *closureExpr = constant.getAbstractClosureExpr()) {
771+
if (auto actorIsolation = closureExpr->getActorIsolation())
772+
return actorIsolation;
773+
}
774+
768775
// If we have actor isolation for our constant, put the isolation onto the
769776
// function. If the isolation is unspecified, we do not return it.
770777
if (auto isolation =

lib/Sema/CSApply.cpp

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1346,8 +1346,32 @@ namespace {
13461346
new (ctx) AutoClosureExpr(/*set body later*/ nullptr, thunkTy, dc);
13471347
thunk->setParameterList(thunkParamList);
13481348
thunk->setThunkKind(AutoClosureExpr::Kind::SingleCurryThunk);
1349+
1350+
// TODO: We should do this in the ActorIsolation checker but for now it is
1351+
// too risky. This is a narrow fix for 6.2.
1352+
//
1353+
// The problem that this is working around is given an autoclosure that
1354+
// returns an autoclosure that partially applies over an instance method,
1355+
// we do not visit the inner autoclosure in the ActorIsolation checker
1356+
// meaning that we do not properly call setActorIsolation on the
1357+
// AbstractClosureExpr that we produce here.
1358+
switch (thunkTy->getIsolation().getKind()) {
1359+
case FunctionTypeIsolation::Kind::NonIsolated:
1360+
case FunctionTypeIsolation::Kind::Parameter:
1361+
case FunctionTypeIsolation::Kind::Erased:
1362+
break;
1363+
case FunctionTypeIsolation::Kind::GlobalActor:
1364+
thunk->setActorIsolation(ActorIsolation::forGlobalActor(
1365+
thunkTy->getIsolation().getGlobalActorType()));
1366+
break;
1367+
case FunctionTypeIsolation::Kind::NonIsolatedCaller:
1368+
thunk->setActorIsolation(
1369+
ActorIsolation::forCallerIsolationInheriting());
1370+
break;
1371+
}
1372+
13491373
cs.cacheType(thunk);
1350-
1374+
13511375
// If the `self` type is existential, it must be opened.
13521376
OpaqueValueExpr *baseOpened = nullptr;
13531377
Expr *origBaseExpr = baseExpr;

test/Inputs/clang-importer-sdk/usr/include/ObjCConcurrency.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,16 @@ typedef void (^NonsendableCompletionHandler)(NSString * _Nullable, NSString * _N
123123
__attribute__((swift_async_error(zero_argument, 3)));
124124
- (void)getIceCreamFlavorWithCompletionHandler:
125125
(void (^)(Flavor flavor, NSError *__nullable error))completionHandler;
126+
127+
@property(class, strong, readonly) SlowServer *standardServer;
128+
- (void)getValueWithKey:(NSString *)valueIdentifier
129+
completion:(void (^)(NSString *__nullable value,
130+
NSError *__nullable error))completionHandler;
131+
- (void)getMainActorValueWithKey:(NSString *)valueIdentifier
132+
completion:
133+
(void (^)(NSString *__nullable value,
134+
NSError *__nullable error))completionHandler
135+
MAIN_ACTOR;
126136
@end
127137

128138
@protocol RefrigeratorDelegate<NSObject>

0 commit comments

Comments
 (0)