rust icon indicating copy to clipboard operation
rust copied to clipboard

Don't derive `PartialEq::ne`.

Open nnethercote opened this issue 2 years ago β€’ 52 comments

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.

nnethercote avatar Jun 29 '22 04:06 nnethercote

r? @nagisa

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

rust-highfive avatar Jun 29 '22 04:06 rust-highfive

r? @ghost

nnethercote avatar Jun 29 '22 04:06 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 avatar Jun 29 '22 04:06 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 avatar Jun 29 '22 04:06 nnethercote

@bors try @rust-timer queue

nnethercote avatar Jun 29 '22 04:06 nnethercote

Awaiting bors try build completion.

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

rust-timer avatar Jun 29 '22 04:06 rust-timer

:hourglass: Trying commit b83ac4453642e90ef0f22cad70ef24d463e3cd45 with merge aecb2de1ee213c6e997fb2bed7f825407b6a7f1c...

bors avatar Jun 29 '22 04:06 bors

@bors try @rust-timer queue

nnethercote avatar Jun 29 '22 05:06 nnethercote

Awaiting bors try build completion.

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

rust-timer avatar Jun 29 '22 05:06 rust-timer

:hourglass: Trying commit 2b04e85e10de560bb30d52774b4c706497d5e804 with merge 60726948eec003f9f0b3eabddd2ddb8fc98dcfdc...

bors avatar Jun 29 '22 05:06 bors

Awaiting bors try build completion.

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

rust-timer avatar Jun 29 '22 05:06 rust-timer

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

bors avatar Jun 29 '22 06:06 bors

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

rust-timer avatar Jun 29 '22 06:06 rust-timer

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.

Mark-Simulacrum avatar Jun 29 '22 12:06 Mark-Simulacrum

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.

CryZe avatar Jun 29 '22 12:06 CryZe

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

rust-timer avatar Jun 29 '22 12:06 rust-timer

Sign off on diagnostics changes.

estebank avatar Jun 29 '22 15:06 estebank

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.

m-ou-se avatar Jun 29 '22 20:06 m-ou-se

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?

nnethercote avatar Jun 29 '22 22:06 nnethercote

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].

m-ou-se avatar Jun 29 '22 23:06 m-ou-se

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. :)

m-ou-se avatar Jun 29 '22 23:06 m-ou-se

There are only about ~800 calls to .ne() on crates.io. (Compared to about 14'000 for .eq().)

m-ou-se avatar Jun 29 '22 23:06 m-ou-se

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?

nnethercote avatar Jun 30 '22 08:06 nnethercote

As for overriding... the docs are clear that eq is a required method and ne 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

Urgau avatar Jun 30 '22 08:06 Urgau

r? @m-ou-se

nnethercote avatar Jul 01 '22 05:07 nnethercote

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:

@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.

nnethercote avatar Jul 04 '22 21:07 nnethercote

Also, the generated docs look like this: Equal

Both methods are labelled as provided, but there's no indication that you must implement one of them.

nnethercote avatar Jul 04 '22 22:07 nnethercote

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?

joshtriplett avatar Jul 05 '22 03:07 joshtriplett

@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:

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 of eq, 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.

Urgau avatar Jul 05 '22 07:07 Urgau

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.

nnethercote avatar Jul 05 '22 07:07 nnethercote

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

m-ou-se avatar Jul 06 '22 20:07 m-ou-se

Let me try to summarize the situation. There are three options, which are probably cumulative, and require increasing amounts of effort.

  1. Stop deriving ne
  • Stop deriving ne methods with derive(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.
  1. 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).
  1. Disallow calling ne
  • Warn/error on calling ne.
  • Requires a language change, such that x != y desugars to !x.eq(&y) instead of x.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.

nnethercote avatar Jul 06 '22 22:07 nnethercote

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

rustbot avatar Jul 06 '22 22:07 rustbot

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.

nnethercote avatar Jul 06 '22 22:07 nnethercote

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.

joshtriplett avatar Jul 06 '22 22:07 joshtriplett

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.

m-ou-se avatar Jul 07 '22 14:07 m-ou-se

:umbrella: The latest upstream changes (presumably #98758) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Jul 08 '22 16:07 bors

:umbrella: The latest upstream changes (presumably #99046) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Jul 15 '22 17:07 bors

@bors try @rust-timer queue

nnethercote avatar Jul 17 '22 12:07 nnethercote

Awaiting bors try build completion.

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

rust-timer avatar Jul 17 '22 12:07 rust-timer

:hourglass: Trying commit dfb71efd6d8905202b8b38a3df377d77b4114695 with merge 20d938ce1f7e0fd48d6caf9a76524ce095691b69...

bors avatar Jul 17 '22 12:07 bors

:sunny: Try build successful - checks-actions Build commit: 20d938ce1f7e0fd48d6caf9a76524ce095691b69 (20d938ce1f7e0fd48d6caf9a76524ce095691b69)

bors avatar Jul 17 '22 14:07 bors

Queued 20d938ce1f7e0fd48d6caf9a76524ce095691b69 with parent 1cd72b734318720adb99dc72147bb8169ef76ffe, future comparison URL.

rust-timer avatar Jul 17 '22 14:07 rust-timer

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

rust-timer avatar Jul 17 '22 16:07 rust-timer

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.

nnethercote avatar Jul 18 '22 03:07 nnethercote

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.)

nnethercote avatar Jul 26 '22 04:07 nnethercote

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 deriving ne, 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

dtolnay avatar Jul 27 '22 17:07 dtolnay

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.

rfcbot avatar Jul 27 '22 17:07 rfcbot

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot avatar Jul 31 '22 01:07 rfcbot

  1. 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 avatar Aug 05 '22 08:08 Pointerbender

@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.

nnethercote avatar Aug 05 '22 14:08 nnethercote

@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?

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).

Pointerbender avatar Aug 05 '22 15:08 Pointerbender

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.

rfcbot avatar Aug 10 '22 01:08 rfcbot

This will be merged soon.

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

nnethercote avatar Aug 16 '22 07:08 nnethercote

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

No, it still requires manually r+.

slanterns avatar Aug 16 '22 08:08 slanterns

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

r? @dtolnay

nnethercote avatar Aug 16 '22 21:08 nnethercote

@bors r+

dtolnay avatar Aug 16 '22 21:08 dtolnay

:pushpin: Commit d4a5b034b75c7ea3d9a7955857f56ecc7d7b9cca has been approved by dtolnay

It is now in the queue for this repository.

bors avatar Aug 16 '22 21:08 bors

:hourglass: Testing commit d4a5b034b75c7ea3d9a7955857f56ecc7d7b9cca with merge 361c599feeefaf6e50efd90658fc9c2222154684...

bors avatar Aug 18 '22 10:08 bors

:sunny: Test successful - checks-actions Approved by: dtolnay Pushing 361c599feeefaf6e50efd90658fc9c2222154684 to master...

bors avatar Aug 18 '22 12:08 bors

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

rust-timer avatar Aug 18 '22 14:08 rust-timer