Skip to content

Commit c3d4458

Browse files
committed
[region-isolation] Fix an off by one error when mapping AST capture indices to SIL level parameter indices.
This problem comes up with the following example: ```swift class A { var description = "" } class B { let a = A() func b() { let asdf = "" Task { @mainactor in a.description = asdf // Sending 'asdf' risks causing data races } } } ``` The specific issue is that the closure we generate actually includes an implicit(any) parameter at the SIL level which occurs after the callee operand but before the captures. This caused the captured variable index from the AST and the one we compute from the partial_apply to differ by 1. So we need to subtract 1 in such a case. That is why we used to print 'asdf' instead of 'a' above. DISCUSSION: This shows an interesting difference between SIL applied arg indices and AST indices. SIL applied arg indices would include the implicit(any) parameter since it is a parameter in the SIL function type. In contrast, this doesn't show up in the formal AST parameters or captures. To make it easier to reason about this, I added a new API to ApplySite called ApplySite::getASTAppliedArgIndex and added large comments to getASTAppliedArgIndex and getAppliedArgIndex that explains the issue. rdar://136593706 #76648
1 parent e3d53dc commit c3d4458

File tree

5 files changed

+64
-10
lines changed

5 files changed

+64
-10
lines changed

include/swift/AST/Expr.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4284,7 +4284,8 @@ class ClosureExpr : public AbstractClosureExpr {
42844284
}
42854285

42864286
/// Whether this closure should implicitly inherit the actor context from
4287-
/// where it was formed. This only affects @Sendable async closures.
4287+
/// where it was formed. This only affects @Sendable and sendable async
4288+
/// closures.
42884289
bool inheritsActorContext() const {
42894290
return Bits.ClosureExpr.InheritActorContext;
42904291
}

include/swift/AST/Types.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4416,6 +4416,10 @@ class SILParameterInfo {
44164416
Sending = 0x2,
44174417

44184418
/// Set if the given parameter is isolated.
4419+
///
4420+
/// This means that the value provides the functions isolation. This implies
4421+
/// that the parameter must be an Optional actor or something that conforms
4422+
/// to AnyActor.
44194423
Isolated = 0x4,
44204424
};
44214425

include/swift/SIL/ApplySite.h

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,13 +338,45 @@ class ApplySite {
338338
}
339339

340340
/// Return the applied argument index for the given operand.
341+
///
342+
/// This corresponds to the index of the operand in the actual
343+
/// SILFunctionType.
344+
///
345+
/// As a result this does not include indirect parameters, but it does include
346+
/// implicit parameters (e.x.: the implicit isolated(any) parameter) that are
347+
/// in the SIL type but are not in the AST type. To ignore such implicit
348+
/// parameters, use getASTAppliedArgIndex.
341349
unsigned getAppliedArgIndex(const Operand &oper) const {
342350
assert(oper.getUser() == Inst);
343351
assert(isArgumentOperand(oper));
344352

345353
return oper.getOperandNumber() - getOperandIndexOfFirstArgument();
346354
}
347355

356+
/// Returns the applied AST argument index for the given operand.
357+
///
358+
/// This is guaranteed to return an index that can be used with AST function
359+
/// types. This means it looks past the callee, indirect function types, as
360+
/// well as any other implicit parameters like isolated(any).
361+
///
362+
/// NOTE: If we add more implicit parameters to SILFunction types that do not
363+
/// appear at the AST level, this code needs to be updated.
364+
unsigned getASTAppliedArgIndex(const Operand &oper) const {
365+
// We rely on the assertions in getAppliedArgIndex to allow for us to assume
366+
// that we have an argument operand. We check later that we do not have an
367+
// isolated(any) parameter.
368+
unsigned appliedArgIndex = getAppliedArgIndex(oper);
369+
if (auto *pai = dyn_cast<PartialApplyInst>(Inst)) {
370+
if (pai->getFunctionType()->getIsolation() ==
371+
SILFunctionTypeIsolation::Erased) {
372+
assert(appliedArgIndex != 0 &&
373+
"isolation(any) does not correspond to an AST argument");
374+
appliedArgIndex -= 1;
375+
}
376+
}
377+
return appliedArgIndex;
378+
}
379+
348380
/// Return the callee's function argument index corresponding to the first
349381
/// applied argument: 0 for full applies; >= 0 for partial applies.
350382
unsigned getCalleeArgIndexOfFirstAppliedArg() const {

lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -891,7 +891,7 @@ bool UseAfterTransferDiagnosticInferrer::initForIsolatedPartialApply(
891891
if (foundCapturedIsolationCrossing.empty())
892892
return false;
893893

894-
unsigned opIndex = ApplySite(op->getUser()).getAppliedArgIndex(*op);
894+
unsigned opIndex = ApplySite(op->getUser()).getASTAppliedArgIndex(*op);
895895
bool emittedDiagnostic = false;
896896
for (auto &p : foundCapturedIsolationCrossing) {
897897
if (std::get<1>(p) != opIndex)
@@ -1131,7 +1131,7 @@ void UseAfterTransferDiagnosticInferrer::infer() {
11311131

11321132
auto *i = transferOp->getUser();
11331133
auto pai = ApplySite::isa(i);
1134-
unsigned captureIndex = pai.getAppliedArgIndex(*transferOp);
1134+
unsigned captureIndex = pai.getASTAppliedArgIndex(*transferOp);
11351135

11361136
auto &state = transferringOpToStateMap.get(transferOp);
11371137
auto captureInfo =
@@ -1640,11 +1640,7 @@ class SentNeverSendableDiagnosticInferrer {
16401640
for (auto &paiOp : ApplySite(pai).getArgumentOperands()) {
16411641
if (valueMap.getTrackableValue(paiOp.get()).getRepresentative() ==
16421642
isolatedValue) {
1643-
// isolated_any causes all partial apply parameters to be shifted by 1
1644-
// due to the implicit isolated any parameter.
1645-
unsigned isIsolatedAny = pai->getFunctionType()->getIsolation() ==
1646-
SILFunctionTypeIsolation::Erased;
1647-
return ApplySite(pai).getAppliedArgIndex(paiOp) - isIsolatedAny;
1643+
return ApplySite(pai).getASTAppliedArgIndex(paiOp);
16481644
}
16491645
}
16501646

@@ -1728,7 +1724,10 @@ bool SentNeverSendableDiagnosticInferrer::initForIsolatedPartialApply(
17281724
if (foundCapturedIsolationCrossing.empty())
17291725
return false;
17301726

1731-
unsigned opIndex = ApplySite(op->getUser()).getAppliedArgIndex(*op);
1727+
// We use getASTAppliedArgIndex instead of getAppliedArgIndex to ensure that
1728+
// we ignore for our indexing purposes any implicit initial parameters like
1729+
// isolated(any).
1730+
unsigned opIndex = ApplySite(op->getUser()).getASTAppliedArgIndex(*op);
17321731
for (auto &p : foundCapturedIsolationCrossing) {
17331732
if (std::get<1>(p) == opIndex) {
17341733
auto loc = RegularLocation(std::get<0>(p).getLoc());
@@ -1944,7 +1943,7 @@ bool SentNeverSendableDiagnosticInferrer::run() {
19441943
if (autoClosureExpr->getThunkKind() == AutoClosureExpr::Kind::AsyncLet) {
19451944
auto *i = op->getUser();
19461945
auto pai = ApplySite::isa(i);
1947-
unsigned captureIndex = pai.getAppliedArgIndex(*op);
1946+
unsigned captureIndex = pai.getASTAppliedArgIndex(*op);
19481947
auto captureInfo =
19491948
autoClosureExpr->getCaptureInfo().getCaptures()[captureIndex];
19501949
auto loc = RegularLocation(captureInfo.getLoc(), false /*implicit*/);

test/Concurrency/transfernonsendable.swift

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1888,3 +1888,21 @@ func nonSendableAllocBoxConsumingVar() async throws {
18881888
try await group.waitForAll()
18891889
}
18901890
}
1891+
1892+
func offByOneWithImplicitPartialApply() {
1893+
class A {
1894+
var description = ""
1895+
}
1896+
1897+
class B {
1898+
let a = A()
1899+
1900+
func b() {
1901+
let asdf = ""
1902+
Task { @MainActor in
1903+
a.description = asdf // expected-tns-warning {{sending 'self' risks causing data races}}
1904+
// expected-tns-note @-1 {{task-isolated 'self' is captured by a main actor-isolated closure. main actor-isolated uses in closure may race against later nonisolated uses}}
1905+
}
1906+
}
1907+
}
1908+
}

0 commit comments

Comments
 (0)