Less verbose output for unused arguments

[perryprog]

Closes #37718

This is my first contribution to rust, so sorry if I'm missing anything!

The output now looks like this:

It's not the prettiest, but whenever #41850 gets resolved, this should be able to be improved.

EDIT: This also does not seem
r? @Mark-Simulacrum

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[Mark-Simulacrum]

This looks good to me, but I'm going to r? @GuillaumeGomez for final review. It looks like we already have the follow up issue which will make this even better: #41850.

[perryprog]

Woot! Thanks!

[perryprog]

Hmm... I'm wondering if it would be better to use something other than span_label, as combining the red error line, with the blue "secondary" errors looks bad, and sends a mixed message.

How would you have just the red "primary" error under the unused arguments, or just the blue "secondary" error lines under the unused arguments?

EDIT: Seems like the red line is from let mut diag = cx.ecx.struct_span_err(cx.fmtsp, "multiple unused formatting arguments");, I'm not sure how to change that line to not use struct_span_err, and I can't experiment until I get home (battery power is fun).

[estebank]

How would you have just the red "primary" error under the unused arguments, or just the blue "secondary" error lines under the unused arguments?

struct_span_err takes a MultiSpan, which means you can do

cx.ecx.struct_span_err(errs.iter().map(|(sp, _)| *sp).collect::<Vec<Span>>(),
                       "multiple unused formatting arguments")

which would give you the following output instead:

error: multiple unused formatting arguments
  --> $DIR/format-foreign.rs:12:5
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |                              ^^^^^^^^  ^^^^^^^  ^

If you keep the for (sp, _) in errs {...} it'd look like this:

error: multiple unused formatting arguments
  --> $DIR/format-foreign.rs:12:5
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |                              ^^^^^^^^  ^^^^^^^  ^ unused
   |                              |          |
   |                              |          unused
   |                              unused

I personally think it looks slightly better without the labels in that case, but I'm not too fussed either way.

[perryprog]

I’ll wait to find out what @Mark-Simulacrum and @GuillaumeGomez prefer.

@estebank where’s the best place to find documentation about this sort of thing? Also, thanks a lot for helping me! I really appreciate it!

[estebank]

@perryprog my best advice would be to read the code. @Manishearth compiles the rustdoc for the compiler if you need a link for somebody else, but I always end up going back to the code itself as there're sometimes comments explaining what's going on. It's a great help to just see how other errors do their thing.

I sometimes add a dependency on backtrace-rs to add a debug! with a specific location's backtrace when I can't find out where it is actually being called from. If you are using debug! statements, you'll probably want to do ./configure --enable-debug (it will slow down builds after stage 0, though). You should probably know about rustc -Z treat-err-as-bug, which will make the compiler panic on the first diagnostic error that is emitted. This coupled with RUST_BACKTRACE=short is sometimes useful to get the callstack, but not always (as sometimes a diagnostic is saved and emitted much later in the run).

[GuillaumeGomez]

The code looks good and the current output seems fine to me (the "unused" is important).

[perryprog]

@GuillaumeGomez just want to make sure I'm clear here: Are you good with this output:

error: multiple unused formatting arguments
  --> $DIR/format-foreign.rs:12:5
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |                              ^^^^^^^^  ^^^^^^^  ^ unused
   |                              |          |
   |                              |          unused
   |                              unused

or are you good with what I have in the PR currently:

---

@estebank that sounds good, I'm just having a bit of trouble getting the code you gave me to work. I changed it a bit to be this:

let mut diag = cx.ecx.struct_span_err(
    errs.iter().map(|&(sp, _)| sp).collect::<Vec<Span>>(),
    "multiple unused formatting arguments"
);

However, it seems like the struct_span_err method doesn't accept a Vec:

    |
800 |                     errs.iter().map(|&(sp, _)| sp).collect::<Vec<Span>>(),
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `syntax_pos::Span`, found struct `std::vec::Vec`
    |
    = note: expected type `syntax_pos::Span`
               found type `std::vec::Vec<syntax_pos::Span>`

[GuillaumeGomez]

I'm good with what you have currently. :)

[perryprog]

@GuillaumeGomez oh, ok 😛. Once #41850 is closed, we should improve the output thought.

I think I’ll write some more tests, and then this’ll be ready for a merge.

[estebank]

However, it seems like the struct_span_err method doesn't accept a Vec:

@perryprog I thought that MultiSpan from Vec<Span> was implemented, my bad. struct_span_err() should be changed to accepting Into<MultiSpan> (as DiagnosticBuilder already accepts that), so that you can provide MultiSpan::from_spans() to it instead of the Vec<Span>.

Having said that, given @GuillaumeGomez comment, you don't need to do that for this PR.

[estebank]

@bors r+

[bors]

📌 Commit 0fcb4fc has been approved by estebank

[perryprog]

I'll add the tests in a separate PR, is that fine?

[estebank]

@bors r-

@perryprog missed the bit about adding more tests, let's add them to this PR so they're self contained (easier to track down the work done after the fact that way). Ping me when you've added them and I'll r+ again.

[perryprog]

I'm just adding some UI tests — should I just add them all inside this file?

[estebank]

Only if they're tightly related to that test, otherwise just create a new file with a descriptive title.

[perryprog]

@estebank ping pong 🏓

[estebank]

@bors r+

[bors]

📌 Commit 5c10db3 has been approved by estebank

[bors]

⌛ Testing commit 5c10db3 with merge f0b07ca...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing f0b07ca to master...