Skip to content

Tweak some diagnostic spans#59084

Merged
bors merged 10 commits intorust-lang:masterfrom
estebank:diagnostic-spans
Mar 24, 2019
Merged

Tweak some diagnostic spans#59084
bors merged 10 commits intorust-lang:masterfrom
estebank:diagnostic-spans

Conversation

@estebank
Copy link
Contributor

@estebank estebank commented Mar 11, 2019

No description provided.

@rust-highfive
Copy link
Contributor

rust-highfive commented Mar 11, 2019

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2019
@estebank
Copy link
Contributor Author

estebank commented Mar 11, 2019

r? @davidtwco

@rust-highfive rust-highfive assigned davidtwco and unassigned cramertj Mar 11, 2019
davidtwco
davidtwco reviewed Mar 11, 2019
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a 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.

src/librustc_resolve/lib.rs Outdated
Copy link
Member

@davidtwco davidtwco Mar 11, 2019 •
edited
Loading

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)

Copy link
Contributor Author

@estebank estebank Mar 11, 2019

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?

Copy link
Member

@davidtwco davidtwco Mar 11, 2019

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.

Copy link
Member

@davidtwco davidtwco Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #59103.

@davidtwco
Copy link
Member

davidtwco commented Mar 11, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 11, 2019

📌 Commit 63432949565d73020222ba99fb487f855b4b0565 has been approved by davidtwco

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2019
@davidtwco davidtwco mentioned this pull request Mar 11, 2019
Open
@estebank
Copy link
Contributor Author

estebank commented Mar 11, 2019

@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

@bors
Copy link
Collaborator

bors commented Mar 11, 2019

📌 Commit a00fa50a3a47274416a3a1d404eb3a3a3afaba07 has been approved by davidtwco

@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 12, 2019
@estebank
Copy link
Contributor Author

estebank commented Mar 12, 2019

@bors r=davidtwco rebased

@bors
Copy link
Collaborator

bors commented Mar 12, 2019

📌 Commit f1ee5bb2c5029a452c7ae8f465f87e3278a640c9 has been approved by davidtwco

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2019
@bors
Copy link
Collaborator

bors commented Mar 17, 2019

☔ The latest upstream changes (presumably #58899) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 17, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor Author

estebank commented Mar 17, 2019

@bors r=davidtwco

@bors
Copy link
Collaborator

bors commented Mar 17, 2019

📌 Commit d1656f1 has been approved by davidtwco

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 17, 2019
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 17, 2019
sanxiyn added a commit to sanxiyn/rust that referenced this pull request Mar 18, 2019
@sanxiyn
Rollup merge of rust-lang#59084 - estebank:diagnostic-spans, r=davidtwco
02c2e84 
Tweak some diagnostic spans
@sanxiyn sanxiyn mentioned this pull request Mar 18, 2019
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Mar 22, 2019
@Mark-Simulacrum
Rollup merge of rust-lang#59084 - estebank:diagnostic-spans, r=davidtwco
ae89abd 
Tweak some diagnostic spans
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Mar 22, 2019
Closed
@bors
Copy link
Collaborator

bors commented Mar 23, 2019

☔ The latest upstream changes (presumably #58929) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2019
@estebank
Copy link
Contributor Author

estebank commented Mar 23, 2019

@bors r=davidtwco

@bors
Copy link
Collaborator

bors commented Mar 23, 2019

📌 Commit 59f0f2e has been approved by davidtwco

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2019
@bors
Copy link
Collaborator

bors commented Mar 23, 2019

⌛ Testing commit 59f0f2e with merge 93f5ba0...

bors added a commit that referenced this pull request Mar 23, 2019
@bors
Auto merge of #59084 - estebank:diagnostic-spans, r=davidtwco
93f5ba0 
Tweak some diagnostic spans
@bors
Copy link
Collaborator

bors commented Mar 24, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: davidtwco
Pushing 93f5ba0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 24, 2019
@bors bors merged commit 59f0f2e into rust-lang:master Mar 24, 2019
@estebank estebank deleted the diagnostic-spans branch November 9, 2023 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidtwco davidtwco davidtwco left review comments

Assignees

@davidtwco davidtwco

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@estebank @rust-highfive @davidtwco @bors @cramertj