Add column number support to Backtrace #79002

Conversation

Contributor

est31 commented on Nov 13

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

Collaborator

rust-highfive commented on Nov 13

r? @dtolnay

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

Member

dtolnay left a comment

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.

Contributor

Author

est31 commented on Nov 13

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

I'll remove them from Debug though.

Member

RalfJung commented on Nov 13

I'll remove them from Debug though.

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

Contributor

Author

est31 commented on Nov 13

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.

Contributor

yaahc commented on Nov 13 •

edited

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

Contributor

Author

est31 commented on Nov 13 •

edited

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

Contributor

yaahc commented on Nov 13

@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.

Member

dtolnay commented on Nov 13

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.

Contributor

Author

est31 commented on Nov 13 •

edited

@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 :).

Member

dtolnay left a comment

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,
}

Member

dtolnay commented on Nov 13

@bors r+

Contributor

bors commented on Nov 13

Commit 016146d has been approved by dtolnay

Member

RalfJung commented on Nov 13

@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.

Member

RalfJung commented on Nov 13 •

edited

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.

Contributor

Author

est31 commented on Nov 14

@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.

Member

RalfJung commented on Nov 14 •

edited

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?

Contributor

Author

est31 commented on Nov 14

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

Member

RalfJung commented on Nov 15

Well, "blame"... with that change, for the first time are we running the "tools" builder in rollup PRs (submodule updates usually were no rolled up), which leads to this new confusion.

Member