deprecate Eq::assert_receiver_is_total_eq and emit FCW on manual impls#149978
Conversation
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
|
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. |
…od, r=madsmtm deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls The `Eq::assert_receiver_is_total_eq` method 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 implement `Eq`. 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_unstable` lint (rust-lang#64266). See also rust-lang/libs-team#704.
…od, r=madsmtm deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls The `Eq::assert_receiver_is_total_eq` method 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 implement `Eq`. 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_unstable` lint (rust-lang#64266). See also rust-lang/libs-team#704.
…od, r=madsmtm deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls The `Eq::assert_receiver_is_total_eq` method 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 implement `Eq`. 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_unstable` lint (rust-lang#64266). See also rust-lang/libs-team#704.
…od, r=madsmtm deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls The `Eq::assert_receiver_is_total_eq` method 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 implement `Eq`. 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_unstable` lint (rust-lang#64266). See also rust-lang/libs-team#704.
…uwer Rollup of 13 pull requests Successful merges: - #153079 (Revert "Move aarch64-apple dist builder to dynamic llvm linking") - #148146 (CI: use alternative disks if available) - #149937 (spliit out `linker-info` from `linker-messages`) - #151771 (Fix: On wasm targets, call `panic_in_cleanup` if panic occurs in cleanup) - #153035 (rustc_target: callconv: powerpc64: Use llvm_abiname rather than target_abi for ABI determination) - #153075 (mGCA: Lower negated literals directly and reject non-integer negations) - #153078 (Remove `QuerySystemFns`) - #149978 (deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls) - #153029 (Rename `rustc::pass_by_value` lint as `rustc::disallowed_pass_by_ref`.) - #153063 (`is_ty_must_use`: do not require a `span` argument) - #153071 (Update books) - #153092 (Remove redundant self usages) - #153094 (Simplify `AppendOnlyVec` iterators) Failed merges: - #153091 (Migration of `LintDiagnostic` - part 4)
Rollup of 13 pull requests Successful merges: - #148146 (CI: use alternative disks if available) - #151771 (Fix: On wasm targets, call `panic_in_cleanup` if panic occurs in cleanup) - #153035 (rustc_target: callconv: powerpc64: Use llvm_abiname rather than target_abi for ABI determination) - #153075 (mGCA: Lower negated literals directly and reject non-integer negations) - #153078 (Remove `QuerySystemFns`) - #153089 (interpret: avoid dummy spans in the stacktrace) - #153111 (Refactor url_parts to return is_absolute instead of out param) - #149978 (deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls) - #153029 (Rename `rustc::pass_by_value` lint as `rustc::disallowed_pass_by_ref`.) - #153063 (`is_ty_must_use`: do not require a `span` argument) - #153071 (Update books) - #153092 (Remove redundant self usages) - #153094 (Simplify `AppendOnlyVec` iterators) Failed merges: - #153091 (Migration of `LintDiagnostic` - part 4)
Rollup merge of #149978 - cyrgani:no-eq-assert-receiver-method, r=madsmtm deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls The `Eq::assert_receiver_is_total_eq` method 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 implement `Eq`. 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_unstable` lint (#64266). See also rust-lang/libs-team#704.
Rollup of 13 pull requests Successful merges: - rust-lang/rust#148146 (CI: use alternative disks if available) - rust-lang/rust#151771 (Fix: On wasm targets, call `panic_in_cleanup` if panic occurs in cleanup) - rust-lang/rust#153035 (rustc_target: callconv: powerpc64: Use llvm_abiname rather than target_abi for ABI determination) - rust-lang/rust#153075 (mGCA: Lower negated literals directly and reject non-integer negations) - rust-lang/rust#153078 (Remove `QuerySystemFns`) - rust-lang/rust#153089 (interpret: avoid dummy spans in the stacktrace) - rust-lang/rust#153111 (Refactor url_parts to return is_absolute instead of out param) - rust-lang/rust#149978 (deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls) - rust-lang/rust#153029 (Rename `rustc::pass_by_value` lint as `rustc::disallowed_pass_by_ref`.) - rust-lang/rust#153063 (`is_ty_must_use`: do not require a `span` argument) - rust-lang/rust#153071 (Update books) - rust-lang/rust#153092 (Remove redundant self usages) - rust-lang/rust#153094 (Simplify `AppendOnlyVec` iterators) Failed merges: - rust-lang/rust#153091 (Migration of `LintDiagnostic` - part 4)
|
@rust-timer build 74d03a9 |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (74d03a9): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.5%, secondary 4.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.1%, secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.3%, secondary 3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 492.161s -> 479.662s (-2.54%) |
| nonself_args: vec![], | ||
| ret_ty: Unit, | ||
| attributes: thin_vec![ | ||
| cx.attr_word(sym::inline, span), |
There was a problem hiding this comment.
Damn, I didn't think about that, but probably this that causes the regression? Inlined functions aren't codegen'd.
@cyrgani will you file a PR to fix it, or should I?
Rollup of 13 pull requests Successful merges: - rust-lang/rust#148146 (CI: use alternative disks if available) - rust-lang/rust#151771 (Fix: On wasm targets, call `panic_in_cleanup` if panic occurs in cleanup) - rust-lang/rust#153035 (rustc_target: callconv: powerpc64: Use llvm_abiname rather than target_abi for ABI determination) - rust-lang/rust#153075 (mGCA: Lower negated literals directly and reject non-integer negations) - rust-lang/rust#153078 (Remove `QuerySystemFns`) - rust-lang/rust#153089 (interpret: avoid dummy spans in the stacktrace) - rust-lang/rust#153111 (Refactor url_parts to return is_absolute instead of out param) - rust-lang/rust#149978 (deprecate `Eq::assert_receiver_is_total_eq` and emit FCW on manual impls) - rust-lang/rust#153029 (Rename `rustc::pass_by_value` lint as `rustc::disallowed_pass_by_ref`.) - rust-lang/rust#153063 (`is_ty_must_use`: do not require a `span` argument) - rust-lang/rust#153071 (Update books) - rust-lang/rust#153092 (Remove redundant self usages) - rust-lang/rust#153094 (Simplify `AppendOnlyVec` iterators) Failed merges: - rust-lang/rust#153091 (Migration of `LintDiagnostic` - part 4)
View all comments
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.