Skip to content

Some total ordering relations can only be expressed as partial orders #2511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gnzlbg opened this issue Aug 2, 2018 · 7 comments
Open
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 2, 2018

TL;DR: one cannot impl Ord<Foo> for Bar.

Currently, we can stablish strict and non-strict partial ordering relations between types:

impl PartialEq<Foo> for Bar { ... }
impl PartialOrd<Foo> for Bar { ... }

but there is no way to state that such a relation is a total order because Ord and Eq are not generic over the RHS like PartialOrd and PartialEq are.

Is this an oversight?

I would find this useful when implementing ordering relations for wrapper types for which it makes no sense to implement Deref. For example:

impl PartialOrd<Wrapping<i32>> for i32 { ... } // TODAY OK
impl PartialEq<Wrapping<i32>> for i32 { ... } // TODAY OK

impl Eq<Wrapping<i32>> for i32 { ... }  // TODAY ERROR
impl Ord<Wrapping<i32>> for i32 { ... }  // TODAY ERROR

To solve this we would probably need to change Eq and Ord from:

pub trait Eq: PartialEq<Self> { ... }
pub trait Ord: Eq + PartialOrd<Self> { ... }

to

pub trait Eq<RHS=Self>: PartialEq<RHS> { ... }
pub trait Ord<RHS=Self>: Eq<RHS> + PartialOrd<RHS> { ... }

Would this be a breaking change? (if so, could it be fixed automatically by rustfix?)

@varkor
Copy link
Member

varkor commented Aug 3, 2018

The min and max trait methods on Ord currently assume Rhs == Self. You'd need a way to make these more generic (which is not immediately obvious without anonymous enums) and making methods more generic often falls afoul of type inference issues.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 3, 2018

@varkor Indeed. I don't think we can make these more generic without breaking backwards compatibility, but we could add a where RHS=Self constraint to them, so that they are not available if RHS != Self:

pub trait Ord<RHS>: Eq<RHS> + PartialOrd<RHS> {
    fn cmp(&self, other: &Self) -> Ordering;

    // continue to work like they do now:    
    fn max(self, other: RHS) -> Self where Self: Sized, RHS=Self {
        if other >= self { other } else { self }
    }
    fn min(self, other: RHS) -> Self where Self: Sized, RHS=Self {
        if self <= other { self } else { other }
    }
}

A radically different alternative might be to just add a new trait Ord2 that returns Either<LHS,RHS> or something similar, and add a blanket impl for Ord, and slowly migrate code to use Ord2:

enum Either<L, R>(L, R);

trait Ord2<RHS>: Eq<RHS> + PartialOrd<RHS> {
    fn cmp(&self, other: &Self) -> Ordering;

    fn max(self, other: RHS) -> Either<Self, RHS> where Self: Sized;
    fn min(self, other: RHS) -> Either<Self, RHS> where Self: Sized;
}

impl<T: Ord> Ord2<T> for T { ...specializable default impls... }

An open question is whether we can make such a trait as ergonomic as Ord to use for the case RHS=Self.

@strega-nil
Copy link

I think the question is, why are max and min operations on Ord?

@burdges
Copy link

burdges commented Sep 2, 2018

I do think Either sounds interesting, especially if we can do:

put type Cow<'a,B> = Either<&'a B,<B as ToOwned>::Owned>
  where B: 'a + ToOwned + ?Sized;

or more likely have conversions that provide symmetry

impl<'a,B> From<Either<&'a B,<B as ToOwned>::Owned>> for Cow<'a,B> { .. }
impl<'a,B> From<Either<<B as ToOwned>::Owned>,&'a B> for Cow<'a,B> { .. }

And replace From with some Coerce trait eventually.

In this case though, isn't this version fully compatible?

pub trait Ord<RHS=Self>: Eq<RHS> + PartialOrd<RHS> {
    fn cmp(&self, other: &Self) -> Ordering;

    fn max(self, other: RHS) -> Self where Self: Sized, RHS: Into<Self> {
        if other >= self { other.into() } else { self }
    }
    fn min(self, other: RHS) -> Self where Self: Sized, RHS: Into<Self> {
        if self <= other { self } else { other.into() }
    }
}

@strega-nil
Copy link

@burdges that seems reasonable

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 3, 2018

I think the question is, why are max and min operations on Ord?

I don't know, but I don't think this is something that we are allowed to change.

@burdges
Copy link

burdges commented Sep 3, 2018

I suppose big num types contain allocations like Vec does, so Ord::max/min consume them, which kinda sucks. It'd be interesting if we had negative reasoning to support this:

pub trait Ord<RHS=Self>: Eq<RHS> + PartialOrd<RHS> {
    fn cmp(&self, other: &Self) -> Ordering;

    fn max(self, other: RHS) -> Self where Self: Sized+Copy, RHS: Into<Self> {
        if other >= self { other.into() } else { self }
    }
    fn min(self, other: RHS) -> Self where Self: Sized+Copy, RHS: Into<Self> {
        if self <= other { self } else { other.into() }
    }

    fn max<'a>(&'a self, other: &'a RHS) -> &Self where Self: Sized+Drop, RHS: Borrow<&Self> {
        if other >= self { other.borrow() } else { self }
    }
    fn min<'a>(&'a self, other: &'a RHS) -> &Self where Self: Sized+Drop, RHS: Borrow<&Self> {
        if self <= other { self } else { other.borrow() }
    }
}

@Centril Centril added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Oct 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

5 participants