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.

Sign up for GitHub

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

Jump to bottom

Use verbose suggestion for dyn-less trait object lint #76215

Closed
wants to merge 1 commit into from
Closed

Use verbose suggestion for dyn-less trait object lint #76215

wants to merge 1 commit into from

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Sep 1, 2020

No description provided.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 Sep 1, 2020
@estebank
Copy link
Contributor Author

estebank commented Sep 1, 2020

r? @Aaron1011

@Aaron1011
Copy link
Member

@estebank: Personally, I prefer the less-verbose style. Was there a particular case that motiviated this change?

@estebank
Copy link
Contributor Author

estebank commented Sep 1, 2020

@Aaron1011 my motivation was #61555. I find that in cases where we suggest adding or removing short tokens like & confuses some people because they don't notice the change. span_suggestion_verbose is a reasonable palliative in my eyes for that.

Aaron1011
Aaron1011 reviewed Sep 3, 2020
View reviewed changes
src/test/ui/traits/trait-bounds-not-on-bare-trait.stderr
@@ -2,9 +2,13 @@ warning: trait objects without an explicit `dyn` are deprecated
--> $DIR/trait-bounds-not-on-bare-trait.rs:7:12
|
LL | fn foo(_x: Foo + Send) {
| ^^^^^^^^^^ help: use `dyn`: `dyn Foo + Send`
| ^^^^^^^^^^ trait object without `dyn`
Copy link
Member

Choose a reason for hiding this comment

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

This is the kind of case where I think the original output is better - we're pointing to the entire type, so having it inline seems just as clear.

It might be worth it to try to detect this case when constructing BuiltinLintDiagnostics::BareTraitObject, and only use the verbose format when the target type is nested within another (e.g. &TraitName or Box<TraitName>).

@Aaron1011
Copy link
Member

Aaron1011 commented Sep 13, 2020 •
edited

@estebank: Sorry, I completely forgot about this PR.

Personally, I'd prefer if we limited this to cases where the previous output is potentially confusing, as I described in my comment. However, I don't feel very strongly about this, so I'll leave it up to you to decide.

@Aaron1011 Aaron1011 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2020
@camelid camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2020
@camelid camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 23, 2020
@crlf0710 crlf0710 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 13, 2020
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-dst Area: Dynamically Sized Types T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 30, 2020
@Dylan-DPC-zz
Copy link

Closing this based on #76215 (comment) and the fact that it has been inactive since then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011 Aaron1011 left review comments

Assignees

@Aaron1011 Aaron1011

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-dst Area: Dynamically Sized Types S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@estebank @rust-highfive @Aaron1011 @Dylan-DPC-zz @nikomatsakis @crlf0710 @jyn514 @camelid