Don't derive PartialEq::ne.

[nnethercote]

Currently we skip deriving PartialEq::ne for C-like (fieldless) enums
and empty structs, thus relying 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.

[rust-highfive]

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

[nnethercote]

r? @ghost

[nnethercote]

@petrochenkov approved this originally in #98376, which contained 6 commits. I ended up merging the first 5 commits and putting the final commit here, for further discussion.

cc:

• @Mark-Simulacrum, who expressed opinions in the previous PR about additional discussion.
• @estebank, because some error messages changed.
• @richkadel, because some coverage test outputs changed.

[nnethercote]

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.

The following program is an example.

struct A(u32);

impl PartialEq for A {
    fn eq(&self, other: &A) -> bool {
        self.0 == other.0
    }

    fn ne(&self, _other: &A) -> bool {
        true // Buggy! Doesn't satisfy `!(self == other)`
    }
}

#[derive(PartialEq)]
struct B(A);

fn main() {
    let a1 = A(1);
    let a2 = A(2);
    println!("a1 == a1: {}", a1 == a1);
    println!("a1 != a1: {} (buggy)", a1 != a1);
    println!("a1 == a2: {}", a1 == a2);
    println!("a1 != a2: {}", a1 != a2);
    println!("");

    let b1 = B(A(1));
    let b2 = B(A(2));
    println!("b1 == b1: {}", b1 == b1);
    // - Old behaviour: prints `true`, because the derived `B::ne` calls
    //   `A::ne`, which is buggy.
    // - New behaviour: prints `false`, because the default `B::ne` just
    //   inverts the results of `B::eq`, which is correct.
    println!("b1 != b1: {} (buggy?)", b1 != b1);
    println!("b1 == b2: {}", b1 == b2);
    println!("b1 != b2: {}", b1 != b2);
}

When compiled with current Rust, the output is this:

a1 == a1: true
a1 != a1: true (buggy)
a1 == a2: false
a1 != a2: true

b1 == b1: true
b1 != b1: true (buggy?)
b1 == b2: false
b1 != b2: true

When compiled with this PR applied, the output is this:

a1 == a1: true
a1 != a1: true (buggy)
a1 == a2: false
a1 != a2: true

b1 == b1: true
b1 != b1: false (buggy?)
b1 == b2: false
b1 != b2: true

[nnethercote]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 2b04e85e10de560bb30d52774b4c706497d5e804 with merge 60726948eec003f9f0b3eabddd2ddb8fc98dcfdc...

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

☀️ Try build successful - checks-actions
Build commit: 60726948eec003f9f0b3eabddd2ddb8fc98dcfdc (60726948eec003f9f0b3eabddd2ddb8fc98dcfdc)

[rust-timer]

Queued 60726948eec003f9f0b3eabddd2ddb8fc98dcfdc with parent 126e3df, future comparison URL.

[Mark-Simulacrum]

Thanks! I'm largely fine with this change. I think another possible unfortunate part of this is that if you somehow have a faster inequality impl, then this means you need to write custom impls all the way up the type hierarchy to make use of that lower-level optimization.

I don't think it's particularly likely that this is a problem though, either.

[CryZe]

then this means you need to write custom impls all the way up the type hierarchy to make use of that lower-level optimization.

This sounds like it essentially deprecates the entire method though, because not only is it rare that you would ever implement this method for an optimization, but also that this optimization is now so brittle that any wrapper / compound type above it immediately loses that optimization too. So that likely means that in practice the method is now close to useless to the point of deprecation.

[rust-timer]

Finished benchmarking commit (60726948eec003f9f0b3eabddd2ddb8fc98dcfdc): comparison url.

Instruction count

• Primary benchmarks: 🎉 relevant improvements found
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.8%	-1.8%	70
Improvements 🎉 (secondary)	-5.9%	-11.4%	27
All 😿🎉 (primary)	-0.8%	-1.8%	70

Max RSS (memory usage)

Results

• Primary benchmarks: 🎉 relevant improvements found
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.8%	3.4%	2
Improvements 🎉 (primary)	-2.1%	-5.3%	6
Improvements 🎉 (secondary)	-3.0%	-4.4%	19
All 😿🎉 (primary)	-2.1%	-5.3%	6

Cycles

Results

• Primary benchmarks: mixed results
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	4.2%	4.2%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-4.4%	-6.1%	3
Improvements 🎉 (secondary)	-8.9%	-15.4%	23
All 😿🎉 (primary)	-2.3%	-6.1%	4

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[richkadel]

Reviewing the coverage results diffs. The coverage results are actually cleaner with this change. LGTM!

[estebank]

Sign off on diagnostics changes.

[slanterns]

What does this mean? Will it happen automatically? Does someone need to r+ it?

No, it still requires manually r+.

[nnethercote]

The final comment period, with a disposition to merge, as per the review above, is now complete.

r? @dtolnay

[dtolnay]

Terrific. :) Very happy about this PR.

[dtolnay]

@bors r+

[bors]

📌 Commit d4a5b03 has been approved by dtolnay

It is now in the queue for this repository.

[bors]

⌛ Testing commit d4a5b03 with merge 361c599...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 361c599 to master...

[rust-timer]

Finished benchmarking commit (361c599): comparison url.

Instruction count

• Primary benchmarks: ✅ relevant improvements found
• Secondary benchmarks: ✅ relevant improvements found

	mean1	max	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.0%	1.0%	1
Improvements ✅ (primary)	-0.7%	-1.4%	65
Improvements ✅ (secondary)	-5.2%	-10.0%	27
All ❌✅ (primary)	-0.7%	-1.4%	65

Max RSS (memory usage)

Results

• Primary benchmarks: ✅ relevant improvements found
• Secondary benchmarks: ✅ relevant improvements found

	mean1	max	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.3%	-3.3%	7
Improvements ✅ (secondary)	-3.2%	-6.1%	20
All ❌✅ (primary)	-1.3%	-3.3%	7

Cycles

Results

• Primary benchmarks: ❌ relevant regression found
• Secondary benchmarks: ✅ relevant improvements found

	mean1	max	count2
Regressions ❌ (primary)	2.8%	2.8%	1
Regressions ❌ (secondary)	2.4%	2.4%	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.3%	-9.7%	21
All ❌✅ (primary)	2.8%	2.8%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³