atomic: remove 'Atomic*' from Debug output

[seanmonstar]

For the same reason that we don't show Vec { data: [0, 1, 2, 3] }, but just the array, the AtomicUsize(1000) is noisy, and seeing just 1000 is likely better.

[rust-highfive]

r? @aidanhs

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

There are some stability concerns here (see the thread in #47619) for a similar change that caused somewhat widespread breakage.

However, I feel that it is warranted to make this change -- with a note for the compatibility notes.

[hanna-kruppe]

I'm not sold on printing just 1000 instead of something indicating the atomic aspect. A better analogy than Vec would be Cell<usize>, which also has interior mutabilty and is in many ways just the single-threaded equivalent to AtomicUsize. And cells are formatted as Cell { value: 1000 }, not just 1000.

One reason for that, I imagine, is that the interior mutability has repercussions for when the value can change, which is something you'd really want to keep in mind while debugging by looking at snapshots of the program state. Thus the reminder that this is not an ordinary integer but one with interior mutability is useful and not just noise. This is even more drastic for atomic primitives than for Cell, since printing an atomic value that another thread is modifying is the very model of a race condition -- assuming these are normal shared-xor-mutable integers could easily be very misleading.

[seanmonstar]

@rkruppe I was actually looking at Cell, and thinking of making another PR to change it also, because I think the Cell { value: ... } is unnecessary noise. I know I personally have many times avoided derive(Debug) because of these outputs, wanting to make debug logs easier to parse (as a human).

I do think that the extra info is probably needed for RefCell and Mutex, since it is possible that the fmt function cannot get a view, and thus needs to show <borrowed> or <locked>.

[aidanhs]

I personally am prefer it as-is (for the same reasons as Cell outlined above).

But up to libs team - at random, r? @withoutboats

[seanmonstar]

I'll share what I'm looking at that prompted me writing this patch:

TRACE foo::bar::baz: poll; rx=Receiver { length: AtomicUsize(0), capacity: AtomicUsize(1000) }
TRACE foo::bar::baz: poll; rx=Receiver { length: AtomicUsize(1), capacity: AtomicUsize(999) }
TRACE foo::bar::baz: poll; rx=Receiver { length: AtomicUsize(2), capacity: AtomicUsize(998) }
TRACE foo::bar::baz: poll; rx=Receiver { length: AtomicUsize(0), capacity: AtomicUsize(1000) }

To me, that is noisy and doesn't provide any benefit, compared to:

TRACE foo::bar::baz: poll; rx=Receiver { length: 0, capacity: 1000 }
TRACE foo::bar::baz: poll; rx=Receiver { length: 1, capacity: 999 }
TRACE foo::bar::baz: poll; rx=Receiver { length: 2, capacity: 998 }
TRACE foo::bar::baz: poll; rx=Receiver { length: 0, capacity: 1000 }

Notably, those counters are actually Arc<AtomicUsize>, but thankfully, it already doesn't print out Arc { value: ... }.

I don't feel that since some internal, private state of the item needs atomic operations to change is important at all when printing a Debug format of the type.

[emilyalbini]

Ping from the release team @withoutboats! This PR needs your review!

[emilyalbini]

Ping from triage! This PR needs a review. Can @withoutboats or someone else from @rust-lang/libs do it?

[alexcrichton]

I'd personally lean towards removing wrapper types where possible in the sense that we don't want to debug Box<T> as "Box { inner: T }". Intuitively I'd add wrapper strings/descriptions where it ends up adding semantic value to the debug output, for example File { fd: 32 } is far more useful than 32, but AtomicUsize(32) isn't conveying much more over 32 (other than maybe that it can be modified with a shared borrow?)

I'd like some input from others at @rust-lang/libs, though, so I've tagged this now as waiting on the team. I also agree though that we'd want to be consistent, so it'd be great to touch up other known cases (like Cell) in this PR when a conclusion is reached as well

[emilyalbini]

Ping from triage @rust-lang/libs! Your opinion is needed :)

[SimonSapin]

+1. I’m in favor of making wrapper types like this and Cell "transparent" in Debug impls.

[alexcrichton]

This was discussed during @rust-lang/libs triage and the conclusion was that this is good to go. Follow-ups can always update the other types in libstd.

@bors: r+

[bors]

📌 Commit c689db2 has been approved by alexcrichton

[bors]

⌛ Testing commit c689db2 with merge 230b97a...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 230b97a to master...