std: Remove #[old_orphan_check] from PartialEq

[alexcrichton]

This is a deprecated attribute that is slated for removal, and it also affects
all implementors of the trait. This commit removes the attribute and fixes up
implementors accordingly. The primary implementation which was lost was the
ability to compare &[T] and Vec<T> (in that order).

This change also modifies the assert_eq! macro to not consider both directions
of equality, only the one given in the left/right forms to the macro. This
modification is motivated due to the fact that &[T] == Vec<T> no longer
compiles, causing hundreds of errors in unit tests in the standard library (and
likely throughout the community as well).

Closes #19470
[breaking-change]

[alexcrichton]

r? @aturon

[rust-highfive]

r? @gankro

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

cc @nikomatsakis, @japaric

[japaric]

One possible way to retain the &[T] == Vec<T> is to add this impl to libcore (untested):

impl<T, U> PartialEq<T, U> for &[T] where U: RhsSlice { .. }

And then impl RhsSlice for Vec/Cow/etc. IIRC, I submitted something similar (perhaps with Deref<[T]>?) when I was working in the first PR for overloaded equality -- at that time it was not accepted because it could let to downstream implementations that would break "symmetry" (a.k.a. if A == B works, then B == A should also work). It may be worth reconsidering since this PR would break symmetry anyway.

[alexcrichton]

I think for now we wouldn't necessarily want to commit to a trait like that, but it should be a backwards compatible extension to allow comparison of slices to more types, right?

Another option would be to leverage As<[T]> if generic conversion traits land so we wouldn't need a one-off trait for this.

[japaric]

Another idea (cc @nikomatsakis): change the a == b/a != b type check code to:

• If A: PartialEq<B> use a.eq(&b)
• else if B: PartialEq<A> use b.eq(&a)
• else raise "doesn't implement PartialEq" error

Advantages

• As soon you impl PartialEq<Foo> for Bar, you get a "symmetric" operator, i.e. both foo == bar and bar == foo work.
• Sidesteps the orphan_check problem, because e.g. you don't need impl PartialEq<Vec<A>> for &[B] (issue: &[B] is not local/covered) you can impl PartialEq<&[B]> for Vec<A> instead.
• Needs only half of the implementations (halves the metadata bloat)
• Can be extended to PartialOrd, e.g. use a.lt(&b) or b.gt(&a)

Disadvantages:

• Generic code/direct method calls remain asymmetric, i.e. given fn my_eq<A: PartialEq<B>>(&A, &B) then my_eq(foo, bar) works but my_eq(bar, foo) doesn't. This only applies if one implements a single direction, implementing both directions is still possible with this proposal (minus orphan_check restrictions).
• Evaluation order may be the reverse of what you wrote. (possible footgun? do/should people rely on the evaluation order of the LHS/RHS?)
• Compiler magic.

I think this "magic" can be removed in a backcompat fashion once we figure out how to encode symmetry in a blanket implementation (currently impl<A, B> PartialEq<A> for B where A: PartialEq<B> { .. } overlaps with every other implementation). On a second thought, adding such blanket impl would probably be a backward incompatible change, so we may not add it post 1.0.

[alexcrichton]

Oh wow, that is an interesting idea! Some thoughts:

• I do think that evaluation order is fairly important to keep as left-to-right as possible personally. I don't think I make much code that relies on it, but it's bound to be quite a surprise for someone at some point!
• I do like the symmetric guarantees that come out of this design, but the lack of symmetric in all locations is unfortunate.

Curious what others think though!

[japaric]

I do think that evaluation order is fairly important to keep as left-to-right as possible personally. I don't think I make much code that relies on it, but it's bound to be quite a surprise for someone at some point!

I though about this and I think is possible to maintain the evaluation order in the "reverse" case with the help of temporaries.

Updated algorithm:

• If A: PartialEq<B> use <a>.eq(&<b>)
• else if B: PartialEq<A> use { let rhs = <a>; let lhs = <b>; lhs.eq(&rhs) }
• else raise "doesn't implement PartialEq" error

I do like the symmetric guarantees that come out of this design, but the lack of symmetric in all locations is unfortunate.

Yes, it's unfortunate. I think the right way to ensure symmetry is to use the type system: (a) use a blanket implementation like I mentioned above (issue: overlapping impls), or (b) make the compiler automatically derive the reverse case even if it has to bypass the orphan_check (can this lead to a violation of coherence?).

[alexcrichton]

Updated algorithm:

Hm yeah, I'd be fine with that.

b) make the compiler automatically derive the reverse case even if it has to bypass the orphan_check

I would personally see this as pushing the envelope of a "bit too much magic" for a case that may not be worth it.

[bors]

☔ The latest upstream changes (presumably #23104) made this pull request unmergeable. Please resolve the merge conflicts.

[aturon]

I think we may want to consider some compiler help for symmetry, as @japaric is proposing, at some point in the future but there's just not scope for it right now.

For the time being, I think we should move forward on this PR so that we can remove the feature gate, and then separately consider simpler workarounds like the blanket impl @japaric initially suggested.

[aturon]

So, the thing that makes me most uncomfortable here is the removal of two-sides checks on assert_eq with the assumption that people will reorder vec/slice comparisons. Seems pretty hacky.

That said, I don't think there's a strong reason for assert_eq to be two-sided, since it's not supposed to be testing the equality operator -- those should be in the unit tests for its own definition. That is, I don't mind the change, but I'm a bit concerned about the arbitrary-seeming fallout here.

All that said... I'm not really sure what else to do. Once generic conversion traits land, we could perhaps use something like the RhsSlice trick but with As<[T]> instead.

Thoughts?

[alexcrichton]

I agree that this makes me feel a little uneasy, but I also agree that this impl would be fine to land as well:

impl<T, R: As<[T]>> PartialEq<R> for [T] {
    fn eq(&self, other: &R) -> bool { ... }
}

[aturon]

OK, let's stage this after the conversion trait implementation, which is in progress.

(Might be good in the meantime to quickly try this with AsSlice to make sure there's not another coherence problem lurking.)

[nikomatsakis]

FWIW, I think I agree with @aturon's assessment of "maybe we can improve this in the future but we should move forward for now". It is mildly annoying that you have to put the vec on the left, though I wonder if this will just be a thing that people get used to. (The orphan rules strongly suggest that you should favor the LHS for the more "specific" type...)

[alexcrichton]

For posterity, it looks like As<[T]> won't actually work out unfortunately:

/home/alex/code/rust4/src/libcore/slice.rs:1578:9: 1578:10 error: the type parameter `B` is not constrained by the impl trait, self type, or predicates [E0207]
/home/alex/code/rust4/src/libcore/slice.rs:1578 impl<A, B, R: As<[B]>> PartialEq<R> for [A] where A: PartialEq<B> {
                                                        ^
error: aborting due to previous error

[bors]

☔ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #23796) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

ping @aturon, this likely needs a decision pretty soon.

[aturon]

@bors: r+ 608fff8

[bors]

⌛ Testing commit 608fff8 with merge 1a48efc...

[bors]

💔 Test failed - auto-linux-64-x-android-t

[alexcrichton]

@bors: retry rollup