DSTify PartialEq, PartialOrd, Eq, Ord

[japaric]

eq, ne, cmp, etc methods now require one less level of indirection when dealing with &str/&[T]

"foo".ne(&"bar") -> "foo".ne("bar")
slice.cmp(&another_slice) -> slice.cmp(another_slice)
// slice and another_slice have type `&[T]`

[breaking-change]

[huonw]

Deriving can be fixed when UFCS allows for Eq::eq(foo, bar) (or something equivalent), which is required for things like #15689 too.

[alexcrichton]

@japaric stage0 looks good to me, thanks!

If you're feeling up to the task, you may be able to kill two birds with one stone by tweaking how deriving works. Right now #[deriving] also doesn't work on structures with a &'a T inside of it, but we should be able to fix that!

We could in theory expand to:

impl ::std::cmp::PartialEq for Foo {
    #[inline]
    fn eq(&self, __arg_0: &Foo) -> ::bool {
        fn eq<Sized? T: PartialEq>(a: &T, b: &T) { a.eq(b) } // add this function
        match *__arg_0 {
            Foo(ref __self_1_0) =>
            match *self {
                Foo(ref __self_0_0) =>
                true && eq(__self_0_0, __self_1_0), // modify function call here
            },
        }
    }
    // ne method omitted for brevity
}

That should in theory solve the problem in this PR, as well as the problem for structures with pointers!

[alexcrichton]

Ah and it seems @huonw was thinking what I was as well :) (and it doesn't even introduce a mini function)

[japaric]

Hmm, so I tried the script on #15689, and it actually works with this PR. The expansion looks like this:

impl <'a> ::std::cmp::PartialEq for Test<'a> {
    #[inline]
    fn eq(&self, __arg_0: &Test<'a>) -> ::bool {
        match (&*self, &*__arg_0) {
            (&Slice(ref __self_0), &Slice(ref __arg_1_0)) =>
            true && (*__self_0) == (*__arg_1_0),
        }
    }
    // ne omitted
}

[aturon]

@nikomatsakis is currently doing some major work on operator overloading. Can we hold off on DST-ifying operators until that's done? Sorry for the delay @japaric.

[aturon]

(I should say: his branch is bootstrapping, and I expect it to land soon.)

[japaric]

@nikomatsakis I tried this branch rebased on top of your operator dispatch PR with and without your [T, ..N] -> [T] coercion patch, and I got these results.

Re: [0u8, 1] == [0u8, 1];

It seems that in your patch only coerces the LHS to [T], but leaves the RHS as [T, ..2] which causes the error shown in the gist. I think we should coerce [T, ..N] -> [T] on both sides of A op B.

[nikomatsakis]

@japaric yes over the weekend I realize that the patch was of course not enough to make the RHS coerce. I'm revising my opinion on whether I think this coercion is worthwhile or not.

[japaric]

@nikomatsakis Alright, let's go ahead with the original plan then. I'll leave this ready to be merged after your operator dispatch PR. And, we can do the coercion part (if we decide to do it) in another PR.

[nikomatsakis]

@japaric Sounds good. I'm not sure what the original plan is, actually -- rewrite in terms of Foo::foo()?

Regardless, it does seem best to add the [T, ..n] => [T] coercion in its own PR. It's probably not too hard to do, even if it's more than 6 lines; but it somehow feels a bit ad-hoc. I guess I wouldn't want to rule it out but I want to wait and see what shakes out with the operators. It makes me a bit sad that it would mean that we can't detect statically a fault like comparing a [T, ..3] and a [T, ..4] (which I would expect to yield a type error).

[japaric]

Sounds good. I'm not sure what the original plan is, actually -- rewrite in terms of Foo::foo()?

No, this just needs a rebase + checking that all tests pass. Expanding to the operator sugar works fine, it also partially fixes (the PartialEq part) #15689. I think the Clone part can be fixed using UFCS, but that can be done in another PR.

It makes me a bit sad that it would mean that we can't detect statically a fault like comparing a [T, ..3] and a [T, ..4](which I would expect to yield a type error).

Ahh, I didn't realize that would happen. I agree, that operation should be a type error. Perhaps, we shouldn't do the coercion, since it's just a workaround until we allow integers as generic parameters, and the impls for up to [T, ..32] may be enough for the time being.

[japaric]

😢 Some run-pass tests are running into SIGILL (I have compiled the details here). I have no idea what could be happening. I'll ask on IRC later.

[alexcrichton]

@japaric have you tried getting a backtrace in gdb when you hit the SIGILL? I've found in the past it's due to an infinite recursion

[japaric]

@alexcrichton Yeah, the problem was infinite recursion. I couldn't pinpoint the source, but it was one of the method definitions that used the method notation in its definition, like this one:

impl<'a, T: Ord> Ord for &'a T {
    #[inline]
    fn cmp(&self, other: & &'a T) -> Ordering { (**self).cmp(*other) }
}

I have changed all these to use UCFS syntax (which looks more readable IMO):

impl<'a, T: Ord> Ord for &'a T {
    #[inline]
    fn cmp(&self, other: & &'a T) -> Ordering { Ord::cmp(*self, *other) }
}

And now the infinite recursion problem is gone.

---

OK. This is ready for review, all tests pass locally. This PR is rebased on top of #18486, so you can skip @nikomatsakis commits. (This also needs #18523, or it'll ICE during make, but I expect that PR to be merged soon)

re-r? @aturon

[alexcrichton]

ping @japaric, needs a rebase?

[aturon]

@japaric @alexcrichton I've reviewed the library changes, and they all look good to me.

@alexcrichton Can you review the changes to deriving and make sure you're happy with them? Feel free to r+ if/when so.

[alexcrichton]

Seems good to me!

[japaric]

@alexcrichton Rebased. (I can squash afterwards)

[alexcrichton]

we're gonna have to make a snapshot real soon after this!

[japaric]

Had to rebase after #18486 landed. (I also squashed the commits)

re-r? @alexcrichton