Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Add column number support to Backtrace #79002
Conversation
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
|
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 I'll remove them from Debug though. |
So, |
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. |
|
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 |
|
@yaahc it is possible to add it in a fully backwards compatibe 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. |
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. |
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 :).
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 :). |
|
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,
} |
|
@bors r+ |
|
|
I never said that. I am in favor of adding column information to both |
|
Given that the I should also clarify that I do not care very strongly about this, though it does stand out as a weird kind if inconsistency. |
|
@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. |
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 |
|
@RalfJung sounds fair. Once this PR is merged I'll make a PR to add such a comment. |
Add column number support to Backtrace Backtrace frames might include column numbers. Print them if they are included.
…as-schievink Rollup of 26 pull requests Successful merges: - rust-lang#78352 (Do not call `unwrap` with `signatures` option enabled) - rust-lang#78590 (refactor: removing alloc::collections::vec_deque ignore-tidy-filelength) - rust-lang#78848 (Bump minimal supported LLVM version to 9) - rust-lang#78856 (Explicitly checking for or-pattern before test) - rust-lang#78948 (test: add `()=()=()=...` to weird-exprs.rs) - rust-lang#78962 (Add a test for r# identifiers) - rust-lang#78963 (Added some unit tests as requested) - rust-lang#78966 (Never inline C variadics, cold functions, functions with incompatible attributes ...) - rust-lang#78968 (Include llvm-as in llvm-tools-preview component) - rust-lang#78969 (Normalize function type during validation) - rust-lang#78980 (Fix rustc_ast_pretty print_qpath resulting in invalid macro input) - rust-lang#78986 (Avoid installing external LLVM dylibs) - rust-lang#78988 (Fix an intrinsic invocation on threaded wasm) - rust-lang#78993 (rustc_target: Fix dash vs underscore mismatches in option names) - rust-lang#79002 (Add column number support to Backtrace) - rust-lang#79003 (rustc_expand: Mark inner `#![test]` attributes as soft-unstable) - rust-lang#79004 (Add `--color` support to bootstrap) - rust-lang#79005 (cleanup: Remove `ParseSess::injected_crate_name`) - rust-lang#79016 (Make `_` an expression, to discard values in destructuring assignments) - rust-lang#79019 (astconv: extract closures into a separate trait) - rust-lang#79026 (Implement BTreeMap::retain and BTreeSet::retain) - rust-lang#79027 (Limit storage duration of inlined always live locals) - rust-lang#79031 (Validate that locals have a corresponding `LocalDecl`) - rust-lang#79034 (rustc_resolve: Make `macro_rules` scope chain compression lazy) - rust-lang#79036 (Move Steal to rustc_data_structures.) - rust-lang#79041 (Rename clean::{ItemEnum -> ItemKind}, clean::Item::{inner -> kind}) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
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. |
|
@bors r=dtolnay |
|
|
Add column number support to Backtrace Backtrace frames might include column numbers. Print them if they are included.
Rollup of 9 pull requests Successful merges: - rust-lang#77939 (Ensure that the source code display is working with DOS backline) - rust-lang#78779 (Introduce `TypeVisitor::BreakTy`) - rust-lang#78863 (Support repr(simd) on ADTs containing a single array field) - rust-lang#78967 (Make codegen tests compatible with extra inlining) - rust-lang#79002 (Add column number support to Backtrace) - rust-lang#79027 (Limit storage duration of inlined always live locals) - rust-lang#79077 (document that __rust_alloc is also magic to our LLVM fork) - rust-lang#79088 (clarify `span_label` documentation) - rust-lang#79097 (Code block invalid html tag lint) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Add column number support to Backtrace Backtrace frames might include column numbers. Print them if they are included.
|
|
|
@bors retry |
Add column number support to Backtrace Backtrace frames might include column numbers. Print them if they are included.
Add column number support to Backtrace Backtrace frames might include column numbers. Print them if they are included.
Rollup of 10 pull requests Successful merges: - rust-lang#77675 (Tidy should not check line lengths in tests) - rust-lang#78961 (Make bad "rust-call" arguments no longer ICE) - rust-lang#79002 (Add column number support to Backtrace) - rust-lang#79082 (Improve the diagnostic for when an `fn` contains qualifiers inside an `extern` block.) - rust-lang#79101 (Don't special case constant operands when lowering intrinsics) - rust-lang#79110 (Remove redundant notes in E0275) - rust-lang#79147 (Highlight MIR as Rust on GitHub) - rust-lang#79149 (Move capture lowering from THIR to MIR) - rust-lang#79156 (Allow using `download-ci-llvm` from directories other than the root) - rust-lang#79164 (Permit standalone generic parameters as const generic arguments in macros) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
|
Tested on commit rust-lang/rust@bf469eb. Direct link to PR: <rust-lang/rust#79002>💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
Backtrace frames might include column numbers.
Print them if they are included.