Skip to content

[region-isolation] Transferring results shouldn't have the following error emitted: "non-sendable type 'NonSendableKlass' returned by implicitly asynchronous call to main actor-isolated function cannot cross actor boundary" #71940

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
merged 5 commits into from
Feb 29, 2024
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
1 change: 1 addition & 0 deletions lib/AST/ASTDumper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4309,6 +4309,7 @@ namespace {
printFlag(T->isSendable(), "Sendable");
printFlag(T->isAsync(), "async");
printFlag(T->isThrowing(), "throws");
printFlag(T->hasTransferringResult(), "transferring_result");
}
if (Type globalActor = T->getGlobalActor()) {
printFieldQuoted(globalActor.getString(), "global_actor");
Expand Down
17 changes: 15 additions & 2 deletions lib/AST/ASTPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3809,7 +3809,20 @@ static bool usesFeatureTransferringArgsAndResults(Decl *decl) {
if (pd->isTransferring())
return true;

// TODO: Results.
if (auto *fDecl = dyn_cast<FuncDecl>(decl)) {
auto fnTy = fDecl->getInterfaceType();
bool hasTransferring = false;
if (auto *ft = llvm::dyn_cast_if_present<FunctionType>(fnTy)) {
if (ft->hasExtInfo())
hasTransferring = ft->hasTransferringResult();
} else if (auto *ft =
llvm::dyn_cast_if_present<GenericFunctionType>(fnTy)) {
if (ft->hasExtInfo())
hasTransferring = ft->hasTransferringResult();
}
if (hasTransferring)
return true;
}

return false;
}
Expand Down Expand Up @@ -7398,7 +7411,7 @@ class TypePrinter : public TypeVisitor<TypePrinter> {
Printer << " -> ";

if (T->hasExtInfo() && T->hasTransferringResult()) {
Printer.printKeyword("transferring", Options);
Printer.printKeyword("transferring ", Options);
}

if (T->hasLifetimeDependenceInfo()) {
Expand Down
33 changes: 28 additions & 5 deletions lib/SILOptimizer/Analysis/RegionAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -417,13 +417,19 @@ static bool isProjectedFromAggregate(SILValue value) {
return visitor.isMerge;
}

static PartialApplyInst *findAsyncLetPartialApplyFromStart(BuiltinInst *bi) {
namespace {
using AsyncLetSourceValue =
llvm::PointerUnion<PartialApplyInst *, ThinToThickFunctionInst *>;
} // namespace

static std::optional<AsyncLetSourceValue>
findAsyncLetPartialApplyFromStart(BuiltinInst *bi) {
// If our operand is Sendable then we want to return nullptr. We only want to
// return a value if we are not
SILValue value = bi->getOperand(1);
auto fType = value->getType().castTo<SILFunctionType>();
if (fType->isSendable())
return nullptr;
return {};

SILValue temp = value;
while (true) {
Expand All @@ -436,10 +442,15 @@ static PartialApplyInst *findAsyncLetPartialApplyFromStart(BuiltinInst *bi) {
value = temp;
}

return cast<PartialApplyInst>(value);
// We can also get a thin_to_thick_function here if we do not capture
// anything. In such a case, we just do not process the partial apply get
if (auto *pai = dyn_cast<PartialApplyInst>(value))
return {{pai}};
return {{cast<ThinToThickFunctionInst>(value)}};
}

static PartialApplyInst *findAsyncLetPartialApplyFromGet(ApplyInst *ai) {
static std::optional<AsyncLetSourceValue>
findAsyncLetPartialApplyFromGet(ApplyInst *ai) {
auto *bi = cast<BuiltinInst>(FullApplySite(ai).getArgument(0));
assert(*bi->getBuiltinKind() ==
BuiltinValueKind::StartAsyncLetWithLocalBuffer);
Expand Down Expand Up @@ -1552,11 +1563,18 @@ class PartitionOpTranslator {
}

void translateAsyncLetGet(ApplyInst *ai) {
auto *pai = findAsyncLetPartialApplyFromGet(ai);
auto source = findAsyncLetPartialApplyFromGet(ai);
assert(source.has_value());

// If we didn't find a partial_apply, then we must have had a
// thin_to_thick_function meaning we did not capture anything.
if (source->is<ThinToThickFunctionInst *>())
return;

// We should always be able to derive a partial_apply since we pattern
// matched against the actual function call to swift_asyncLet_get in our
// caller.
auto *pai = source->get<PartialApplyInst *>();
assert(pai && "AsyncLet Get should always have a derivable partial_apply");

ApplySite applySite(pai);
Expand Down Expand Up @@ -2965,6 +2983,11 @@ static bool canComputeRegionsForFunction(SILFunction *fn) {
if (!fn->getASTContext().LangOpts.hasFeature(Feature::RegionBasedIsolation))
return false;

assert(fn->getASTContext().LangOpts.StrictConcurrencyLevel ==
StrictConcurrency::Complete &&
"Need strict concurrency to be enabled for RegionBasedIsolation to be "
"enabled as well");

// If this function does not correspond to a syntactic declContext and it
// doesn't have a parent module, don't check it since we cannot check if a
// type is sendable.
Expand Down
7 changes: 5 additions & 2 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3508,8 +3508,11 @@ namespace {
diagnoseApplyArgSendability(apply, getDeclContext());
}

// Check for sendability of the result type.
if (diagnoseNonSendableTypes(
// Check for sendability of the result type if we do not have a
// transferring result.
if ((!ctx.LangOpts.hasFeature(Feature::TransferringArgsAndResults) ||
!fnType->hasTransferringResult()) &&
diagnoseNonSendableTypes(
fnType->getResult(), getDeclContext(),
/*inDerivedConformance*/Type(),
apply->getLoc(),
Expand Down
30 changes: 30 additions & 0 deletions test/Concurrency/transfernonsendable.sil
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ case none
case some(T)
}

sil @swift_asyncLet_get : $@convention(thin) @async (Builtin.RawPointer, Builtin.RawPointer) -> ()
sil @swift_asyncLet_finish : $@convention(thin) @async (Builtin.RawPointer, Builtin.RawPointer) -> ()

/////////////////
// MARK: Tests //
/////////////////
Expand Down Expand Up @@ -144,4 +147,31 @@ bb0:

%9999 = tuple ()
return %9999 : $()
}

sil @implicitClosure : $@convention(thin) @Sendable @async @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <NonSendableKlass>

sil [ossa] @asyncLetWithThinToThickFunction : $@convention(thin) @async () -> () {
bb0:
%0 = enum $Optional<Builtin.Executor>, #Optional.none!enumelt
hop_to_executor %0 : $Optional<Builtin.Executor>
%2 = alloc_stack $NonSendableKlass
%3 = address_to_pointer %2 : $*NonSendableKlass to $Builtin.RawPointer
%4 = enum $Optional<Builtin.RawPointer>, #Optional.none!enumelt
%5 = function_ref @implicitClosure : $@convention(thin) @Sendable @async @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <NonSendableKlass>
%6 = convert_function %5 : $@convention(thin) @Sendable @async @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <NonSendableKlass> to $@convention(thin) @async @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <NonSendableKlass>
%7 = thin_to_thick_function %6 : $@convention(thin) @async @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <NonSendableKlass> to $@noescape @async @callee_guaranteed @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <NonSendableKlass>
%8 = builtin "startAsyncLetWithLocalBuffer"<NonSendableKlass>(%4 : $Optional<Builtin.RawPointer>, %7 : $@noescape @async @callee_guaranteed @substituted <τ_0_0> () -> (@out τ_0_0, @error any Error) for <NonSendableKlass>, %3 : $Builtin.RawPointer) : $Builtin.RawPointer
%9 = function_ref @swift_asyncLet_get : $@convention(thin) @async (Builtin.RawPointer, Builtin.RawPointer) -> ()
%10 = apply %9(%8, %3) : $@convention(thin) @async (Builtin.RawPointer, Builtin.RawPointer) -> ()
hop_to_executor %0 : $Optional<Builtin.Executor>
%12 = load [copy] %2 : $*NonSendableKlass
destroy_value %12 : $NonSendableKlass
%14 = function_ref @swift_asyncLet_finish : $@convention(thin) @async (Builtin.RawPointer, Builtin.RawPointer) -> ()
%15 = apply %14(%8, %3) : $@convention(thin) @async (Builtin.RawPointer, Builtin.RawPointer) -> ()
hop_to_executor %0 : $Optional<Builtin.Executor>
%17 = builtin "endAsyncLetLifetime"(%8 : $Builtin.RawPointer) : $()
dealloc_stack %2 : $*NonSendableKlass
%19 = tuple ()
return %19 : $()
}
16 changes: 15 additions & 1 deletion test/Concurrency/transfernonsendable_asynclet.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
////////////////////////

/// Classes are always non-sendable, so this is non-sendable
class NonSendableKlass { // expected-complete-note 92{{}}
class NonSendableKlass { // expected-complete-note 94{{}}
// expected-tns-note @-1 {{}}
var field: NonSendableKlass? = nil
var field2: NonSendableKlass? = nil

Expand Down Expand Up @@ -38,6 +39,7 @@ func useInOut<T>(_ x: inout T) {}
func useValue<T>(_ x: T) -> T { x }
func useValueWrapInOptional<T>(_ x: T) -> T? { x }

@MainActor func returnValueFromMain<T>() async -> T { fatalError() }
@MainActor func transferToMain<T>(_ t: T) async {}
@MainActor func transferToMainInt<T>(_ t: T) async -> Int { 5 }
@MainActor func transferToMainIntOpt<T>(_ t: T) async -> Int? { 5 }
Expand Down Expand Up @@ -735,3 +737,15 @@ func asyncLet_Let_NormalUse_Simple2() async {
let _ = await y
useValue(x)
}

func asyncLetWithoutCapture() async {
// Make sure that we setup y correctly as fresh.
//
// NOTE: Error below will go away in next commit.
async let x: NonSendableKlass = await returnValueFromMain()
// expected-warning @-1 {{non-sendable type 'NonSendableKlass' returned by implicitly asynchronous call to main actor-isolated function cannot cross actor boundary}}
let y = await x
await transferToMain(y) // expected-tns-warning {{transferring value of non-Sendable type 'NonSendableKlass' from nonisolated context to main actor-isolated context}}
// expected-complete-warning @-1 {{passing argument of non-sendable type 'NonSendableKlass' into main actor-isolated context may introduce data races}}
useValue(y) // expected-tns-note {{access here could race}}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-swift-frontend -emit-sil -disable-availability-checking -enable-experimental-feature TransferringArgsAndResults -verify -enable-experimental-feature RegionBasedIsolation %s -o /dev/null
// RUN: %target-swift-frontend -emit-sil -parse-as-library -disable-availability-checking -strict-concurrency=complete -enable-experimental-feature TransferringArgsAndResults -verify -enable-experimental-feature RegionBasedIsolation %s -o /dev/null

// REQUIRES: asserts

Expand Down Expand Up @@ -174,8 +174,7 @@ actor MyActor {
}

@MainActor func canAssignTransferringIntoGlobalActor3(_ x: transferring Klass) async {
await transferToCustom(globalKlass) // expected-warning {{transferring 'globalKlass' may cause a race}}
// expected-note @-1 {{transferring main actor-isolated 'globalKlass' to global actor 'CustomActor'-isolated callee could cause races between global actor 'CustomActor'-isolated and main actor-isolated uses}}
await transferToCustom(globalKlass) // expected-warning {{task isolated value of type 'Klass' transferred to global actor 'CustomActor'-isolated context}}
}

func canTransferAssigningIntoLocal(_ x: transferring Klass) async {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-swift-frontend -emit-sil -disable-experimental-parser-round-trip -disable-availability-checking -enable-experimental-feature TransferringArgsAndResults -verify -enable-experimental-feature RegionBasedIsolation %s -o /dev/null
// RUN: %target-swift-frontend -emit-sil -parse-as-library -strict-concurrency=complete -disable-experimental-parser-round-trip -disable-availability-checking -enable-experimental-feature TransferringArgsAndResults -verify -enable-experimental-feature RegionBasedIsolation %s -o /dev/null

// REQUIRES: concurrency
// REQUIRES: asserts
Expand Down Expand Up @@ -49,6 +49,8 @@ func transferAsyncResultWithTransferringArg(_ x: transferring NonSendableKlass)
func transferAsyncResultWithTransferringArg2(_ x: transferring NonSendableKlass, _ y: NonSendableKlass) async -> transferring NonSendableKlass { NonSendableKlass() }
func transferAsyncResultWithTransferringArg2Throwing(_ x: transferring NonSendableKlass, _ y: NonSendableKlass) async throws -> transferring NonSendableKlass { NonSendableKlass() }

@MainActor func transferAsyncResultMainActor() async -> transferring NonSendableKlass { NonSendableKlass() }

@MainActor var globalNonSendableKlass = NonSendableKlass()

/////////////////
Expand Down Expand Up @@ -106,3 +108,11 @@ func transferReturnArg(_ x: NonSendableKlass) -> transferring NonSendableKlass {
func transferReturnArgTuple(_ x: transferring NonSendableKlass) -> transferring (NonSendableKlass, NonSendableKlass) {
return (x, x)
}

func useTransferredResultMainActor() async {
let _ = await transferAsyncResultMainActor()
}

func useTransferredResult() async {
let _ = await transferAsyncResult()
}
2 changes: 1 addition & 1 deletion test/Parse/transferring.swift
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %target-typecheck-verify-swift -disable-experimental-parser-round-trip -disable-availability-checking -enable-experimental-feature TransferringArgsAndResults
// RUN: %target-typecheck-verify-swift -disable-experimental-parser-round-trip -disable-availability-checking -enable-experimental-feature TransferringArgsAndResults -strict-concurrency=complete -enable-experimental-feature RegionBasedIsolation

// REQUIRES: asserts

Expand Down
6 changes: 3 additions & 3 deletions test/Serialization/transferring.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -enable-experimental-feature RegionBasedIsolation -enable-experimental-feature TransferringArgsAndResults -disable-experimental-parser-round-trip -module-name transferring_test -emit-module -o %t/transferring_test.swiftmodule %S/Inputs/transferring.swift
// RUN: %target-swift-frontend -enable-experimental-feature RegionBasedIsolation -enable-experimental-feature TransferringArgsAndResults -disable-experimental-parser-round-trip -emit-sil -I %t %s | %FileCheck %s
// RUN: %target-sil-opt -enable-experimental-feature RegionBasedIsolation -enable-experimental-feature TransferringArgsAndResults %t/transferring_test.swiftmodule | %FileCheck -check-prefix=AST %s
// RUN: %target-swift-frontend -strict-concurrency=complete -enable-experimental-feature RegionBasedIsolation -enable-experimental-feature TransferringArgsAndResults -disable-experimental-parser-round-trip -module-name transferring_test -emit-module -o %t/transferring_test.swiftmodule %S/Inputs/transferring.swift
// RUN: %target-swift-frontend -strict-concurrency=complete -enable-experimental-feature RegionBasedIsolation -enable-experimental-feature TransferringArgsAndResults -disable-experimental-parser-round-trip -emit-sil -I %t %s | %FileCheck %s
// RUN: %target-sil-opt -strict-concurrency=complete -enable-experimental-feature RegionBasedIsolation -enable-experimental-feature TransferringArgsAndResults %t/transferring_test.swiftmodule | %FileCheck -check-prefix=AST %s

// REQUIRES: concurrency
// REQUIRES: asserts
Expand Down