Disentangle Debug and Display for Ty.

[nnethercote]

The Debug impl for Ty just calls the Display impl for Ty. This is surprising and annoying. In particular, it means Debug doesn't show as much information as Debug for TyKind does. And Debug is used in some user-facing error messages, which seems bad.

This commit changes the Debug impl for Ty to call the Debug impl for TyKind. It also does a number of follow-up changes to preserve existing output, many of which involve inserting
with_no_trimmed_paths! calls. It also adds Display impls for UserType and Canonical.

Some tests have changes to expected output:

• Those that use the rustc_abi(debug) attribute.
• Those that use the rustc_layout(debug) attribute.
• Those that use the EMIT_MIR annotation.

In each case the output is slightly uglier than before. This isn't ideal, but it's pretty weird (particularly for the attributes) that the output is using Debug in the first place. They're fairly obscure attributes (I hadn't heard of them) so I'm not worried by this.

For async-is-unwindsafe.stderr, there is one line that now lacks a full path. This is a consistency improvement, because all the other mentions of Context in this test lack a path.

[rustbot]

r? @oli-obk

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

[nnethercote]

This has a lot of overlap with #107084, but is a bit smaller. There are some follow-ups that could be done, but I think it's worth focusing on the minimal core to start with.

r? @compiler-errors

[compiler-errors]

r=me if you fix the layout tests, or else maybe we should further discuss the tradeoffs of using debug vs display for that...

[nnethercote]

All comments addressed. Thanks for the fast review.

@bors try

[bors]

⌛ Trying commit 7d8c291 with merge 30419fb...

[bors]

☀️ Try build successful - checks-actions
Build commit: 30419fb (30419fb77cdf2f0cc1d5fc1700123372bf34385d)

[nnethercote]

@bors r=compiler-errors

[bors]

📌 Commit 64ea8eb has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 64ea8eb with merge 7d1e416...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 7d1e416 to master...

[rust-timer]

Finished benchmarking commit (7d1e416): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.2%]	4
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	3
All ❌✅ (primary)	-0.3%	[-0.3%, -0.2%]	4

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 628.501s -> 630.224s (0.27%)
Artifact size: 317.64 MiB -> 317.56 MiB (-0.02%)

[RalfJung]

It looks like this made the debug printing of TyAndLayout, MPlaceTy, mir::ConstantKind, ty::Const and a bunch of other types that don't have Display implementations (or whose Display hides too much information) a lot more verbose and much harder to read. That's a problem, I have to stare at that output regularly. So for me this PR is strictly a regression. We don't have Display implementations for those types since having to maintain those by hand is too much effort (or because that impl is actually user-visible and made to be nice rather than informative to a compiler dev).

Having Display and Debug on types be identical was a deliberate choice for this exact reason. (We use this strategy for a bunch of common compiler types, otherwise a lot of Debug output would be completely unreadable. Other examples are AllocId, Size and Align.)

Can we find a solution to that? IMO the previous situation was actually pretty reasonable; if you wanted the full output you'd debug-print ty.kind() and if you wanted the nice output you'd print ty.

[nnethercote]

Printing ty.kind() wasn't a satisfactory solution. It would work for the outer layer, but nested values would end up getting printed with Display.

Here are some principles that I think are relevant here.

• Debug and Display impls serve different purposes, and shouldn't be mixed. As per the docs:
  • "Debug should format the output in a programmer-facing, debugging context."
  • "Display is for user-facing output."
• If default Debug output is too ugly, then a nicer Debug impl should be written. The Display impl shouldn't be used.
• Debug output should not hide anything, i.e. a value should be reconstructable from its Debug output.
  • The old Debug implementation did not do this and this was a problem for me. I was doing some logging-based profiling of Ty values and distinct values were being printed as if they were identical, which prevented me from getting useful data.
  • In make ty::Ty: Debug not call the Display impl #107084, @BoxyUwU had a similar-but-different motivation: "This means that ICE messages, debug statements etc in rustc all print out things that are missing useful details."

The old code violated several of these principles, which worked ok in some cases but caused problems in other cases.

We don't have Display implementations for those types since having to maintain those by hand is too much effort (or because that impl is actually user-visible and made to be nice rather than informative to a compiler dev).

Why is Display too hard to implement? I'll guess it's because TyAndLayout is a type with lots of nesting, and it's really convenient to use {:#?}, and Debug output for most of the fields is fine, it's just the Tys within for which it's too verbose?

(This is a good illustration of the old code's shortcomings: in order to print out TyAndLayout a particular way, a global decision has been about the printing of Ty, which works well for TyAndLayout but is a problem elsewhere.)

The nicest solution here would be an improved Debug impl for ConstKind, one that is more compact while still preserving all information. That would be useful for TyAndLayout and everywhere else.

Alternatively, it is possible to implement Display for a type in a way that leans heavily on Debug to print most of the fields. Which is mixing the two, but in a less objectionable way. This can be done in a way where using {:#} will give nice multi-line output for large types.

A more hacky solution: #107084 introduced a hand-written Debug impl for TyAndLayout that addressed some of your concerns; it used Display for the ty field while using Debug for everything else. (This required hand-written Debug impls for ArgAbi and FnAbi as well.) It still mixes Display and Debug impls, but for TyAndLayout which is not as widely used as Ty, so it's still an improvement over the old code.

[RalfJung]

Printing ty.kind() wasn't a satisfactory solution. It would work for the outer layer, but nested values would end up getting printed with Display.

That sounds like something that can be fixed by changing the debug-printing of TyKind, without changing type printing for the entire compiler?

"Debug should format the output in a programmer-facing, debugging context."

Yeah, and when I debug the compiler, I need output that fits on a screen. This PR breaks that.

I've literally never wanted to see the debug printing this PR produces. So you cannot claim that your implementation is more suited for compiler debugging than the old one. It is just suited for debugging a different part of the compiler.

(This is a good illustration of the old code's shortcomings: in order to print out TyAndLayout a particular way, a global decision has been about the printing of Ty, which works well for TyAndLayout but is a problem elsewhere.)

The new code has the same shortcoming. You just optimized printing types for a different part of the compiler.

(And it's not just TyAndLayout, as I mentioned it's also ty::Const and mir::ConstantKind and probably a lot more.)

The nicest solution here would be an improved Debug impl for ConstKind, one that is more compact while still preserving all information. That would be useful for TyAndLayout and everywhere else.

It would be an improved, hand-written Debug impl for every single of these types that have a Ty field, not just ConstKind. I don't see how you can call that "nice".

a hand-written Debug impl for TyAndLayout that addressed some of your concerns;

That only helps for a subset of the types, namely those that embed TyAndLayout, but not those that embed Ty. Since that PR, we've adjusted MPlaceTy and friends to no longer print the entire TyAndLayout, but only the Ty inside it, so the TyAndLayout implementation wouldn't even help. We'll also have to adjust all the interpreter types. And then there's ty::Const and mir::ConstantKind and basically everything else in rustc_middle::mir -- I don't think we want the verbose printing for any of the MIR types. It's a bad default for all of them, IMO.

[oli-obk]

We could change TyKind::Debug to a manual impl that is less verbose, but still preserves all information recursively

[BoxyUwU]

TyKind: Debug does have a manual impl that's less verbose, and so does ConstKind. I worked on that for a while after i stopped working on my version of this PR. The impls aren't perfect but tuples actually print as tuples, refs print as refs etc. There's just a few things that still have old style derived debug outputs. (adts for one which is unfortunate and probably the biggest source of verbosity)

[RalfJung]

#115866 fixes the fallout for the interpreter and TyAndLayout. (I took the relevant commit from your #107084.) MIR printing does not seem to have completely exploded so a lot of it seems to already use the Display variant.

However, mir::ConstantKind seems to still have a derived Debug, so that will be messy.

There's just a few things that still have old style derived debug outputs. (adts for one which is unfortunate and probably the biggest source of verbosity)

Yeah, having ADTs not print out the raw AdtDef + GenericArgs would help a lot. Also, the sizes of array types are super verbose.

[nnethercote]

Thanks for doing #115866, I think that's a good follow-up.

Yeah, having ADTs not print out the raw AdtDef + GenericArgs would help a lot.

👍

[RalfJung]

#115884 should help with arrays.

[jackh726]

Another pretty clear regression in type printing from this is placeholders:

placeholder=!2_BoundRegion { var: 0, kind: BrNamed(DefId(0:8 ~ higher_ranked_lifetime_error[3c91]::assert_all::'_), '_) }

as opposed to

placeholder=!2_0

(impl here)

I think overall, printing of type info is quite difficult to get "right". While I can understand the sentiment of "Debug formatting shouldn't hide anything", I think there's more nuance to that. Reading through debug logs is difficult - it's made even more difficult when there is a ton of extra info around that's not strictly needed to understand the general idea of what's going on.

(Asking because I haven't really read through the code) How does this interact with -Z verbose? I'm wondering if it makes sense to have the opposite flag (i.e. -Z not-verbose). I can certainly understand wanting to have all the info available by default for ICEs, backtraces, etc. where we might get a report and want that info, but I think we should also be cognizant of the developer experience that this extra verbosity brings.

I don't think I'd go as far to say this should be reverted until an MCP resolves, but I do think it probably would have been reasonable to have opened one for it (though, I can imagine this sort of problem might not have been expected). It's clear I'm not the only one who's run into this.

[nnethercote]

placeholder=!2_BoundRegion { var: 0, kind: BrNamed(DefId(0:8 ~ higher_ranked_lifetime_error[3c91]::assert_all::'_), '_) }

Much of this is the printing of the DefId, which is a strange case -- its Debug impl does a lookup in order to print extra stuff that is outside the DefId. I've never found this extra stuff useful but I guess others have?

If that extra stuff was gone it would become:

placeholder=!2_BoundRegion { var: 0, kind: BrNamed(DefId(0:8), '_) }

which would be a lot shorter. And it's not hard to imagine some more concise custom syntax for BoundRegion and BoundRegionKind.

[RalfJung]

The DefId without the extra stuff seems completely useless? One has to know what "0:8” refers to, after all. It's like a reference, which prints what it points to, even for Debug.

[RalfJung]

But yeah given the large impact of this change it is definitely MCP-worthy.

[nnethercote]

The DefId without the extra stuff seems completely useless? One has to know what "0:8” refers to, after all. It's like a reference, which prints what it points to, even for Debug.

Sure, but is dereferencing it the right thing to do in Debug output? It's very verbose. Merely printing everything in a type has been controversial up above, so I'm interested to see extra stuff being pulled in, in some cases.

[RalfJung]

I think if it prints a DefId it needs to print what it points to. A DefId without that is about as useful as a raw memory address. The only reasonable way to shorten that output is to omit the DefId entirely. I don't work with placeholder types though so I cannot say what is most useful to debug them.

[nnethercote]

I think printing a raw memory address is potentially a reasonable thing to do. It's what you'd get with a derived Debug impl, for example.

[RalfJung]

impl Debug for &T doesn't print raw memory addresses. I don't see any reason why we'd do that in the compiler.

Sure, sometimes it's what you want, and then you manually cast to a raw ptr or use {:p}.

[nnethercote]

The raw memory address is a distraction. A better example would be a type with an index. There are many examples. Printing the index by itself isn't that informative. But I think DefId is the only one where an index is used to lookup the corresponding table.

[RalfJung]

Another problem caused by this PR is that the output of a failing assert_eq!(ty1, ty2) is now sometimes unreadable:

 left: Binder(fn(&ReLateBound(DebruijnIndex(0), BoundRegion { var: 0, kind: BrAnon(None) }) Arena<ReLateBound(DebruijnIndex(0), BoundRegion { var: 1, kind: BrAnon(None) })>, &ReLateBound(DebruijnIndex(0), BoundRegion { var: 2, kind: BrAnon(None) }) Guard<ReLateBound(DebruijnIndex(0), BoundRegion { var: 3, kind: BrAnon(None) })>, Foo<ReLateBound(DebruijnIndex(0), BoundRegion { var: 1, kind: BrAnon(None) }), ReLateBound(DebruijnIndex(0), BoundRegion { var: 0, kind: BrAnon(None) })>) -> TokenFoo<ReLateBound(DebruijnIndex(0), BoundRegion { var: 1, kind: BrAnon(None) }), ReLateBound(DebruijnIndex(0), BoundRegion { var: 2, kind: BrAnon(None) }), ReLateBound(DebruijnIndex(0), BoundRegion { var: 3, kind: BrAnon(None) }), ReErased>, [Region(BrAnon(None)), Region(BrAnon(None)), Region(BrAnon(None)), Region(BrAnon(None))])
right: Binder(fn(&ReLateBound(DebruijnIndex(0), BoundRegion { var: 0, kind: BrAnon(None) }) Arena<ReErased>, &ReErased Guard<ReErased>, Foo<ReErased, ReErased>) -> TokenFoo<ReErased, ReErased, ReErased, ReErased>, [Region(BrAnon(None))])

@nnethercote could you file an MCP so we can collect these issues in a better place than a merged PR?

[nnethercote]

I'm travelling for the next week. I can file something when I get back, if someone else doesn't want to file something in the meantime.