Multiple unused formatting argument are now labels, less unused argument spam

[Nickforall]

As my first contribution to the Rust compiler I took a shot at the issue #37718. Instead of printing a list of notes for every individual unused argument, it now displays what arguments are unused underneath each argument, using the span_label function from the DiagnosticBuilder struct.

Example:

error: multiple unused formatting arguments
  -->  /Users/nickvernij/Programming/rust/src/test/ui/macros/format-foreign.rs:12:5
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^--------^^-------^^-^^
   |                              |         |        |
   |                              |         |        unused
   |                              |         unused
   |                              unused
   |

I modified the test that was used before this change, and added a test that checks this method with a function whose arguments are written on multiple lines.

I ran the style guideline test as described in the contributing guidelines, and a test on the UI tests. Again, this is my first PR here, so I might've missed something somewhere.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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.

[alexcrichton]

r? @jonathandturner

[shepmaster]

Thanks for the hard work! We'll make sure that @jonathandturner (or someone else) reviews this soon!

[sophiajt]

Thanks for the PR!

Let's see, taking a look I think this is an improvement, though I'm not sure if this:

error: multiple unused formatting arguments
  -->  /Users/nickvernij/Programming/rust/src/test/ui/macros/format-foreign.rs:12:5
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^--------^^-------^^-^^
   |                              |         |        |
   |                              |         |        unused
   |                              |         unused
   |                              unused
   |

Reads better than what we have now. Generally, we've been trying to avoid having both primary and secondary underlines on the same spans. This makes the line harder for the eye to parse.

In theory, I think we support having multiple lines as the primary error, so using the MultiSpan in this case.

The design I'm wondering if we can get to is something like this:

error: multiple unused formatting arguments
  -->  /Users/nickvernij/Programming/rust/src/test/ui/macros/format-foreign.rs:12:5
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |                              ^^^^^^^^  ^^^^^^^  ^  
   |                              |         |        |
   |                              ------------------------- unused

That said, I'm not sure if that's an easy fix. If not, then yeah, I think what you have is probably okay.

[Nickforall]

Thank you for your response. I think it's definitely easier to parse for the eye than the current output, but I get your point.

I'm not too familiar with the codebase yet, so I'll look around in the code/docs a bit, and see if I can get something that looks more like the output you're proposing here.

[sophiajt]

@Nickforall - yeah, definitely.

Here's the MultiSpan I thought you could use: https://github.com/rust-lang/rust/blob/master/src/librustc_errors/diagnostic.rs#L25

If you put the spans together and created a MultiSpan, and used that... I'm honestly sure what will happen 😅 but perhaps it'll work?

Like I said, if it doesn't we can definitely go in the direction of this PR, but I at least wanted to explore a little first.

[Nickforall]

I've looked through the documentation for span_labels/multispans and throught other errors, but haven't really found a similar case.

So my guess here is that it requires work in the code that renders the error, I've looked through that code, and it doesn't seem like an easy fix to me.

[sophiajt]

I'm fine approving this if you want to file a follow up issue to do the next part of the work to see if someone else wants to do it

[arielb1]

@Nickforall and @jonathandturner - friendly ping to keep this on your radar.

[sophiajt]

I believe we're waiting on author feedback on this one.

[Nickforall]

@arielb1 @jonathandturner I'm quite busy with school this week, I set a reminder in my agenda for the weekend to finish this one.

[Nickforall]

Hi @jonathandturner, I created the follow-up issue (#41850)

[sophiajt]

Can we put a label here also?

[sophiajt]

Same

[sophiajt]

Same

[sophiajt]

@Nickforall - great, thanks for filing the issue. Looks like we're close. I didn't see this missing label last time, but I think after we add that we should be good to go.

[arielb1]

🕸 Closing due to inactivity to keep our backlog clean 🕸. Feel free to reopen @Nickforall!