Auto merge of rust-lang#98655 - nnethercote:dont-derive-PartialEq-ne, r=dtolnay

Don't derive `PartialEq::ne`.

Currently we skip deriving `PartialEq::ne` for C-like (fieldless) enums
and empty structs, thus reyling on the default `ne`. This behaviour is
unnecessarily conservative, because the `PartialEq` docs say this:

> Implementations must ensure that eq and ne are consistent with each other:
>
> `a != b` if and only if `!(a == b)` (ensured by the default
> implementation).

This means that the default implementation (`!(a == b)`) is always good
enough. So this commit changes things such that `ne` is never derived.

The motivation for this change is that not deriving `ne` reduces compile
times and binary sizes.

Observable behaviour may change if a user has defined a type `A` with an
inconsistent `PartialEq` and then defines a type `B` that contains an
`A` and also derives `PartialEq`. Such code is already buggy and
preserving bug-for-bug compatibility isn't necessary.

Two side-effects of the change:
- There is only one error message produced for types where `PartialEq`
  cannot be derived, instead of two.
- For coverage reports, some warnings about generated `ne` methods not
  being executed have disappeared.

Both side-effects seem fine, and possibly preferable.

Author: bors

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

@@ -15,14 +15,8 @@ pub fn expand_deriving_partial_eq(
     item: &Annotatable,
     push: &mut dyn FnMut(Annotatable),
 ) {
-    fn cs_op(
-        cx: &mut ExtCtxt<'_>,
-        span: Span,
-        substr: &Substructure<'_>,
-        op: BinOpKind,
-        combiner: BinOpKind,
-        base: bool,
-    ) -> BlockOrExpr {
+    fn cs_eq(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
+        let base = true;
         let expr = cs_fold(
             true, // use foldl
             cx,
@@ -47,39 +41,22 @@ pub fn expand_deriving_partial_eq(
                             cx.expr_deref(field.span, expr.clone())
                         }
                     };
-                    cx.expr_binary(field.span, op, convert(&field.self_expr), convert(other_expr))
+                    cx.expr_binary(
+                        field.span,
+                        BinOpKind::Eq,
+                        convert(&field.self_expr),
+                        convert(other_expr),
+                    )
+                }
+                CsFold::Combine(span, expr1, expr2) => {
+                    cx.expr_binary(span, BinOpKind::And, expr1, expr2)
                 }
-                CsFold::Combine(span, expr1, expr2) => cx.expr_binary(span, combiner, expr1, expr2),
                 CsFold::Fieldless => cx.expr_bool(span, base),
             },
         );
         BlockOrExpr::new_expr(expr)
     }
 
-    fn cs_eq(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
-        cs_op(cx, span, substr, BinOpKind::Eq, BinOpKind::And, true)
-    }
-    fn cs_ne(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
-        cs_op(cx, span, substr, BinOpKind::Ne, BinOpKind::Or, false)
-    }
-
-    macro_rules! md {
-        ($name:expr, $f:ident) => {{
-            let inline = cx.meta_word(span, sym::inline);
-            let attrs = vec![cx.attribute(inline)];
-            MethodDef {
-                name: $name,
-                generics: Bounds::empty(),
-                explicit_self: true,
-                nonself_args: vec![(self_ref(), sym::other)],
-                ret_ty: Path(path_local!(bool)),
-                attributes: attrs,
-                unify_fieldless_variants: true,
-                combine_substructure: combine_substructure(Box::new(|a, b, c| $f(a, b, c))),
-            }
-        }};
-    }
-
     super::inject_impl_of_structural_trait(
         cx,
         span,
@@ -88,13 +65,20 @@ pub fn expand_deriving_partial_eq(
         push,
     );
 
-    // avoid defining `ne` if we can
-    // c-like enums, enums without any fields and structs without fields
-    // can safely define only `eq`.
-    let mut methods = vec![md!(sym::eq, cs_eq)];
-    if !is_type_without_fields(item) {
-        methods.push(md!(sym::ne, cs_ne));
-    }
+    // No need to generate `ne`, the default suffices, and not generating it is
+    // faster.
+    let inline = cx.meta_word(span, sym::inline);
+    let attrs = vec![cx.attribute(inline)];
+    let methods = vec![MethodDef {
+        name: sym::eq,
+        generics: Bounds::empty(),
+        explicit_self: true,
+        nonself_args: vec![(self_ref(), sym::other)],
+        ret_ty: Path(path_local!(bool)),
+        attributes: attrs,
+        unify_fieldless_variants: true,
+        combine_substructure: combine_substructure(Box::new(|a, b, c| cs_eq(a, b, c))),
+    }];
 
     let trait_def = TraitDef {
         span,

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

@@ -1625,19 +1625,3 @@ where
         StaticEnum(..) | StaticStruct(..) => cx.span_bug(trait_span, "static function in `derive`"),
     }
 }
-
-/// Returns `true` if the type has no value fields
-/// (for an enum, no variant has any fields)
-pub fn is_type_without_fields(item: &Annotatable) -> bool {
-    if let Annotatable::Item(ref item) = *item {
-        match item.kind {
-            ast::ItemKind::Enum(ref enum_def, _) => {
-                enum_def.variants.iter().all(|v| v.data.fields().is_empty())
-            }
-            ast::ItemKind::Struct(ref variant_data, _) => variant_data.fields().is_empty(),
-            _ => false,
-        }
-    } else {
-        false
-    }
-}

library/core/src/cmp.rs

@@ -38,8 +38,10 @@ use self::Ordering::*;
 ///
 /// Implementations must ensure that `eq` and `ne` are consistent with each other:
 ///
-/// - `a != b` if and only if `!(a == b)`
-///   (ensured by the default implementation).
+/// - `a != b` if and only if `!(a == b)`.
+///
+/// The default implementation of `ne` provides this consistency and is almost
+/// always sufficient. It should not be overridden without very good reason.
 ///
 /// If [`PartialOrd`] or [`Ord`] are also implemented for `Self` and `Rhs`, their methods must also
 /// be consistent with `PartialEq` (see the documentation of those traits for the exact
@@ -225,7 +227,8 @@ pub trait PartialEq<Rhs: ?Sized = Self> {
     #[stable(feature = "rust1", since = "1.0.0")]
     fn eq(&self, other: &Rhs) -> bool;
 
-    /// This method tests for `!=`.
+    /// This method tests for `!=`. The default implementation is almost always
+    /// sufficient, and should not be overridden without very good reason.
     #[inline]
     #[must_use]
     #[stable(feature = "rust1", since = "1.0.0")]

src/test/run-make/coverage-reports/expected_show_coverage.doctest.txt

@@ -23,12 +23,6 @@
    22|      1|//! ```
    23|      2|//! #[derive(Debug, PartialEq)]
                        ^1
-  ------------------
-  | Unexecuted instantiation: <rust_out::main::_doctest_main____coverage_doctest_rs_22_0::SomeError as core::cmp::PartialEq>::ne
-  ------------------
-  | <rust_out::main::_doctest_main____coverage_doctest_rs_22_0::SomeError as core::cmp::PartialEq>::eq:
-  |   23|      2|//! #[derive(Debug, PartialEq)]
-  ------------------
    24|      1|//! struct SomeError {
    25|      1|//!     msg: String,
    26|      1|//! }

src/test/run-make/coverage-reports/expected_show_coverage.issue-83601.txt

@@ -2,12 +2,6 @@
     2|       |
     3|      3|#[derive(Debug, PartialEq, Eq)]
                               ^2
-  ------------------
-  | <issue_83601::Foo as core::cmp::PartialEq>::eq:
-  |    3|      2|#[derive(Debug, PartialEq, Eq)]
-  ------------------
-  | Unexecuted instantiation: <issue_83601::Foo as core::cmp::PartialEq>::ne
-  ------------------
     4|       |struct Foo(u32);
     5|       |
     6|      1|fn main() {

src/test/run-make/coverage-reports/expected_show_coverage.issue-84561.txt

@@ -2,12 +2,6 @@
     2|       |
     3|       |// expect-exit-status-101
     4|     21|#[derive(PartialEq, Eq)]
-  ------------------
-  | <issue_84561::Foo as core::cmp::PartialEq>::eq:
-  |    4|     21|#[derive(PartialEq, Eq)]
-  ------------------
-  | Unexecuted instantiation: <issue_84561::Foo as core::cmp::PartialEq>::ne
-  ------------------
     5|       |struct Foo(u32);
     6|      1|fn test3() {
     7|      1|    let is_true = std::env::args().len() == 1;

src/test/run-make/coverage-reports/expected_show_coverage.partial_eq.txt

@@ -3,11 +3,6 @@
     3|       |
     4|      2|#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
                        ^0            ^0      ^0     ^1       ^1 ^0^0
-  ------------------
-  | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialEq>::ne
-  ------------------
-  | Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialEq>::eq
-  ------------------
     5|       |pub struct Version {
     6|       |    major: usize,
     7|       |    minor: usize,

src/test/ui/derives/derives-span-PartialEq-enum-struct-variant.rs

@@ -7,7 +7,6 @@ struct Error;
 enum Enum {
    A {
      x: Error //~ ERROR
-//~^ ERROR
    }
 }
 

src/test/ui/derives/derives-span-PartialEq-enum-struct-variant.stderr

@@ -18,26 +18,6 @@ help: consider annotating `Error` with `#[derive(PartialEq)]`
 LL | #[derive(PartialEq)]
    |
 
-error[E0369]: binary operation `!=` cannot be applied to type `Error`
-  --> $DIR/derives-span-PartialEq-enum-struct-variant.rs:9:6
-   |
-LL | #[derive(PartialEq)]
-   |          --------- in this derive macro expansion
-...
-LL |      x: Error
-   |      ^^^^^^^^
-   |
-note: an implementation of `PartialEq<_>` might be missing for `Error`
-  --> $DIR/derives-span-PartialEq-enum-struct-variant.rs:4:1
-   |
-LL | struct Error;
-   | ^^^^^^^^^^^^ must implement `PartialEq<_>`
-   = note: this error originates in the derive macro `PartialEq` (in Nightly builds, run with -Z macro-backtrace for more info)
-help: consider annotating `Error` with `#[derive(PartialEq)]`
-   |
-LL | #[derive(PartialEq)]
-   |
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0369`.

src/test/ui/derives/derives-span-PartialEq-enum.rs

@@ -7,8 +7,7 @@ struct Error;
 enum Enum {
    A(
      Error //~ ERROR
-//~^ ERROR
-     )
+   )
 }
 
 fn main() {}