Keep printing extra comments in MIR dumps

[lqd]

#112346 seems to have removed comments from MIR dumps altogether, not just from mir-opt dumps or in the mir-opt tests. It's unclear to me if it was intentional or not.

In any case, that in turn made the NLL mir dumps (-Zdump-mir=nll) lose all the important information without -Zmir-include-spans. For context, in case anyone reading this doesn't know: people working with borrowck data deal with CFG points/MIR locations in both region and liveness values, and the extra type info also contains lifetimes, and is used to reason about outlives constraints.

For example:

| Inferred Region Values
...
| '?2 | U0 | {bb0[0..=1], bb1[0..=4], bb2[0..=4], bb3[0], bb4[0..=6], bb5[0..=3], bb6[0], bb7[0..=8], bb8[0], bb9[0..=6], bb10[0..=8], bb11[0..=1], bb12[0], bb13[0], '?2}
| '?3 | U0 | {bb0[1], bb1[0..=4], bb2[0..=4], bb3[0], bb4[0..=6], bb5[0..=3], bb6[0], bb7[0..=8], bb10[0..=8]}
| '?4 | U0 | {bb0[0..=1], bb1[0..=4], bb2[0..=4], bb3[0], bb4[0..=6], bb5[0..=3], bb6[0], bb7[0..=8], bb8[0], bb9[0..=6], bb10[0..=8], bb11[0..=1], bb12[0], bb13[0], '?1}
| '?5 | U0 | {bb4[4..=6]}
...
| Inference Constraints
...
| '?12 live at {bb7[8]}
| '?14 live at {bb1[0..=4], bb2[0..=4], bb3[0], bb4[0..=6], bb5[0..=3], bb6[0], bb7[0..=8], bb10[0..=8]}

_15 = [closure@closures-in-loops.rs:22:16: 22:18] { x: move _16 }; // bb7[6]: scope 2 at closures-in-loops.rs:22:16: 22:37
                                         // closure
                                         // + def_id: DefId(0:4 ~ closures_in_loops[58cd]::repreated_unique_borrow::{closure#0})
                                         // + args: [
                                         //     i16,
                                         //     extern "rust-call" fn(()),
                                         //     (&'?9 mut &'?10 mut std::string::String,),
                                         // ]

Removing the comments is unfortunate here, so this PR keeps printing this information by default, for now.

Since -Zmir-include-spans is also used to display/ignore the extra comments passed to pretty printing, I've renamed it to -Zmir-include-extra-comments to describe it more generally, and flipped back the default to true to not regress NLL dumps.

Hopefully the new name and help text will also make it easier to find (arguably in these NLL dumps the statement location is the root of the info and not the spans, and I had to search GH PRs to find how to have it printed again).

To be clear, it's fine if we want to stop printing these comments by default if e.g. more people are annoyed by them than being helped. In that case, being able to force printing in specific passes could be good to preserve the NLL dumps' quality, or we can just as well also let the contributors and community know that they should adopt dumping with -Zmir-include-extra-comments.

r? @saethlin

[saethlin]

I agree that the previous flag name was bad. I've been considering changing it because I wanted to turn it on once and couldn't remember what it was or grep for it easily in rustc -Zhelp.

I wanted them off for UI tests to prevent standard library changes from breaking mir-opt tests.

I wanted these comments off by default everywhere because when I am discussing MIR optimizations I often want to paste MIR dumps into Discord, Zulip, or GitHub (in a comment). These comments are inconvenient in that context because they primarily contain useless/redundant information. Take this for example: https://godbolt.org/z/as759j1j6 The additional information in the comments is not enlightening at all, they will cause every platform to add a scrollbar because the comments have dramatically increased the width. Then on Discord things are triply bad; the amount of text in the comments causes the MIR dump to run well over the message size limit, and MIR dumps that somehow manage to be less than 2000 characters are automatically flagged as spam and deleted because the comments are so repetitious. So I repeatedly found myself engaging in a very silly exercise where I would get a MIR dump I wanted to discuss then have to manually edit out all the comments.

Now, the problem this has produced for you: NLL MIR dumps. I have never used these before. But also, it looks to me like the comment that you really want on by default here is not one that has ever been problematic for me. There are many places in the MIR dump code where we print MIR comments. Can we classify those to have something like -Zmir-comments=statements,terminators,debuginfo or something like that, where the default is just printing the comments that you find problematic to omit?

I would prefer to get the default right if we can, because the playground doesn't permit passing arbitrary compiler flags. You can only have the default behavior.

[bors]

☔ The latest upstream changes (presumably #115231) made this pull request unmergeable. Please resolve the merge conflicts.

[apiraino]

@lqd @saethlin I'm looking at this PR and I am unclear if there's agreement on these extra comments (as far as I can see both sides have good points). Is it the case for an MCP? Perhaps it was a good case also before proceedng with #112346

I am also reading that after #112346 some relevant code from the playground was dropped.

Opinions? thanks :)

[saethlin]

I'm waiting for a response from lqd because I don't understand use cases outside mine very well and I'd like to understand if what I suggested made sense.

I don't know about the playground. I was surprised to learn that it was depending on the comments, since the MIR dumps all start with

// WARNING: This output format is intended for human consumers only
// and is subject to change without notice. Knock yourself out.

[shepmaster]

I was surprised to learn that it was depending on the comments, since the MIR dumps all start with

I am aware of that comment, indeed I committed it ¹ as part of making a stable way to view MIR in the playground.

I parse the comment to mean "when this output changes, you don't have a right to complain about it". I have attempted to not complain about the removal of the source information, only to indicate that its absence is felt.

That being said, nothing about that comment indicates that the information currently provided in the MIR dump should not be used — such interpretation would be quite silly. The previous output included source information and I used it. The new output does not have source information to use and I commented on that.

In fact, I think the comment indicates that rustc could change the MIR output to replace all identifiers with 🤷 or delete all whitespace. I expect that if we did such a thing, there would be some comments on an issue that we could say "well, we told you not to get used to the file format" and we'd be 100% technically correct, the best kind of correct.

Footnotes

1. Sadly, I can't lay claim to the actual fun wording; that was all nikomatsakis. ↩

[Dylan-DPC]

@lqd any updates on this? thanks

[camelid]

I personally would like these comments to stay on by default. They're useful for understanding how MIR corresponds to source code, in the same way that godbolt highlights the relevant source code based on the cursor in the assembly code. I could imagine they're also helpful when fine-tuning the spans used in MIR-level diagnostics, like from the borrow-checker. The playground is an easy way to, well, play around with how code is compiled, and the debuginfo provided in the comments is helpful to that end.

If it's kept as opt-in, then on stable and beta RUSTC_BOOTSTRAP would need to be used, which seems non-ideal especially for the playground, where it would allow use of nightly features. And it becomes less discoverable for new rustc devs that this span information even exists. IMO it's easier to go looking for a flag that hides information than one that shows information you may not have known existed.

[lqd]

Sorry, I haven't had time to come back to this PR yet.
I'm personally not sure what the defaults should be either, as it seems some comments are useful and others aren't.

I suspect camelid, you're thinking of some of the OP's and not the e.g. // scope 1 at /rustc/bdbbb6c6a718d4d196131aa16bafb60e850311d9/library/core/src/option.rs:1072:18: 1072:19 saethlin was concerned about? (And I agree with Ben that these don't seem particularly helpful).

I'm sure we could classify those more precisely, and get some acceptable results. Maybe we could also just keep these lame ones off by default when they don't contain any other information, and keep the other more helpful ones on by default.

[camelid]

I suspect camelid, you're thinking of some of the OP's and not the e.g. // scope 1 at /rustc/bdbbb6c6a718d4d196131aa16bafb60e850311d9/library/core/src/option.rs:1072:18: 1072:19 saethlin was concerned about? (And I agree with Ben that these don't seem particularly helpful).

I'm not bothered by them, since they give information about the source of inlined code, but indeed my main concern is with span information from local code. However, I'm curious why they don't seem helpful to you? In general, it seems useful to me to be able to see what source code corresponds to what MIR code. Is it the excessive length of the path (due to the rustc version info)?

[lqd]

It's not that e.g. the inlined spans are not useful ever, but that they don't provide a lot of value in this particular context (for the local ones it's more arguable), and whether that is the more common use-case for people.

The spans and comments we add are useful to me every time I need to look at a dump, even more so under -Zdump-mir=nll. I don't expect this to be representative of most uses or of a wider audience. As I said in the OP, to me it's fine if we want to stop printing these comments by default if e.g. more people are annoyed by them than being helped.

(But also, I'm not personally invested enough to try and gather consensus or anything :3 you've already opened a zulip topic so let's see where that goes)

[oskgo]

@lqd Any updates on this? Thanks

[lqd]

Not since the last update #114652 (comment) and the zulip thread doesn't seem to have reached a conclusion so I'll close this.