From 2178d9893f59480e63e79b4b7afdbd696ea1317e Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Thu, 24 Nov 2022 11:06:11 +0900 Subject: [PATCH 1/5] [Concurrency] tasklocal withValue inside actor is safe --- .../BackDeployConcurrency/TaskLocal.swift | 3 ++- stdlib/public/Concurrency/TaskLocal.swift | 2 +- .../Runtime/async_task_locals_basic.swift | 19 +++++++++++++++ .../async_task_locals_basic_warnings.swift | 23 +++++++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 test/Concurrency/async_task_locals_basic_warnings.swift diff --git a/stdlib/public/BackDeployConcurrency/TaskLocal.swift b/stdlib/public/BackDeployConcurrency/TaskLocal.swift index d5eeb4d89f647..92f6b99e6af0f 100644 --- a/stdlib/public/BackDeployConcurrency/TaskLocal.swift +++ b/stdlib/public/BackDeployConcurrency/TaskLocal.swift @@ -134,8 +134,9 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// /// If the value is a reference type, it will be retained for the duration of /// the operation closure. + @Sendable @discardableResult - public func withValue(_ valueDuringOperation: Value, operation: () async throws -> R, + public func withValue(_ valueDuringOperation: Value, operation: @Sendable () async throws -> R, file: String = #file, 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) diff --git a/stdlib/public/Concurrency/TaskLocal.swift b/stdlib/public/Concurrency/TaskLocal.swift index 90f06449c5f79..36446a0e91b85 100644 --- a/stdlib/public/Concurrency/TaskLocal.swift +++ b/stdlib/public/Concurrency/TaskLocal.swift @@ -135,7 +135,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// If the value is a reference type, it will be retained for the duration of /// the operation closure. @discardableResult - public func withValue(_ valueDuringOperation: Value, operation: () async throws -> R, + public func withValue(_ valueDuringOperation: Value, operation: @Sendable () 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) diff --git a/test/Concurrency/Runtime/async_task_locals_basic.swift b/test/Concurrency/Runtime/async_task_locals_basic.swift index 056f9136341ae..8aae82873b7bd 100644 --- a/test/Concurrency/Runtime/async_task_locals_basic.swift +++ b/test/Concurrency/Runtime/async_task_locals_basic.swift @@ -199,6 +199,24 @@ func withLocal_body_mustNotEscape() async { _ = something // silence not used warning } +@available(SwiftStdlib 5.1, *) +actor Worker { + @TaskLocal + static var declaredInActor: String = "" + + func setAndRead() async { + print("setAndRead") // CHECK: setAndRead + await Worker.$declaredInActor.withValue("value-1") { + await printTaskLocalAsync(Worker.$declaredInActor) // CHECK-NEXT: TaskLocal(defaultValue: ) (value-1) + } + } +} + +@available(SwiftStdlib 5.1, *) +func inside_actor() async { + await Worker().setAndRead() +} + @available(SwiftStdlib 5.1, *) @main struct Main { static func main() async { @@ -210,5 +228,6 @@ func withLocal_body_mustNotEscape() async { await nested_3_onlyTopContributes() await nested_3_onlyTopContributesAsync() await nested_3_onlyTopContributesMixed() + await inside_actor() } } diff --git a/test/Concurrency/async_task_locals_basic_warnings.swift b/test/Concurrency/async_task_locals_basic_warnings.swift new file mode 100644 index 0000000000000..462f01767a949 --- /dev/null +++ b/test/Concurrency/async_task_locals_basic_warnings.swift @@ -0,0 +1,23 @@ +// 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-typecheck-verify-swift -I %t -disable-availability-checking -warn-concurrency -parse-as-library +// REQUIRES: concurrency + +// REQUIRES: concurrency + +@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 work() + } + } + + func work() async { + print("Hello \(Test.local ?? 0)") + } +} From b3c335798d4f60131b9dedf7830ae95d603d429c Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Tue, 29 Nov 2022 08:01:55 +0900 Subject: [PATCH 2/5] Better fix, using inheriting executor --- stdlib/public/BackDeployConcurrency/TaskLocal.swift | 4 ++-- stdlib/public/Concurrency/TaskLocal.swift | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/stdlib/public/BackDeployConcurrency/TaskLocal.swift b/stdlib/public/BackDeployConcurrency/TaskLocal.swift index 92f6b99e6af0f..7482fc1f3716d 100644 --- a/stdlib/public/BackDeployConcurrency/TaskLocal.swift +++ b/stdlib/public/BackDeployConcurrency/TaskLocal.swift @@ -134,9 +134,9 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// /// If the value is a reference type, it will be retained for the duration of /// the operation closure. - @Sendable @discardableResult - public func withValue(_ valueDuringOperation: Value, operation: @Sendable () async throws -> R, + @_unsafeInheritExecutor + public func withValue(_ valueDuringOperation: Value, operation: () async throws -> R, file: String = #file, 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) diff --git a/stdlib/public/Concurrency/TaskLocal.swift b/stdlib/public/Concurrency/TaskLocal.swift index 36446a0e91b85..8a422c37efbca 100644 --- a/stdlib/public/Concurrency/TaskLocal.swift +++ b/stdlib/public/Concurrency/TaskLocal.swift @@ -135,7 +135,8 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// If the value is a reference type, it will be retained for the duration of /// the operation closure. @discardableResult - public func withValue(_ valueDuringOperation: Value, operation: @Sendable () async throws -> R, + @_unsafeInheritExecutor + public func withValue(_ valueDuringOperation: 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) From d122fd68fabd23966e03e1365a9d3adc33a72665 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Fri, 9 Dec 2022 17:46:34 +0900 Subject: [PATCH 3/5] add _backDeploy and availability necessary for it --- stdlib/public/BackDeployConcurrency/TaskLocal.swift | 10 ++++++++++ stdlib/public/Concurrency/TaskLocal.swift | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/stdlib/public/BackDeployConcurrency/TaskLocal.swift b/stdlib/public/BackDeployConcurrency/TaskLocal.swift index 7482fc1f3716d..ca619ce4ab199 100644 --- a/stdlib/public/BackDeployConcurrency/TaskLocal.swift +++ b/stdlib/public/BackDeployConcurrency/TaskLocal.swift @@ -102,6 +102,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible self.defaultValue = defaultValue } + @usableFromInline var key: Builtin.RawPointer { unsafeBitCast(self, to: Builtin.RawPointer.self) } @@ -135,7 +136,10 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// If the value is a reference type, it will be retained for the duration of /// the operation closure. @discardableResult + @inline(__always) @_unsafeInheritExecutor + @available(SwiftStdlib 5.1, *) // back deploy requires we declare the availability explicitly on this method + @_backDeploy(before: macOS 9999, iOS 9999, watchOS 9999, tvOS 9999) // SwiftStdlib 5.8 but it doesn't work with _backDeploy public func withValue(_ valueDuringOperation: Value, operation: () async throws -> R, file: String = #file, line: UInt = #line) async rethrows -> R { // check if we're not trying to bind a value from an illegal context; this may crash @@ -161,6 +165,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// If the value is a reference type, it will be retained for the duration of /// the operation closure. @discardableResult + @inline(__always) public func withValue(_ valueDuringOperation: Value, operation: () throws -> R, file: String = #file, line: UInt = #line) rethrows -> R { // check if we're not trying to bind a value from an illegal context; this may crash @@ -172,6 +177,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible return try operation() } + @inline(__always) public var projectedValue: TaskLocal { get { self @@ -213,6 +219,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible // ==== ------------------------------------------------------------------------ @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localValuePush") func _taskLocalValuePush( key: Builtin.RawPointer/*: Key*/, @@ -220,16 +227,19 @@ func _taskLocalValuePush( ) // where Key: TaskLocal @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localValuePop") func _taskLocalValuePop() @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localValueGet") func _taskLocalValueGet( key: Builtin.RawPointer/*Key*/ ) -> UnsafeMutableRawPointer? // where Key: TaskLocal @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localsCopyTo") func _taskLocalsCopy( to target: Builtin.NativeObject diff --git a/stdlib/public/Concurrency/TaskLocal.swift b/stdlib/public/Concurrency/TaskLocal.swift index 8a422c37efbca..de15e014a614a 100644 --- a/stdlib/public/Concurrency/TaskLocal.swift +++ b/stdlib/public/Concurrency/TaskLocal.swift @@ -102,6 +102,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible self.defaultValue = defaultValue } + @usableFromInline var key: Builtin.RawPointer { unsafeBitCast(self, to: Builtin.RawPointer.self) } @@ -136,6 +137,8 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// the operation closure. @discardableResult @_unsafeInheritExecutor + @available(SwiftStdlib 5.1, *) // back deploy requires we declare the availability explicitly on this method + @_backDeploy(before: macOS 9999, iOS 9999, watchOS 9999, tvOS 9999) // SwiftStdlib 5.8 but it doesn't work with _backDeploy public func withValue(_ valueDuringOperation: 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 @@ -161,6 +164,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// If the value is a reference type, it will be retained for the duration of /// the operation closure. @discardableResult + @inline(__always) public func withValue(_ valueDuringOperation: Value, operation: () throws -> R, file: String = #fileID, line: UInt = #line) rethrows -> R { // check if we're not trying to bind a value from an illegal context; this may crash @@ -172,6 +176,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible return try operation() } + @inline(__always) public var projectedValue: TaskLocal { get { self @@ -213,6 +218,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible // ==== ------------------------------------------------------------------------ @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localValuePush") func _taskLocalValuePush( key: Builtin.RawPointer/*: Key*/, @@ -220,16 +226,19 @@ func _taskLocalValuePush( ) // where Key: TaskLocal @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localValuePop") func _taskLocalValuePop() @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localValueGet") func _taskLocalValueGet( key: Builtin.RawPointer/*Key*/ ) -> UnsafeMutableRawPointer? // where Key: TaskLocal @available(SwiftStdlib 5.1, *) +@usableFromInline @_silgen_name("swift_task_localsCopyTo") func _taskLocalsCopy( to target: Builtin.NativeObject From 342ce7ea198fcce84a84b0c932de8d42244761c2 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Sat, 10 Dec 2022 09:55:11 +0900 Subject: [PATCH 4/5] cleaning up attributes --- .../public/BackDeployConcurrency/TaskLocal.swift | 15 ++++++--------- stdlib/public/Concurrency/TaskLocal.swift | 14 ++++++-------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/stdlib/public/BackDeployConcurrency/TaskLocal.swift b/stdlib/public/BackDeployConcurrency/TaskLocal.swift index ca619ce4ab199..f42de64d3a70f 100644 --- a/stdlib/public/BackDeployConcurrency/TaskLocal.swift +++ b/stdlib/public/BackDeployConcurrency/TaskLocal.swift @@ -102,7 +102,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible self.defaultValue = defaultValue } - @usableFromInline + @_alwaysEmitIntoClient var key: Builtin.RawPointer { unsafeBitCast(self, to: Builtin.RawPointer.self) } @@ -136,10 +136,10 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// If the value is a reference type, it will be retained for the duration of /// the operation closure. @discardableResult - @inline(__always) + @inlinable @_unsafeInheritExecutor @available(SwiftStdlib 5.1, *) // back deploy requires we declare the availability explicitly on this method - @_backDeploy(before: macOS 9999, iOS 9999, watchOS 9999, tvOS 9999) // SwiftStdlib 5.8 but it doesn't work with _backDeploy + @_backDeploy(before: SwiftStdlib 5.8) public func withValue(_ valueDuringOperation: Value, operation: () async throws -> R, file: String = #file, line: UInt = #line) async rethrows -> R { // check if we're not trying to bind a value from an illegal context; this may crash @@ -164,8 +164,8 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// /// If the value is a reference type, it will be retained for the duration of /// the operation closure. + @inlinable @discardableResult - @inline(__always) public func withValue(_ valueDuringOperation: Value, operation: () throws -> R, file: String = #file, line: UInt = #line) rethrows -> R { // check if we're not trying to bind a value from an illegal context; this may crash @@ -177,7 +177,6 @@ public final class TaskLocal: Sendable, CustomStringConvertible return try operation() } - @inline(__always) public var projectedValue: TaskLocal { get { self @@ -219,7 +218,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible // ==== ------------------------------------------------------------------------ @available(SwiftStdlib 5.1, *) -@usableFromInline +@_alwaysEmitIntoClient @_silgen_name("swift_task_localValuePush") func _taskLocalValuePush( key: Builtin.RawPointer/*: Key*/, @@ -227,19 +226,17 @@ func _taskLocalValuePush( ) // where Key: TaskLocal @available(SwiftStdlib 5.1, *) -@usableFromInline +@_alwaysEmitIntoClient @_silgen_name("swift_task_localValuePop") func _taskLocalValuePop() @available(SwiftStdlib 5.1, *) -@usableFromInline @_silgen_name("swift_task_localValueGet") func _taskLocalValueGet( key: Builtin.RawPointer/*Key*/ ) -> UnsafeMutableRawPointer? // where Key: TaskLocal @available(SwiftStdlib 5.1, *) -@usableFromInline @_silgen_name("swift_task_localsCopyTo") func _taskLocalsCopy( to target: Builtin.NativeObject diff --git a/stdlib/public/Concurrency/TaskLocal.swift b/stdlib/public/Concurrency/TaskLocal.swift index de15e014a614a..0bf4c4841ddff 100644 --- a/stdlib/public/Concurrency/TaskLocal.swift +++ b/stdlib/public/Concurrency/TaskLocal.swift @@ -102,7 +102,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible self.defaultValue = defaultValue } - @usableFromInline + @_alwaysEmitIntoClient var key: Builtin.RawPointer { unsafeBitCast(self, to: Builtin.RawPointer.self) } @@ -135,10 +135,11 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// /// If the value is a reference type, it will be retained for the duration of /// the operation closure. + @inlinable @discardableResult @_unsafeInheritExecutor @available(SwiftStdlib 5.1, *) // back deploy requires we declare the availability explicitly on this method - @_backDeploy(before: macOS 9999, iOS 9999, watchOS 9999, tvOS 9999) // SwiftStdlib 5.8 but it doesn't work with _backDeploy + @_backDeploy(before: SwiftStdlib 5.8) public func withValue(_ valueDuringOperation: 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 @@ -163,8 +164,8 @@ public final class TaskLocal: Sendable, CustomStringConvertible /// /// If the value is a reference type, it will be retained for the duration of /// the operation closure. + @inlinable @discardableResult - @inline(__always) public func withValue(_ valueDuringOperation: Value, operation: () throws -> R, file: String = #fileID, line: UInt = #line) rethrows -> R { // check if we're not trying to bind a value from an illegal context; this may crash @@ -176,7 +177,6 @@ public final class TaskLocal: Sendable, CustomStringConvertible return try operation() } - @inline(__always) public var projectedValue: TaskLocal { get { self @@ -218,7 +218,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible // ==== ------------------------------------------------------------------------ @available(SwiftStdlib 5.1, *) -@usableFromInline +@_alwaysEmitIntoClient @_silgen_name("swift_task_localValuePush") func _taskLocalValuePush( key: Builtin.RawPointer/*: Key*/, @@ -226,19 +226,17 @@ func _taskLocalValuePush( ) // where Key: TaskLocal @available(SwiftStdlib 5.1, *) -@usableFromInline +@_alwaysEmitIntoClient @_silgen_name("swift_task_localValuePop") func _taskLocalValuePop() @available(SwiftStdlib 5.1, *) -@usableFromInline @_silgen_name("swift_task_localValueGet") func _taskLocalValueGet( key: Builtin.RawPointer/*Key*/ ) -> UnsafeMutableRawPointer? // where Key: TaskLocal @available(SwiftStdlib 5.1, *) -@usableFromInline @_silgen_name("swift_task_localsCopyTo") func _taskLocalsCopy( to target: Builtin.NativeObject From f9c2bc04021f74ba9787781ceaf67fa0cc1a6563 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Tue, 13 Dec 2022 10:39:46 +0900 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Doug Gregor --- stdlib/public/BackDeployConcurrency/TaskLocal.swift | 4 ++-- stdlib/public/Concurrency/TaskLocal.swift | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/stdlib/public/BackDeployConcurrency/TaskLocal.swift b/stdlib/public/BackDeployConcurrency/TaskLocal.swift index f42de64d3a70f..63883f9dbaac5 100644 --- a/stdlib/public/BackDeployConcurrency/TaskLocal.swift +++ b/stdlib/public/BackDeployConcurrency/TaskLocal.swift @@ -218,7 +218,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible // ==== ------------------------------------------------------------------------ @available(SwiftStdlib 5.1, *) -@_alwaysEmitIntoClient +@usableFromInline @_silgen_name("swift_task_localValuePush") func _taskLocalValuePush( key: Builtin.RawPointer/*: Key*/, @@ -226,7 +226,7 @@ func _taskLocalValuePush( ) // where Key: TaskLocal @available(SwiftStdlib 5.1, *) -@_alwaysEmitIntoClient +@usableFromInline @_silgen_name("swift_task_localValuePop") func _taskLocalValuePop() diff --git a/stdlib/public/Concurrency/TaskLocal.swift b/stdlib/public/Concurrency/TaskLocal.swift index 0bf4c4841ddff..b208ad62c5ad7 100644 --- a/stdlib/public/Concurrency/TaskLocal.swift +++ b/stdlib/public/Concurrency/TaskLocal.swift @@ -218,7 +218,7 @@ public final class TaskLocal: Sendable, CustomStringConvertible // ==== ------------------------------------------------------------------------ @available(SwiftStdlib 5.1, *) -@_alwaysEmitIntoClient +@usableFromInline @_silgen_name("swift_task_localValuePush") func _taskLocalValuePush( key: Builtin.RawPointer/*: Key*/, @@ -226,7 +226,7 @@ func _taskLocalValuePush( ) // where Key: TaskLocal @available(SwiftStdlib 5.1, *) -@_alwaysEmitIntoClient +@usableFromInline @_silgen_name("swift_task_localValuePop") func _taskLocalValuePop()