Skip to content

Commit

Permalink
Don't derive PartialEq::ne.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nnethercote committed Jul 31, 2022
1 parent 038f9e6 commit d4a5b03
Show file tree
Hide file tree
Showing 18 changed files with 38 additions and 252 deletions.
66 changes: 25 additions & 41 deletions compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down
16 changes: 0 additions & 16 deletions compiler/rustc_builtin_macros/src/deriving/generic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1637,19 +1637,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
}
}
9 changes: 6 additions & 3 deletions library/core/src/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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|//! }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ struct Error;
enum Enum {
A {
x: Error //~ ERROR
//~^ ERROR
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
3 changes: 1 addition & 2 deletions src/test/ui/derives/derives-span-PartialEq-enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ struct Error;
enum Enum {
A(
Error //~ ERROR
//~^ ERROR
)
)
}

fn main() {}
22 changes: 1 addition & 21 deletions src/test/ui/derives/derives-span-PartialEq-enum.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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.rs:9:6
|
LL | #[derive(PartialEq)]
| --------- in this derive macro expansion
...
LL | Error
| ^^^^^
|
note: an implementation of `PartialEq<_>` might be missing for `Error`
--> $DIR/derives-span-PartialEq-enum.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`.
1 change: 0 additions & 1 deletion src/test/ui/derives/derives-span-PartialEq-struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ struct Error;
#[derive(PartialEq)]
struct Struct {
x: Error //~ ERROR
//~^ ERROR
}

fn main() {}
22 changes: 1 addition & 21 deletions src/test/ui/derives/derives-span-PartialEq-struct.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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-struct.rs:8:5
|
LL | #[derive(PartialEq)]
| --------- in this derive macro expansion
LL | struct Struct {
LL | x: Error
| ^^^^^^^^
|
note: an implementation of `PartialEq<_>` might be missing for `Error`
--> $DIR/derives-span-PartialEq-struct.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`.
1 change: 0 additions & 1 deletion src/test/ui/derives/derives-span-PartialEq-tuple-struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ struct Error;
#[derive(PartialEq)]
struct Struct(
Error //~ ERROR
//~^ ERROR
);

fn main() {}
22 changes: 1 addition & 21 deletions src/test/ui/derives/derives-span-PartialEq-tuple-struct.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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-tuple-struct.rs:8:5
|
LL | #[derive(PartialEq)]
| --------- in this derive macro expansion
LL | struct Struct(
LL | Error
| ^^^^^
|
note: an implementation of `PartialEq<_>` might be missing for `Error`
--> $DIR/derives-span-PartialEq-tuple-struct.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`.
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ struct NoCloneOrEq;
#[derive(PartialEq)]
struct E {
x: NoCloneOrEq //~ ERROR binary operation `==` cannot be applied to type `NoCloneOrEq`
//~^ ERROR binary operation `!=` cannot be applied to type `NoCloneOrEq`
}
#[derive(Clone)]
struct C {
Expand Down
24 changes: 2 additions & 22 deletions src/test/ui/derives/deriving-no-inner-impl-error-message.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,8 @@ help: consider annotating `NoCloneOrEq` with `#[derive(PartialEq)]`
LL | #[derive(PartialEq)]
|

error[E0369]: binary operation `!=` cannot be applied to type `NoCloneOrEq`
--> $DIR/deriving-no-inner-impl-error-message.rs:5:5
|
LL | #[derive(PartialEq)]
| --------- in this derive macro expansion
LL | struct E {
LL | x: NoCloneOrEq
| ^^^^^^^^^^^^^^
|
note: an implementation of `PartialEq<_>` might be missing for `NoCloneOrEq`
--> $DIR/deriving-no-inner-impl-error-message.rs:1:1
|
LL | struct NoCloneOrEq;
| ^^^^^^^^^^^^^^^^^^ 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 `NoCloneOrEq` with `#[derive(PartialEq)]`
|
LL | #[derive(PartialEq)]
|

error[E0277]: the trait bound `NoCloneOrEq: Clone` is not satisfied
--> $DIR/deriving-no-inner-impl-error-message.rs:10:5
--> $DIR/deriving-no-inner-impl-error-message.rs:9:5
|
LL | #[derive(Clone)]
| ----- in this derive macro expansion
Expand All @@ -53,7 +33,7 @@ help: consider annotating `NoCloneOrEq` with `#[derive(Clone)]`
LL | #[derive(Clone)]
|

error: aborting due to 3 previous errors
error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0369.
For more information about an error, try `rustc --explain E0277`.
Loading

0 comments on commit d4a5b03

Please sign in to comment.