rust
rust copied to clipboard
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.
r? @nagisa
(rust-highfive has picked a reviewer for you, use r? to override)
r? @ghost
@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.
Observable behaviour may change if a user has defined a type
A
with an inconsistentPartialEq
and then defines a typeB
that contains anA
and also derivesPartialEq
. 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
@bors try @rust-timer queue
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
:hourglass: Trying commit b83ac4453642e90ef0f22cad70ef24d463e3cd45 with merge aecb2de1ee213c6e997fb2bed7f825407b6a7f1c...
@bors try @rust-timer queue
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
:hourglass: Trying commit 2b04e85e10de560bb30d52774b4c706497d5e804 with merge 60726948eec003f9f0b3eabddd2ddb8fc98dcfdc...
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
:sunny: Try build successful - checks-actions
Build commit: 60726948eec003f9f0b3eabddd2ddb8fc98dcfdc (60726948eec003f9f0b3eabddd2ddb8fc98dcfdc
)
Queued 60726948eec003f9f0b3eabddd2ddb8fc98dcfdc with parent 126e3df4065623802eda752e79839d1b6fde59be, future comparison URL.
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.
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.
Finished benchmarking commit (60726948eec003f9f0b3eabddd2ddb8fc98dcfdc): comparison url.
Instruction count
- Primary benchmarks: π relevant improvements found
- Secondary benchmarks: π relevant improvements found
mean[^1] | max | count[^2] | |
---|---|---|---|
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
mean[^1] | max | count[^2] | |
---|---|---|---|
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
mean[^1] | max | count[^2] | |
---|---|---|---|
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 |
[^1]: the arithmetic mean of the percent change [^2]: number of relevant changes
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
Sign off on diagnostics changes.
This means that the default implementation (!(a == b)) is always good enough.
If we start relying on that, then we should probably just deprecate ne
or at least warn when overriding the default implementation. There would be no reason ever to implement it manually.
If we start relying on that, then we should probably just deprecate
ne
or at least warn when overriding the default implementation.
How would I go about doing that?
We should first figure out how often ne
is overridden, to figure out if there are any legitimate use cases of that we souldn't break. And then if we do decide that ne
is unuseful, we need to decide whether to only warn when implementing/overriding the method, or also when calling it. Doing only the first probably requires writing a new lint, but doing both can be achieved with #[deprecated].
We should first figure out how often ne is overridden, to figure out if there are any legitimate use cases of that we souldn't break.
I quickly grepped through all of crates.io for fn ne(&self,
, and didn't see any implementation that isn't just identical to the default one. :)
There are only about ~800 calls to .ne()
on crates.io. (Compared to about 14'000 for .eq()
.)
Calling ne
seems fine to me, especially if that includes using !=
.
As for overriding... the docs are clear that eq
is a required method and ne
is a provided method. Is a warning really necessary?
As for overriding... the docs are clear that
eq
is a required method andne
is a provided method. Is a warning really necessary?
This could change if #[rustc_must_implement_one_of(eq, ne)]
is applied to Eq
/PartialEq
. This was one of the use case for implementing rustc_must_implement_one_of
, there is even an UI test that test this this possibility: https://github.com/rust-lang/rust/blob/a9eb9c52f3e8d8b6402e6acc69b9bcfc4f371d58/src/test/ui/traits/default-method/rustc_must_implement_one_of.rs#L3-L12
r? @m-ou-se
This could change if
#[rustc_must_implement_one_of(eq, ne)]
is applied toEq
/PartialEq
. This was one of the use case for implementingrustc_must_implement_one_of
, there is even an UI test that test this this possibility:
@Urgau: This feature doesn't work how I expected. Here I've implemented both eq
and neq
and with a nightly compiler I don't get any warnings:
#![feature(rustc_attrs)]
#[rustc_must_implement_one_of(eq, neq)]
trait Equal {
fn eq(&self, other: &Self) -> bool { !self.neq(other) }
fn neq(&self, other: &Self) -> bool { !self.eq(other) }
}
struct S(u32);
impl Equal for S {
fn eq(&self, other: &Self) -> bool { self.0 == other.0 }
fn neq(&self, other: &Self) -> bool { self.0 != other.0 }
}
fn main() {}
If I remove both fn definitions from impl Equal for S
I get "missing one of eq
, neq
in implementation", which matches what I expected.
Also, the generated docs look like this:
Both methods are labelled as provided, but there's no indication that you must implement one of them.
While it seems like there might be value in deprecating the implementation of ne
, I also don't think that needs to be a hard gate on modifying the derive. Modifying the derive is straightforward and a fairly clear win, while deprecating the implementation (but not calling) of ne
would require further compiler work for something that seems likely to only produce actual warnings for a tiny handful of crates (and probably not important warnings for any of them unless they happen to have a bug in their implementation).
Could we just start an FCP to stop deriving ne
, and consider any deprecations separately?
@Urgau: This feature doesn't work how I expected. Here I've implemented both
eq
andneq
and with a nightly compiler I don't get any warnings:
I'm not the one who implemented it (cc @WaffleLapkin) but from what I understand this is the expected behavior, it's not intended to be exclusive but more of "at least". I don't know why it wouldn't be okay to implement both of them.
If I remove both fn definitions from
impl Equal for S
I get "missing one ofeq
,neq
in implementation", which matches what I expected.
Yeah, that the "at least".
Also, the generated docs look like this: [..] Both methods are labelled as provided, but there's no indication that you must implement one of them.
rustdoc doens't yet have any logic for it, in part because no one used it in std, it could be implemented relatively simply (in my opinion). I wouldn't block on it.
I see, thanks for the explanation. So the rustc_must_implement_one_of
name is a little ambiguous. rustc_must_implement_at_least_one_of
would be clearer.
One data point that came up in the libs-api meeting just now, is that something similar happened in the past for PartialOrd, although there the 'partial'ness makes things more complicated: https://github.com/rust-lang/rust/pull/81384
Let me try to summarize the situation. There are three options, which are probably cumulative, and require increasing amounts of effort.
- Stop deriving
ne
- Stop deriving
ne
methods withderive(PartialEq)
because the default impl is good enough. - Users can still override
ne
without any warning. Existing correctness requirements still hold: "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)." - We could update the documentation to strongly recommend not implementing
ne
.
- Disallow overriding
ne
- Warn/error on overriding
ne
but allow it to be called. - Requires a new type of deprecation be implemented.
- Might prevent some clever
ne
implementation being written that is somehow better than just!eq
(but we don't have examples of this).
- Disallow calling
ne
- Warn/error on calling
ne
. - Requires a language change, such that
x != y
desugars to!x.eq(&y)
instead ofx.ne(&y)
. - Will break some existing code (e.g. 800 crates have
ne
calls).
Unsurprisingly, I think option 1 is the best, and it's what this PR currently does.
- There appears to be consensus that this approach is clearly valid, according to language semantics, and existing practice on crates.io.
- The
le
/lt
/ge
/gt
change took much the same approach, and so is a precedent for this. -
ne
isn't overridden much, seemingly, so the practical impacts of the change seem like they would be small.
Option 2 is substantially more work. It would add a new type of deprecation that doesn't seem like it would get much use. In terms of practical impact it would prevent the possibility of incompatible eq
/ne
combinations, but that doesn't seem to be a problem in practice.
Option 3 is even more work. It also seems like a non-starter, for backwards compatibility reasons.
Hey! It looks like you've submitted a new PR for the library teams!
If this PR contains changes to any rust-lang/rust
public library APIs then please comment with @rustbot label +T-libs-api -T-libs
to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.
Examples of T-libs-api
changes:
- Stabilizing library features
- Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
- Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
- Changing public documentation in ways that create new stability guarantees
- Changing observable runtime behavior of library APIs
We could update the documentation to strongly recommend not implementing ne.
I just pushed new changes that include such documentation updates. I added a note to the trait docs and the ne
docs.
FWIW, I think option 3 doesn't require changing the desugaring of !=
, as long as that desugaring doesn't produce a deprecation warning. And it sounds like many of the calls to ne
are inside implementations of ne
, so in practice that deprecation might not be a problem.
That said, I do personally believe we should start by removing the derive of ne
, and then separately decide about deprecating ne
entirely.
Might prevent some clever ne implementation being written that is somehow better than just !eq (but we don't have examples of this).
If ne
can be written more efficiently than eq
, then that eq
implementation should've just been !ne
, in which case overriding the default ne
implementation would've been unnecessary.
Disallow [..]
It also seems like a non-starter, for backwards compatibility reasons.
There's a significant difference between disallowing and deprecating. Deprecating doesn't break any crates, and is invisible for dependencies due to cap-lints
.
Option 2 is substantially more work. It would add a new type of deprecation that doesn't seem like it would get much use
It's actually not much work at all to implement. This case of #[deprecate]
is already checked and handled separately, here: https://github.com/rust-lang/rust/blob/3dcb616888aac50d55160b025266d555dad937d9/compiler/rustc_passes/src/stability.rs#L763-L764
[..] that doesn't seem like it would get much use
I think such a lint would see quite some use once we get final
trait methods. Then every default trait method in the ecosystem that should've been final
would most likely get such an 'overriding deprecated' attribute if it can't be turned into final
right away due to backwards compatibility.
:umbrella: The latest upstream changes (presumably #98758) made this pull request unmergeable. Please resolve the merge conflicts.
:umbrella: The latest upstream changes (presumably #99046) made this pull request unmergeable. Please resolve the merge conflicts.
@bors try @rust-timer queue
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
:hourglass: Trying commit dfb71efd6d8905202b8b38a3df377d77b4114695 with merge 20d938ce1f7e0fd48d6caf9a76524ce095691b69...
:sunny: Try build successful - checks-actions
Build commit: 20d938ce1f7e0fd48d6caf9a76524ce095691b69 (20d938ce1f7e0fd48d6caf9a76524ce095691b69
)
Queued 20d938ce1f7e0fd48d6caf9a76524ce095691b69 with parent 1cd72b734318720adb99dc72147bb8169ef76ffe, future comparison URL.
Finished benchmarking commit (20d938ce1f7e0fd48d6caf9a76524ce095691b69): comparison url.
Instruction count
- Primary benchmarks: π relevant improvements found
- Secondary benchmarks: mixed results
mean[^1] | max | count[^2] | |
---|---|---|---|
Regressions πΏ (primary) |
N/A | N/A | 0 |
Regressions πΏ (secondary) |
2.2% | 3.2% | 7 |
Improvements π (primary) |
-0.7% | -1.5% | 51 |
Improvements π (secondary) |
-5.1% | -10.0% | 27 |
All πΏπ (primary) | -0.7% | -1.5% | 51 |
Max RSS (memory usage)
Results
- Primary benchmarks: π relevant improvements found
- Secondary benchmarks: π relevant improvements found
mean[^1] | max | count[^2] | |
---|---|---|---|
Regressions πΏ (primary) |
N/A | N/A | 0 |
Regressions πΏ (secondary) |
6.3% | 9.8% | 2 |
Improvements π (primary) |
-1.8% | -2.9% | 2 |
Improvements π (secondary) |
-3.1% | -5.1% | 14 |
All πΏπ (primary) | -1.8% | -2.9% | 2 |
Cycles
Results
- Primary benchmarks: π relevant improvements found
- Secondary benchmarks: π relevant improvements found
mean[^1] | max | count[^2] | |
---|---|---|---|
Regressions πΏ (primary) |
N/A | N/A | 0 |
Regressions πΏ (secondary) |
N/A | N/A | 0 |
Improvements π (primary) |
-2.7% | -2.8% | 2 |
Improvements π (secondary) |
-5.4% | -11.9% | 32 |
All πΏπ (primary) | -2.7% | -2.8% | 2 |
[^1]: the arithmetic mean of the percent change [^2]: number of relevant changes
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
The perf improvements are slightly reduced from before. This is because I've been improving the generated output for derives in other PRs, which means the ne
methods being removed are now a little more compact than they were before. Still nice wins, though.
It's actually not much work at all to implement. This case of
#[deprecate]
is already checked and handled separately, here:https://github.com/rust-lang/rust/blob/3dcb616888aac50d55160b025266d555dad937d9/compiler/rustc_passes/src/stability.rs#L763-L764
@m-ou-se Can you elaborate? That file does stability annotating and checking. I don't understand how it's related to deprecations. (I have no familiarity with deprecation handling within the compiler.)
While it seems like there might be value in deprecating the implementation of
ne
, I also don't think that needs to be a hard gate on modifying the derive.β[...]βCould we just start an FCP to stop derivingne
, and consider any deprecations separately?
Quoted from @joshtriplett's comment: https://github.com/rust-lang/rust/pull/98655#issuecomment-1174560163.
I agree and I would be prepared to approve removing the derived ne
, and pursuing a deprecation elsewhere.
This is "option 1" from @nnethercote's summary comment https://github.com/rust-lang/rust/pull/98655#issuecomment-1176827089, which lists the rest of the rationale and options.
@rust-lang/libs-api: @rfcbot fcp merge
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:
- [x] @Amanieu
- [x] @BurntSushi
- [x] @dtolnay
- [x] @joshtriplett
- [ ] @m-ou-se
- [x] @yaahc
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!
See this document for info about what commands tagged team members can give me.
:bell: This is now entering its final comment period, as per the review above. :bell:
- Disallow calling
ne
Even though option 1 was chosen, I'm just commenting here to leave a use case as a data point on why having a ne
method is handy: most formal verification tools for Rust let the programmer attach "contracts" to a method signature, which are used in the verification of Rust programs. Even though the implementations of eq
and ne
are shared, these might have slightly different contracts, for the purpose of guiding the verification to run faster (without it, verification may not always finish due to the complex nature of program verification). This wouldn't be possible without a ne
method :)
@Pointerbender: thanks for the info. If I've understood correctly you are saying there are cases where hand-writing both eq
and ne
is useful? If so, that's presumably an argument in favour of continuing to allow the overriding of ne
?
Can you give a little more detail or perhaps an example of a formal verification contract where ne
would be useful? I'm curious what it would look like.
@Pointerbender: thanks for the info. If I've understood correctly you are saying there are cases where hand-writing both
eq
andne
is useful? If so, that's presumably an argument in favour of continuing to allow the overriding ofne
?
That's correct. Although in all cases I can think of, even the hand-written versions would probably end up being implemented as eq() <==> !ne()
. The difference here lies in the possibility to be able to write contracts for the separate methods.
Can you give a little more detail or perhaps an example of a formal verification contract where
ne
would be useful? I'm curious what it would look like.
Sure thing! Imagine a custom data structure that holds a collection of elements. A contract may look like something like this:
impl<T: PartialEq + Eq> MyDataStructure<T> {
#[pure]
pub fn contains(&self, element: &T) -> bool { ... }
}
impl<T: PartialEq + Eq> PartialEq for MyDataStructure<T> {
#[pure]
#[ensures(result <==> forall(|t: &T| self.contains(t) == other.contains(t)))]
fn eq(&self, other: &Self) -> bool { ... }
#[pure]
#[ensures(result <==> exists(|t: &T| self.contains(t) != other.contains(t)))]
fn ne(&self, other: &Self) -> bool { !self.eq(other) }
}
The above example uses Prusti syntax. The idea behind writing contracts is that these are built up in a modular fashion so to make the task of proving correctness tractable. I.e. a caller that calls ne()
only sees the contract of ne()
and not that of eq()
even though ne()
calls eq()
(if this would recurse into the entire call-tree, then the search space would quickly grow too large - by limiting contracts to the method boundaries, the proof is greatly simplified without sacrificing correctness).
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.
This will be merged soon.
What does this mean? Will it happen automatically? Does someone need to r+ it?
What does this mean? Will it happen automatically? Does someone need to r+ it?
No, it still requires manually r+.
The final comment period, with a disposition to merge, as per the review above, is now complete.
r? @dtolnay
@bors r+
:pushpin: Commit d4a5b034b75c7ea3d9a7955857f56ecc7d7b9cca has been approved by dtolnay
It is now in the queue for this repository.
:hourglass: Testing commit d4a5b034b75c7ea3d9a7955857f56ecc7d7b9cca with merge 361c599feeefaf6e50efd90658fc9c2222154684...
:sunny: Test successful - checks-actions Approved by: dtolnay Pushing 361c599feeefaf6e50efd90658fc9c2222154684 to master...
Finished benchmarking commit (361c599feeefaf6e50efd90658fc9c2222154684): comparison url.
Instruction count
- Primary benchmarks: β relevant improvements found
- Secondary benchmarks: β relevant improvements found
mean[^1] | max | count[^2] | |
---|---|---|---|
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
mean[^1] | max | count[^2] | |
---|---|---|---|
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
mean[^1] | max | count[^2] | |
---|---|---|---|
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 |
[^1]: the arithmetic mean of the percent change [^2]: number of relevant changes
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
@rustbot label: -perf-regression