SE-0258: property wrappers, revise the Atomic example.

[atrick]

Use 'class' rather than 'struct' because there is no way to make a
struct property atomic from within the wrapper.

TSAN will correctly report an error if you use a struct Atomic
wrapper. Some developers have been baffled by the TSAN failure, which
actually correctly identifies the bug.

Fixes rdar://79173755 (TSAN violation on standard Atomic property wrappers)

[atrick]

@lorentey @stephentyrone I was using this PR to explain the issues with implementing atomics via property wrappers. But it may be more appropriate to remove references to atomics completely. After all, people know by now why property wrappers are useful, and we have a swift-atomics package for this use case. Is there any value at all in talking about atomic wrappers?

Maybe I should just add a short section explaining why the atomic wrapper example was removed instead.

[rjmccall]

Suggestion:

The Atomic property wrapper is a class rather than a struct because the memory guarded by synchronization primitives must be independent from the wrapper. In Swift's formal memory access model, methods on a value types are considered to access the entire value, and so calling the wrappedValue getter formally reads the entire stored wrapper, while calling the setter of wrappedValue formally modifies the entire stored wrapper. This happens outside of the wrapper implementation, and the wrapper has no ability to affect it. If this formal access actually causes the memory to be copied, the atomicity of the operation will be broken. Because of this, using a struct is not semantically correct, and tools such as Thread Sanitizer will correctly report races when the atomic is used concurrently. Methods on classes, in contrast, are just passed a reference to self and do not automatically formally access its memory, so they can internally ensure the memory is only accessed using appropriate atomics. Using a class wrapper does mean that the wrapped value will have reference semantics and so different copies of it will share the same atomic storage. Allowing an atomic wrapper with value semantics is an interesting future direction.

[lorentey]

These two don’t make much sense to me — Value isn’t constrained to a protocol that supports these. (Neither is it constrained to things that can implement load with a custom ordering, but that could be considered an implementation detail that was elided in this illustration.)

[lorentey]

@atrick I agree; I don’t think this has any practical benefit, but it has at least a couple deadly problems.

Hiding atomic operations behind regular-looking getter/setters seems like a very, very bad idea to me. Atomic operations must be explicit; having to spell out load(ordering:)/store(ordering:) is a feature, not a bug.

The setter is a particularly unfortunate idea — it allows obvious(?) race conditions like atomicInt += 1 or atomicArray.sort() to compile with no diagnostics. (And TSan won’t detect them, either. 😭)

Converting this wrapper to a class also has the unfortunate (but unavoidable) side effect of violating value semantics if it is ever used on a struct property. In practice, we’d want to constrain this to be only used within classes. (Which, IIRC, isn’t a thing we can do within this proposal.)

(An Atomic wrapper would perhaps make more sense if it was a higher level, locking construct that only provided a getter, but if you can’t mutate it, what exactly is the point of it? If this is the intended use case, I also don’t understand what the memory ordering arguments are supposed to mean.)

I think we should either remove this completely or we should at least add an extremely loud warning that this is not a good idea to implement in practice. (I know people keep trying to do it, and I expect that at least some of these attempts are directly inspired by this document.)

[atrick]

@atrick I agree; I don’t think this has any practical benefit, but it has at least a couple deadly problems.

This pattern is actively harmful for addressing race conditions. At the time this SE was reviewed, there simply wasn't any reasonable alternative. The most helpful thing would be to remove the example and add John's language to the revisions section to document the issue along with a link to the swift-atomics package. None of this is a necessary part of the property wrapper proposal, but given that the pattern was introduced here it's the best place to document the problem.

[DougGregor]

@atrick it looks like there's one last comment from @lorentey about increment()/decrement() that should still be addressed before merging. Is there anything else?

[atrick]

@DougGregor I should revise this PR. Rather than merge it as-is, we should remove the Atomic example completely. It isn't needed to justify the feature and it's harmful as guidance to any programmer looking to implement atomics. I should explain why the Atomic wrapper was removed in the revisions section for people who come here looking for it and point to the swift-atomics package instead.

[atrick]

@lorentey would you care to take a quick look at this draft. I do believe it's finally ready to merge.

[lorentey]

This looks great!

[rjmccall]

Looks good to me, too. Merging.

[CrystDragon]

Sorry to leave my comment here, because there's no issue page for this repo.

Isn't the current text somewhat contradictory?

In section Revisions - Changes from the accepted proposal:

For those who have already attempted to implement something similar, here is the original example, and why it is incorrect:

But the code below this statement is in fact correct, because the code uses a class, while the original example uses a struct.

@propertyWrapper
class Atomic<Value> {
  private var _value: Value
  ...
}

The text may mislead readers into thinking using class here is also incorrect.

[lorentey]

Using a class is incorrect as well! The last paragraph of the new section explains the issue — it is a fundamental problem with the idea of implementing atomic access entirely through load/store operations.

The property wrapper is encouraging race conditions by providing an illusion of convenience: operations such as foo.counter += 1 would not actually execute as a single atomic operation. Allowing such syntax would not be productive, as its use would unavoidably lead to incorrect results.

[CrystDragon]

@lorentey Thanks for your clarification.