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

Multiple unused formatting argument notes should use multispan #37718

Closed
brson opened this issue Nov 11, 2016 · 13 comments · Fixed by #43323
Closed

Multiple unused formatting argument notes should use multispan #37718

brson opened this issue Nov 11, 2016 · 13 comments · Fixed by #43323
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@brson
Copy link
Contributor

brson commented Nov 11, 2016

After #37613, the output for format! errors looks like:

error: multiple unused formatting arguments
  --> $DIR/format-foreign.rs:12:5
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: argument never used
  --> $DIR/format-foreign.rs:12:30
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |                              ^^^^^^^^
note: argument never used
  --> $DIR/format-foreign.rs:12:40
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |                                        ^^^^^^^
note: argument never used
  --> $DIR/format-foreign.rs:12:49
   |
12 |     println!("%.*3$s %s!/n", "Hello,", "World", 4);
   |                                                 ^
   = help: `%.*3$s` should be written as `{:.2$}`
   = help: `%s` should be written as `{}`
   = note: printf formatting not supported; see the documentation for `std::fmt`
= note: this error originates in a macro outside of the current crate

that's a lot of lines dedicated to underlining macro acruments. They should all be output in one multispan.

cc @DanielKeep @estebank @jonathandturner @GuillaumeGomez

@brson brson added A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Nov 11, 2016
@GuillaumeGomez
Copy link
Member

Hum indeed. If no one has taken it in 12 hours, I'll give it a look.

@DanielKeep
Copy link
Contributor

@GuillaumeGomez I'm poking at it right now. I think I need to fix ExtCtxt's methods to accept <S: Into<MultiSpan>> instead of just Span, and then I can use MultiSpan directly.

After all, I created the problem, I should probably fix it. :P

@GuillaumeGomez
Copy link
Member

Perfect. Good luck! :)

DanielKeep added a commit to DanielKeep/rust that referenced this issue Nov 18, 2016
Unused arguments are attached to the main error as labels.  This drops
the traditional named/positional message split (except in the case of a
*single* unused argument) for "unused" in interest of terseness.

Closes rust-lang#37718.
@cramertj
Copy link
Member

I believe this should be closed.

@GuillaumeGomez
Copy link
Member

Hold on, it seems the PR is still open.

@steveklabnik steveklabnik added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@perryprog
Copy link
Contributor

Seeing as there isn’t anyone working on this anymore, I’ll be happy to take over.

This is my first issue, (woot), so I’m not exactly sure where to start. I assume I should just fork the repo, change format.rs to use span_lable, and fix the tests?

Pinging @Nickforall and @DanielKeep in case you want to work on this still.

@Nickforall
Copy link

@perryprog I am not working on this anymore.

@Mark-Simulacrum
Copy link
Member

@perryprog Those steps may be correct; @GuillaumeGomez or someone else may be able to verify for you. You can also look at #41256 and #37729 which appear to be previous PRs which attempted to do this. Let us know if you'd like any help!

@perryprog
Copy link
Contributor

@Mark-Simulacrum Awesome! I'm super excited. Anyone specific I should r??

@estebank
Copy link
Contributor

@perryprog bors will select someone automatically if you don't specify anyone, but I think any of me, @GuillaumeGomez, @Mark-Simulacrum, @jonathandturner or @nikomatsakis would be available for review and or guidance.

@perryprog
Copy link
Contributor

For some reason I seem to be getting test errors unrelated to my changes. (ran ./x.py test --stage 1)

@Mark-Simulacrum
Copy link
Member

Those tests shouldn't actually be running in --stage 1... cc @alexcrichton. For these changes, I would suggest ./x.py test --stage 1 src/test/{compile-fail,ui}, I think that should be enough.

@perryprog
Copy link
Contributor

Gotcha, it’s possible I copy pasted from the wrong command (on mobile can’t check right now), but I’ll use that command for testing and see if it works.

I’m kinda busy for the rest of the week but I’ll keep working on it in my free time. I’ll comment here until I get a PR ready with any updates I have.

bors added a commit that referenced this issue Jul 22, 2017
Less verbose output for unused arguments

Closes #37718

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

The output now looks like this:
<img width="831" alt="screen shot 2017-07-18 at 5 01 32 pm" src="https://user-images.githubusercontent.com/12972285/28347566-dbfa9962-6c05-11e7-8730-c2e8062a04cc.png">

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants