-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
deprecate Eq::assert_receiver_is_total_eq and emit FCW on manual impls
#149978
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
base: main
Are you sure you want to change the base?
Conversation
| Warn, | ||
| "manual implementation of the internal `Eq::assert_receiver_is_total_eq` method", | ||
| @future_incompatible = FutureIncompatibleInfo { | ||
| reason: fcw!(FutureReleaseError #150000), |
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.
TODO: create a real tracking issue once there is consensus on this change
This comment has been minimized.
This comment has been minimized.
faf29de to
81dc07d
Compare
Eq::assert_receiver_is_total_eq and emit a FCW on manual …Eq::assert_receiver_is_total_eq and emit FCW on manual impls
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.
Implementation looks good, feel free to r=me on that once you have t-libs signoff (FCP?).
Procedurally, this might also need t-lang approval for the lint name IIUC? Or can t-libs decide that?
|
Since there's never a valid reason to manually implement this method, having an FCW urging people to remove their manual implementation seems like the best way to go. @rfcbot merge libs-api lang |
|
Error encounted: |
|
@rfcbot merge |
|
Error encounted: |
|
@rfcbot merge libs-api,lang |
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
I don't agree with this. The reason is the same reason that the standard library's derived I am open to deprecating the method, but I would not want to do so while |
|
Ah, relevant line: rust/compiler/rustc_lint/src/builtin.rs Line 3244 in 81dc07d
"Manual" means not from a macro expansion. Then this seems fine. In the longer term, I would prefer to fully deprecate |
|
const _: () = {
fn assert_receiver_is_total_eq<Generics>() {
..
}
};So even custom derives of |
compiler/rustc_lint/src/builtin.rs
Outdated
| impl<'tcx> LateLintPass<'tcx> for EqAssertReceiverIsTotalEq { | ||
| fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx rustc_hir::ImplItem<'tcx>) { | ||
| if let ImplItemImplKind::Trait { defaultness: _, trait_item_def_id: Ok(trait_item_def_id) } = | ||
| item.impl_kind | ||
| && item.ident.name == sym::assert_receiver_is_total_eq | ||
| && cx.tcx.is_diagnostic_item(sym::assert_receiver_is_total_eq, trait_item_def_id) | ||
| && !item.span.from_expansion() | ||
| { | ||
| cx.emit_span_lint( | ||
| EQ_ASSERT_RECEIVER_IS_TOTAL_EQ_IMPL, | ||
| item.span, | ||
| EqInternalMethodImplemented, | ||
| ); | ||
| } | ||
| } | ||
| } |
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 gather this !item.span.from_expansion() check suppresses the lint for impls from all macro expansions, i.e. even for user-defined macros. Given that we want to catch as much as possible so as to be able to make this into an error, perhaps a more strict check is in order?
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 updated the lint to fire in every case and added an explicit #[allow(internal_eq_trait_method_impls)] to the method generated by derive(Eq).
|
Reminder, once the PR becomes ready for a review, use |
81dc07d to
62743e8
Compare
|
Changes to the code generated for builtin derived traits. cc @nnethercote |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
62743e8 to
4ee1588
Compare
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.
|
☔ The latest upstream changes (presumably #151051) made this pull request unmergeable. Please resolve the merge conflicts. |
The
Eq::assert_receiver_is_total_eqmethod is purely meant as an implementation detail by#[derive(Eq)]to add checks that all fields of the type the derive is applied to also implementEq.The method is already
#[doc(hidden)]and has a comment saying// This should never be implemented by hand..Unfortunately, it has been stable since 1.0 and there are some cases on GitHub (https://github.com/search?q=assert_receiver_is_total_eq&type=code) where people have implemented this method manually, sometimes even with actual code in the method body (example: https://github.com/Shresht7/codecrafters-redis-rust/blob/31f0ec453c504b4ab053a7b1c3ff548ff36a9db5/src/parser/resp/types.rs#L255).
To prevent further confusion from this, this PR is deprecating the method and adds a FCW when it is manually implemented (this is necessary as the deprecation warning is not emitted when the method is implemented, only when it is called).
This is similar to what was previously done with the
soft_unstablelint (#64266).See also rust-lang/libs-team#704.