From 134f5def366c338a0d412c24baf9f686840a25cc 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 (cherry picked from commit 3de72c54528405c7ed738c11fd662a52f6ba7051) --- 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 05addbf8425f3..bb1d38750581c 100644 --- a/lib/Sema/CSApply.cpp +++ b/lib/Sema/CSApply.cpp @@ -6494,18 +6494,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; } @@ -7747,6 +7760,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 9391caf61e011..7caf6a07beb74 100644 --- a/lib/Sema/TypeCheckConcurrency.cpp +++ b/lib/Sema/TypeCheckConcurrency.cpp @@ -4699,6 +4699,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 3c321ec5b4ba86f7f61a59ef2575a4b7302ab5cf 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. (cherry picked from commit bb62e961b8a7b4d27d3fe784e9b46a4b489f5e48) --- 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 f08c99371d1245cef28898361ddeb8f8706dfc7d 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 (cherry picked from commit eee53aad1bb3b86474e3a5db96fbe70a25a3be44) --- .../Runtime/startSynchronously.swift | 134 +++++++----------- 1 file changed, 49 insertions(+), 85 deletions(-) diff --git a/test/Concurrency/Runtime/startSynchronously.swift b/test/Concurrency/Runtime/startSynchronously.swift index 27ebea1557b45..bbbb9418f9be8 100644 --- a/test/Concurrency/Runtime/startSynchronously.swift +++ b/test/Concurrency/Runtime/startSynchronously.swift @@ -64,6 +64,23 @@ actor MyGlobalActor { static let shared: MyGlobalActor = MyGlobalActor() } +final class NaiveQueueExecutor: SerialExecutor { + let queue: DispatchQueue + + init(queue: DispatchQueue) { + self.queue = queue + } + + public func enqueue(_ job: consuming ExecutorJob) { + let unowned = UnownedJob(job) + print("NaiveQueueExecutor(\(self.queue.label)) enqueue... [thread:\(getCurrentThreadID())]") + queue.async { + print("NaiveQueueExecutor(\(self.queue.label)) enqueue: run [thread:\(getCurrentThreadID())]") + unowned.runSynchronously(on: self.asUnownedSerialExecutor()) + } + } +} + // Test on all platforms func syncOnMyGlobalActor() -> [Task] { MyGlobalActor.shared.preconditionIsolated("Should be executing on the global actor here") @@ -189,7 +206,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() @@ -203,11 +220,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 @@ -219,35 +231,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) { - 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 { +actor Recipient: RecipientProtocol { + func callAndSuspend(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)) } } @@ -274,8 +271,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)") @@ -290,22 +287,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() } @@ -323,6 +304,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:.*]]] @@ -333,38 +336,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 -final class NaiveQueueExecutor: SerialExecutor { - let queue: DispatchQueue - - init(queue: DispatchQueue) { - self.queue = queue - } - - public func enqueue(_ job: consuming ExecutorJob) { - let unowned = UnownedJob(job) - print("NaiveQueueExecutor(\(self.queue.label)) enqueue... [thread:\(getCurrentThreadID())]") - queue.async { - print("NaiveQueueExecutor(\(self.queue.label)) enqueue: run [thread:\(getCurrentThreadID())]") - unowned.runSynchronously(on: self.asUnownedSerialExecutor()) - } - } -} - -actor RecipientOnQueue { +actor RecipientOnQueue: RecipientProtocol { let executor: NaiveQueueExecutor nonisolated let unownedExecutor: UnownedSerialExecutor @@ -373,30 +352,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)) } }