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 derivable trait on E0277 error #95525

Merged
merged 4 commits into from
Apr 5, 2022
Merged

Suggest derivable trait on E0277 error #95525

merged 4 commits into from
Apr 5, 2022

Conversation

ohno418
Copy link
Contributor

@ohno418 ohno418 commented Mar 31, 2022

Closes #95099 .

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

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2022
@compiler-errors
Copy link
Member

I think this diagnostic needs to be a bit more specific.

Ideally it should only suggest deriving Copy (and other traits) on ADTs that satisfy all (or at least most?) of the conditions of those traits implied by the derive macro. For example, we shouldn't suggest derive(Copy) on this MyNoncopyStruct.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@wesleywiser
Copy link
Member

r? @compiler-errors

compiler-errors
compiler-errors requested changes Apr 1, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I left an idea for a way to limit the suggestions only to places that are relatively(*) accurate, so we don't suggest to derive Copy on, e.g.

struct Foo {
  s: String,
}

(*) = It's not exactly how the derive macro works, but it should limit really inaccurate suggestions at least.

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
compiler-errors
compiler-errors requested changes Apr 3, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

A few more changes to fix predicate_must_hold_modulo_regions check for PartialEq and PartialOrd, then I think it's ready!

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
compiler-errors
compiler-errors requested changes Apr 4, 2022
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

sorry for the back and forth-- almost there, but one more change. also @ohno418 , can you finally squash all of the commits that start with "fix"? is fine to have multiple commits, but 8 seems like too much.

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved
@ohno418 ohno418 force-pushed the suggest-derivable-trait-E0277 branch from e9beb6d to de23782 Compare April 5, 2022 02:56
@compiler-errors
Copy link
Member

Wonderful. Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit de23782 has been approved by compiler-errors

@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 Apr 5, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 5, 2022
@Dylan-DPC
Rollup merge of rust-lang#95525 - ohno418:suggest-derivable-trait-E02…
c95357a 
…77, r=compiler-errors

Suggest derivable trait on E0277 error

Closes rust-lang#95099 .
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 5, 2022
Closed
@Dylan-DPC
Copy link
Member

failed in rollup

@bors r-

@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 Apr 5, 2022
@compiler-errors
Copy link
Member

@ohno418, can you re-bless the test suite? then we can try to land this again.

@ohno418
Copy link
Contributor Author

ohno418 commented Apr 5, 2022 •
edited
Loading

@compiler-errors
Done.
(I have overlooked the --compare-mode nll option.)

@compiler-errors
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit b831b60 has been approved by compiler-errors

@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 Apr 5, 2022
@Dylan-DPC
Copy link
Member

@bors rollup=maybe

@bors
Copy link
Contributor

bors commented Apr 5, 2022

⌛ Testing commit b831b60 with merge 8068b794100ac89988924fd0d3582d5fa17eda5a...

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 5, 2022
@Dylan-DPC
Rollup merge of rust-lang#95525 - ohno418:suggest-derivable-trait-E02…
e028d93 
…77, r=compiler-errors

Suggest derivable trait on E0277 error

Closes rust-lang#95099 .
@Dylan-DPC Dylan-DPC mentioned this pull request Apr 5, 2022
Closed
@Dylan-DPC
Copy link
Member

@bors retry (yield to rollup containing this)

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@Dylan-DPC Dylan-DPC mentioned this pull request Apr 5, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2022
@bors
Auto merge of rust-lang#95680 - Dylan-DPC:rollup-7jldtnz, r=Dylan-DPC
d4c7839 
Rollup of 4 pull requests

Successful merges:

 - rust-lang#95525 (Suggest derivable trait on E0277 error)
 - rust-lang#95654 (diagnostics: use correct span for const generics)
 - rust-lang#95660 (Update panic docs to make it clearer when to use panic vs Result)
 - rust-lang#95670 (Refactor: remove unused function parameters)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 661b0e5 into rust-lang:master Apr 5, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 5, 2022
@ohno418 ohno418 deleted the suggest-derivable-trait-E0277 branch April 5, 2022 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors requested changes

Assignees

@compiler-errors compiler-errors

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.62.0
Development

Successfully merging this pull request may close these issues.

E0277 for #[derive(_)]-able trait on local type should have structured suggestion
8 participants
@ohno418 @rust-highfive @compiler-errors @rust-log-analyzer @wesleywiser @bors @Dylan-DPC @rustbot