From 3de72c54528405c7ed738c11fd662a52f6ba7051 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 16 Apr 2025 17:11:48 -0700 Subject: [PATCH 1/3] [TypeChecker] Allow closures to assume `nonisolated(nonsending)` Always infer `nonisolated(nonsending)` from context directly on a closure unless the closure is marked as `@concurrent`, otherwise the closure is not going to get correct isolation and going to hop to the wrong executor in its preamble. Resolves: rdar://149107104 --- lib/Sema/CSApply.cpp | 42 ++++++++++++++++--- lib/Sema/TypeCheckConcurrency.cpp | 7 ++++ .../attr_execution/conversions_silgen.swift | 21 ++++++++++ 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/lib/Sema/CSApply.cpp b/lib/Sema/CSApply.cpp index c8e910c4661ee..34c85ad473694 100644 --- a/lib/Sema/CSApply.cpp +++ b/lib/Sema/CSApply.cpp @@ -6514,18 +6514,31 @@ ArgumentList *ExprRewriter::coerceCallArguments( return ArgumentList::createTypeChecked(ctx, args, newArgs); } -/// Whether the given expression is a closure that should inherit -/// the actor context from where it was formed. -static bool closureInheritsActorContext(Expr *expr) { +/// Looks through any non-semantic expressions and a capture list +/// to find out whether the given expression is an explicit closure. +static ClosureExpr *isExplicitClosureExpr(Expr *expr) { if (auto IE = dyn_cast(expr)) - return closureInheritsActorContext(IE->getSubExpr()); + return isExplicitClosureExpr(IE->getSubExpr()); if (auto CLE = dyn_cast(expr)) - return closureInheritsActorContext(CLE->getClosureBody()); + return isExplicitClosureExpr(CLE->getClosureBody()); + + return dyn_cast(expr); +} - if (auto CE = dyn_cast(expr)) +/// Whether the given expression is a closure that should inherit +/// the actor context from where it was formed. +static bool closureInheritsActorContext(Expr *expr) { + if (auto *CE = isExplicitClosureExpr(expr)) return CE->inheritsActorContext(); + return false; +} +/// Determine whether the given expression is a closure that +/// is explicitly marked as `@concurrent`. +static bool isClosureMarkedAsConcurrent(Expr *expr) { + if (auto *CE = isExplicitClosureExpr(expr)) + return CE->getAttrs().hasAttribute(); return false; } @@ -7767,6 +7780,23 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType, } } + // If we have a ClosureExpr, then we can safely propagate the + // 'nonisolated(nonsending)' isolation if it's not explicitly + // marked as `@concurrent`. + if (toEI.getIsolation().isNonIsolatedCaller() && + (fromEI.getIsolation().isNonIsolated() && + !isClosureMarkedAsConcurrent(expr))) { + auto newFromFuncType = fromFunc->withIsolation( + FunctionTypeIsolation::forNonIsolatedCaller()); + if (applyTypeToClosureExpr(cs, expr, newFromFuncType)) { + fromFunc = newFromFuncType->castTo(); + // Propagating 'nonisolated(nonsending)' might have satisfied the entire + // conversion. If so, we're done, otherwise keep converting. + if (fromFunc->isEqual(toType)) + return expr; + } + } + if (ctx.LangOpts.isDynamicActorIsolationCheckingEnabled()) { // Passing a synchronous global actor-isolated function value and // parameter that expects a synchronous non-isolated function type could diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp index 019c98dbc7f32..70bf5d09fbf27 100644 --- a/lib/Sema/TypeCheckConcurrency.cpp +++ b/lib/Sema/TypeCheckConcurrency.cpp @@ -4696,6 +4696,13 @@ ActorIsolation ActorIsolationChecker::determineClosureIsolation( } } + // `nonisolated(nonsending)` inferred from the context makes + // the closure caller isolated. + if (auto *closureTy = getType(closure)->getAs()) { + if (closureTy->getIsolation().isNonIsolatedCaller()) + return ActorIsolation::forCallerIsolationInheriting(); + } + // If a closure has an isolated parameter, it is isolated to that // parameter. for (auto param : *closure->getParameters()) { diff --git a/test/Concurrency/attr_execution/conversions_silgen.swift b/test/Concurrency/attr_execution/conversions_silgen.swift index 64cc5b920d16d..fdf493014b9c2 100644 --- a/test/Concurrency/attr_execution/conversions_silgen.swift +++ b/test/Concurrency/attr_execution/conversions_silgen.swift @@ -425,3 +425,24 @@ func conversionsFromSyncToAsync(_ x: @escaping @Sendable (NonSendableKlass) -> V let _: @concurrent (SendableKlass) async -> Void = y let _: @concurrent (NonSendableKlass) async -> Void = z } + +func testThatClosuresAssumeIsolation(fn: inout nonisolated(nonsending) (Int) async -> Void) { + // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCcfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional) -> () + let _: nonisolated(nonsending) () async -> Void = { + 42 + } + + func testParam(_: nonisolated(nonsending) () async throws -> Void) {} + + // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCXEfU0_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional) -> @error any Error + testParam { 42 } + + // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaXEfU1_ : $@convention(thin) @async () -> () + testParam { @concurrent in 42 } + + // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFySiYaYCcfU2_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional, Int) -> () + fn = { _ in } + + // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFySiYacfU3_ : $@convention(thin) @async (Int) -> () + fn = { @concurrent _ in } +} From bb62e961b8a7b4d27d3fe784e9b46a4b489f5e48 Mon Sep 17 00:00:00 2001 From: Pavel Yaskevich Date: Wed, 16 Apr 2025 18:21:41 -0700 Subject: [PATCH 2/3] [SILGen] Concurrency: Fix caller isolated closures to hop to the right actor Closures that are caller isolated used to still hop to the generic executor, which is incorrect. --- lib/SILGen/SILGenConcurrency.cpp | 6 +++++- .../attr_execution/conversions_silgen.swift | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/SILGen/SILGenConcurrency.cpp b/lib/SILGen/SILGenConcurrency.cpp index 9275dfa988749..bea50739dedcd 100644 --- a/lib/SILGen/SILGenConcurrency.cpp +++ b/lib/SILGen/SILGenConcurrency.cpp @@ -206,10 +206,14 @@ void SILGenFunction::emitExpectedExecutorProlog() { switch (actorIsolation.getKind()) { case ActorIsolation::Unspecified: case ActorIsolation::Nonisolated: - case ActorIsolation::CallerIsolationInheriting: case ActorIsolation::NonisolatedUnsafe: break; + case ActorIsolation::CallerIsolationInheriting: + assert(F.isAsync()); + setExpectedExecutorForParameterIsolation(*this, actorIsolation); + break; + case ActorIsolation::Erased: llvm_unreachable("closure cannot have erased isolation"); diff --git a/test/Concurrency/attr_execution/conversions_silgen.swift b/test/Concurrency/attr_execution/conversions_silgen.swift index fdf493014b9c2..90aa3885cd0c3 100644 --- a/test/Concurrency/attr_execution/conversions_silgen.swift +++ b/test/Concurrency/attr_execution/conversions_silgen.swift @@ -428,6 +428,8 @@ func conversionsFromSyncToAsync(_ x: @escaping @Sendable (NonSendableKlass) -> V func testThatClosuresAssumeIsolation(fn: inout nonisolated(nonsending) (Int) async -> Void) { // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCcfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional) -> () + // CHECK: bb0([[EXECUTOR:%.*]] : @guaranteed $Optional): + // CHECK: hop_to_executor [[EXECUTOR]] let _: nonisolated(nonsending) () async -> Void = { 42 } @@ -435,14 +437,22 @@ func testThatClosuresAssumeIsolation(fn: inout nonisolated(nonsending) (Int) asy func testParam(_: nonisolated(nonsending) () async throws -> Void) {} // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCXEfU0_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional) -> @error any Error + // CHECK: bb0([[EXECUTOR:%.*]] : @guaranteed $Optional): + // CHECK: hop_to_executor [[EXECUTOR]] testParam { 42 } // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaXEfU1_ : $@convention(thin) @async () -> () + // CHECK: [[GENERIC_EXECUTOR:%.*]] = enum $Optional, #Optional.none!enumelt + // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] testParam { @concurrent in 42 } // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFySiYaYCcfU2_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional, Int) -> () + // CHECK: bb0([[EXECUTOR:%.*]] : @guaranteed $Optional, %1 : $Int): + // CHECK: hop_to_executor [[EXECUTOR]] fn = { _ in } // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFySiYacfU3_ : $@convention(thin) @async (Int) -> () + // CHECK: [[GENERIC_EXECUTOR:%.*]] = enum $Optional, #Optional.none!enumelt + // CHECK: hop_to_executor [[GENERIC_EXECUTOR]] fn = { @concurrent _ in } } From eee53aad1bb3b86474e3a5db96fbe70a25a3be44 Mon Sep 17 00:00:00 2001 From: Konrad 'ktoso' Malawski Date: Thu, 17 Apr 2025 17:32:40 +0900 Subject: [PATCH 3/3] harden the startSynchronously.swift test a bit --- .../Runtime/startSynchronously.swift | 100 ++++++------------ 1 file changed, 32 insertions(+), 68 deletions(-) diff --git a/test/Concurrency/Runtime/startSynchronously.swift b/test/Concurrency/Runtime/startSynchronously.swift index 8d8bacd171029..a03b1095fbd3f 100644 --- a/test/Concurrency/Runtime/startSynchronously.swift +++ b/test/Concurrency/Runtime/startSynchronously.swift @@ -294,7 +294,7 @@ syncOnNonTaskThread(synchronousTask: behavior) // CHECK: after startSynchronously, outside; cancel (wakeup) the synchronous task! [thread:[[CALLING_THREAD3]]] print("\n\n==== ------------------------------------------------------------------") -print("callActorFromStartSynchronousTask()") +print("callActorFromStartSynchronousTask() - not on specific queue") callActorFromStartSynchronousTask(recipient: .recipient(Recipient())) // CHECK: callActorFromStartSynchronousTask() @@ -308,11 +308,6 @@ callActorFromStartSynchronousTask(recipient: .recipient(Recipient())) // CHECK-NOT: ERROR! // CHECK: inside startSynchronously, call rec.sync() done -// CHECK-NOT: ERROR! -// CHECK: inside startSynchronously, call rec.async() -// CHECK-NOT: ERROR! -// CHECK: inside startSynchronously, call rec.async() done - // CHECK-NOT: ERROR! // CHECK: inside startSynchronously, done @@ -324,35 +319,20 @@ enum TargetActorToCall { } protocol RecipientProtocol where Self: Actor { - func sync(syncTaskThreadID: ThreadID) async - func async(syncTaskThreadID: ThreadID) async + func callAndSuspend(syncTaskThreadID: ThreadID) async } // default actor, must not declare an 'unownedExecutor' -actor Recipient { - func sync(syncTaskThreadID: ThreadID) { +actor Recipient: RecipientProtocol { + func callAndSuspend(syncTaskThreadID: ThreadID) async { self.preconditionIsolated() print("\(Recipient.self)/\(#function) Current actor thread id = \(getCurrentThreadID()) @ :\(#line)") if compareThreadIDs(syncTaskThreadID, .equal, getCurrentThreadID()) { print("NOTICE: Actor must not run on the synchronous task's thread :\(#line)") } - } - - func async(syncTaskThreadID: ThreadID) async { - self.preconditionIsolated() - // Dispatch may end up reusing the thread used to service the queue so we - // cannot truly assert exact thread identity in such tests. - // Usually this will be on a different thread by now though. - print("\(Recipient.self)/\(#function) Current actor thread id = \(getCurrentThreadID()) @ :\(#line)") - if compareThreadIDs(syncTaskThreadID, .equal, getCurrentThreadID()) { - print("NOTICE: Actor must not run on the synchronous task's thread :\(#line)") - } - - await Task { - self.preconditionIsolated() - }.value + try? await Task.sleep(for: .milliseconds(100)) } } @@ -379,8 +359,8 @@ func callActorFromStartSynchronousTask(recipient rec: TargetActorToCall) { print("inside startSynchronously, call rec.sync() [thread:\(getCurrentThreadID())] @ :\(#line)") switch rec { - case .recipient(let recipient): await recipient.sync(syncTaskThreadID: innerTID) - case .recipientOnQueue(let recipient): await recipient.sync(syncTaskThreadID: innerTID) + case .recipient(let recipient): await recipient.callAndSuspend(syncTaskThreadID: innerTID) + case .recipientOnQueue(let recipient): await recipient.callAndSuspend(syncTaskThreadID: innerTID) } print("inside startSynchronously, call rec.sync() done [thread:\(getCurrentThreadID())] @ :\(#line)") @@ -395,22 +375,6 @@ func callActorFromStartSynchronousTask(recipient rec: TargetActorToCall) { print("NOTICE: Task resumed on same thread as it entered the synchronous task!") } - print("inside startSynchronously, call rec.async() [thread:\(getCurrentThreadID())] @ :\(#line)") - switch rec { - case .recipient(let recipient): await recipient.async(syncTaskThreadID: innerTID) - case .recipientOnQueue(let recipient): await recipient.async(syncTaskThreadID: innerTID) - } - print("inside startSynchronously, call rec.async() done [thread:\(getCurrentThreadID())] @ :\(#line)") - - print("Inner thread id = \(innerTID)") - print("Current thread id = \(getCurrentThreadID())") - // Dispatch may end up reusing the thread used to service the queue so we - // cannot truly assert exact thread identity in such tests. - // Usually this will be on a different thread by now though. - if compareThreadIDs(innerTID, .equal, getCurrentThreadID()) { - print("NOTICE: Task resumed on same thread as it entered the synchronous task!") - } - print("inside startSynchronously, done [thread:\(getCurrentThreadID())] @ :\(#line)") sem1.signal() } @@ -428,6 +392,28 @@ print("callActorFromStartSynchronousTask() - actor in custom executor with its o let actorQueue = DispatchQueue(label: "recipient-actor-queue") callActorFromStartSynchronousTask(recipient: .recipientOnQueue(RecipientOnQueue(queue: actorQueue))) + +// 50: callActorFromStartSynchronousTask() +// 51: before startSynchronously [thread:0x00007000054f5000] @ :366 +// 52: inside startSynchronously [thread:0x00007000054f5000] @ :372 +// 53: inside startSynchronously, call rec.sync() [thread:0x00007000054f5000] @ :380 +// 54: Recipient/sync(syncTaskThreadID:) Current actor thread id = 0x000070000567e000 @ :336 +// 55: inside startSynchronously, call rec.sync() done [thread:0x000070000567e000] @ :385 +// 56: Inner thread id = 0x00007000054f5000 +// 57: Current thread id = 0x000070000567e000 +// 60: after startSynchronously [thread:0x00007000054f5000] @ :418 +// 61: - async work on queue +// 62: - async work on queue +// 63: - async work on queue +// 64: - async work on queue +// 65: - async work on queue +// 67: - async work on queue +// 68: - async work on queue +// 69: - async work on queue +// 71: Inner thread id = 0x00007000054f5000 +// 72: Current thread id = 0x000070000567e000 +// 73: inside startSynchronously, done [thread:0x000070000567e000] @ :414 + // CHECK-LABEL: callActorFromStartSynchronousTask() - actor in custom executor with its own queue // No interleaving allowed between "before" and "inside": // CHECK: before startSynchronously [thread:[[CALLING_THREAD4:.*]]] @@ -438,21 +424,14 @@ callActorFromStartSynchronousTask(recipient: .recipientOnQueue(RecipientOnQueue( // allowing the 'after startSynchronously' to run. // // CHECK-NEXT: inside startSynchronously, call rec.sync() [thread:[[CALLING_THREAD4]]] -// CHECK: NaiveQueueExecutor(recipient-actor-queue) enqueue // CHECK: after startSynchronously // CHECK-NOT: ERROR! // CHECK: inside startSynchronously, call rec.sync() done -// CHECK-NOT: ERROR! -// CHECK: inside startSynchronously, call rec.async() -// CHECK: NaiveQueueExecutor(recipient-actor-queue) enqueue -// CHECK-NOT: ERROR! -// CHECK: inside startSynchronously, call rec.async() done - // CHECK-NOT: ERROR! // CHECK: inside startSynchronously, done -actor RecipientOnQueue { +actor RecipientOnQueue: RecipientProtocol { let executor: NaiveQueueExecutor nonisolated let unownedExecutor: UnownedSerialExecutor @@ -461,30 +440,15 @@ actor RecipientOnQueue { self.unownedExecutor = executor.asUnownedSerialExecutor() } - func sync(syncTaskThreadID: ThreadID) { - self.preconditionIsolated() - dispatchPrecondition(condition: .onQueue(self.executor.queue)) - - print("\(Recipient.self)/\(#function) Current actor thread id = \(getCurrentThreadID()) @ :\(#line)") - if compareThreadIDs(syncTaskThreadID, .equal, getCurrentThreadID()) { - print("NOTICE: Actor must not run on the synchronous task's thread :\(#line)") - } - } - - func async(syncTaskThreadID: ThreadID) async { + func callAndSuspend(syncTaskThreadID: ThreadID) async { self.preconditionIsolated() dispatchPrecondition(condition: .onQueue(self.executor.queue)) - // Dispatch may end up reusing the thread used to service the queue so we - // cannot truly assert exact thread identity in such tests. - // Usually this will be on a different thread by now though. print("\(Recipient.self)/\(#function) Current actor thread id = \(getCurrentThreadID()) @ :\(#line)") if compareThreadIDs(syncTaskThreadID, .equal, getCurrentThreadID()) { print("NOTICE: Actor must not run on the synchronous task's thread :\(#line)") } - await Task { - self.preconditionIsolated() - }.value + try? await Task.sleep(for: .milliseconds(100)) } }