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

Propagate macro backtraces more often, improve formatting diagnostics #24003

Merged
merged 5 commits into from
Apr 12, 2015

Conversation

rprichard
Copy link
Contributor

  • In noop_fold_expr, call new_span in these cases:

    • ExprMethodCall's identifier
    • ExprField's identifier
    • ExprTupField's integer

    Calling new_span for ExprMethodCall's identifier is necessary to print
    an acceptable diagnostic for write!(&2, ""). We see this error:

    <std macros>:2:20: 2:66 error: type `&mut _` does not implement any method in scope named `write_fmt`
    <std macros>:2 ( & mut * $ dst ) . write_fmt ( format_args ! ( $ ( $ arg ) * ) ) )
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    

    With this change, we also see a macro expansion backtrace leading to
    the write!(&2, "") call site.

  • After fully expanding a macro, we replace the expansion expression's
    span with the original span. Call fld.new_span to add a backtrace to
    this span. (Note that I'm call new_span after bt.pop(), so the macro
    just expanded isn't on the backtrace.)

    The motivating example for this change is println!("{}"). The format
    string literal is concat!($fmt, "arg") and is inside the libstd macro.
    We need to see the backtrace to find the println! call site.

  • Add a backtrace to the format_args! format expression span.

r? alexcrichton

Addresses #23459

@rust-highfive
Copy link
Collaborator

r? @pcwalton

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

@rprichard
Copy link
Contributor Author

r? @alexcrichton

cc @huonw @alexcrichton

@rprichard
Copy link
Contributor Author

It occurs to me that #23459 actually had a much better motivating example than write!(&2, ...) -- rather than 2, the argument could be an std::fs::File, but std::io::Write hasn't been used. That's far more likely to happen.

@huonw
Copy link
Member

huonw commented Apr 3, 2015

Great! Could you add some tests for these? E.g. for the write! case, one could test it by creating a file like macro-backtrace-invalid-internals.rs in src/test/compile-fail.

macro_rules! foo { //~ NODE in expansion of
     () => {
          1.fake(); //~ ERROR does not implement any method
     }
}

fn main() {
     foo!(); //~ NOTE expansion site
}

Similarly one could have macro-backtrace-println.rs etc.

@huonw huonw assigned huonw and unassigned pcwalton Apr 3, 2015
@rprichard
Copy link
Contributor Author

Yes, I can add some tests.

@rprichard
Copy link
Contributor Author

I added new tests:

  • I expanded the internal-unstable compile-fail test to also test method calls and field accesses, which are both fixed in this PR.
  • I added three new compile-fail tests, macro-backtrace-{invalid-internals,nested,println}.rs

@rprichard
Copy link
Contributor Author

I've noticed that the backtraces introduced with this PR can appear repeatedly:

rprichard@ryan:~$ fixld ~/work/rust-staging2/build/install/bin/rustc test.rs 
<std macros>:2:20: 2:66 error: type `&mut std::fs::File` does not implement any method in scope named `write_fmt`
<std macros>:2 ( & mut * $ dst ) . write_fmt ( format_args ! ( $ ( $ arg ) * ) ) )
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<std macros>:1:1: 2:68 note: in expansion of write!
<std macros>:4:1: 4:62 note: expansion site
<std macros>:1:1: 4:66 note: in expansion of writeln!
test.rs:9:13: 9:45 note: expansion site
<std macros>:2:66: 2:66 help: methods from traits can only be called if the trait is in scope; the following trait is implemented but not in scope, perhaps add a `use` for it:
<std macros>:1:1: 2:68 note: in expansion of write!
<std macros>:4:1: 4:62 note: expansion site
<std macros>:1:1: 4:66 note: in expansion of writeln!
test.rs:9:13: 9:45 note: expansion site
<std macros>:2:66: 2:66 help: candidate #1: use `std::io::Write`
<std macros>:1:1: 2:68 note: in expansion of write!
<std macros>:4:1: 4:62 note: expansion site
<std macros>:1:1: 4:66 note: in expansion of writeln!
test.rs:9:13: 9:45 note: expansion site
error: aborting due to previous error

The second two backtraces are obnoxious. rustc is trying to print a list of candidates, but it's hard to read because we output a backtrace after each line. It's the same span for all the lines, too, so the backtraces are redundant.

I suspect the fix is for fileline_note and fileline_help to omit the macro backtrace.

I'm also wondering about the first span_note call in rustc_typeck::check::method::suggest::report_error. Why isn't it fileline_note?

Also: ExtCtxt::print_backtrace was added in 2011-08-05 and reduced to a no-op ten days later. I think I'm going to remove it. Probably needs a separate PR.

@huonw
Copy link
Member

huonw commented Apr 6, 2015

I suspect the fix is for fileline_note and fileline_help to omit the macro backtrace.

I agree.

I'm also wondering about the first span_note call in rustc_typeck::check::method::suggest::report_error. Why isn't it fileline_note?

We haven't been very good about using fileline_* when it would be more appropriate. That is definitely one example.

@@ -453,7 +453,7 @@ fn emit(dst: &mut EmitterWriter, cm: &codemap::CodeMap, rsp: RenderSpan,
try!(highlight_lines(dst, cm, sp, lvl, cm.span_to_lines(sp)));
}
}
if sp != COMMAND_LINE_SP {
if sp.expn_id != COMMAND_LINE_EXPN && rsp.is_full_span() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, could you add a brief comment about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some code a few lines up doing the same sort of sp.expn_id to COMMAND_LINE_EXPN comparison:

    // We cannot check equality directly with COMMAND_LINE_SP
    // since PartialEq is manually implemented to ignore the ExpnId
    let ss = if sp.expn_id == COMMAND_LINE_EXPN {
    ...

I'll factor out the sp.expn_id == COMMAND_LINE_EXPN to a local variable and put the comment there.

@huonw
Copy link
Member

huonw commented Apr 6, 2015

Looks good, r=me with one little comment.

@bors
Copy link
Contributor

bors commented Apr 7, 2015

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

@rprichard
Copy link
Contributor Author

There seems to be a general problem where we don't output macro backtraces until after the expansion phase. The format_args! change in this PR fixes this problem for the format string, but there are other instances of the problem, including format_args! arguments:

macro_rules! format_template {
    ($tmp:tt) => (format!($tmp, a=123, b=456))
}
fn main() {
    format_template!("{a}{b}");
    format_template!("{a}");
    format_template!("{a}{b}");
    format_template!("{a}");
}
rprichard@ryan:~/mess$ rustc format-template.rs 
format-template.rs:2:42: 2:45 error: named argument never used
format-template.rs:2     ($tmp:tt) => (format!($tmp, a=123, b=456))
                                                              ^~~
format-template.rs:2:42: 2:45 error: named argument never used
format-template.rs:2     ($tmp:tt) => (format!($tmp, a=123, b=456))
                                                              ^~~
error: aborting due to 2 previous errors

@rprichard
Copy link
Contributor Author

@huonw I rebased my changes, factored out a is_command_line variable with a 2-line comment above it, and squashed one of the commit's that just reordered imports from the previous commit.

@bors
Copy link
Contributor

bors commented Apr 11, 2015

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

 * In noop_fold_expr, call new_span in these cases:
    - ExprMethodCall's identifier
    - ExprField's identifier
    - ExprTupField's integer

   Calling new_span for ExprMethodCall's identifier is necessary to print
   an acceptable diagnostic for write!(&2, ""). We see this error:

       <std macros>:2:20: 2:66 error: type `&mut _` does not implement any method in scope named `write_fmt`
       <std macros>:2 ( & mut * $ dst ) . write_fmt ( format_args ! ( $ ( $ arg ) * ) ) )
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

   With this change, we also see a macro expansion backtrace leading to
   the write!(&2, "") call site.

 * After fully expanding a macro, we replace the expansion expression's
   span with the original span. Call fld.new_span to add a backtrace to
   this span. (Note that I'm call new_span after bt.pop(), so the macro
   just expanded isn't on the backtrace.)

   The motivating example for this change is println!("{}"). The format
   string literal is concat!($fmt, "arg") and is inside the libstd macro.
   We need to see the backtrace to find the println! call site.

 * Add a backtrace to the format_args! format expression span.

Addresses rust-lang#23459
It was added in 2011-08-05 and reduced to a no-op ten days later.
@rprichard
Copy link
Contributor Author

@huonw I rebased my PR to account for @nikomatsakis's diagnostic improvements.

I rewrote the Suppress the macro backtrace commit. The other commits should be unchanged.

@huonw
Copy link
Member

huonw commented Apr 12, 2015

@bors r+

@bors
Copy link
Contributor

bors commented Apr 12, 2015

📌 Commit ddbdf51 has been approved by huonw

@bors
Copy link
Contributor

bors commented Apr 12, 2015

⌛ Testing commit ddbdf51 with merge feeb23d...

bors added a commit that referenced this pull request Apr 12, 2015
 * In `noop_fold_expr`, call `new_span` in these cases:
    - `ExprMethodCall`'s identifier
    - `ExprField`'s identifier
    - `ExprTupField`'s integer

   Calling `new_span` for `ExprMethodCall`'s identifier is necessary to print
   an acceptable diagnostic for `write!(&2, "")`. We see this error:
   ```
   <std macros>:2:20: 2:66 error: type `&mut _` does not implement any method in scope named `write_fmt`
   <std macros>:2 ( & mut * $ dst ) . write_fmt ( format_args ! ( $ ( $ arg ) * ) ) )
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   ```
   With this change, we also see a macro expansion backtrace leading to
   the `write!(&2, "")` call site.

 * After fully expanding a macro, we replace the expansion expression's
   span with the original span. Call `fld.new_span` to add a backtrace to
   this span. (Note that I'm call `new_span` after `bt.pop()`, so the macro
   just expanded isn't on the backtrace.)

   The motivating example for this change is `println!("{}")`. The format
   string literal is `concat!($fmt, "arg")` and is inside the libstd macro.
   We need to see the backtrace to find the `println!` call site.

 * Add a backtrace to the `format_args!` format expression span.

r?  alexcrichton

Addresses #23459
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants