Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace macro backtraces with labeled local uses #35702

Merged
merged 3 commits into from Aug 18, 2016

Conversation

Projects
None yet
6 participants
@jonathandturner
Copy link
Contributor

jonathandturner commented Aug 15, 2016

This PR (which builds on #35688) follows from the conversations on how best to handle the macro backtraces. The feeling there was that there were two different "groups" of users.

The first group, the macro users, rarely (and likely never) want to see the macro backtrace. This is often more confusing to users as it will be talking about code they didn't write.

The second group, the macro writers, are trying to debug a macro. They'll want to see something of the backtrace so that they can see where it's going wrong and what the steps were to get there.

For the first group, it seems clear that we don't want to show any macro backtrace. For the second group, we want to show enough to help the macro writer.

This PR uses a heuristic. It will only show any backtrace steps if they are in the same crate that is being compiled. This keeps errors in foreign crates from showing to users that didn't need them.

Additionally, in asking around I repeated heard that the middle steps of the backtrace are rarely, if ever, used in practice. This PR takes and applies this knowledge. Now, instead of a full backtrace, the user is given the error underline inside of a local macro as well as the use site as a secondary label. This effectively means seeing the root of the error and the top of the backtrace, eliding the middle steps.

Rather than being the "perfect solution", this PR opts to take an incremental step towards a better experience. Likely it would help to have additional macro debugging tools, as they could be much more verbose than we'd likely want to use in the error messages themselves.

Some examples follow.

Example 1

Before:

screen shot 2016-08-15 at 4 13 18 pm

After:

screen shot 2016-08-15 at 4 13 03 pm

Example 2

Before:

screen shot 2016-08-15 at 4 14 35 pm

After:

screen shot 2016-08-15 at 4 15 01 pm

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Aug 15, 2016

r? @pnkfelix

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

@jonathandturner jonathandturner changed the title Add backtrace labels Replace macro backtraces with labeled local uses Aug 15, 2016

@jonathandturner

This comment has been minimized.

Copy link
Contributor Author

jonathandturner commented Aug 15, 2016

span: &mut MultiSpan,
children: &mut Vec<SubDiagnostic>) {
let mut spans_updated = self.fix_multispan_in_std_macros(span);
for i in 0..children.len() {

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Aug 15, 2016

Member

Why can't this be for child in &mut children?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 16, 2016

This looks quite good; my only complaints are:

  • I'd still like to have a comment explaining what the "Fixup macro span" thing is doing a bit
  • we should have a cross-crate text, to show the behavior when macro is not local

To make a cross-crate text, you can create a test with an auxiliary directory, and add a // aux-build:foo.rs (to build auxiliary/foo.rs). So you'd have files like:

src/test/compile-fail/cross-crate-macro-backtrace/main.rs // <-- main file for the text
src/test/compile-fail/cross-crate-macro-backtrace/auxiliary/foo.rs // <-- crate
replacements_occurred = true;
}
}
for i in 0..self.span_labels.len() {

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Aug 16, 2016

Member

Both of these should be for span in &mut .. compatible, I think.

@jonathandturner

This comment has been minimized.

Copy link
Contributor Author

jonathandturner commented Aug 17, 2016

@nikomatsakis - nits addressed, new ui test added, comment added.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 17, 2016

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 17, 2016

@jonathandturner wonderful, r=me, but needs rebase =)

span: &mut MultiSpan,
children: &mut Vec<SubDiagnostic>) {
let mut spans_updated = self.fix_multispan_in_std_macros(span);
for child in &mut children.iter_mut() {

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Aug 17, 2016

Member

Why &mut children.iter_mut()? Shouldn't just &mut children be enough?

This comment has been minimized.

@jonathandturner

jonathandturner Aug 17, 2016

Author Contributor

Probably a little overkill? &mut children doesn't compile:

error: the trait bound `std::vec::Vec<SubDiagnostic>: std::iter::Iterator` is not satisfied [--explain E0277]
   --> src/librustc_errors/emitter.rs:468:9
    |>
468 |>         for child in &mut children {
    |>         ^
note: `std::vec::Vec<SubDiagnostic>` is not an iterator; maybe try calling `.iter()` or a similar method
note: required because of the requirements on the impl of `std::iter::Iterator` for `&mut std::vec::Vec<SubDiagnostic>`

But I probably could drop the &mut in front.

This comment has been minimized.

@Mark-Simulacrum

Mark-Simulacrum Aug 17, 2016

Member

That's... odd, since it works in a simpler example: https://is.gd/Ep6Unm.

fn main() {
    let mut v = vec![1, 2];

    for el in &mut v {
        *el += 10;
    }

    println!("{:?}", v);
}

Edit: Actually, children is already &mut Vec<_>, so just for child in children should work, if I understand iterators correctly.

This comment has been minimized.

@jonathandturner

jonathandturner Aug 17, 2016

Author Contributor

Also doesn't work...

error: use of moved value: `*children` [--explain E0382]
   --> src/librustc_errors/emitter.rs:472:13
    |>
468 |>         for child in children {
    |>                      -------- value moved here
...
472 |>             children.push(SubDiagnostic {
    |>             ^^^^^^^^ value used here after move
note: move occurs because `children` has type `&mut std::vec::Vec<SubDiagnostic>`, which does not implement the `Copy` trait

Sticking with the one that does :)

@jonathandturner jonathandturner force-pushed the jonathandturner:add_backtrace_labels branch from ce60c9e to 54d42cc Aug 17, 2016

@jonathandturner

This comment has been minimized.

Copy link
Contributor Author

jonathandturner commented Aug 17, 2016

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 17, 2016

📌 Commit 54d42cc has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 18, 2016

⌛️ Testing commit 54d42cc with merge 169b612...

bors added a commit that referenced this pull request Aug 18, 2016

Auto merge of #35702 - jonathandturner:add_backtrace_labels, r=nikoma…
…tsakis

Replace macro backtraces with labeled local uses

This PR (which builds on #35688) follows from the conversations on how best to [handle the macro backtraces](https://internals.rust-lang.org/t/improving-macro-errors/3809).  The feeling there was that there were two different "groups" of users.

The first group, the macro users, rarely (and likely never) want to see the macro backtrace.  This is often more confusing to users as it will be talking about code they didn't write.

The second group, the macro writers, are trying to debug a macro.  They'll want to see something of the backtrace so that they can see where it's going wrong and what the steps were to get there.

For the first group, it seems clear that we don't want to show *any* macro backtrace.  For the second group, we want to show enough to help the macro writer.

This PR uses a heuristic.  It will only show any backtrace steps if they are in the same crate that is being compiled.  This keeps errors in foreign crates from showing to users that didn't need them.

Additionally, in asking around I repeated heard that the middle steps of the backtrace are rarely, if ever, used in practice.  This PR takes and applies this knowledge.  Now, instead of a full backtrace, the user is given the error underline inside of a local macro as well as the use site as a secondary label.  This effectively means seeing the root of the error and the top of the backtrace, eliding the middle steps.

Rather than being the "perfect solution", this PR opts to take an incremental step towards a better experience.  Likely it would help to have additional macro debugging tools, as they could be much more verbose than we'd likely want to use in the error messages themselves.

Some examples follow.

**Example 1**

Before:

<img width="1275" alt="screen shot 2016-08-15 at 4 13 18 pm" src="https://cloud.githubusercontent.com/assets/547158/17682828/3948cea2-6303-11e6-93b4-b567e9d62848.png">

After:

<img width="596" alt="screen shot 2016-08-15 at 4 13 03 pm" src="https://cloud.githubusercontent.com/assets/547158/17682832/3d670d8c-6303-11e6-9bdc-f30a30bf11ac.png">

**Example 2**

Before:

<img width="918" alt="screen shot 2016-08-15 at 4 14 35 pm" src="https://cloud.githubusercontent.com/assets/547158/17682870/722225de-6303-11e6-9175-336a3f7ce308.png">

After:

<img width="483" alt="screen shot 2016-08-15 at 4 15 01 pm" src="https://cloud.githubusercontent.com/assets/547158/17682872/7582cf6c-6303-11e6-9235-f67960f6bd4c.png">

@bors bors merged commit 54d42cc into rust-lang:master Aug 18, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details

@jonathandturner jonathandturner deleted the jonathandturner:add_backtrace_labels branch Aug 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.