Skip to content
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

[TaskLocals] v4: @TaskLocal, binding through UnsafeCurrentTask, copy propagation #1337

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Apr 22, 2021

Massive changes of surface API:

  • defining keys is now done via @TaskLocal wrappers
  • added ability to bind values in synchronous functions through the UnsafeCurrentTask API; it is unsafe-ish because it can be abused and must use unsafe tasks
  • cleanup of wording to use the new terminology
  • add the future options to future work
  • allude to [Concurrency] Add "async" operation for continuing work asynchronously. swift#37007 [Concurrency] Add "async" operation for continuing work asynchronously. #37007 rather than saying we'd propagate values to detached tasks (which we do not want)

This is all implemented in:

We need to decide if we should implement erroring if they're defined as non-static. We could implement it via a similar subscript like we do _enclosingInstance but say there that the instance is Void or Never.

Thank you @hborla for the help on the property wrapper design.

…, async/send rather than force propagation through detach
@filip-sakel
Copy link
Contributor

Would it be possible to utilize projected values to avoid the awkward Library.myTaskValue.get() call? Instead, we could write:

enum MyLibrary {
  @TaskLocal
  static var spicePreference: SpicePreference?
}

func myFoodPrepFunction() {
  MyLibrary.spicePreference // .blackPepper

  // DESIGN #1
  // An alternative to bindTo(_:body:) is withValue(_:body:).
  MyLibrary.$spicePreference.bindTo(.redPepperFlakes) {
    MyLibrary.spicePreference // .redPepperFlakes
  }

  // DESIGN #2 
  // Some alternatives to withTaskLocal(_:boundTo:body:) are: 
  // withTaskLocal(_:of:body:) and bindTaskLocal(_:to:body:)
  withTaskLocal(MyLibrary.$spicePreference, boundTo: .redPepperFlakes) { 
    MyLibrary.spicePreference // .redPepperFlakes
  }
}

@lattner
Copy link
Collaborator

lattner commented Apr 22, 2021

in a quick skim, this looks like a really great step forward!

@ktoso
Copy link
Contributor Author

ktoso commented Apr 26, 2021

@filip-sakel let's move the discussion to the review thread, I replied here: https://forums.swift.org/t/se-0311-task-local-values/47478/42

I'll reply there. edit: replied


Thanks @lattner ! I'm unsure on process if I should merge this or keep it around as PR 🤔

@lattner
Copy link
Collaborator

lattner commented Apr 26, 2021

Stay tuned, the review period is over tomorrow anyway, so we'll probably rerun a new proposal revision. Starting this as a new proposal round has the advantage of splitting the thread into its own discussion for clarity.

@rjmccall rjmccall merged commit 73680fb into swiftlang:main Apr 27, 2021
@ktoso ktoso deleted the wip-tasklocals branch April 27, 2021 23:33
@ktoso
Copy link
Contributor Author

ktoso commented Apr 27, 2021

Sounds good to give it another round, thanks folks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants