[Concurrency] Remove _unsafeInheritExecutor from public APIs, use #isolation (#72578)

ktoso · web-flow · commit dfcf1054ddbf · 2024-04-05T03:57:54.000-07:00
diff --git a/stdlib/public/Concurrency/CheckedContinuation.swift b/stdlib/public/Concurrency/CheckedContinuation.swift
@@ -281,19 +281,37 @@ extension CheckedContinuation {
 /// - SeeAlso: `withCheckedThrowingContinuation(function:_:)`
 /// - SeeAlso: `withUnsafeContinuation(function:_:)`
 /// - SeeAlso: `withUnsafeThrowingContinuation(function:_:)`
-@available(SwiftStdlib 5.1, *)
-@_unsafeInheritExecutor // ABI compatibility with Swift 5.1
 @inlinable
 @_unavailableInEmbedded
+@available(SwiftStdlib 5.1, *)
+#if !$Embedded
+@backDeployed(before: SwiftStdlib 6.0)
+#endif
 public func withCheckedContinuation<T>(
-    function: String = #function,
-    _ body: (CheckedContinuation<T, Never>) -> Void
+  isolation: isolated (any Actor)? = #isolation,
+  function: String = #function,
+  _ body: (CheckedContinuation<T, Never>) -> Void
+) async -> T {
+  return await withUnsafeContinuation {
+    body(CheckedContinuation(continuation: $0, function: function))
+  }
+}
+
+@available(SwiftStdlib 5.1, *)
+@usableFromInline
+@_unsafeInheritExecutor // ABI compatibility with Swift 5.1
+@_unavailableInEmbedded
+@_silgen_name("$ss23withCheckedContinuation8function_xSS_yScCyxs5NeverOGXEtYalF")
+internal func __abi_withCheckedContinuation<T>(
+  function: String = #function,
+  _ body: (CheckedContinuation<T, Never>) -> Void
 ) async -> T {
   return await withUnsafeContinuation {
     body(CheckedContinuation(continuation: $0, function: function))
   }
 }
 
+
 /// Invokes the passed in closure with a checked continuation for the current task.
 ///
 /// The body of the closure executes synchronously on the calling task, and once it returns
@@ -322,13 +340,30 @@ public func withCheckedContinuation<T>(
 /// - SeeAlso: `withCheckedContinuation(function:_:)`
 /// - SeeAlso: `withUnsafeContinuation(function:_:)`
 /// - SeeAlso: `withUnsafeThrowingContinuation(function:_:)`
-@available(SwiftStdlib 5.1, *)
-@_unsafeInheritExecutor // ABI compatibility with Swift 5.1
 @inlinable
 @_unavailableInEmbedded
+@available(SwiftStdlib 5.1, *)
+#if !$Embedded
+@backDeployed(before: SwiftStdlib 6.0)
+#endif
 public func withCheckedThrowingContinuation<T>(
-    function: String = #function,
-    _ body: (CheckedContinuation<T, Error>) -> Void
+  isolation: isolated (any Actor)? = #isolation,
+  function: String = #function,
+  _ body: (CheckedContinuation<T, Error>) -> Void
+) async throws -> T {
+  return try await withUnsafeThrowingContinuation {
+    body(CheckedContinuation(continuation: $0, function: function))
+  }
+}
+
+@available(SwiftStdlib 5.1, *)
+@usableFromInline
+@_unsafeInheritExecutor // ABI compatibility with Swift 5.1
+@_unavailableInEmbedded
+@_silgen_name("$ss31withCheckedThrowingContinuation8function_xSS_yScCyxs5Error_pGXEtYaKlF")
+internal func __abi_withCheckedThrowingContinuation<T>(
+  function: String = #function,
+  _ body: (CheckedContinuation<T, Error>) -> Void
 ) async throws -> T {
   return try await withUnsafeThrowingContinuation {
     body(CheckedContinuation(continuation: $0, function: function))
diff --git a/stdlib/public/Concurrency/PartialAsyncTask.swift b/stdlib/public/Concurrency/PartialAsyncTask.swift
@@ -598,9 +598,9 @@ internal func _resumeUnsafeThrowingContinuationWithError<T>(
 /// - SeeAlso: `withCheckedContinuation(function:_:)`
 /// - SeeAlso: `withCheckedThrowingContinuation(function:_:)`
 @available(SwiftStdlib 5.1, *)
-@_unsafeInheritExecutor
 @_alwaysEmitIntoClient
 public func withUnsafeContinuation<T>(
+  isolation: isolated (any Actor)? = #isolation,
   _ fn: (UnsafeContinuation<T, Never>) -> Void
 ) async -> T {
   return await Builtin.withUnsafeContinuation {
@@ -634,9 +634,9 @@ public func withUnsafeContinuation<T>(
 /// - SeeAlso: `withCheckedContinuation(function:_:)`
 /// - SeeAlso: `withCheckedThrowingContinuation(function:_:)`
 @available(SwiftStdlib 5.1, *)
-@_unsafeInheritExecutor
 @_alwaysEmitIntoClient
 public func withUnsafeThrowingContinuation<T>(
+  isolation: isolated (any Actor)? = #isolation,
   _ fn: (UnsafeContinuation<T, Error>) -> Void
 ) async throws -> T {
   return try await Builtin.withUnsafeThrowingContinuation {
diff --git a/stdlib/public/Concurrency/Task.cpp b/stdlib/public/Concurrency/Task.cpp
@@ -346,9 +346,9 @@ static SerialExecutorRef executorForEnqueuedJob(Job *job) {
   return SerialExecutorRef::generic();
 #else
   void *jobQueue = job->SchedulerPrivate[Job::DispatchQueueIndex];
-  if (jobQueue == DISPATCH_QUEUE_GLOBAL_EXECUTOR)
+  if (jobQueue == DISPATCH_QUEUE_GLOBAL_EXECUTOR) {
     return SerialExecutorRef::generic();
-  else
+  } else
     return SerialExecutorRef::forOrdinary(reinterpret_cast<HeapObject*>(jobQueue),
                     _swift_task_getDispatchQueueSerialExecutorWitnessTable());
 #endif
diff --git a/stdlib/public/Concurrency/TaskLocal.swift b/stdlib/public/Concurrency/TaskLocal.swift
@@ -181,10 +181,27 @@ public final class TaskLocal<Value: Sendable>: Sendable, CustomStringConvertible
   /// the operation closure.
   @inlinable
   @discardableResult
-  @_unsafeInheritExecutor
-  @backDeployed(before: SwiftStdlib 5.8)
-  public func withValue<R>(_ valueDuringOperation: Value, operation: () async throws -> R,
+  @available(SwiftStdlib 5.1, *)
+  @backDeployed(before: SwiftStdlib 6.0)
+  public func withValue<R>(_ valueDuringOperation: Value,
+                           operation: () async throws -> R,
+                           isolation: isolated (any Actor)? = #isolation,
                            file: String = #fileID, line: UInt = #line) async rethrows -> R {
+    return try await withValueImpl(
+      valueDuringOperation,
+      operation: operation,
+      isolation: isolation,
+      file: file, line: line)
+  }
+
+  @usableFromInline
+  @discardableResult
+  @_unsafeInheritExecutor // ABI compatibility with Swift 5.1
+  @available(SwiftStdlib 5.1, *)
+  @_silgen_name("$ss9TaskLocalC9withValue_9operation4file4lineqd__x_qd__yYaKXESSSutYaKlF")
+  internal func __abi_withValue<R>(_ valueDuringOperation: Value,
+                                   operation: () async throws -> R,
+                                   file: String = #fileID, line: UInt = #line) async rethrows -> R {
     return try await withValueImpl(valueDuringOperation, operation: operation, file: file, line: line)
   }
 
@@ -206,11 +223,30 @@ public final class TaskLocal<Value: Sendable>: Sendable, CustomStringConvertible
   /// to swift_task_de/alloc for the copy as follows:
   /// - withValue contains the compiler-emitted calls swift_task_de/alloc.
   /// - withValueImpl contains the calls to _taskLocalValuePush/Pop
+  @inlinable
+  @discardableResult
+  @available(SwiftStdlib 5.1, *)
+  @backDeployed(before: SwiftStdlib 6.0)
+  internal func withValueImpl<R>(_ valueDuringOperation: __owned Value,
+                                 operation: () async throws -> R,
+                                 isolation: isolated (any Actor)?,
+                                 file: String = #fileID, line: UInt = #line) async rethrows -> R {
+    // check if we're not trying to bind a value from an illegal context; this may crash
+    _checkIllegalTaskLocalBindingWithinWithTaskGroup(file: file, line: line)
+
+    _taskLocalValuePush(key: key, value: consume valueDuringOperation)
+    defer { _taskLocalValuePop() }
+
+    return try await operation()
+  }
+
   @inlinable
   @discardableResult
   @_unsafeInheritExecutor
+  @available(SwiftStdlib 5.1, *)
   @backDeployed(before: SwiftStdlib 5.9)
-  internal func withValueImpl<R>(_ valueDuringOperation: __owned Value, operation: () async throws -> R,
+  internal func withValueImpl<R>(_ valueDuringOperation: __owned Value,
+                                 operation: () async throws -> R,
                                  file: String = #fileID, line: UInt = #line) async rethrows -> R {
     // check if we're not trying to bind a value from an illegal context; this may crash
     _checkIllegalTaskLocalBindingWithinWithTaskGroup(file: file, line: line)
@@ -221,6 +257,7 @@ public final class TaskLocal<Value: Sendable>: Sendable, CustomStringConvertible
     return try await operation()
   }
 
+
   /// Binds the task-local to the specific value for the duration of the
   /// synchronous operation.
   ///
diff --git a/stdlib/toolchain/Compatibility56/Concurrency/Actor.cpp b/stdlib/toolchain/Compatibility56/Concurrency/Actor.cpp
@@ -67,7 +67,7 @@ class ExecutorTrackingInfo {
 
   /// Unconditionally initialize a fresh tracking state on the
   /// current state, shadowing any previous tracking state.
-  /// leave() must be called beforet the object goes out of scope.
+  /// leave() must be called before the object goes out of scope.
   void enterAndShadow(ExecutorRef currentExecutor) {
     ActiveExecutor = currentExecutor;
     SavedInfo = ActiveInfoInThread.get();
diff --git a/test/Concurrency/Runtime/continuation_validation.swift b/test/Concurrency/Runtime/continuation_validation.swift
@@ -16,11 +16,72 @@
 
 import StdlibUnittest
 
+@MainActor
+@available(SwiftStdlib 5.1, *)
+func test_isolation_withUnsafeContinuation() async {
+  // This test specifically should have only one suspension point,
+  // as it would trigger a problem with the previous @_unsafeInheritExecutor
+  // implementation, where we optimize away a switch accidentally, causing
+  // wrong isolation.
+  await withUnsafeContinuation { continuation in
+    MainActor.shared.assertIsolated() // OK
+    continuation.resume(returning: ())
+  }
+}
+@MainActor
+@available(SwiftStdlib 5.1, *)
+func test_isolation_withUnsafeThrowingContinuation() async {
+  // See comment in `test_isolation_withUnsafeContinuation` about exact test case shape
+  try! await withUnsafeThrowingContinuation { continuation in
+    MainActor.shared.assertIsolated() // OK
+    continuation.resume(returning: ())
+  }
+}
+@MainActor
+@available(SwiftStdlib 5.1, *)
+func test_isolation_withCheckedContinuation() async {
+  // See comment in `test_isolation_withUnsafeContinuation` about exact test case shape
+  await withCheckedContinuation { continuation in
+    MainActor.shared.assertIsolated() // OK
+    continuation.resume(returning: ())
+  }
+}
+@MainActor
+@available(SwiftStdlib 5.1, *)
+func test_isolation_withCheckedThrowingContinuation() async {
+  // See comment in `test_isolation_withUnsafeContinuation` about exact test case shape
+  try! await withCheckedThrowingContinuation { continuation in
+    MainActor.shared.assertIsolated() // OK
+    continuation.resume(returning: ())
+  }
+}
+
 @main struct Main {
   static func main() async {
     let tests = TestSuite("ContinuationValidation")
 
     if #available(SwiftStdlib 5.1, *) {
+      tests.test("withUnsafeThrowingContinuation: continuation should be on calling isolation") {
+        await Task.detached {
+          await test_isolation_withUnsafeThrowingContinuation()
+        }.value
+      }
+      tests.test("withUnsafeContinuation: continuation should be on calling isolation") {
+        await Task.detached {
+          await test_isolation_withUnsafeContinuation()
+        }.value
+      }
+      tests.test("withCheckedContinuation: continuation should be on calling isolation") {
+        await Task.detached {
+          await test_isolation_withCheckedContinuation()
+        }.value
+      }
+      tests.test("withCheckedThrowingContinuation: continuation should be on calling isolation") {
+        await Task.detached {
+          await test_isolation_withCheckedThrowingContinuation()
+        }.value
+      }
+
       tests.test("trap on double resume of unchecked continuation") {
         expectCrashLater(withMessage: "may have already been resumed")
 
diff --git a/test/Concurrency/async_task_locals_basic_warnings.swift b/test/Concurrency/async_task_locals_basic_warnings.swift
@@ -7,19 +7,18 @@
 // REQUIRES: concurrency
 // REQUIRES: asserts
 
-@available(SwiftStdlib 5.1, *)
 actor Test {
 
   @TaskLocal static var local: Int?
 
   func run() async {
     // This should NOT produce any warnings, the closure withValue uses is @Sendable:
-    await Test.$local.withValue(42) {
+    await Self.$local.withValue(42) {
       await work()
     }
   }
 
   func work() async {
-    print("Hello \(Test.local ?? 0)")
+    print("Hello \(Self.local ?? 0)")
   }
 }
diff --git a/test/Concurrency/async_task_locals_basic_warnings_bug_isolation.swift b/test/Concurrency/async_task_locals_basic_warnings_bug_isolation.swift
@@ -0,0 +1,28 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -emit-module-path %t/OtherActors.swiftmodule -module-name OtherActors %S/Inputs/OtherActors.swift -disable-availability-checking
+
+// RUN: %target-swift-frontend -I %t  -disable-availability-checking -strict-concurrency=complete -parse-as-library %s -emit-sil -o /dev/null -verify
+// RUN: %target-swift-frontend -I %t  -disable-availability-checking -strict-concurrency=complete -parse-as-library %s -emit-sil -o /dev/null -verify -enable-upcoming-feature RegionBasedIsolation
+
+// REQUIRES: concurrency
+// REQUIRES: asserts
+
+// FIXME: rdar://125078448 is resolved
+// XFAIL: *
+
+actor Test {
+
+  @TaskLocal static var local: Int?
+
+  func withTaskLocal(isolation: isolated (any Actor)? = #isolation,
+                     _ body: (consuming NonSendableValue, isolated (any Actor)?) -> Void) async {
+    Self.$local.withValue(12) {
+      // Unexpected errors here:
+      //  error: unexpected warning produced: transferring 'body' may cause a race; this is an error in the Swift 6 language mode
+      //  error: unexpected note produced: actor-isolated 'body' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses
+      body(NonSendableValue(), isolation)
+    }
+  }
+}
+
+class NonSendableValue {}
diff --git a/test/abi/macOS/arm64/concurrency.swift b/test/abi/macOS/arm64/concurrency.swift
@@ -270,6 +270,14 @@ Added: _swift_task_getPreferredTaskExecutor
 Added: _swift_task_popTaskExecutorPreference
 Added: _swift_task_pushTaskExecutorPreference
 
+// Adopt #isolation in with...Continuation APIs
+// Swift.withCheckedThrowingContinuation<A>(isolation: isolated Swift.Actor?, function: Swift.String, _: (Swift.CheckedContinuation<A, Swift.Error>) -> ()) async throws -> A
+Added: _$ss31withCheckedThrowingContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5Error_pGXEtYaKlF
+Added: _$ss31withCheckedThrowingContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5Error_pGXEtYaKlFTu
+// Swift.withCheckedContinuation<A>(isolation: isolated Swift.Actor?, function: Swift.String, _: (Swift.CheckedContinuation<A, Swift.Never>) -> ()) async -> A
+Added: _$ss23withCheckedContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5NeverOGXEtYalF
+Added: _$ss23withCheckedContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5NeverOGXEtYalFTu
+
 // Updated signature for withTaskExecutorPreference to used typed throws and #isolation
 // Swift.withTaskExecutorPreference<A, B where B: Swift.Error>(_: Swift.TaskExecutor?, isolation: isolated Swift.Actor?, operation: () async throws(B) -> A) async throws(B) -> A
 Added: _$ss26withTaskExecutorPreference_9isolation9operationxSch_pSg_ScA_pSgYixyYaq_YKXEtYaq_YKs5ErrorR_r0_lF
@@ -303,3 +311,11 @@ Added: _$sScfsE13checkIsolatedyyF
 Added: _$sScf13checkIsolatedyyFTj
 // method descriptor for Swift.SerialExecutor.checkIsolated() -> ()
 Added: _$sScf13checkIsolatedyyFTq
+
+// #isolated adoption in TaskLocal.withValue
+// Swift.TaskLocal.withValueImpl<A>(_: __owned A, operation: () async throws -> A1, isolation: isolated Swift.Actor?, file: Swift.String, line: Swift.UInt) async throws -> A1
+Added: _$ss9TaskLocalC13withValueImpl_9operation9isolation4file4lineqd__xn_qd__yYaKXEScA_pSgYiSSSutYaKlF
+Added: _$ss9TaskLocalC13withValueImpl_9operation9isolation4file4lineqd__xn_qd__yYaKXEScA_pSgYiSSSutYaKlFTu
+// Swift.TaskLocal.withValue<A>(_: A, operation: () async throws -> A1, isolation: isolated Swift.Actor?, file: Swift.String, line: Swift.UInt) async throws -> A1
+Added: _$ss9TaskLocalC9withValue_9operation9isolation4file4lineqd__x_qd__yYaKXEScA_pSgYiSSSutYaKlF
+Added: _$ss9TaskLocalC9withValue_9operation9isolation4file4lineqd__x_qd__yYaKXEScA_pSgYiSSSutYaKlFTu
diff --git a/test/abi/macOS/x86_64/concurrency.swift b/test/abi/macOS/x86_64/concurrency.swift
@@ -270,6 +270,14 @@ Added: _swift_task_getPreferredTaskExecutor
 Added: _swift_task_popTaskExecutorPreference
 Added: _swift_task_pushTaskExecutorPreference
 
+// Adopt #isolation in with...Continuation APIs
+// Swift.withCheckedThrowingContinuation<A>(isolation: isolated Swift.Actor?, function: Swift.String, _: (Swift.CheckedContinuation<A, Swift.Error>) -> ()) async throws -> A
+Added: _$ss31withCheckedThrowingContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5Error_pGXEtYaKlF
+Added: _$ss31withCheckedThrowingContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5Error_pGXEtYaKlFTu
+// Swift.withCheckedContinuation<A>(isolation: isolated Swift.Actor?, function: Swift.String, _: (Swift.CheckedContinuation<A, Swift.Never>) -> ()) async -> A
+Added: _$ss23withCheckedContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5NeverOGXEtYalF
+Added: _$ss23withCheckedContinuation9isolation8function_xScA_pSgYi_SSyScCyxs5NeverOGXEtYalFTu
+
 // Updated signature for withTaskExecutorPreference to used typed throws and #isolation
 // Swift.withTaskExecutorPreference<A, B where B: Swift.Error>(_: Swift.TaskExecutor?, isolation: isolated Swift.Actor?, operation: () async throws(B) -> A) async throws(B) -> A
 Added: _$ss26withTaskExecutorPreference_9isolation9operationxSch_pSg_ScA_pSgYixyYaq_YKXEtYaq_YKs5ErrorR_r0_lF
@@ -303,3 +311,11 @@ Added: _$sScfsE13checkIsolatedyyF
 Added: _$sScf13checkIsolatedyyFTj
 // method descriptor for Swift.SerialExecutor.checkIsolated() -> ()
 Added: _$sScf13checkIsolatedyyFTq
+
+// #isolated adoption in TaskLocal.withValue
+// Swift.TaskLocal.withValueImpl<A>(_: __owned A, operation: () async throws -> A1, isolation: isolated Swift.Actor?, file: Swift.String, line: Swift.UInt) async throws -> A1
+Added: _$ss9TaskLocalC13withValueImpl_9operation9isolation4file4lineqd__xn_qd__yYaKXEScA_pSgYiSSSutYaKlF
+Added: _$ss9TaskLocalC13withValueImpl_9operation9isolation4file4lineqd__xn_qd__yYaKXEScA_pSgYiSSSutYaKlFTu
+// Swift.TaskLocal.withValue<A>(_: A, operation: () async throws -> A1, isolation: isolated Swift.Actor?, file: Swift.String, line: Swift.UInt) async throws -> A1
+Added: _$ss9TaskLocalC9withValue_9operation9isolation4file4lineqd__x_qd__yYaKXEScA_pSgYiSSSutYaKlF
+Added: _$ss9TaskLocalC9withValue_9operation9isolation4file4lineqd__x_qd__yYaKXEScA_pSgYiSSSutYaKlFTu
diff --git a/test/api-digester/stability-concurrency-abi.test b/test/api-digester/stability-concurrency-abi.test

Original file line number	Diff line number	Diff line change
`@@ -7,19 +7,18 @@`
`7`	`7`	`// REQUIRES: concurrency`
`8`	`8`	`// REQUIRES: asserts`
`9`	`9`
`10`		`-@available(SwiftStdlib 5.1, *)`
`11`	`10`	`actor Test {`
`12`	`11`
`13`	`12`	`@TaskLocal static var local: Int?`
`14`	`13`
`15`	`14`	`func run() async {`
`16`	`15`	`// This should NOT produce any warnings, the closure withValue uses is @Sendable:`
`17`		`- await Test.$local.withValue(42) {`
	`16`	`+ await Self.$local.withValue(42) {`
`18`	`17`	`await work()`
`19`	`18`	`}`
`20`	`19`	`}`
`21`	`20`
`22`	`21`	`func work() async {`
`23`		`- print("Hello \(Test.local ?? 0)")`
	`22`	`+ print("Hello \(Self.local ?? 0)")`
`24`	`23`	`}`
`25`	`24`	`}`