Overloaded comparison traits #19167

japaric · 2014-11-21T00:44:00Z

Comparison traits have gained an Rhs input parameter that defaults to Self. And now the comparison operators can be overloaded to work between different types. In particular, this PR allows the following operations (and their commutative versions):

&str == String == CowString
&[A] == &mut [B] == Vec<C> == CowVec<D> == [E, ..N] (for N up to 32)
&mut A == &B (for Sized A and B)

Where A, B, C, D, E may be different types that implement PartialEq. For example, these comparisons are now valid: string == "foo", and vec_of_strings == ["Hello", "world"].

[breaking-change]s

Since the == may now work on different types, operations that relied on the old "same type restriction" to drive type inference, will need to be type annotated. These are the most common fallout cases:

some_vec == some_iter.collect(): collect needs to be type annotated: collect::<Vec<_>>()
slice == &[a, b, c]: RHS doesn't get coerced to an slice, use an array instead [a, b, c]
lhs == []: Change expression to lhs.is_empty()
lhs == some_generic_function(): Type annotate the RHS as necessary

cc #19148

r? @aturon

japaric · 2014-11-21T05:03:28Z

I tried to cover all the slice comparisons with the following blanket impl:

impl<T, Sized? Lhs: AsSlice<T>, Sized? Rhs: AsSlice<T>> PartialEq<Rhs> for Lhs { /* ... */ }

But sadly, it conflicts with the PartialEq implementation fort fixed size arrays. The conflict can be resolved by converting the AsSlice's T parameter into an output associated type, but AI is still too buggy to do that at the moment.

That blanket impl also has the downside that it allows some odd comparisons like &[1] == Ok(1) and vec![0] != None, since Option and Result also implement AsSlice.

I guess I'll manually implement PartialEq between the types that make sense.

alexcrichton · 2014-11-21T21:08:06Z

Whoa that is super weird (vec![0] != None), We may want to not go super crazy on the blanket impls just yet if it allows constructs like that.

japaric · 2014-11-21T21:23:10Z

@alexcrichton BTW, the current Equiv trait does allow some weird stuff as well: [].equiv(&None::<u8>) returns true.

So, I actually changed the bound from AsSlice<T> to Deref<[T], and it gives much saner comparisons: like Vec<String> == &[&'static str]. For example, I changed a line of code from:

// A heap allocation just to get a the `==` operator to type check!
if matches.opt_strs("passes").as_slice() == &["list".to_string()] { /* .. */ }

to

if matches.opt_strs("passes") == ["list"] { /* .. */ }

I'll push the updated branch in a bit...

japaric · 2014-11-21T23:21:19Z

I've pushed the updated branch, I've added impls that allow using the == on these different types:

String == &str
Vec == &[_] == &mut [_] == [_, ..N] (and == Box<[_]> in the future)
InternedString == &str

In the case of slices/vectors you can use == on slices that contain different types but implement PartialEq, for example you can compare Vec<String> ==[&'static str, ..2] (and in the future == &[CowString])

There is some fallout that I'm still fixing, these are common issues that I have encounter:

slice == &[a, b], the &[a, b] literal gets treated as &[T, ..N], and since &[T] == &[T, ..N] is not covered by the impls, it fails to compile. This worked before this PR because both sides were forced to have the same type, and the &[a, b] got coerced to &[T]. The solution is to change the expression to slice == [a, b], which work fine for arrays with less than 32 elements.
vec == iter.collect() needs type annotation on the collect method. Since the sides of the comparison may have different types, the compiler can't force the collect output to be Vec<_>.
(And the worst) lhs == mem::transmute(rhs). Again the compiler can't force the output of the transmute to match the type of lhs, so type annotations are required. This pattern is heavily used in the tests of std::os::path to convert between &str and &[u8], and I'm still thinking on how to fix the fallout in the less painful way possible.

These PartialEq impls make a lot of as_slice method calls unnecessary, but I haven't attempted to remove them yet.

Thoughts up to this point? Do we want all these PartialEq impls? Is the fallout in type inference acceptable?

One more thing, impl PartialEq<Rhs> for Lhs needs the default_type_params feature gate, whereas impl PartialEq for Foo and #[deriving(PartialEq)] do not.

alexcrichton · 2014-11-22T17:41:16Z

src/libcollections/string.rs

+    fn ne(&self, other: &Rhs) -> bool { PartialEq::ne(&**self, &**other) }
+}
+
+impl<'a> PartialEq<String> for &'a str {


In theory, shouldn't this also be PartialEq<Rhs> where Rhs: Deref<str>? I'm real bad about thinking about coherence violations though!

I tried that, it conflicts with this other impl:

impl<Sized? A, Sized? B> PartialEq<&B> for &A { /* .. */ }

because &str implements Deref<str>. In other words, when Lhs = Rhs = &str and A = B = str then &str gets two implementations.

Is there a reason this is for &'a str rather than str?

In the long run, with negative where clauses, I think we'll be able to do better with asymmetric deref impls.

For now, I think it might be best to do just impl PartialEq<str> for String and impl PartialEq<String> for str to keep equality symmetric.

Is there a reason this is for &'a str rather than str?

Yes, with this you can do string == "Hello". LHS has type String, RHS has type &str. If I change it to the the unsized version, that comparison won't work, instead you'll have to write string == *"Hello"

Make sense; presumably &string == "Hello" would work as well. The basic problem is that the blanket impl for adding a layer of & has to add it to both sides, or else it runs afoul of coherence.

Anyway, I think this is fine for now, and the main thing is for the equality impls to be symmetric.

alexcrichton · 2014-11-22T17:45:57Z

@japaric this seems pretty awesome, I'm quite excited about this! Some thoughts about the points you raised:

vec == iter.collect()

This seems ok, I'm not sure it's all that common of a pattern. In general this is a worry of making operations more generic than they used to be which is to require more type annotations because inference has less to go on.

slice == &[a, b]

This is a little worrying, as I would definitely be expecting this to work. cc @aturon, this is one of the unfortunate spots where coercions seem like they should work but end up not working out.

lhs == mem::transmute(rhs)

This should not be a common pattern at all. A function like this should definitely be in a helper function to make the transmute a little safer. Which is to say, I'm not super duper worried about this.

japaric · 2014-11-23T21:56:05Z

Re slice == &[a, b]

Perhaps a little late, but it just occurred to me that we could impl PartialEq<&[B, ..N]> for &[A] (in a more general way) to avoid breaking this pattern.

Re: the lhs == mem::transmute(rhs) usage in Path tests.

Turns out the transmutes were not necessary so I removed all of them. BTW, I'm referring to these tests.

Another pattern that broke is: some_vec == vec![], this doesn't compile because is not clear what's the type of the RHS: it could be Vec<&'static str> or Vec<String> (both are valid comparisons). I ended changing those cases to some_vec.is_empty()/slice.is_empty()

japaric · 2014-11-23T22:07:16Z

I have updated the branch, it now fully passes make check. So, you can see how big is the coercion/inference fallout.

aturon · 2014-11-23T22:13:46Z

Thanks @japaric -- I plan to review this today or tomorrow.

aturon · 2014-11-24T20:26:41Z

src/libcollections/vec.rs

+    fn ne(&self, other: &Rhs) -> bool { PartialEq::ne(&**self, &**other) }
+}
+
+impl<'a, A, B> PartialEq<Vec<B>> for &'a [A] where A: PartialEq<B> {


Same issues here as with strings: it's not clear why there's an impl on &[A] rather than [A], and I think it's best if the impls are symmetric (even if that means they're less flexible for now).

See my previous comment. (The one about String/&str)

aturon · 2014-11-24T20:59:01Z

This looks good to me, though I left some comments about a few concerns.

I'm not particularly worried about the extra type annotations or shifting with arrays, since almost all cases of these were in tests. Also, @alexcrichton, there's been a bit of churn about exactly what coercions may apply to arrays vs slices; I think what this PR is doing there is pretty reasonable.

I especially liked the point about iter::order, which demonstrates a trait design tradeoff I hadn't fully considered before, and gives a stronger reason to make iterators use associated types (granted we were planning to anyway). Kudos for figuring this out!

Anyway, let me know what you think about the comments I left, and nice work as usual :)

japaric · 2014-11-27T07:50:19Z

@aturon Added commit deprecating Equiv, rebased, added [breaking-change] message, and passed make check. r?

aturon · 2014-12-01T04:17:30Z

Hey, so sorry for the slow response -- I've been offline for the US Thanksgiving holiday.

This largely looks good, but I continue to be a bit worried about asymmetric implementations of ParialEq -- did you have any thoughts about that issue? I worry that it will lead to surprising situations where a == b compiles and b == a does not.

japaric · 2014-12-01T05:35:18Z

This largely looks good, but I continue to be a bit worried about asymmetric implementations of ParialEq -- did you have any thoughts about that issue? I worry that it will lead to surprising situations where a == b compiles and b == a does not.

At some point I tried a "commutative" blanket impl, but it generated "trait overflow" compiler errors during trait/method resolution.

This is what I tried (see it in the playpen)

impl<Lhs, Rhs> PartialEq<Rhs> for Lhs where Rhs: PartialEq<Lhs> {
    fn eq(&self, rhs: &Rhs) -> bool {
        rhs.eq(self)
    }
}

struct Foo;
struct Bar;

fn overflow() {
    Foo == Bar;
}

And I got:

eq.rs:22:5: 22:15 error: overflow evaluating the trait `Sized` for the type `_`
eq.rs:22     Foo == Bar;
             ^~~~~~~~~~
eq.rs:22:5: 22:15 error: overflow evaluating the trait `Sized` for the type `Foo`
eq.rs:22     Foo == Bar;
             ^~~~~~~~~~
eq.rs:22:5: 22:15 error: overflow evaluating the trait `PartialEq<_>` for the type `Foo`
eq.rs:22     Foo == Bar;
             ^~~~~~~~~~
error: aborting due to 3 previous errors

I think that may work when/if we get either negative equality bounds: where Rhs: PartialEq<Lhs>, Rhs != Lhs, or with negative bounds: where Rhs: PartialEq<Lhs> + !PartialEq.

Until that, we'll (and the same applies to library authors) have to take care of manually implementing both impl PartialEq<B> for A and it's commutative: impl Partial<A> for B. AFAIK, this PR has symmetric implementations for strings and slices, but I can add a "commutative-equality" run pass test as a sanity check.

aturon · 2014-12-02T21:56:45Z

@japaric Right, we can't provide a blanket commutative impl, given the way that dispatch works. But my specific worry was about cases to do with e.g. string slices like the following:

impl<Rhs> PartialEq<Rhs> for String where Rhs: Deref<str> { ... }
impl<'a> PartialEq<String> for &'a str { ... }

These are not obviously symmetric to me, but I'm probably missing something.

japaric · 2014-12-02T22:37:37Z

These are not obviously symmetric to me, but I'm probably missing something.

Yeah, we can't write it in a symmetric way because we get conflicting implementations:

impl<Rhs> PartialEq<Rhs> for String where Rhs: Deref<str> { ... }
impl<Lhs> PartialEq<String> for Lhs where Lhs: Deref<str> { ... }

Collides when Rhs = String and when Lhs = String :-(

The trick is that String and &str implement Deref<str>, so

impl<Rhs> PartialEq<Rhs> for String where Rhs: Deref<str> { ... }

takes care of String == &str and String = String. And

impl<'a> PartialEq<String> for &'a str { ... }

takes care of &str == String. And libcore's

impl<'a, 'b, Sized? A, Sized? B> PartialEq<&'b B> for &'a A { ... }

takes care of &str == &str

(That reminds me, that I should add 2 more impls to cover the 9 comparisons of CowString, &str and String)

Alternatively, we could manually write all the combinations without using the Deref blanket impl. At the cost of writing more impls. Not sure if that's less confusing, because the impls will still be scattered over many crates.

aturon · 2014-12-02T23:02:01Z

@japaric Right, so I understood that having Deref would give you String and &str -- my worry is, does it give you more, that you don't give on the other side? I.e., what happens if additional types implement Deref<str>? Wouldn't that introduce asymmetry?

japaric · 2014-12-02T23:21:48Z

what happens if additional types implement Deref? Wouldn't that introduce asymmetry?

Yes, it does. If you do impl Deref<str> for MyString, now you can do [Cow]String == MyString even though MyString doesn't implement PartialEq.

It's a tradeoff:

With the blanket Deref impl, you only need 2 more impl and then you can do the 16 combinations of comparisons between &str, String, CowString and MyString:
- impl PartialEq<Rhs> for MyString where Rhs: Deref<str>
- impl PartialEq<MyString> for &str
Without the blanket Deref impl, you'll have to add 7 more impls to accomplish the same thing, but you don't get the surprising [Cow]String == MyString when you add impl Deref<str> for MyString.

aturon · 2014-12-02T23:32:42Z

@japaric OK, I think we're on the same page now.

As far as the tradeoff goes, my thoughts are:

I believe we should have a strong convention that any impl of PartialEq should be accompanied by a symmetric impl.
In general, if there's a tradeoff between APIs and implementation work, I will always favor doing more implementation work to support better APIs.

I just talked to @alexcrichton and he agrees with the above.

Can you change this PR to guarantee symmetry in all cases? For dealing with the combinatorial explosion, you could consider using macros.

japaric · 2014-12-03T05:25:25Z

Rebased and passes make check. I've replaced the Deref blanket impl with manual impls. I have also implemented overloaded comparison with Cow types.

r? @aturon

aturon · 2014-12-03T15:22:46Z

@japaric Looks great to me! r=me after a squash.

- String == &str == CowString - Vec == &[T] == &mut [T] == [T, ..N] == CowVec - InternedString == &str

japaric · 2014-12-03T15:43:18Z

@aturon squashed!

@aturon

Comparison traits have gained an `Rhs` input parameter that defaults to `Self`. And now the comparison operators can be overloaded to work between different types. In particular, this PR allows the following operations (and their commutative versions): - `&str` == `String` == `CowString` - `&[A]` == `&mut [B]` == `Vec<C>` == `CowVec<D>` == `[E, ..N]` (for `N` up to 32) - `&mut A` == `&B` (for `Sized` `A` and `B`) Where `A`, `B`, `C`, `D`, `E` may be different types that implement `PartialEq`. For example, these comparisons are now valid: `string == "foo"`, and `vec_of_strings == ["Hello", "world"]`. [breaking-change]s Since the `==` may now work on different types, operations that relied on the old "same type restriction" to drive type inference, will need to be type annotated. These are the most common fallout cases: - `some_vec == some_iter.collect()`: `collect` needs to be type annotated: `collect::<Vec<_>>()` - `slice == &[a, b, c]`: RHS doesn't get coerced to an slice, use an array instead `[a, b, c]` - `lhs == []`: Change expression to `lhs.is_empty()` - `lhs == some_generic_function()`: Type annotate the RHS as necessary cc #19148 r? @aturon

@alexcrichton

Now that we have an overloaded comparison (`==`) operator, and that `Vec`/`String` deref to `[T]`/`str` on method calls, many `as_slice()`/`as_mut_slice()`/`to_string()` calls have become redundant. This patch removes them. These were the most common patterns: - `assert_eq(test_output.as_slice(), "ground truth")` -> `assert_eq(test_output, "ground truth")` - `assert_eq(test_output, "ground truth".to_string())` -> `assert_eq(test_output, "ground truth")` - `vec.as_mut_slice().sort()` -> `vec.sort()` - `vec.as_slice().slice(from, to)` -> `vec.slice(from_to)` --- Note that e.g. `a_string.push_str(b_string.as_slice())` has been left untouched in this PR, since we first need to settle down whether we want to favor the `&*b_string` or the `b_string[]` notation. This is rebased on top of #19167 cc @alexcrichton @aturon

@alexcrichton

Now that we have an overloaded comparison (`==`) operator, and that `Vec`/`String` deref to `[T]`/`str` on method calls, many `as_slice()`/`as_mut_slice()`/`to_string()` calls have become redundant. This patch removes them. These were the most common patterns: - `assert_eq(test_output.as_slice(), "ground truth")` -> `assert_eq(test_output, "ground truth")` - `assert_eq(test_output, "ground truth".to_string())` -> `assert_eq(test_output, "ground truth")` - `vec.as_mut_slice().sort()` -> `vec.sort()` - `vec.as_slice().slice(from, to)` -> `vec.slice(from_to)` --- Note that e.g. `a_string.push_str(b_string.as_slice())` has been left untouched in this PR, since we first need to settle down whether we want to favor the `&*b_string` or the `b_string[]` notation. This is rebased on top of #19167 cc @alexcrichton @aturon

@alexcrichton

Now that we have an overloaded comparison (`==`) operator, and that `Vec`/`String` deref to `[T]`/`str` on method calls, many `as_slice()`/`as_mut_slice()`/`to_string()` calls have become redundant. This patch removes them. These were the most common patterns: - `assert_eq(test_output.as_slice(), "ground truth")` -> `assert_eq(test_output, "ground truth")` - `assert_eq(test_output, "ground truth".to_string())` -> `assert_eq(test_output, "ground truth")` - `vec.as_mut_slice().sort()` -> `vec.sort()` - `vec.as_slice().slice(from, to)` -> `vec.slice(from_to)` --- Note that e.g. `a_string.push_str(b_string.as_slice())` has been left untouched in this PR, since we first need to settle down whether we want to favor the `&*b_string` or the `b_string[]` notation. This is rebased on top of #19167 cc @alexcrichton @aturon

fix: Fix detection of ref patterns for path patterns

alexcrichton reviewed Nov 22, 2014
View reviewed changes

aturon reviewed Nov 24, 2014
View reviewed changes

japaric force-pushed the rhs-cmp branch from 0360e7d to 141f53a Compare November 27, 2014 07:48

japaric changed the title ~~[WIP] Reform comparison traits~~ Overloaded comparison traits Nov 27, 2014

japaric mentioned this pull request Nov 28, 2014

Remove unnecessary as[_mut]_slice/to_string() calls #19378

Closed

japaric pushed a commit to japaric/rust that referenced this pull request Dec 1, 2014

[Don't review] PR rust-lang#19167

c8fcb5d

japaric mentioned this pull request Dec 1, 2014

[WIP] Don't rely on the type of lhs when type checking lhs OP rhs #19434

Closed

Jorge Aparicio added 2 commits December 2, 2014 18:52

libcore: add Rhs input parameter to comparison traits

b258de7

Test PartialEq multidispatch

2578de9

japaric force-pushed the rhs-cmp branch from 141f53a to 6dad28a Compare December 3, 2014 05:21

Jorge Aparicio added 5 commits December 3, 2014 10:41

Overload the == operator

2840d58

- String == &str == CowString - Vec == &[T] == &mut [T] == [T, ..N] == CowVec - InternedString == &str

Replace equiv method calls with == operator sugar

b32b24d

Remove unused transmutes from tests

eac635d

Fix fallout

09707d7

Deprecate Equiv

5cfac94

japaric force-pushed the rhs-cmp branch from 6dad28a to 5cfac94 Compare December 3, 2014 15:43

bors closed this Dec 4, 2014

bors merged commit 5cfac94 into rust-lang:master Dec 4, 2014

kyledewey mentioned this pull request Dec 4, 2014

Removed now-deprecated Equiv implementation hyperium/hyper#176

Merged

pfalabella mentioned this pull request Dec 5, 2014

fix build and de-slice rust-rosetta/rust-rosetta#315

Merged

japaric deleted the rhs-cmp branch December 16, 2014 02:10

lnicola pushed a commit to lnicola/rust that referenced this pull request Feb 17, 2025

Merge pull request rust-lang#19167 from ChayimFriedman2/fix-ref-pat

2a0d16f

fix: Fix detection of ref patterns for path patterns

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overloaded comparison traits #19167

Overloaded comparison traits #19167

japaric commented Nov 21, 2014

japaric commented Nov 21, 2014

alexcrichton commented Nov 21, 2014

japaric commented Nov 21, 2014

japaric commented Nov 21, 2014

alexcrichton Nov 22, 2014

japaric Nov 22, 2014

aturon Nov 24, 2014

japaric Nov 24, 2014

aturon Nov 24, 2014

alexcrichton commented Nov 22, 2014

japaric commented Nov 23, 2014

japaric commented Nov 23, 2014

aturon commented Nov 23, 2014

aturon Nov 24, 2014

japaric Nov 24, 2014

aturon commented Nov 24, 2014

japaric commented Nov 27, 2014

aturon commented Dec 1, 2014

japaric commented Dec 1, 2014

aturon commented Dec 2, 2014

japaric commented Dec 2, 2014

aturon commented Dec 2, 2014

japaric commented Dec 2, 2014

aturon commented Dec 2, 2014

japaric commented Dec 3, 2014

aturon commented Dec 3, 2014

japaric commented Dec 3, 2014

Overloaded comparison traits #19167

Overloaded comparison traits #19167

Conversation

japaric commented Nov 21, 2014

japaric commented Nov 21, 2014

alexcrichton commented Nov 21, 2014

japaric commented Nov 21, 2014

japaric commented Nov 21, 2014

alexcrichton Nov 22, 2014

Choose a reason for hiding this comment

japaric Nov 22, 2014

Choose a reason for hiding this comment

aturon Nov 24, 2014

Choose a reason for hiding this comment

japaric Nov 24, 2014

Choose a reason for hiding this comment

aturon Nov 24, 2014

Choose a reason for hiding this comment

alexcrichton commented Nov 22, 2014

japaric commented Nov 23, 2014

japaric commented Nov 23, 2014

aturon commented Nov 23, 2014

aturon Nov 24, 2014

Choose a reason for hiding this comment

japaric Nov 24, 2014

Choose a reason for hiding this comment

aturon commented Nov 24, 2014

japaric commented Nov 27, 2014

aturon commented Dec 1, 2014

japaric commented Dec 1, 2014

aturon commented Dec 2, 2014

japaric commented Dec 2, 2014

aturon commented Dec 2, 2014

japaric commented Dec 2, 2014

aturon commented Dec 2, 2014

japaric commented Dec 3, 2014

aturon commented Dec 3, 2014

japaric commented Dec 3, 2014