-
Notifications
You must be signed in to change notification settings - Fork 7
don't generate extra impl
for Eq
assertions
#128
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
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
Rollup merge of #145145 - fee1-dead-contrib:push-qnmpmtmtpkkr, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
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 specifically removed this a while ago because assert_receiver_is_total_eq()
is and undocumented method and therefor unstable.
But I agree that the current state is not ideal and we actually considered splitting the crate into a normal and proc-macro crate for this reason specifically.
incorrect. the method is marked as #[stable] since Rust 1.0. It's not going to be removed just because it is doc(hidden). If the libs team wanted to leave it open for removal they would have used the unstable attribute. |
All methods in Std marked with EDIT: the function was called I'm happy to be told otherwise if you can find me something concrete on that topic, I myself don't have anything concrete on it. |
This was removed in this PR: rust-lang/rust@ #[unstable(feature = "libstd_io_internals", issue = "42788")]
#[doc(no_inline, hidden)]
pub use self::stdio::{set_panic, set_print}; The method we're using in this PR is stable: #[stable(feature = "rust1", since = "1.0.0")]
fn assert_receiver_is_total_eq(&self) {} This is stable Rust so there's no way the libs team can remove it as it would be a breaking change. |
So
is not true. Rust stdlib maintainers cannot make breaking changes to stable code. That's outlined in https://rust-lang.github.io/rfcs/1105-api-evolution.html - |
Ah, good catch.
I'm not seeing that |
Worst case scenario: I'm happy to take a quote from a Rust team member on e.g. Zulip or something. |
See also https://rustc-dev-guide.rust-lang.org/stability.html. I'm a Rust team member, although I primary work with the compiler (I'm a compiler maintainer). I am confident that |
Eq
already has a methodassert_receiver_is_total_eq
that allows you to put asserts in. This PR makes it so we use that instead of generating a new trait and impl every time (which could be a lot).This is also a cleanup because apparently
additional_impl
is only used byEq
(and arguably quite confusing asadditional_impl
is used to generate theEq
impl while the "main"impl
was the extra impl) so we can remove that now.generating a
struct __AssertEq
each time is still a little wasteful, but given thatderive-where
is a single crate (so no "normal" support crate to share types) it might be unavoidable.