Stop leaking implementation detail in Debug impls of opaque types

[LukasKalbertodt]

This changes the Debug impl of:

• core::any::TypeId
• core::mem::Discriminant
• std::thread::ThreadId
• std::time::Instant

All these types are explicitly described as "opaque" in the documentation, so they shouldn't print implementation details in their Debug implementation IMO.

The change for ThreadId can possibly be reverted in the future, when the method thread_id_value is stabilized: #67939. The same goes for Discriminant once/if the DiscriminantKind trait gets stabilized. But until then, these are opaque types.

No one should rely on the output of Debug impls, so this shouldn't break anything. However, just to be sure, crater was used before for changes like this.

Curious to hear what you think about this change.

[rust-highfive]

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

[pnkfelix]

My own instinct is that the Debug trait is for developers, and that developers sometimes benefit from seeing implementation details. The primary purpose of privacy restrictions are 1. to enable local representational changes without danger of breaking the broader ecosystem and 2. to enable local reasoning about module or type level invariants.

While exposing internal details in Debug does allow a determined developer to sidestep both 1 and 2 above, my gut feeling is that its no different from someone using transmute to get similar access: We cannot stop a determined developer from shooting themselves in the foot in cases like this.

In any case, I also think that the benefits of leaving the information exposed are merely hypotheses on my part; I cannot currently point to an instance where a bug was resolved because the concrete TypeId was included in the Debug output. (If our debugger support were more robust, to the point where one could reliably get debug-formatted output of values from gdb or lldb, then I might have a different attitude here.) But I also think that the benefits of hiding the information are also merely hypothetical.

So I guess I'm waiting to see what others think, but if it were my decision alone, I'd probably decline merging this PR, at least in the absence of more concrete motivation.

[LukasKalbertodt]

Your reasoning makes sense to me.
What I kind of forgot to consider is that these "opaque" types do have some APIs to gather information, e.g. they implement Eq. So they are not fully opaque. If I print two TypeIds for debugging and get two times "TypeId(_)", then I (as a human) cannot know if those are the same TypeIds. And I guess just printing a number (which happens to be the internal representation) is an easy way to give the developer the ability to compare two values to one another.

I would say that "parsing the Debug output" could happen more easily than developers using transmute to access internal fields, but I agree that both things are very unlikely and that we cannot reasonably protect against developers who don't notice that both of these things are bad ideas.

To still add one piece of motivation for merging this PR: I vaguely remember that, as a Rust beginner, I was confused a few times about being able to see fields and their values in the Debug implementation, but that I couldn't access them. I remember that I thought "if they are shown to me here, then surely there is a way to access them". This just lead to me struggling for a while before finally understanding that that specific type indeed does not expose that information. I am pretty sure this happened with Instant in my case and that I tried to access the field tv_sec. It's nothing super important, but I still wanted to bring this up.

I very much hope this PR will not distract too much from more important work and does not waste a lot of t-libs time. I'm fine with either merging or closing!

[dtolnay]

I would prefer not to make this change. IMO the correct way to design Debug impls is by putting in whatever makes them most useful, and it would be hard to argue that TypeId(_) is "more useful" than TypeId(0), for reasons pointed out in #82416 (comment).

There is an improvement opportunity however to make the Debug representation of all these types consistent among one another. Currently we're printing TypeId { t: 0 } as a struct and Discriminant(0), ThreadId(0) as tuple structs. I wouldn't mind agreeing on one or the other representation.

Regarding "if they are shown to me here, then surely there is a way to access them", changing the format to TypeId(private 0) would be quite reasonable and effective I think.

[LukasKalbertodt]

Lets just close this PR then. Everyone (including me by now) seems to agree that this specific change is not a good idea. The debug output could be improved still, but that will have to happen in a different PR.