-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Tweak some diagnostic spans #59084
Tweak some diagnostic spans #59084
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
r? @davidtwco |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor question, feel free to r=me.
@@ -5115,7 +5115,7 @@ impl<'a> Resolver<'a> { | |||
// extra for the comma. | |||
span.lo().0 - (prev_comma.as_bytes().len() as u32) - 1 | |||
)); | |||
err.span_suggestion( | |||
err.tool_only_span_suggestion( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about this change. I agree that it makes the error much cleaner, but without the help: ...
message how would someone know that they can automatically have the problem fixed?
(as a slight aside, I think we should try use try cc groups like @rust-lang/wg-diagnostics
when landing PRs like this or the one that introduced tool_only_span_suggestion
- I had no idea this existed and it'd have been nice to follow along with it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tool_only_span_diagnostics
was landed over the all hands because it was needed for a different change, so it's pretty recent.
Tools still will see the same json output, so vscode and rustfix would still be able to apply these. I guess I hadn't thought of the use case of someone running rustfix after running rustc iteratively. Should we mark these some other way? Cyan asterisk next to the error or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern is that users who know when they see help: ...
that it is something that can be automatically applied won't know that for this error.
Something like a cyan asterisk would be ideal to draw attention to that (it might even raise awareness of rustfix being able to fix these things if everyone's error messages had a cyan asterisk suddenly appear). I wouldn't block this PR on that, so I'll r+ this and we can file a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #59103.
@bors r+ |
📌 Commit 63432949565d73020222ba99fb487f855b4b0565 has been approved by |
@davidtwco added a small tweak to fix some less than stellar spans and marked the other duplicate import suggestion as tool only. @bors r=davidtwco |
📌 Commit a00fa50a3a47274416a3a1d404eb3a3a3afaba07 has been approved by |
This comment has been minimized.
This comment has been minimized.
a00fa50
to
f1ee5bb
Compare
@bors r=davidtwco rebased |
📌 Commit f1ee5bb2c5029a452c7ae8f465f87e3278a640c9 has been approved by |
☔ The latest upstream changes (presumably #58899) made this pull request unmergeable. Please resolve the merge conflicts. |
f1ee5bb
to
b53ca90
Compare
This comment has been minimized.
This comment has been minimized.
@bors r=davidtwco |
📌 Commit d1656f1 has been approved by |
Tweak some diagnostic spans
Tweak some diagnostic spans
☔ The latest upstream changes (presumably #58929) made this pull request unmergeable. Please resolve the merge conflicts. |
d1656f1
to
59f0f2e
Compare
@bors r=davidtwco |
📌 Commit 59f0f2e has been approved by |
☀️ Test successful - checks-travis, status-appveyor |
No description provided.