Skip to content

Commit 0a06c00

Browse files
committed
[region-isolation] When determining isolation from a class_method, check for global actor isolation first.
Before this change in the following code, we would say that message is isolated to the actor instead of the global actor isolation of the actor's method: ```swift class Message { ... } actor MessageHolder { @mainactor func hold(_ message: Message) { ... } } @mainactor func sendMessage() async { let messageHolder = MessageHolder() let message = Message() // We identified messageHolder.hold as being MessageHolder isolated // instead of main actor isolated. messageHolder.hold(message) Task { @mainactor in print(message) } } ``` I also used this as an opportunity to simplify the logic in this part of the code. Specifically, I had made it so that multiple interesting cases were handled in the same conditional statement in a manner that it made it hard to know which cases were actually being handled and why it was correct. Now I split that into two separate if statements with comments that make it clear what we are actually trying to pattern match against. rdar://130980933
1 parent 549566e commit 0a06c00

File tree

3 files changed

+57
-22
lines changed

3 files changed

+57
-22
lines changed

include/swift/AST/ActorIsolation.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,12 @@ class ActorIsolation {
180180
return parameterIndex;
181181
}
182182

183+
/// Returns true if this actor-instance isolation appllies to the self
184+
/// parameter of a method.
185+
bool isActorInstanceForSelfParameter() const {
186+
return getActorInstanceParameter() == 0;
187+
}
188+
183189
bool isSILParsed() const { return silParsed; }
184190

185191
bool isActorIsolated() const {

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -604,40 +604,55 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
604604
bool isNonIsolatedUnsafe = exprAnalysis.hasNonisolatedUnsafe();
605605
{
606606
auto isolation = swift::getActorIsolation(dre->getDecl());
607-
if (isolation.isActorIsolated() &&
608-
(isolation.getKind() != ActorIsolation::ActorInstance ||
609-
isolation.getActorInstanceParameter() == 0)) {
610-
if (cmi->getOperand()->getType().isAnyActor()) {
607+
608+
if (isolation.isActorIsolated()) {
609+
// Check if we have a global actor and handle it appropriately.
610+
if (isolation.getKind() == ActorIsolation::GlobalActor) {
611+
bool localNonIsolatedUnsafe =
612+
isNonIsolatedUnsafe | isolation.isNonisolatedUnsafe();
613+
return SILIsolationInfo::getGlobalActorIsolated(
614+
cmi, isolation.getGlobalActor())
615+
.withUnsafeNonIsolated(localNonIsolatedUnsafe);
616+
}
617+
618+
// In this case, we have an actor instance that is self.
619+
if (isolation.getKind() != ActorIsolation::ActorInstance &&
620+
isolation.isActorInstanceForSelfParameter()) {
621+
bool localNonIsolatedUnsafe =
622+
isNonIsolatedUnsafe | isolation.isNonisolatedUnsafe();
611623
return SILIsolationInfo::getActorInstanceIsolated(
612-
cmi, cmi->getOperand(),
613-
cmi->getOperand()
614-
->getType()
615-
.getNominalOrBoundGenericNominal());
624+
cmi, cmi->getOperand(),
625+
cmi->getOperand()
626+
->getType()
627+
.getNominalOrBoundGenericNominal())
628+
.withUnsafeNonIsolated(localNonIsolatedUnsafe);
616629
}
617-
return SILIsolationInfo::getGlobalActorIsolated(
618-
cmi, isolation.getGlobalActor());
619630
}
620-
621-
isNonIsolatedUnsafe |= isolation.isNonisolatedUnsafe();
622631
}
623632

624633
if (auto type = dre->getType()->getNominalOrBoundGenericNominal()) {
625634
if (auto isolation = swift::getActorIsolation(type)) {
626-
if (isolation.isActorIsolated() &&
627-
(isolation.getKind() != ActorIsolation::ActorInstance ||
628-
isolation.getActorInstanceParameter() == 0)) {
629-
if (cmi->getOperand()->getType().isAnyActor()) {
635+
if (isolation.isActorIsolated()) {
636+
// Check if we have a global actor and handle it appropriately.
637+
if (isolation.getKind() == ActorIsolation::GlobalActor) {
638+
bool localNonIsolatedUnsafe =
639+
isNonIsolatedUnsafe | isolation.isNonisolatedUnsafe();
640+
return SILIsolationInfo::getGlobalActorIsolated(
641+
cmi, isolation.getGlobalActor())
642+
.withUnsafeNonIsolated(localNonIsolatedUnsafe);
643+
}
644+
645+
// In this case, we have an actor instance that is self.
646+
if (isolation.getKind() != ActorIsolation::ActorInstance &&
647+
isolation.isActorInstanceForSelfParameter()) {
648+
bool localNonIsolatedUnsafe =
649+
isNonIsolatedUnsafe | isolation.isNonisolatedUnsafe();
630650
return SILIsolationInfo::getActorInstanceIsolated(
631651
cmi, cmi->getOperand(),
632652
cmi->getOperand()
633653
->getType()
634654
.getNominalOrBoundGenericNominal())
635-
.withUnsafeNonIsolated(isNonIsolatedUnsafe);
636-
}
637-
638-
if (auto globalIso = SILIsolationInfo::getGlobalActorIsolated(
639-
cmi, isolation.getGlobalActor())) {
640-
return globalIso.withUnsafeNonIsolated(isNonIsolatedUnsafe);
655+
.withUnsafeNonIsolated(localNonIsolatedUnsafe);
641656
}
642657
}
643658
}

test/Concurrency/transfernonsendable.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ actor MyActor {
4040
func useSendableFunction(_: @Sendable () -> Void) {}
4141
func useNonSendableFunction(_: () -> Void) {}
4242
func doSomething() {}
43+
@MainActor func useKlassMainActor(_ x: NonSendableKlass) {}
4344
}
4445

4546
final actor FinalActor {
@@ -1809,3 +1810,16 @@ actor FunctionWithSendableResultAndIsolationActor {
18091810
return ""
18101811
}
18111812
}
1813+
1814+
@MainActor
1815+
func testThatGlobalActorTakesPrecedenceOverActorIsolationOnMethods() async {
1816+
let a = MyActor()
1817+
let ns = NonSendableKlass()
1818+
1819+
// 'ns' should be main actor isolated since useKlassMainActor is @MainActor
1820+
// isolated. Previously we would let MyActor take precedence here...
1821+
a.useKlassMainActor(ns)
1822+
1823+
// Meaning we would get an error here.
1824+
Task { @MainActor in print(ns) }
1825+
}

0 commit comments

Comments
 (0)