proposal: sync/atomic: deprecate AddXxx, CompareAndSwapXxx, LoadXxx, StoreXxx, SwapXxx

[uji]

Go 1.19 defines new atomic types Bool, Int32, Int64, Uint32, Uint64, Uintptr, and Pointer (release note)

Using these types is more convenient and secure than using AddXxx, CompareAndSwapXxx, LoadXxx, StoreXxx, SwapXxx.
(ex. func AddInt32(addr *int32, delta int32) (new int32))

Moreover, the performance is almost the same, we do not believe there are any situations where AddXxx, CompareAndSwapXxx, LoadXxx, StoreXxx, SwapXxx should be used, and deprecation would limit the user's choices and improve the experience.

Is there a reason that we can not deprecate these functions?

[mvdan]

Here's a reason to not deprecate them: if an existing exported struct type has an int64 field used with atomics like StoreInt64, changing that field type to atomic.Int64 would technically be a breaking change. For example:

type Foo {
    Counter int64
}
--
type Foo {
    Counter atomic.Int64 // breaking change
}

We could still add a hint to the old APIs to point out that the new named types exist, though.

[randall77]

I don't think there is any need to deprecate the old functions. They work. There's nothing wrong with them.

I think if someone was starting a new codebase they should use the new types+methods, but in existing codebases there's no reason to force/nudge people to use the new way.
So maybe a comment saying "consider using ... instead" would be ok, but deprecation is more strident than that.

[zigo101]

The old way is more memory efficient than new ways on 32-bit architectures, which is another reason it should not be deprecated.

[zikaeroh]

The old way is more memory efficient than new ways on 32-bit architectures, which is another reason it should not be deprecated.

That doesn't seem right; I thought that part of the reason these types were added was so they'd get compiler support to prevent misalignment on those architectures. You might get a smaller struct if you don't use them, but your atomic operations might not work either, right?

[bcmills]

@mvdan, it's true that changing the type of an existing field would be a breaking change, but “avoiding a breaking API change” is IMO one of the few good reasons to add new usage of a deprecated API.

[bcmills]

@randall77

They work. There's nothing wrong with them.

I guess that's true of the 32-bit functions, but for the 64-bit functions there is a prominent BUG comment that also gets rendered on https://pkg.go.dev/sync/atomic and perhaps in other places.

It may be true that “they work” subject to that caveat, but that seems like a far cry from “nothing wrong with them”. 😅

The Pointer variant also has the downside of being inherently type-unsafe to use. (The other variants are often also type-unsafe in practice, since one may end up converting between a named integer type and the operand type, but they're at least theoretically usable for variables of unadorned types.)

[mvdan]

@bcmills I guess you're right. By my reasoning that switching to (or from) any named type can be a breaking change, we would never be able to deprecate a type. But that's clearly not right; sometimes there are good reasons to deprecate a type.

[bcmills]

I think if someone was starting a new codebase they should use the new types+methods, but in existing codebases there's no reason to force/nudge people to use the new way.

Doesn't “deprecate” imply exactly that — avoid in new code, still works for existing code? (That's certainly how I interpret it, but maybe I've spent too long in Google's codebases. 🤔)

[mvdan]

Doesn't “deprecate” imply exactly that — avoid in new code, still works for existing code?

I think there's a slight disconnect because some people treat deprecation notices as more imperative. For example, staticcheck uses them as warnings, and some people like me treat staticcheck warnings as CI errors. I would have to go out of my way to tell staticcheck that, in a particular case, that deprecation warning is OK to keep because I must follow semver. Which I guess is fine for the edge case - but that is why "still works for existing code" isn't true out of the box for any staticcheck users.

[bcmills]

@go101

The old way is more memory efficient than new ways on 32-bit architectures, which is another reason it should not be deprecated.

Which cases are more memory-efficient? The new way does force correct alignment for 64-bit fields, but the implementation of the old functions also requires the user to manually perform that same alignment in portable code.

[randall77]

I would be ok with deprecating just the 64-bit ops on 32-bit platforms.