core: Make most atomic functions generic

[nahla-nee]

View all comments

CC #130539

The existing PR (#153407) linked in the tracking issue has been dormant for a few months now and can't be merged due to conflicts. Additionally, it only implemented a few functions. This is a big change that implements most atomic functions generically for most types, with notable exceptions being AtomicBool (due to special emulation being difficult to work around), and AtomicPtr (due to not wanting to trample on strict provenance docs).

This is a pretty big change so included below is a list of large changes that were made and the reasoning for them:

1. Adjusted contract for AtomicPrimitive by changing requirement for the associated Storage type.
   Don't permit fewer validity invariants. Existing standard library code does several unsafe casts
   that directly transmute or cast Self to T and vice versa. For example from_mut will accept any
   mutable reference to T and cast it to a mutable reference to Atomic with no invariant checks.
   This condition is doubly asserted by the addition of the stipulation that transmuting between T
   and T::Storage must be valid.

2. Added the associated type OpType to Atomic primitive. Some types (bool) require being cast in
   order to perform operations atomic to an integer or pointer type. Adding this type to the
   contract allows for Atomic to also define generic code for functions such as store/load.

3. Introduced several new traits. These traits match the different "types" of atomics and allow
   specific types to opt into automatic generic implementations of certain classes of atomic
   functions.

4. Introduced new impl_atomic_traits macro to replace existing impl macro. This is mainly to
   support having so many new traits as opposed to just one.

5. Simplified the atomic_int macro and deleted large chunks of it. All the functions implemented
   by this macro are now handled by generics.

P.S. some attributes have been trampled over, specifically the stability attributes for both
const and non-const atomic functions. This is because In some cases a function would be
defined on different atomics with different stability versions, and matching each one to its
type would be a pain. Per a conversation on Zulip that included a library maintainer, it was
suggested that I use the newest version from competing implementation for each function. See
thread titled "How to handle modifying items with stable attribute".

[rustbot]

Thanks for the pull request, and welcome! The Rust Project has assigned @JohnTitor (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey

[nahla-nee]

I already got this check from tidy and I had to force push past it. Not only does it "look" wrong it also can't be satisfied as far as I'm seeing. It's essentially asking me to indent that block further, and if I were to do that it simply asks for it to be indented further yet again. I'm not sure if I'm missing something or not.

[nahla-nee]

Seems I got too over zealous. I didn't realize that the lack of unstable impls would cause a problem for the 128 bit atomics. Previous atomic int macro restored (under different name) to implement needed functions for 128bit atomics, and a manual implementation of AtomicPrimitive has been added for them.

[nahla-nee]

Ok, actually fixed now. Added another trait so that implementing AtomicPrimitive doesn't auto implement any traits, and the legacy macro now doesn't cause errors due to not being used on platforms without 128 bit atomics.

[JohnTitor]

r? libs

[Darksonn]

Why this particular API? Did this go through an ACP (Api Change Proposal)?

[nahla-nee]

I'm sorry I'm not entirely sure. This is my first time trying to contribute to Rust, so I just went through the CONTRIBUTING.md and a bit of the development guide before finding an issue and starting to work on it. I figured since it was part of steps of the tracking issue linked above that it had already gone through some kind of review and been accepted.

I might be confused but I don't think this changes the API. It introduces some unstable traits that are marked as implementation details, and derives those traits for various primitives but it doesn't introduce any new (stable) functionality and is backwards compatible. Should I create an ACP for this?

[Darksonn]

The first thing I will say is that this PR is too large and tries to do too many things at once. Regardless of everything else, it needs to be split into multiple smaller PRs. Compare with #153407, which avoids most of the traits by only implementing this for a subset of the methods on Atomic*.

Did you speak with the author of #153407 before filing this to confirm they do not wish to work on this anymore?

It introduces some unstable traits that are marked as implementation details, and derives those traits for various primitives but it doesn't introduce any new (stable) functionality and is backwards compatible.

Remember that the goal for most unstable APIs is to eventually be stabilized.

An ACP is usually filed before the unstable version of the API is implemented. This is because the purpose of the ACP process is to ask the library team whether this API is likely to succeed down the line, and is a mechanism to avoid spending time on implementing something that the library team doesn't think is a good idea.

For instance, a good question for the library team would be whether a design that involves this many traits is the correct API we want when this is eventually stabilized.

You can read more about the process for library changes here.

As another factor, just because an API is unstable, it still affects how things are rendered in the html documentation. This change looks like it would significantly change how the docs are rendered which is another important concern.

I figured since it was part of steps of the tracking issue linked above that it had already gone through some kind of review and been accepted.

So I'm guessing this came out of ACP#443, where the library team agreed that moving to a generic Atomic<_> type seems like a good idea in general. So there is some general agreement that we want the methods to be made into generic methods, but no discussion about precisely how that should be done.

[nahla-nee]

The first thing I will say is that this PR is too large and tries to do too many things at once. Regardless of everything else, it needs to be split into multiple smaller PRs.

That makes sense, sorry about that.

Did you speak with the author of #153407 before filing this to confirm they do not wish to work on this anymore?

I did not, I guess I assumed since it's been 6 months.

Remember that the goal for most unstable APIs is to eventually be stabilized. An ACP is usually filed before the unstable version of the API is implemented.

This makes sense, I didn't consider that. I'll go ahead and close this PR and write an ACP, if that's approved then I'll break this up into smaller chunk and resubmit.

[Darksonn]

No worries at all. I would also recommend joining our zulip. There's a t-libs channel, which would be a good place to discuss this kind of thing in a more informal setting than an ACP.