New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unsafe task-local binding #2222
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the naming looks like it's enough of a barrier to keep folks from using it unless absolutely necessary 👍
```swift | ||
func order() async { | ||
$wasabiPreference.unsafePushValue(.with) | ||
defer { TL.$number.unsafePopValue() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is TL.$number
coming from? Shouldn't this be $wasabiPreference.unsafePopValue()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, thanks will fix
@WasabiPreference(.with) | ||
func order() async { | ||
// $wasabiPreference.unsafePushValue(.with) | ||
// defer { TL.$number.unsafePopValue() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, I'm confused about the TL.$number
.
/// - SeeAlso: ``withValue(_:file:line:)`` | ||
/// - SeeAlso: ``unsafePushValue(_:file:line:)`` | ||
@inlinable | ||
public func unsafePopValue(file: String = #fileID, line: UInt = #line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you also want to include the body of this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we're not showing implementation details -- it is swift runtime builtins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I'd remove the body from unsafePushValue
as well: https://github.com/apple/swift-evolution/pull/2222/files/258dcb1baf3ca64625fa5de307cf86da5d562de1#diff-78f598930dd0a3b9e4f0bcb4c50b84efd89866f095a2bc84bd7193753b8a57cdR166
Co-authored-by: Moritz Lang <hi@slashmo.codes>
|
||
```swift | ||
func order() async { | ||
$wasabiPreference.withValue(.with) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, what is .with
here? It is not mentioned anywhere in the text or declared in the examples—is this some existing concurrency API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the value set for the wasabiPreference
task-local within the closure scope. I suppose there are .with
and .without
, but I think this should be written down as part of the proposal. CC @ktoso
The $taskLocal.withValue
API is the existing (safe) API to bind task-local values.
The Language Steering Group is going to sit on this until we figure out in the function-body macros proposal whether this sort of thing really has to be necessary. I think we're hoping that it doesn't. |
Sounds good, agreed! I've put it into "draft" to signal that this isn't going to be reviewed for the time being. |
This provides building blocks necessary to build tracing macros (or similar) which use body "preamble" macros.