Skip to content

[rbi] Remove code that caused us to misidentify certain captured parameters as sending. #82427

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
35 changes: 12 additions & 23 deletions lib/SILOptimizer/Analysis/RegionAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -520,34 +520,23 @@ static bool isAsyncLetBeginPartialApply(PartialApplyInst *pai) {

/// Returns true if this is a function argument that is able to be sent in the
/// body of our function.
///
/// Needs to stay in sync with SILIsolationInfo::get(SILArgument *).
static bool canFunctionArgumentBeSent(SILFunctionArgument *arg) {
// Indirect out parameters can never be sent.
if (arg->isIndirectResult() || arg->isIndirectErrorResult())
return false;

// If we have a function argument that is closure captured by a Sendable
// closure, allow for the argument to be sent.
//
// DISCUSSION: The reason that we do this is that in the case of us
// having an actual Sendable closure there are two cases we can see:
//
// 1. If we have an actual Sendable closure, the AST will emit an
// earlier error saying that we are capturing a non-Sendable value in a
// Sendable closure. So we want to squelch the error that we would emit
// otherwise. This only occurs when we are not in swift-6 mode since in
// swift-6 mode we will error on the earlier error... but in the case of
// us not being in swift 6 mode lets not emit extra errors.
//
// 2. If we have an async-let based Sendable closure, we want to allow
// for the argument to be sent in the async let's statement and
// not emit an error.
//
// TODO: Once the async let refactoring change this will no longer be needed
// since closure captures will have sending parameters and be
// non-Sendable.
if (arg->isClosureCapture() &&
arg->getFunction()->getLoweredFunctionType()->isSendable())
return true;
// If we have a closure capture...
if (arg->isClosureCapture()) {
// And that closure capture is from an async let, treat it as sending. This
// is because we allow for disconnected values to be sent into async let
// closures.
if (auto declRef = arg->getFunction()->getDeclRef();
declRef && declRef.isAsyncLetClosure) {
return true;
}
}

// Otherwise, we only allow for the argument to be sent if it is explicitly
// marked as a 'sending' parameter.
Expand Down
12 changes: 8 additions & 4 deletions lib/SILOptimizer/Utils/SILIsolationInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -970,11 +970,15 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) {
// async lets.
//
// This pattern should only come up with async lets. See comment in
// isTransferrableFunctionArgument.
// canFunctionArgumentBeSent in RegionAnalysis.cpp. This code needs to stay in
// sync with that code.
if (!fArg->isIndirectResult() && !fArg->isIndirectErrorResult() &&
fArg->isClosureCapture() &&
fArg->getFunction()->getLoweredFunctionType()->isSendable())
return SILIsolationInfo::getDisconnected(false /*nonisolated(unsafe)*/);
fArg->isClosureCapture()) {
if (auto declRef = arg->getFunction()->getDeclRef();
declRef && declRef.isAsyncLetClosure) {
return SILIsolationInfo::getDisconnected(false /*nonisolated(unsafe)*/);
}
}

// Before we do anything further, see if we have an isolated parameter. This
// handles isolated self and specifically marked isolated.
Expand Down
2 changes: 2 additions & 0 deletions test/Concurrency/sendable_checking.swift
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,8 @@ struct DowngradeForPreconcurrency {
// expected-note@-3 2 {{a function type must be marked '@Sendable' to conform to 'Sendable'}}
// expected-warning@-4 {{expression is 'async' but is not marked with 'await'; this is an error in the Swift 6 language mode}}
// expected-note@-5 {{calls to parameter 'completion' from outside of its actor context are implicitly asynchronous}}
// expected-complete-and-tns-warning @-7 {{passing closure as a 'sending' parameter risks causing data races between code in the current task and concurrent execution of the closure}}
// expected-complete-and-tns-note @-7 {{closure captures 'completion' which is accessible to code in the current task}}
}
}
}
Expand Down
27 changes: 22 additions & 5 deletions test/Concurrency/transfernonsendable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1772,13 +1772,12 @@ extension MyActor {
_ = self
_ = sc

Task { // expected-tns-warning {{sending value of non-Sendable type '() async -> ()' risks causing data races}}
// expected-tns-note @-1 {{Passing value of non-Sendable type '() async -> ()' as a 'sending' argument to initializer 'init(name:priority:operation:)' risks causing races in between local and caller code}}
_ = sc
Task { // expected-tns-warning {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
_ = sc // expected-tns-note {{closure captures 'self'-isolated 'sc'}}
}

Task { // expected-tns-note {{access can happen concurrently}}
_ = sc
Task { // expected-tns-warning {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
_ = sc // expected-tns-note {{closure captures 'self'-isolated 'sc'}}
}
}
}
Expand Down Expand Up @@ -2095,3 +2094,21 @@ func inferLocationOfCapturedTaskIsolatedSelfCorrectly() {
}
}

func avoidThinkingClosureParameterIsSending() {
@MainActor
final class Foo {
let value = NonSendableKlass()

func perform() {
Task { [value] in
await value.asyncCall() // expected-tns-warning {{sending 'value' risks causing data races}}
// expected-tns-note @-1 {{sending main actor-isolated 'value' to nonisolated instance method 'asyncCall()' risks causing data races between nonisolated and main actor-isolated uses}}
}

Task {
await value.asyncCall() // expected-tns-warning {{sending 'self.value' risks causing data races}}
// expected-tns-note @-1 {{sending main actor-isolated 'self.value' to nonisolated instance method 'asyncCall()' risks causing data races between nonisolated and main actor-isolated uses}}
}
}
}
}
9 changes: 4 additions & 5 deletions test/Concurrency/transfernonsendable_typed_errors.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,12 @@ extension MyActor {
_ = self
_ = sc

Task { // expected-error {{sending value of non-Sendable type '() async -> ()' risks causing data races}}
// expected-note @-1 {{Passing value of non-Sendable type '() async -> ()' as a 'sending' argument to initializer 'init(name:priority:operation:)' risks causing races in between local and caller code}}
_ = sc
Task { // expected-error {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
_ = sc // expected-note {{closure captures 'self'-isolated 'sc'}}
}

Task { // expected-note {{access can happen concurrently}}
_ = sc
Task { // expected-error {{passing closure as a 'sending' parameter risks causing data races between 'self'-isolated code and concurrent execution of the closure}}
_ = sc // expected-note {{closure captures 'self'-isolated 'sc'}}
}
}
}
Expand Down