-
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
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 |
|
I did ask in Zulip: https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/.60.23.5Bstable.5D.60.20but.20.60.23.5Bdoc.28hidden.29.5D.60. But please let me know if you find out otherwise. |
Oh well. That part is really unintuitive. Thanks for asking! I stand corrected |
Eqalready has a methodassert_receiver_is_total_eqthat 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_implis only used byEq(and arguably quite confusing asadditional_implis used to generate theEqimpl while the "main"implwas the extra impl) so we can remove that now.generating a
struct __AssertEqeach time is still a little wasteful, but given thatderive-whereis a single crate (so no "normal" support crate to share types) it might be unavoidable.