Adopt Swift 6.0 #isolation; Resolves async closure behavior in withSpan #148
Conversation
**Motivation:**
This has been a problem for a long time, where we could not safely
non sendable actor state from withSpan because the closure would "hop
off". Now with #isolation we can handle it properly in Swift 6.0
**Modifications:**
Adopt #isolation parameter in all with... APIs with async closures.
**Result:**
Usages like this will not warn nor produce errors anymore:
```
actor Foo {
var bar = 0
func foo() async {
var num = 0
await withSpan(#function) { _ in
// await self.withSpanWorkaround(#function) { _ in
self.bar += 1
await work()
num += 1
}
}
}
```
| @@ -315,18 +315,21 @@ public func withSpan<T>( | |||
| /// - instant: the time instant at which the span started | |||
| /// - context: The `ServiceContext` providing information on where to start the new ``Span``. | |||
| /// - kind: The ``SpanKind`` of the new ``Span``. | |||
There was a problem hiding this comment.
This doc comment is wrong, should be ofKind to match the public API
There was a problem hiding this comment.
Are you sure? https://www.swift.org/documentation/docc/writing-symbol-documentation-in-your-source-files shows to document the actual second name, as those are unique and the prior labels aren't.
Sources/Tracing/Tracer.swift
Outdated
| @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal ServiceContext | ||
| public func withSpan<T, Instant: TracerInstant>( | ||
| _ operationName: String, | ||
| at instant: @autoclosure () -> Instant, | ||
| context: @autoclosure () -> ServiceContext = .current ?? .topLevel, | ||
| ofKind kind: SpanKind = .internal, | ||
| isolation: isolated (any Actor)? = #isolation, |
There was a problem hiding this comment.
This parameter isn't documented now right?
There was a problem hiding this comment.
hm?
this adds docs above here: /// - isolation: Defaulted parameter for inheriting isolation of calling actor
|
I kept "old" API around but made it deprecated and disfavored in 6.0+. I'll merge but if anyone has any concerns let me know and we'll follow up |
| /// - function: The function name in which the span was started | ||
| /// - fileID: The `fileID` where the span was started. | ||
| /// - line: The file line where the span was started. | ||
| /// - operation: The operation that this span should be measuring | ||
| /// - Returns: the value returned by `operation` | ||
| /// - Throws: the error the `operation` has thrown (if any) | ||
| #if swift(>=6.0) |
There was a problem hiding this comment.
Is #if swift(>=6.0) correct here? Should this use #if compiler(>=6.0) instead, if a Swift 6 compiler in 5.x mode understands #isolation? I just tried to adopt this in pre-6 code with a 6.0 compiler and it was using the previous overload.
| #endif | ||
|
|
||
| @_disfavoredOverload | ||
| @available(*, deprecated, message: "Prefer #isolation version of this API") |
There was a problem hiding this comment.
Should the deprecated attribute be applied here when the #isolation API isn't available? Should this be in the same #if as above?
|
Thanks @porglezomp lemme check this |
Motivation:
This has been a problem for a long time, where we could not safely non sendable actor state from withSpan because the closure would "hop off". Now with #isolation we can handle it properly in Swift 6.0
Modifications:
Adopt #isolation parameter in all with... APIs with async closures.
Result:
Usages like this will not warn nor produce errors anymore:
resolves rdar://113848512