Skip to content

Commit 06c32d7

Browse files
gottesmmgottesmm
committed
[region-isolation] Teach SIL isolation inference how to infer applies isolation from their callee's isolation.
This fixes a few issues I missed in the past bit of commits. I need to fix one issue around async let, but I am going to fix it when I do a sweep across async let.
1 parent 74ac12c commit 06c32d7

File tree

7 files changed

+165
-30
lines changed

7 files changed

+165
-30
lines changed

‎lib/SILOptimizer/Analysis/RegionAnalysis.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1830,6 +1830,10 @@ class PartitionOpTranslator {
18301830
// NOTE: We want to process indirect parameters as if they are
18311831
// parameters... so we process them in nonTransferringParameters.
18321832
for (auto &op : fas.getOperandsWithoutSelf()) {
1833+
// If op is the callee operand, skip it.
1834+
if (fas.isCalleeOperand(op))
1835+
continue;
1836+
18331837
if (!fas.getArgumentConvention(op).isIndirectOutParameter() &&
18341838
fas.getArgumentParameterInfo(op).hasOption(
18351839
SILParameterInfo::Sending)) {
@@ -2014,6 +2018,9 @@ class PartitionOpTranslator {
20142018

20152019
/// If the passed SILValue is NonSendable, then create a fresh region for it,
20162020
/// otherwise do nothing.
2021+
///
2022+
/// By default this is initialized with disconnected isolation info unless \p
2023+
/// isolationInfo is set.
20172024
void translateSILAssignFresh(SILValue val) {
20182025
return translateSILMultiAssign(TinyPtrVector<SILValue>(val),
20192026
TinyPtrVector<SILValue>());
@@ -3338,6 +3345,7 @@ RegionAnalysisValueMap::initializeTrackableValue(
33383345
return {{iter.first->first, iter.first->second}, false};
33393346
}
33403347

3348+
// If we did not insert, just return the already stored value.
33413349
self->stateIndexToEquivalenceClass[iter.first->second.getID()] = value;
33423350
iter.first->getSecond().setIsolationRegionInfo(newInfo);
33433351

‎lib/SILOptimizer/Utils/SILIsolationInfo.cpp

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -349,20 +349,27 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
349349
}
350350

351351
if (fas.hasSelfArgument()) {
352-
auto &self = fas.getSelfArgumentOperand();
353-
if (fas.getArgumentParameterInfo(self).hasOption(
352+
auto &selfOp = fas.getSelfArgumentOperand();
353+
CanType selfASTType = selfOp.get()->getType().getASTType();
354+
selfASTType =
355+
selfASTType->lookThroughAllOptionalTypes()->getCanonicalType();
356+
357+
if (fas.getArgumentParameterInfo(selfOp).hasOption(
354358
SILParameterInfo::Isolated)) {
355-
CanType astType = self.get()->getType().getASTType();
356-
if (auto *nomDecl =
357-
astType->lookThroughAllOptionalTypes()->getAnyActor()) {
359+
if (auto *nomDecl = selfASTType->getAnyActor()) {
358360
// TODO: We really should be doing this based off of an Operand. Then
359361
// we would get the SILValue() for the first element. Today this can
360362
// only mess up isolation history.
361363
return SILIsolationInfo::getActorInstanceIsolated(
362-
SILValue(), self.get(), nomDecl);
364+
SILValue(), selfOp.get(), nomDecl);
363365
}
364366
}
365367
}
368+
369+
// See if we can infer isolation from our callee.
370+
if (auto isolationInfo = get(fas.getCallee())) {
371+
return isolationInfo;
372+
}
366373
}
367374

368375
if (auto *pai = dyn_cast<PartialApplyInst>(inst)) {
@@ -456,6 +463,9 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
456463
// Treat function ref as either actor isolated or sendable.
457464
if (auto *fri = dyn_cast<FunctionRefInst>(inst)) {
458465
auto isolation = fri->getReferencedFunction()->getActorIsolation();
466+
467+
// First check if we are actor isolated at the AST level... if we are, then
468+
// create the relevant actor isolated.
459469
if (isolation.isActorIsolated()) {
460470
if (isolation.isGlobalActor()) {
461471
return SILIsolationInfo::getGlobalActorIsolated(
@@ -476,6 +486,38 @@ SILIsolationInfo SILIsolationInfo::get(SILInstruction *inst) {
476486
"Implement this!");
477487
}
478488

489+
// Then check if we have something that is nonisolated unsafe.
490+
if (isolation.isNonisolatedUnsafe()) {
491+
// First check if our function_ref is a method of a global actor isolated
492+
// type. In such a case, we create a global actor isolated
493+
// nonisolated(unsafe) so that if we assign the value to another variable,
494+
// the variable still says that it is the appropriate global actor
495+
// isolated thing.
496+
//
497+
// E.x.:
498+
//
499+
// @MainActor
500+
// struct X { nonisolated(unsafe) var x: NonSendableThing { ... } }
501+
//
502+
// We want X.x to be safe to use... but to have that 'z' in the following
503+
// is considered MainActor isolated.
504+
//
505+
// let z = X.x
506+
//
507+
auto *func = fri->getReferencedFunction();
508+
auto funcType = func->getLoweredFunctionType();
509+
auto selfParam = funcType->getSelfInstanceType(
510+
fri->getModule(), func->getTypeExpansionContext());
511+
if (auto *nomDecl = selfParam->getNominalOrBoundGenericNominal()) {
512+
auto isolation = swift::getActorIsolation(nomDecl);
513+
if (isolation.isGlobalActor()) {
514+
return SILIsolationInfo::getGlobalActorIsolated(
515+
fri, isolation.getGlobalActor())
516+
.withUnsafeNonIsolated(true);
517+
}
518+
}
519+
}
520+
479521
// Otherwise, lets look at the AST and see if our function ref is from an
480522
// autoclosure.
481523
if (auto *autoclosure = fri->getLoc().getAsASTNode<AutoClosureExpr>()) {

‎test/Concurrency/async_let_isolation.swift

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// RUN: %target-swift-frontend -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=targeted -verify-additional-prefix without-transferring-
33
// RUN: %target-swift-frontend -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -disable-region-based-isolation-with-strict-concurrency -verify-additional-prefix without-transferring- -disable-transferring-args-and-results-with-region-based-isolation -disable-sending-args-and-results-with-region-based-isolation
44
// RUN: %target-swift-frontend -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -disable-transferring-args-and-results-with-region-based-isolation -disable-sending-args-and-results-with-region-based-isolation -verify-additional-prefix without-transferring-
5-
// RUN: %target-swift-frontend -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=complete
5+
// RUN: %target-swift-frontend -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -verify-additional-prefix tns-
66

77
// REQUIRES: concurrency
88
// REQUIRES: asserts
@@ -16,13 +16,14 @@ actor MyActor {
1616

1717
func testAsyncLetIsolation() async {
1818
async let x = self.synchronous()
19-
2019
async let y = await self.asynchronous()
21-
2220
async let z = synchronous()
2321

2422
var localText = text
23+
// TODO: We should allow this since text is Sendable and localText is a
24+
// separate box. localText should be disconnected.
2525
async let w = localText.removeLast() // expected-without-transferring-warning {{mutation of captured var 'localText' in concurrently-executing code}}
26+
// expected-tns-warning @-1 {{task or actor isolated value cannot be sent}}
2627

2728
_ = await x
2829
_ = await y
@@ -31,6 +32,19 @@ actor MyActor {
3132
}
3233
}
3334

35+
final class MyFinalActor {
36+
let immutable: Int = 17
37+
var text: [String] = []
38+
39+
func testAsyncLetIsolation() async {
40+
var localText = text
41+
async let w = localText.removeLast() // expected-without-transferring-warning {{mutation of captured var 'localText' in concurrently-executing code}}
42+
43+
_ = await w
44+
45+
}
46+
}
47+
3448
func outside() async {
3549
let a = MyActor()
3650
async let x = a.synchronous() // okay, await is implicit

‎test/Concurrency/sendable_checking.swift

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -237,11 +237,20 @@ func testConversionsAndSendable(a: MyActor, s: any Sendable, f: @Sendable () ->
237237
await a.g(f)
238238
}
239239

240+
@available(SwiftStdlib 5.1, *)
241+
actor CustomActorInstance {}
242+
243+
@available(SwiftStdlib 5.1, *)
244+
@globalActor
245+
struct CustomActor {
246+
static let shared = CustomActorInstance()
247+
}
248+
240249
@available(SwiftStdlib 5.1, *)
241250
final class NonSendable {
242-
// expected-note @-1 4 {{class 'NonSendable' does not conform to the 'Sendable' protocol}}
251+
// expected-note @-1 5 {{class 'NonSendable' does not conform to the 'Sendable' protocol}}
243252
// TransferNonSendable emits 3 fewer errors here.
244-
// expected-targeted-and-complete-note @-3 5 {{class 'NonSendable' does not conform to the 'Sendable' protocol}}
253+
// expected-targeted-and-complete-note @-3 7 {{class 'NonSendable' does not conform to the 'Sendable' protocol}}
245254
// expected-complete-and-tns-note @-4 {{class 'NonSendable' does not conform to the 'Sendable' protocol}}
246255
var value = ""
247256

@@ -270,19 +279,52 @@ final class NonSendable {
270279

271280
@MainActor
272281
var x: Int { 0 }
282+
283+
@CustomActor
284+
var y: Int { 0 }
285+
286+
var z: Int { 0 }
273287
}
274288

289+
// This is not an error since t.update and t.x are both main actor isolated. We
290+
// still get the returning main actor-isolated property 'x' error though.
275291
@available(SwiftStdlib 5.1, *)
276292
func testNonSendableBaseArg() async {
277293
let t = NonSendable()
278294
await t.update()
279295
// expected-targeted-and-complete-warning @-1 {{passing argument of non-sendable type 'NonSendable' into main actor-isolated context may introduce data races}}
280-
// expected-tns-warning @-2 {{sending 't' risks causing data races}}
281-
// expected-tns-note @-3 {{sending 't' to main actor-isolated instance method 'update()' risks causing data races between main actor-isolated and local nonisolated uses}}
282296

283297
_ = await t.x
284298
// expected-warning @-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to main actor-isolated property 'x' cannot cross actor boundary}}
285-
// expected-tns-note@-2 {{access can happen concurrently}}
299+
}
300+
301+
// We get the region isolation error here since t.y is custom actor isolated.
302+
@available(SwiftStdlib 5.1, *)
303+
func testNonSendableBaseArg2() async {
304+
let t = NonSendable()
305+
await t.update()
306+
// expected-targeted-and-complete-warning @-1 {{passing argument of non-sendable type 'NonSendable' into main actor-isolated context may introduce data races}}
307+
// expected-tns-warning @-2 {{sending 't' risks causing data races}}
308+
// TODO: Improve the diagnostic so that we say custom actor isolated instead since t.y
309+
// is custom actor isolated.
310+
// expected-tns-note @-5 {{sending 't' to main actor-isolated instance method 'update()' risks causing data races between main actor-isolated and local nonisolated uses}}
311+
312+
_ = await t.y
313+
// expected-warning @-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to global actor 'CustomActor'-isolated property 'y' cannot cross actor boundary}}
314+
// expected-tns-note @-2 {{access can happen concurrently}}
315+
}
316+
317+
// We get the region isolation error here since t.z is not isolated.
318+
@available(SwiftStdlib 5.1, *)
319+
func testNonSendableBaseArg3() async {
320+
let t = NonSendable()
321+
await t.update()
322+
// expected-targeted-and-complete-warning @-1 {{passing argument of non-sendable type 'NonSendable' into main actor-isolated context may introduce data races}}
323+
// expected-tns-warning @-2 {{sending 't' risks causing data races}}
324+
// expected-tns-note @-3 {{sending 't' to main actor-isolated instance method 'update()' risks causing data races between main actor-isolated and local nonisolated uses}}
325+
326+
_ = t.z
327+
// expected-tns-note @-1 {{access can happen concurrently}}
286328
}
287329

288330
@available(SwiftStdlib 5.1, *)

‎test/Concurrency/silisolationinfo_inference.sil

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ bb0:
121121
destroy_addr %0 : $*NonSendableKlass
122122

123123
// Then do it indirect. The write before should have inhibited the inference.
124-
%f2 = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
124+
%f2 = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
125125
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f2(%0) : $@convention(thin) @async () -> @out NonSendableKlass
126126
destroy_addr %0 : $*NonSendableKlass
127127
dealloc_stack %0 : $*NonSendableKlass
@@ -140,7 +140,7 @@ bb0:
140140
%0 = alloc_stack $NonSendableKlass
141141
debug_value [trace] %0 : $*NonSendableKlass
142142

143-
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
143+
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
144144
apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %f(%0) : $@convention(thin) @async () -> @out NonSendableKlass
145145
destroy_addr %0 : $*NonSendableKlass
146146

@@ -162,7 +162,7 @@ bb0:
162162
%0 = alloc_stack $NonSendableKlass
163163
debug_value [trace] %0 : $*NonSendableKlass
164164

165-
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
165+
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
166166
apply [caller_isolation=nonisolated] [callee_isolation=actor_instance] %f(%0) : $@convention(thin) @async () -> @out NonSendableKlass
167167
destroy_addr %0 : $*NonSendableKlass
168168

@@ -193,7 +193,7 @@ bb2:
193193
br bb3
194194

195195
bb3:
196-
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
196+
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
197197
apply [caller_isolation=nonisolated] [callee_isolation=actor_instance] %f(%0) : $@convention(thin) @async () -> @out NonSendableKlass
198198
destroy_addr %0 : $*NonSendableKlass
199199

@@ -227,7 +227,7 @@ bb2:
227227
br bb3
228228

229229
bb3:
230-
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
230+
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
231231
apply [caller_isolation=nonisolated] [callee_isolation=actor_instance] %f(%0) : $@convention(thin) @async () -> @out NonSendableKlass
232232
destroy_addr %0 : $*NonSendableKlass
233233

@@ -261,7 +261,7 @@ bb2:
261261
br bb3
262262

263263
bb3:
264-
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
264+
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
265265
apply [caller_isolation=nonisolated] [callee_isolation=actor_instance] %f(%0) : $@convention(thin) @async () -> @out NonSendableKlass
266266
destroy_addr %0 : $*NonSendableKlass
267267

@@ -297,7 +297,7 @@ bb2:
297297
br bb3
298298

299299
bb3:
300-
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
300+
%f = function_ref @constructNonSendableKlassIndirectAsync : $@convention(thin) @async () -> @out NonSendableKlass
301301
apply [caller_isolation=nonisolated] [callee_isolation=actor_instance] %f(%0) : $@convention(thin) @async () -> @out NonSendableKlass
302302
destroy_addr %0 : $*NonSendableKlass
303303

@@ -699,4 +699,3 @@ bb7:
699699
%9999 = tuple ()
700700
return %9999 : $()
701701
}
702-

‎test/Concurrency/transfernonsendable_nonisolatedunsafe.swift

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
// MARK: Declarations //
1212
////////////////////////
1313

14-
class NonSendableKlass { // expected-complete-note 96{{}}
14+
class NonSendableKlass { // expected-complete-note 98{{}}
1515
var field: NonSendableKlass? = nil
1616
}
1717

@@ -588,6 +588,8 @@ enum NonIsolatedUnsafeComputedEnum: Sendable {
588588
@CustomActor struct CustomActorNonIsolatedUnsafeFieldAddressOnlyStruct<T> {
589589
nonisolated(unsafe) let nonIsolatedUnsafeLetObject = NonSendableKlass()
590590
nonisolated(unsafe) var nonIsolatedUnsafeVarObject = NonSendableKlass()
591+
nonisolated(unsafe) var nonIsolatedUnsafeVarComputedObject: NonSendableKlass { NonSendableKlass() }
592+
591593
var t: T? = nil
592594

593595
func test() async {
@@ -606,6 +608,19 @@ enum NonIsolatedUnsafeComputedEnum: Sendable {
606608
// expected-tns-warning @-1 {{sending 'x' risks causing data races}}
607609
// expected-tns-note @-2 {{sending global actor 'CustomActor'-isolated 'x' to main actor-isolated global function 'transferToMainDirect' risks causing data races between main actor-isolated and global actor 'CustomActor'-isolated uses}}
608610
// expected-complete-warning @-3 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
611+
612+
let x2 = nonIsolatedUnsafeVarObject
613+
await transferToMainDirect(x2)
614+
// expected-tns-warning @-1 {{sending 'x2' risks causing data races}}
615+
// expected-tns-note @-2 {{sending global actor 'CustomActor'-isolated 'x2' to main actor-isolated global function 'transferToMainDirect' risks causing data races between main actor-isolated and global actor 'CustomActor'-isolated uses}}
616+
// expected-complete-warning @-3 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
617+
618+
let x3 = nonIsolatedUnsafeVarComputedObject
619+
await transferToMainDirect(x3)
620+
// expected-tns-warning @-1 {{sending 'x3' risks causing data races}}
621+
// expected-tns-note @-2 {{sending global actor 'CustomActor'-isolated 'x3' to main actor-isolated global function 'transferToMainDirect' risks causing data races between main actor-isolated and global actor 'CustomActor'-isolated uses}}
622+
// expected-complete-warning @-3 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
623+
609624
print(x)
610625
}
611626
}
@@ -627,15 +642,17 @@ enum NonIsolatedUnsafeComputedEnum: Sendable {
627642
let x = nonIsolatedUnsafeVarObject
628643
await transferToMainDirect(x)
629644
// expected-tns-warning @-1 {{sending 'x' risks causing data races}}
630-
// expected-tns-note @-2 {{sending 'x' to main actor-isolated global function 'transferToMainDirect' risks causing data races between main actor-isolated and local global actor 'CustomActor'-isolated uses}}
645+
// expected-tns-note @-2 {{sending global actor 'CustomActor'-isolated 'x' to main actor-isolated global function 'transferToMainDirect' risks causing data races between main actor-isolated and global actor 'CustomActor'-isolated uses}}
631646
// expected-complete-warning @-3 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
632-
print(x) // expected-tns-note {{access can happen concurrently}}
647+
print(x)
633648
}
634649
}
635650

636651
struct NonIsolatedUnsafeFieldNonSendableStruct {
637652
nonisolated(unsafe) let nonIsolatedUnsafeLetObject = NonSendableKlass()
638653
nonisolated(unsafe) var nonIsolatedUnsafeVarObject = NonSendableKlass()
654+
nonisolated(unsafe) var nonIsolatedUnsafeVarComputedObject: NonSendableKlass { NonSendableKlass() }
655+
639656
let letObject = NonSendableKlass()
640657
var varObject = NonSendableKlass()
641658

0 commit comments

Comments
 (0)