Skip to content

[region-isolation] Assigning into a transferring parameter should be a store, not a transfer. #71999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions include/swift/AST/DiagnosticsSIL.def
Original file line number Diff line number Diff line change
Expand Up @@ -950,9 +950,6 @@ ERROR(regionbasedisolation_transfer_yields_race_with_isolation, none,
ERROR(regionbasedisolation_isolated_capture_yields_race, none,
"%1 closure captures value of non-Sendable type %0 from %2 context; later accesses to value could race",
(Type, ActorIsolation, ActorIsolation))
ERROR(regionbasedisolation_transfer_yields_race_transferring_parameter, none,
"transferring value of non-Sendable type %0 into transferring parameter; later accesses could race",
(Type))
ERROR(regionbasedisolation_transfer_yields_race_stronglytransferred_binding, none,
"binding of non-Sendable type %0 accessed after being transferred; later accesses could race",
(Type))
Expand Down
10 changes: 0 additions & 10 deletions lib/SILOptimizer/Analysis/RegionAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1986,16 +1986,6 @@ class PartitionOpTranslator {
SILValue srcValue = src->get();

if (auto nonSendableDest = tryToTrackValue(destValue)) {
// Before we do anything check if we have an assignment into an
// alloc_stack for a consuming transferring parameter... in such a case,
// we need to handle this specially.
if (nonSendableDest->isTransferringParameter()) {
if (auto nonSendableSrc = tryToTrackValue(srcValue)) {
return translateSILAssignmentToTransferringParameter(
*nonSendableDest, dest, *nonSendableSrc, src);
}
}

// In the following situations, we can perform an assign:
//
// 1. A store to unaliased storage.
Expand Down
108 changes: 5 additions & 103 deletions lib/SILOptimizer/Mandatory/TransferNonSendable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,6 @@ class UseAfterTransferDiagnosticInferrer {
/// error though.
TypedRaceWithoutKnownIsolationCrossing = 2,

/// Used if the error is due to a transfer into an assignment into a
/// transferring parameter.
AssignmentIntoTransferringParameter = 3,

/// Set to true if this is a use of a normal value that was strongly
/// transferred.
UseOfStronglyTransferredValue = 4,
Expand Down Expand Up @@ -501,14 +497,6 @@ class UseAfterTransferDiagnosticInferrer {
{}, inferredType, SILLocation::invalid());
}

static UseDiagnosticInfo
forTypedAssignmentIntoTransferringParameter(SILLocation loc,
Type inferredType) {
return UseDiagnosticInfo(
UseDiagnosticInfoKind::AssignmentIntoTransferringParameter, loc, {},
inferredType, SILLocation::invalid());
}

static UseDiagnosticInfo
forTypedUseOfStronglyTransferredValue(SILLocation loc, Type inferredType) {
return UseDiagnosticInfo(
Expand Down Expand Up @@ -578,12 +566,6 @@ class UseAfterTransferDiagnosticInferrer {
void initForApply(const Operand *op, ApplyExpr *expr);
void initForAutoclosure(const Operand *op, AutoClosureExpr *expr);

void initForAssignmentToTransferringParameter(const Operand *op) {
appendUseInfo(
UseDiagnosticInfo::forTypedAssignmentIntoTransferringParameter(
baseLoc, op->get()->getType().getASTType()));
}

void initForUseOfStronglyTransferredValue(const Operand *op) {
appendUseInfo(UseDiagnosticInfo::forTypedUseOfStronglyTransferredValue(
baseLoc, op->get()->getType().getASTType()));
Expand Down Expand Up @@ -726,31 +708,11 @@ struct UseAfterTransferDiagnosticInferrer::Walker : ASTWalker {
}
};

static SILValue getDestOfStoreOrCopyAddr(Operand *op) {
if (auto *si = dyn_cast<StoreInst>(op->getUser()))
return si->getDest();

if (auto *copyAddr = dyn_cast<CopyAddrInst>(op->getUser()))
return copyAddr->getDest();

return SILValue();
}

void UseAfterTransferDiagnosticInferrer::init(const Operand *op) {
baseLoc = op->getUser()->getLoc();
baseInferredType = op->get()->getType().getASTType();
auto *nonConstOp = const_cast<Operand *>(op);

// Before we do anything, see if the transfer instruction was into a
// transferring parameter alloc_stack. In such a case, we emit a special
// message.
if (auto destValue = getDestOfStoreOrCopyAddr(nonConstOp)) {
auto trackedValue = valueMap.getTrackableValue(destValue);
if (trackedValue.isTransferringParameter()) {
return initForAssignmentToTransferringParameter(op);
}
}

// Otherwise, see if our operand's instruction is a transferring parameter.
if (auto fas = FullApplySite::isa(nonConstOp->getUser())) {
assert(!fas.getArgumentConvention(*nonConstOp).isIndirectOutParameter() &&
Expand Down Expand Up @@ -831,8 +793,8 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
if (transferOpToRequireInstMultiMap.empty())
return;

LLVM_DEBUG(llvm::dbgs()
<< "Visiting found transfer+[requireInsts] for diagnostics.\n");
LLVM_DEBUG(llvm::dbgs() << "Emitting use after transfer diagnostics.\n");

auto &astContext = regionInfo->getFunction()->getASTContext();
for (auto [transferOp, requireInsts] :
transferOpToRequireInstMultiMap.getRange()) {
Expand Down Expand Up @@ -940,14 +902,6 @@ void TransferNonSendableImpl::emitUseAfterTransferDiagnostics() {
info.getInferredType())
.highlight(info.getLoc().getSourceRange());
break;
case UseDiagnosticInfoKind::AssignmentIntoTransferringParameter:
diagnoseError(
astContext, info.getLoc(),
diag::
regionbasedisolation_transfer_yields_race_transferring_parameter,
info.getInferredType())
.highlight(info.getLoc().getSourceRange());
break;
case UseDiagnosticInfoKind::TypedIsolationCrossingDueToCapture:
auto isolation = info.getIsolationCrossing();
diagnoseError(astContext, info.getLoc(),
Expand Down Expand Up @@ -995,10 +949,6 @@ class TransferNonTransferrableDiagnosticInferrer {

/// Used if we have a named variable.
NamedIsolation = 5,

/// Used if a non-transferring function argument is assigned to a
/// transferring function argument.
FunctionArgumentAssignedToStronglyTransferredParam = 6,
};

class UseDiagnosticInfo {
Expand Down Expand Up @@ -1047,16 +997,6 @@ class TransferNonTransferrableDiagnosticInferrer {
inferredType};
}

static UseDiagnosticInfo
forFunctionArgumentAssignedToStronglyTransferredParam(
Identifier funcArgName, Identifier transferredParam) {
return {UseDiagnosticInfoKind::
FunctionArgumentAssignedToStronglyTransferredParam,
{},
funcArgName,
transferredParam};
}

private:
UseDiagnosticInfo(
UseDiagnosticInfoKind kind,
Expand All @@ -1069,15 +1009,14 @@ class TransferNonTransferrableDiagnosticInferrer {
};

private:
RegionAnalysisValueMap &valueMap;
TransferredNonTransferrableInfo info;
std::optional<UseDiagnosticInfo> diagnosticInfo;
SourceLoc loc;

public:
TransferNonTransferrableDiagnosticInferrer(
RegionAnalysisValueMap &valueMap, TransferredNonTransferrableInfo info)
: valueMap(valueMap), info(info),
TransferredNonTransferrableInfo info)
: info(info),
loc(info.transferredOperand->getUser()->getLoc().getSourceLoc()) {}

/// Gathers diagnostics. Returns false if we emitted a "I don't understand
Expand Down Expand Up @@ -1122,28 +1061,6 @@ bool TransferNonTransferrableDiagnosticInferrer::run() {
auto *op = info.transferredOperand;
auto loc = info.transferredOperand->getUser()->getLoc();

// Before we do anything, see if the transfer instruction was into a
// transferring parameter alloc_stack. In such a case, we emit a special
// message.
if (auto destValue = getDestOfStoreOrCopyAddr(op)) {
auto trackedValue = valueMap.getTrackableValue(destValue);
if (trackedValue.isTransferringParameter()) {
auto valueName = inferNameFromValue(op->get());
auto paramName =
inferNameFromValue(trackedValue.getRepresentative().getValue());
if (!valueName || !paramName) {
diagnoseError(op->getUser(),
diag::regionbasedisolation_unknown_pattern);
return false;
}

diagnosticInfo = UseDiagnosticInfo::
forFunctionArgumentAssignedToStronglyTransferredParam(*valueName,
*paramName);
return true;
}
}

if (auto *sourceApply = loc.getAsASTNode<ApplyExpr>()) {
std::optional<ApplyIsolationCrossing> isolation = {};

Expand Down Expand Up @@ -1233,8 +1150,7 @@ void TransferNonSendableImpl::emitTransferredNonTransferrableDiagnostics() {
auto &astContext = regionInfo->getFunction()->getASTContext();
for (auto info : transferredNonTransferrable) {
auto *op = info.transferredOperand;
TransferNonTransferrableDiagnosticInferrer diagnosticInferrer(
regionInfo->getValueMap(), info);
TransferNonTransferrableDiagnosticInferrer diagnosticInferrer(info);
if (!diagnosticInferrer.run())
continue;

Expand Down Expand Up @@ -1306,20 +1222,6 @@ void TransferNonSendableImpl::emitTransferredNonTransferrableDiagnostics() {
diagnosticInfo.getIsolationCrossing().getCallerIsolation(),
diagnosticInfo.getIsolationCrossing().getCalleeIsolation());
break;
case UseDiagnosticInfoKind::
FunctionArgumentAssignedToStronglyTransferredParam:
diagnoseError(
astContext, loc,
diag::
regionbasedisolation_stronglytransfer_assignment_yields_race_name,
diagnosticInfo.getName(),
diagnosticInfo.getTransferredParamIdentifier());
diagnoseNote(
astContext, loc,
diag::regionbasedisolation_stronglytransfer_taskisolated_assign_note,
diagnosticInfo.getName(),
diagnosticInfo.getTransferredParamIdentifier());
break;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,47 +195,58 @@ func canTransferAssigningIntoLocal2(_ x: transferring Klass) async {
// MARK: Transferring is "var" like //
//////////////////////////////////////

// Assigning into a transferring parameter is a transfer!
func assigningIsATransfer(_ x: transferring Klass) async {
// Ok, this is disconnected.
// Assigning into a transferring parameter is a merge.
func assigningIsAMerge(_ x: transferring Klass) async {
let y = Klass()

// y is transferred into x.
x = y // expected-warning {{transferring value of non-Sendable type 'Klass' into transferring parameter; later accesses could race}}
x = y

// Since y is now in x, we can't use y anymore so we emit an error.
useValue(y) // expected-note {{access here could race}}
// We can still transfer y since x is disconnected.
await transferToMain(y)
}

func assigningIsATransferNoError(_ x: transferring Klass) async {
// Ok, this is disconnected.
func assigningIsAMergeError(_ x: transferring Klass) async {
let y = Klass()

// y is transferred into x.
x = y

useValue(x)
// We can still transfer y since x is disconnected.
await transferToMain(y) // expected-warning {{transferring value of non-Sendable type 'Klass' from nonisolated context to main actor-isolated context}}

useValue(x) // expected-note {{access here could race}}
}

func assigningIsATransferAny(_ x: transferring Any) async {
func assigningIsAMergeAny(_ x: transferring Any) async {
// Ok, this is disconnected.
let y = getAny()

// y is transferred into x.
x = y // expected-warning {{transferring value of non-Sendable type 'Any' into transferring parameter; later accesses could race}}
x = y

await transferToMain(y)
}

func assigningIsAMergeAnyError(_ x: transferring Any) async {
// Ok, this is disconnected.
let y = getAny() // expected-note {{variable defined here}}

// Since y is now in x, we can't use y anymore so we emit an error.
useValue(y) // expected-note {{access here could race}}
x = y

await transferToMain(y) // expected-warning {{transferring 'y' may cause a race}}
// expected-note @-1 {{'y' is transferred from nonisolated caller to main actor-isolated callee}}

useValue(x) // expected-note {{access here could race}}
}

func canTransferAfterAssign(_ x: transferring Any) async {
// Ok, this is disconnected.
let y = getAny()

// y is transferred into x.
await transferToMain(x)

x = y

await transferToMain(x)
useValue(x)
}

func canTransferAfterAssignButUseIsError(_ x: transferring Any) async {
Expand Down Expand Up @@ -281,10 +292,10 @@ func mergeDoesNotEliminateEarlierTransfer(_ x: transferring NonSendableStruct) a
await transferToMain(x) // expected-warning {{transferring 'x' may cause a race}}
// expected-note @-1 {{'x' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}

// y is assigned into a field of x, so we treat this like a merge.
x.first = y
// y is assigned into a field of x.
x.first = y // expected-note {{access here could race}}

useValue(x) // expected-note {{access here could race}}
useValue(x)
}

func mergeDoesNotEliminateEarlierTransfer2(_ x: transferring NonSendableStruct) async {
Expand All @@ -299,12 +310,7 @@ func mergeDoesNotEliminateEarlierTransfer2(_ x: transferring NonSendableStruct)
await transferToMain(x) // expected-warning {{transferring 'x' may cause a race}}
// expected-note @-1 {{'x' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}

// y is assigned into a field of x, so we treat this like a merge.
x.first = y // expected-warning {{transferring value of non-Sendable type 'Klass' into transferring parameter; later accesses could race}}

useValue(x) // expected-note {{access here could race}}

useValue(y) // expected-note {{access here could race}}
x.first = y // expected-note {{access here could race}}
}

func doubleArgument() async {
Expand All @@ -320,34 +326,25 @@ func testTransferSrc(_ x: transferring Klass) async {
}

func testTransferOtherParam(_ x: transferring Klass, y: Klass) async {
x = y // expected-warning {{assigning 'y' to transferring parameter 'x' may cause a race}}
// expected-note @-1 {{'y' is a task isolated value that is assigned into transferring parameter 'x'. Transferred uses of 'x' may race with caller uses of 'y'}}
x = y
}

func testTransferOtherParamTuple(_ x: transferring Klass, y: (Klass, Klass)) async {
x = y.0 // expected-warning {{assigning 'y.0' to transferring parameter 'x' may cause a race}}
// expected-note @-1 {{'y.0' is a task isolated value that is assigned into transferring parameter 'x'. Transferred uses of 'x' may race with caller uses of 'y.0'}}
}

func testTransferOtherParamStruct(_ x: transferring Klass, y: NonSendableStruct) async {
x = y.first // expected-warning {{assigning 'y.first' to transferring parameter 'x' may cause a race}}
// expected-note @-1 {{'y.first' is a task isolated value that is assigned into transferring parameter 'x'. Transferred uses of 'x' may race with caller uses of 'y.first'}}
}

func testTransferOtherParamTupleStruct(_ x: transferring Klass, y: (NonSendableStruct, NonSendableStruct)) async {
x = y.1.second // expected-warning {{assigning 'y.1.second' to transferring parameter 'x' may cause a race}}
// expected-note @-1 {{'y.1.second' is a task isolated value that is assigned into transferring parameter 'x'. Transferred uses of 'x' may race with caller uses of 'y.1.second'}}
}

func testTransferOtherParamClassStructTuple(_ x: transferring Klass, y: KlassWithNonSendableStructPair) async {
x = y.ns1.second // expected-warning {{assigning 'y.ns1.second' to transferring parameter 'x' may cause a race}}
// expected-note @-1 {{'y.ns1.second' is a task isolated value that is assigned into transferring parameter 'x'}}
x = y.ns2.1.second // expected-warning {{assigning 'y.ns2.1.second' to transferring parameter 'x' may cause a race}}
// expected-note @-1 {{'y.ns2.1.second' is a task isolated value that is assigned into transferring parameter 'x'. Transferred uses of 'x' may race with caller uses of 'y.ns2.1.second'}}
x = y.0
}

func useSugaredTypeNameWhenEmittingTaskIsolationError(_ x: @escaping @MainActor () async -> ()) {
func fakeInit(operation: transferring @escaping () async -> ()) {}

fakeInit(operation: x) // expected-warning {{task isolated value of type '@MainActor () async -> ()' passed as a strongly transferred parameter}}
}

// Make sure we error here on only the second since x by being assigned a part
// of y becomes task isolated
func testMergeWithTaskIsolated(_ x: transferring Klass, y: Klass) async {
await transferToMain(x)
x = y
// TODO: We need to say that this is task isolated.
await transferToMain(x) // expected-warning {{transferring 'x' may cause a race}}
// expected-note @-1 {{transferring nonisolated 'x' to main actor-isolated callee could cause races between main actor-isolated and nonisolated uses}}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ func testPassArgumentAsTransferringParameter(_ x: NonSendableType) async {

func testAssignmentIntoTransferringParameter(_ x: transferring NonSendableType) async {
let y = NonSendableType()
x = y // expected-error {{transferring value of non-Sendable type 'NonSendableType' into transferring parameter; later accesses could race}}
useValue(y) // expected-note {{access here could race}}
await transferToMain(x)
x = y
useValue(y)
}

func testAssigningParameterIntoTransferringParameter(_ x: transferring NonSendableType, _ y: NonSendableType) async {
x = y // expected-error {{assigning 'y' to transferring parameter 'x' may cause a race}}
// expected-note @-1 {{'y' is a task isolated value that is assigned into transferring parameter 'x'. Transferred uses of 'x' may race with caller uses of 'y'}}
x = y
}

func testIsolationCrossingDueToCapture() async {
Expand Down