Skip to content

Swap primary/secondary spans in multiple unused formatting arguments diagnostic #55350

New issue

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

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

Already on GitHub? # to your account

Closed
estebank opened this issue Oct 25, 2018 · 6 comments · Fixed by #57140
Closed

Swap primary/secondary spans in multiple unused formatting arguments diagnostic #55350

estebank opened this issue Oct 25, 2018 · 6 comments · Fixed by #57140
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-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

For tooling's sake, make the following string literal the primary span:

error: multiple unused formatting arguments
 --> src/main.rs:1:30
  |
1 | fn main() { println!("Test", 1, 2) }
  |                      ------  ^  ^
  |                      |
  |                      multiple missing formatting specifiers

This way we can keep the current visual output, while giving the primary span a label that can be used by dev-tools.

CC dense-analysis/ale#1696

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Oct 25, 2018
@estebank estebank added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Oct 25, 2018
@RyanSquared
Copy link

Because, at the moment, there's nothing preventing this same scenario from coming up elsewhere, I'm wondering if this is by design. If it is, it might be better to make the ALE handler work on a system where it grabs the first, and then "upgrades" if it has a primary span with a label.

@estebank
Copy link
Contributor Author

All primary spans should have a label. That not being the case is an accident of being a newer feature.

ALE could use the main message if the primary span has no label. That would be a good idea to do in order to avoid these cases as we slowly fix them.

@RyanSquared
Copy link

ALE could use the main message if the primary span has no label.

And, as I glazed over earlier, it appears to do just that. I'll see about testing it more and possibly merging the branch as-is. Glad to know I found a bug, though.

Perhaps some assurance could be made when running tests, or at the very least a scream test - when creating a span and changing the primary-ness, raise an ICE?

@estebank
Copy link
Contributor Author

Perhaps some assurance could be made when running tests, or at the very least a scream test - when creating a span and changing the primary-ness, raise an ICE?

I'm not sure what you mean by this.

@RyanSquared
Copy link

If it's the case where every primary span should have a label, why is it not required? Is there any way, at least for the purpose of debugging, to make an assurance that all uses of a primary span implies having a label? Afterwards, it would be possible to run tests at least in this debugging mode, and every primary span without a label would result in an error.

@estebank
Copy link
Contributor Author

@RyanSquared because doing so will be a big undertaking and in some cases it's hard to come up with a nice label for the primary span (due to the compiler's lack of context). We've been doing it piecemeal to keep it 1) at a reasonable size for anyone that can contribute and 2) to make sure that the primary span doesn't just duplicate the main message (which requires thought to be taken, it isn't a mechanical change). On all cases where there isn't a label, the main message can be used by tools.

estebank added a commit to estebank/rust that referenced this issue Dec 27, 2018
bors added a commit that referenced this issue Dec 29, 2018
Tweaks to format string diagnostics

Add label spans and fix incorrect spans.

Fix #55155, fix #55350.
JohnHeitmann pushed a commit to JohnHeitmann/rust that referenced this issue Jan 5, 2019
# for free to join this conversation on GitHub. Already have an account? # 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-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants