SE-0311 (2nd review): Task-local values

Evolution Proposal Reviews
1

Hello, Swift community.

The second review of SE-0311: Task-local values begins now and runs through May 11th, 2021.

The first review received a lot of very useful feedback. In response, the author has made major changes to the proposal. The Core Team has decided to put up the revised proposal for de novo review; that is, you should essentially review this proposal as if it were an entirely different proposal.

The principal change is that task-local values are now declared a property wrapper:

extension MyLibrary {
  @TaskLocal(default: RequestID.empty)
  static var requestID
}

@TaskLocal is a normal property wrapper except that it may only be applied to a static property or a global variable. (This restriction is achieved in a general way which can be adopted by other property wrappers, should they wish to.)

This second review was initiated with a slightly different version of the proposal than is now under consideration. Early feedback made it obvious that the proposal was not really ready for review and needed further revision. The author has now made revisions that the Core Team believes are sufficient for the review to continue. We have extended the review (it originally ended on May 4th) to give reviewers enough time to consider all of these changes.

If you are interested in the exact changes from the initial proposal, you can find the first set of revisions here, while the second set of revisions is here. The announcement of the review extension is here.

This review is part of the larger concurrency feature, which we are reviewing in several parts. While we've tried to make it independent of other concurrency proposals that have not yet been reviewed, it may have some dependencies that we've failed to eliminate. Please do your best to review it on its own merits, while still understanding its relationship to the larger feature. However, if you see any problematic interactions with proposals that have already been reviewed, that are currently in review, or that are currently being pitched, that is highly valuable feedback.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager. If you do email me directly, please put "SE-0311" somewhere in the subject line.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

As always, thank you for contributing to Swift.

John McCall
Review Manager

8 Likes
2

In this snippet from the proposal:

enum MyLibrary { 
    @TaskLocal(default: nil)
    static var requestID: TaskLocal<RequestID?>.Access   
}

Can we write something more succinct like this?

enum MyLibrary { 
    @TaskLocal<RequestID?>(default: nil)
    static var requestID
}
3 Likes
3

A difference in this API from the prior version is how multiple bindings compose:

Before:

Task.withLocal(\.first, boundTo: 1)
    .withLocal(\.second, boundTo: 2) {
    // ...
}

After:

MyLibrary.first.withValue(1) {
    MyLibrary.second.withValue(2) {
        // ...
    }
}

How often might we expect multiple task local values to be bound in succession like this? Is this situation uncommon enough that we don't anticipate burden from the additional layer of nesting?

5 Likes
4

After playing around with property wrappers in general, this would appear to be the case. Further, by adding this extension:

public extension TaskLocal where Value: ExpressibleByNilLiteral {
    convenience init() {
        self.init(default: nil)
    }
}

All of the following have the same result:

@TaskLocal(default: Optional<Int>.none)
static var primum

@TaskLocal(default: nil as Int?)
static var secundum

@TaskLocal(default: nil)
static var tertium: TaskLocal<Int?>.Access

@TaskLocal() // the parentheses are required here
static var quartum: TaskLocal<Int?>.Access

@TaskLocal<Int?>(default: nil)
static var quintum

@TaskLocal<Int?>()
static var sextum

Beauty is in the eye of the beholder, but to mine, the last one is the prettiest. It would be even prettier if we could get rid of those parentheses. I'd like to see that extension in the official implementation, but of course I can add it to my own codebase manually if it is not to be.

Regarding the detailed design, the Access type strikes me as unnecessary. Could it not be simplified like so (parts omitted for brevity):

@propertyWrapper
public final class TaskLocal<Value> {
    public var wrappedValue: TaskLocal<Value> {
        get { self }
    }
    public func get() -> Value { ... }
    public func withValue<R>(...) async rethrows -> R { ... }
}

This would result in the same API, except that any mention of TaskLocal<T>.Access becomes TaskLocal<T>. With regards to the above example, tertium and quartum would change to have type TaskLocal<Int?> rather than TaskLocal<Int?>.Access.

4 Likes
5

Thank you for splitting the review process. As I understand it, a property wrapper has been suggested in order to use MyLibrary.requestID to get a task local and MyLibrary.$requestID.withValue(_:do:) to bind it. However, because it would have been too easy to get a task local and because of the discrepancy between getting and setting a task local (the $-prefix for projected values), it has been decided to move everything in the wrapped type, thus adding Access. Since it is usual to spell out the type of a wrapped property, the way property wrappers have been used in this case it's a bit unexpected.

I get that the property wrapper is used for

  • displaying an informative error message when the user tries to set a task local by assignment (currently it doesn't work since the message in @available isn't emitted on assignments and a general "Setter for 'foo' is unavailable" shows up instead, but that can be improved separately),
  • making the property wrapper attach-able only to static properties (with the newly added mechanism to require "no enclosing instances"),

but wouldn't the following be more straightforward (assuming that static let can be compiler-enforced specifically for TaskLocals)?

enum MyLibrary {
  static let requestID = TaskLocal(default: RequestID.empty)
}
1 Like
6

I agree with @xAlien95; the wrapper here seems gratuitous.

I think of wrappers as value-processing ones (like @Lowercased), which constitute the majority of wrappers, and wrappers used to interface with hidden API (like @Namespace).

Task locals belong to the latter category. Yet, they neither project values through the dollar-sign-prefix syntax, nor do they vend a token through the wrapped value directly — relying on a two-step process of accessing the static property and calling get(), instead.

Sometimes property wrappers are used to emphasize declarations (see @Namespace). However, @TaskLocal requires that the default value be declared next to the property-wrapper attribute adding syntactic weight to that attribute rather than the static property.

Furthermore, @TaskLocal boasts the ability to prohibit instance-level applications with a hidden property-wrapper feature; an internal compiler check, though, could achieve the same (as Chris mentioned) while reducing API complexity.

7

Taking off my review-manager hat: @ktoso, did you consider using property wrapper projections here? It seems unfortunate that a reference to the task-local variable ends up producing an Access value instead of, well, the value of the variable.

If the wrapper used a projection for the extended interface, it could expose the value with an effectful property, and the result would be something like this:

@TaskLocal
static var requestID: RequestID?

func readExample() async {
  if let id = await requestID {
    ...
  }
}

func writeExample(id: RequestID) async {
  await $requestID.withValue(id) {
    ...
  }
}

Now, since setting a value is always a scoped operation and can't be expressed as an assignment, you'd always have to use a projection (the $ variable) to do it. We could consider sugaring that eventually if people felt it was a serious problem. Personally, I think the nesting is more objectionable than the $, and that's unavoidable without a new feature for scoped operations.

9 Likes
8

Thanks for chiming in John.

Yes I have considered that, well started out with that even, as people were demanding the wrapper approach.

The syntax you suggest is indeed what we end up with then, with one caveat though: the read cannot be an async get because we must be able to read those values from synchronous functions, so this is a no go:

  if let id = await requestID {

and we'd have to go with:

  if let id = requestID {

which had me nervous about making this read seem super simple while it's heavier than "just" a static property read.

The reason the read must be allowed from synchronous code is simple: imagine swift-log, or os-log etc... they are definitely not async functions, yet are one of the primary consumers of such values in order to print correlation IDs etc. It would be very unfortunate to force simple readers of properties to have to go through UnsafeCurrentTask to perform the reads somehow.

If we don't worry about that... then yes, this API would work:

@TaskLocal
static var requestID: RequestID?

func readExample() async {
  if let id = requestID {
    ...
  }
}

func writeExample(id: RequestID) async {
  await $requestID.withValue(id) {
    ...
  }
}

I don't love the $ but as you say, it's not a "deal breaker" perhaps.

9

In practice I think this is very rare. A specific instrumentation or tracing library usually uses one value to carry all information it needs.

It's true that it is harder to express this API with the property wrappers, it would come back to some form of Task.withLocal($requestID, boundTo: xxx).withLocal($other, boundTo: yyy) { ... }.

10

I was thinking that you could have a "taskless" accessor on the projection, but you're right, there's no real reason to force the getter to be async in the first place.

3 Likes
11

Right yeah, so that's an API that can work.

At this point it really boils down to preferences... If we like the property wrapper in general:

  • do we prefer value to be a read and $value.withValue to be the binding, or
  • do we prefer value.get() to be the read and value.withValue to be the binding.

For the latter, declarations have to use the type inside the wrapper, like @pertti explored some more, and for the prior, we can just use the : Type style which is a bit more normal but IMO makes the use sites more ugly.

Neither of the styles is a huge win over the other I feel and I’d be okey with either...

Declarations are very rare so having them be a bit annoying is better than every read/bind being annoying I thought, thus the wrapped value of different type than the T idea.

12

I'll chime in and note that types derived from initial values are an abomination for non-local variables (…IMHO, but it also has compile time implications). That's a big point towards the property wrapper form for me, even though it makes the access look very lightweight.

2 Likes
13

If we used projections, reading would be a little cleaner (losing the get()) and writing would be a little uglier (adding the $). I don't know how to evaluate the trade-off; I know that writing is very common in tracing code. But frankly, writing is inherently cumbersome right now because of the nested closure.

I think the right long-term solution here would need to build on top of a "using-function" feature, which would be a coroutine-based feature like read/modify that would allow you to initialize and destroy something without introducing a nesting level / semantic context difference at use sites. Basically, run some code, return something to your caller, and then get resumed later to clean things up. That would bee a very general feature with broad applicability. If we had that, we could then come up with some sort of "scoped overwrite" feature, like:

scoped_overwrite requestID = id   // <- intentionally terrible syntax

which by default would desugar as:

let old = <variable>
<variable> = new
defer { <variable> = old }

but which property wrappers would have some ability to customize, which naturally they would do with a using-function.

5 Likes
14

The $ doesn't really seem to be an issue, as that's how all property wrappers with projections work. As property wrappers grow in popularity this sort of syntax should become fairly common.

2 Likes
15

Yeah that's true, thanks for chiming in.

Yeah, that's fair as well. Indeed setting is annoying with the nesting anyway.

It's also true that setting stuff is usually somewhere deep in some framework/runtime/library internals and users of given library don't really have to do so. As long as reading and declaring is nice to read, that's what most people will be seeing.

Setting is only done by a fairly limited set of people -- the library/framework developers most of the time. So having it be slightly uglier is the right tradeoff.

Yeah indeed, that'd be some nice future work to de-clutter this pattern.

With all that it really seems we have but one option:

@TaskLocal
static var requestID: RequestID?

func readExample() async {
  if let id = requestID {
    ...
  }
}

func writeExample(id: RequestID) async {
  await $requestID.withValue(id) {
    ...
  }
}

and embrace the $ :wink: :moneybag:

I'll change the implementation adjustment PR to do that.

6 Likes
16

If you'd like to modify the proposal to that, please open a PR against swift-evolution, and I'll talk to the rest of the core team about what to do. Presumably we'll just extend the review a little longer.

2 Likes
17

Okey, I’ll let you know.

Poking at the implementation now to make sure it’s all actually correct and we didn’t miss something.

Sorry about the amount of back-and-forth on the surface API of this feature... Funnily enough, the runtime details have never changed nor have had much concerns about, so at least that seems stable.

5 Likes
18

I think the first of these is clearly ”going with the grain” of property wrappers, and the other is ”going against the grain”. Reading a value from a property wrapper is precisely what wrappedValue is for, and exposing more API is precisely what projectedValue is for.

If reading the value as value makes the read feel too lightweight it sounds like the real issue is that property wrappers are the wrong tool here (if you’ll allow the mixed metaphor).

8 Likes
19

I think this is the right approach if we are to use property wrappers. This approach feels perfectly inline with what property wrappers were designed for, and it provides nicer diagnostics if the user accesses the value incorrectly. The get() approach would lead to pretty poor diagnostics if the user forgets the get(), because they'd probably end up with type errors mentioning the TaskLocal<...>.Access type.

For what it's worth, the "not O(1) access" isn't a huge concern for me, and we can always make this more obvious through tooling. It should of course be documented, and we could potentially propagate documentation from the wrapped/projected value in the property wrapper to the computed properties when the wrapper is applied through SourceKit.

13 Likes
20

This is already supported today with property wrappers. If you drop the parenthesis and the property wrapper has an init(), the property wrapper will be default initialized. Default initialization also allows the type of the property to be inferred, even when you don't include the parenthesis, e.g.

@propertyWrapper
struct DefaultInitWrapper {
  var wrappedValue: Int = 10
}

struct S {
  @DefaultInitWrapper var value // okay, value is inferred as `Int`
}
4 Likes