Add column number support to Backtrace

[est31]

Backtrace frames might include column numbers.
Print them if they are included.

[rust-highfive]

r? @dtolnay

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

[dtolnay]

Is this useful? I think I would prefer not to have these in the Debug representation. It just increases the chance of output lines being wrapped and harder to read.

[est31]

@dtolnay you can have foo(bar(), bar()). Then which of the two bar's are you inside?

I'll remove them from Debug though.

[RalfJung]

I'll remove them from Debug though.

So, Display has more information than Debug? That seems strange.

[est31]

So, Display has more information than Debug? That seems strange.

Personally I don't really care either way and will adjust this PR to what the libs team thinks it should contain. Note though that the Debug impl can always be changed, while an argument can be made that the Display impl can't be changed after stabilization.

[yaahc]

If we're worried about changes to the display / debug formats we could just add the column to the Frame type so that users can define custom formats on top of fn frames() via #78299

[est31]

@yaahc it is possible to add it in a fully backwards compatibe way but it'd feel like technical debt to me.

[yaahc]

@yaahc it is possible to add it in a fully backwards compatible way but it'd feel like technical debt to me.

How so? I don't see the backwards compatibility as an issue given that the interface isn't even stable yet and I don't see how it's technical debt, given we're talking about not adding it to the default display / debug formats because @RalfJung and @dtolnay feel it would make the output harder to read.

[dtolnay]

Is this useful? I think I would prefer not to have these in the Debug representation. It just increases the chance of output lines being wrapped and harder to read.

@dtolnay you can have foo(bar(), bar()). Then which of the two bar's are you inside?

I'll remove them from Debug though.

I missed that this PR affects not just Debug but Display as well (via backtrace-rs/src/print.rs) -- my preference is to have the column in neither representation. I think the vast majority of the time this would just be noise.

Definitely interested in supporting custom rendering of frames though.

[est31]

@yaahc

I don't see the backwards compatibility as an issue given that the interface isn't even stable yet

Right now backwards compatibility is not a concern, indeed. What I wanted to say with my statement above should be seen with the condition of stabilization in mind: once Backtrace stabilizes, a case can be made that columns may not be added to the output of Display, but they can be added to Debug.

It's also important to mention #72981 for context.

Regarding what I said about technical debt, note that I understood your comment in a way that you were trying to say that backwards compatibility isn't a concern because one can just add custom formatting APIs to std, or wrapper types that implement Display but print it in slightly different ways, etc. The original version of your comment before the two edits may have more clearly conveyed your intent, but the one I read was the second or third version of the comment. Anyways this becomes very meta now, let's move on :).

@dtolnay

my preference is to have the column in neither representation. I think the vast majority of the time this would just be noise.

Thanks for clarifying. I'll give two more arguments trying to convince you that it's not noise:

Another good use case is opening the line in the editor. E.g. I copy the line with the filename/line/col string and give it as a command to kate. Then it opens precisely at the location it's called at instead of me having to move my cursor inside the line. I perceive that as way more comfortable than having to search the invocation in that line manually myself. In IDEs like vscode you can ctrl+click a path printed in the terminal and if it has line and column info it allows more precise jumps.

Second it would be consistent with panics printing column numbers as well as rustc error messages printing them. Back in 2017, you've r+d the FCP in #46762, approving the addition of column numbers to panic printing.

I hope these can swing your opinion. If not, as a last resort one can document that the display impl of backtraces may print column numbers at some point in the future and implementations parsing the output have to be ready for it.

Regarding the PR, I'll revert it to the old stage, to appease @RalfJung as the change was mainly due to a misunderstanding. @dtolnay if the revert makes matters worse please say so :).

[dtolnay]

Thanks for the details. That's convincing to me as far as Display, but neither point applies to Debug, so I would prefer to keep the column out of Debug still. Backtrace's Debug impl is actually quite sensitive because people get it spewed into their terminal when they unwrap an error containing a backtrace (e.g. the following kind of thing). See #69038 (review).

#[derive(Debug, Error)]
struct Error {
    source: std::io::Error,
    backtrace: Backtrace,
}

[dtolnay]

@bors r+

[bors]

📌 Commit 016146d has been approved by dtolnay

[RalfJung]

@RalfJung and @dtolnay feel it would make the output harder to read.

I never said that. I am in favor of adding column information to both Display and Debug. I consider it part of the implicit contract of these two traits that Debug might show more information, but will never show less information than Display.

[RalfJung]

Given that the Debug impl seems to already be quite far from the internal representation, I think there are plenty of ways to include column number information that are not unduly verbose, e.g. by putting it together with the line number, or by putting both of them together with the filename.

I should also clarify that I do not care very strongly about this, though it does stand out as a weird kind if inconsistency.

[est31]

@RalfJung apparently some people parse the output of the Debug format, and the lang team doesn't want the Debug format to be changed until there are better ways to get the individual frames on stable Rust: #65280 (comment)

I guess that's not a guarantee that the Debug format won't ever change once backtrace is stable, but that it won't change for the time being until something like #78299 is merged and stabilized.

I agree that it's a bit inconsistent, but maybe the issue can be addressed at a later point in time, e.g. part of the proposed format changes in #65280.

Until then, see this PR as a step towards fixing #71706 :). Since you filed the issue, the panic backtrace has gotten column numbers in addition to the column number of the initial panic payload that's managed outside of the backtrace system. So this PR makes the two formats more consistent.

[RalfJung]

apparently some people parse the output of the Debug format

Oh wow that's horrible.^^ (But I understand there is no better way currently.)

This is certainly not high-priority or urgent. What about leaving a comment in the Debug impl saying that some information, such as column numbers, is deliberately omitted, and linking some of the issues you mentioned to explain why?

[est31]

@RalfJung sounds fair. Once this PR is merged I'll make a PR to add such a comment.

[RalfJung]

@bors r=dtolnay

[bors]

📌 Commit 43bfbb1 has been approved by dtolnay

[bors]

⌛ Testing commit 43bfbb1 with merge 7abd61b4f1ae114b09caaa97da2ece7854314e57...

[bors]

💥 Test timed out

[est31]

@bors retry

[bors]

⌛ Testing commit 43bfbb1 with merge bf469eb...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing bf469eb to master...

[rust-highfive]

📣 Toolstate changed by #79002!

Tested on commit bf469eb.
Direct link to PR: #79002

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).