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

Suggest to swap a struct and a trait in trait impls #97812

Merged
merged 2 commits into from
Jun 11, 2022
Merged

Suggest to swap a struct and a trait in trait impls #97812

merged 2 commits into from
Jun 11, 2022

Conversation

TaKO8Ki
Copy link
Member

@TaKO8Ki TaKO8Ki commented Jun 7, 2022

closes #89590

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 7, 2022
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Jun 7, 2022
@TaKO8Ki TaKO8Ki force-pushed the suggest-to-swap-struct-and-trait branch from 5183c5b to 9db03b9 Compare June 7, 2022 00:53
@petrochenkov
Copy link
Contributor

I don't think this suggestion justifies technical debt that it introduces, and I would personally close #89590 as wontfix.
The "expected trait, found struct Struct" message clearly identifies what is expected in the position before for.
@rustbot author

@rustbot rustbot 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 Jun 8, 2022
@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Jun 8, 2022

@petrochenkov
Thank you for your review. I would like to know what others think about this issue. @rust-lang/wg-diagnostics

estebank
estebank reviewed Jun 8, 2022
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

If we could either move the logic out of the way so that the code is easier to read, or simplify and only signal that we're resolving an impl item header and emit not a structured suggestion but a note, it would be better.

@petrochenkov are you concerned with the growth of DiagnosticMetadata, the added logic, or both?

compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved
src/test/ui/suggestions/suggest-swapping-self-ty-and-trait-edition-2021.stderr Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved
@TaKO8Ki TaKO8Ki requested a review from estebank June 9, 2022 10:09
@davidtwco
Copy link
Member

r? @estebank

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Jun 10, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2022
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 10, 2022

📌 Commit 5639e52 has been approved by estebank

@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 Jun 10, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 11, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 11, 2022
@bors
Auto merge of rust-lang#97980 - Dylan-DPC:rollup-l8exe4b, r=Dylan-DPC
7092d42 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#96868 (Stabilize explicit_generic_args_with_impl_trait)
 - rust-lang#97703 (some additional `need_type_info.rs` cleanup)
 - rust-lang#97812 (Suggest to swap a struct and a trait in trait impls)
 - rust-lang#97958 (ExitStatus docs fixups)
 - rust-lang#97967 (Mention `infer::Trace` methods on `infer::At` methods' docs)
 - rust-lang#97972 (Update #[doc(html_playground_url)] documentation to mention what the request will be)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 640019b into rust-lang:master Jun 11, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 11, 2022
@TaKO8Ki TaKO8Ki deleted the suggest-to-swap-struct-and-trait branch June 11, 2022 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank Awaiting requested review from estebank

Assignees

@estebank estebank

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

Suggest to swap a struct and a trait in trait impls where appropriate
7 participants
@TaKO8Ki @rust-highfive @petrochenkov @davidtwco @estebank @bors @rustbot