Skip to content
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

Fully generalize cmp instances #20063

Open
aturon opened this issue Dec 20, 2014 · 19 comments
Open

Fully generalize cmp instances #20063

aturon opened this issue Dec 20, 2014 · 19 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Dec 20, 2014

With cmp/ops reform, all of the comparison traits allow the types of the two sides to differ. However, the traits provide a default type for the right-hand size that is the same as the left-hand side. Thus, an impl like:

impl<T: PartialEq> PartialEq for Rc<T> { ... }

is more limited than it needs to be; it could instead be

impl<U, T: PartialEq<U>> PartialEq<Rc<U>> for Rc<T> { ... }

(Of course, you could imagine being even more general than that, allowing Rc values to be compared to other values.)

The various impls in the standard library should probably be generalized along these lines; currently a few are but most aren't.

@aturon
Copy link
Member Author

aturon commented Dec 20, 2014

cc @japaric @alexcrichton

I don't think we need to do this prior to stabilization, since we can always safely generalize in the future. I'd also like to put some thought into how general we want to make things, but the example above seems like a good minimum.

@aturon
Copy link
Member Author

aturon commented Dec 20, 2014

A bit of follow up: I'm seeing some cases where e.g. the PartialEq implementation has been generalized like the above, but the Eq hasn't.

@aturon aturon mentioned this issue Dec 20, 2014
alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 31, 2014
This patch marks `PartialEq`, `Eq`, `PartialOrd`, and `Ord` as
`#[stable]`, as well as the majorify of manual implementaitons of these
traits. The traits match the [reform RFC](rust-lang/rfcs#439).

In the future, many of the impls should be generalized; see rust-lang#20063.
However, there is no problem stabilizing the less general impls, since
generalizing later is not a breaking change.

r? @alexcrichton
@steveklabnik
Copy link
Member

What's the status of this with #20065 merged?

@steveklabnik
Copy link
Member

I am guessing the status is that it's done, so I'm giving it a close. Let me know if there's anything specific still missing.

@aturon
Copy link
Member Author

aturon commented Feb 16, 2015

@steveklabnik No, this hasn't been done yet. Basically it would require a sweep through all existing impls to generalize them.

I'm not sure where you want to track tickets like this. It's generally backwards compatible to make these changes, and not exactly high priority, but something nice for expressiveness and consistency (and definitely does not require an RFC).

@aturon aturon reopened this Feb 16, 2015
@steveklabnik
Copy link
Member

Yeah, it's hard because it's not directly actionable, it's more like 'double check all these things.' It'd be nice if we had an actual list.

@japaric
Copy link
Member

japaric commented Feb 16, 2015

I think the following command gives you a "Good Enough" list of implementations of PartialEq<Rhs=Self> on types that have generic parameters:

rustc -Z unstable-options --pretty=expanded libcore/lib.rs | grep 'impl <.*PartialEq '

Example output on liballoc:

    impl <T: ?Sized + PartialEq> PartialEq for Box<T> {
    impl <T: PartialEq> PartialEq for Arc<T> {
    impl <T: PartialEq> PartialEq for Rc<T> {

After running that command on every crate, and filtering out types that only have lifetime parameters like CovariantLifetime<'a>, I ended up with the following list:

liballoc
  - Arc<T>
  - Box<T>
  - Rc<T>
libcollections
  - BTreeMap<K, V>
  - BTreeSet<T>
  - DList<A>
  - EnumSet<E>
  - RingBuf<A>
  - VecMap<V>
libcore
  - (A, B)
  - (A, B, C)
  - (A, B, C, D)
  - (A, B, C, D, E)
  - (A, B, C, D, E, F)
  - (A, B, C, D, E, F, G)
  - (A, B, C, D, E, F, G, H)
  - (A, B, C, D, E, F, G, H, I)
  - (A, B, C, D, E, F, G, H, I, J)
  - (A, B, C, D, E, F, G, H, I, J, K)
  - (A, B, C, D, E, F, G, H, I, J, K, L)
  - (A,)
  - *const T
  - *mut T
  - Cell<T>
  - ContravariantType<T>
  - CovariantType<T>
  - InvariantType<T>
  - MinMaxResult<T>
  - NonZero<T>
  - Option<T>
  - PhantomData<T>
  - Range<Idx>
  - RangeFrom<Idx>
  - RangeTo<Idx>
  - RefCell<T>
  - Result<T, E>
  - extern "C" fn() -> _R
  - extern "C" fn(A) -> _R
  - extern "C" fn(A, B) -> _R
  - extern "C" fn(A, B, C) -> _R
  - extern "C" fn(A, B, C, D) -> _R
  - extern "C" fn(A, B, C, D, E) -> _R
librustc
  - Binder<T>
  - Obligation<'tcx, T>
  - OutlivesPredicate<A, B>
  - TrackMatchMode<T>
  - VarValueKey<K>
  - VecPerParamSpace<K>
  - VtableImplData<'tcx, N>
libstd
  - HashMap<K, V, S>
  - HashSet<T, S>
  - SendError<T>
  - TrySendError<T>
libsyntax
  - OwnedSlice<T>
  - P<T>
  - Spanned<T>
libtest
  - Summary<T>

I'll let @aturon pick the subset that would be nice to have for 1.0 (Option, Result, the smart pointers, the tuples?). Also, I don't think generalizing all of them actually makes sense.

Disclaimer: This list is not exhaustive, because some lines got wrapped around by the pretty printer and the grep expression expects a single line match.

@aturon
Copy link
Member Author

aturon commented Mar 5, 2015

@japaric Sorry for the delay, this comment was buried in my inbox (and I was away on vacation).

Anyway:

Option, Result, the smart pointers, the tuples?

That makes good sense to me as a 1.0 starting point, and I certainly agree that not all of the listed types should get this treatment.

@shepmaster
Copy link
Member

cc #20927

@shepmaster
Copy link
Member

I took a shot at just implementing this manually for Option. Unfortunately, it immediately runs into ambiguous types:

src/libsyntax/attr.rs:455:12: 455:27 error: unable to infer enough type information about `_`; type annotations required [E0282]
src/libsyntax/attr.rs:455         if feature == None && tag != "deprecated" {
                                     ^~~~~~~~~~~~~~~

I feel like there should be a nicer solution here. I'm pretty sure that adding type annotations to every None in existence is not going to play out well 😸 .

@huonw
Copy link
Member

huonw commented May 26, 2015

Argh! Sounds like it may not be possible to generalise. :(

@huonw
Copy link
Member

huonw commented Aug 11, 2015

It seems type parameter defaults may help to solve the issue @shepmaster met:

#![feature(default_type_parameter_fallback)]

enum Maybe<T> { Nothing, Just(T) }
use Maybe::*;

impl<T, U = T> PartialEq<Maybe<U>> for Maybe<T>
    where T: PartialEq<U>
{
    fn eq(&self, other: &Maybe<U>) -> bool {
        match (self, other) {
            (&Nothing, &Nothing) => true,
            (&Just(ref a), &Just(ref b)) => a == b,
            _ => false,
        }
    }
}

fn main() {
    println!("{}", Nothing == Just("foo"));

    println!("{}", Just("foo") == Nothing);
}

Without the default, both println!s fail to compile, but with the U = T default Just("foo") == Nothing works, and with T = U as the default Nothing == Just("foo") works. It doesn't seem possible to get both to work (e.g. T = U, U = T doesn't work).

@withoutboats
Copy link
Contributor

How frustrating (and interesting!). Is it possible to improve default type parameters to allow mutual identity defaults?

@jroesch
Copy link
Member

jroesch commented Aug 12, 2015

You aren't currently allowed to forward declare defaults, we might be able to lift that requirement having both be equal should just give you this set of equalities:

$0 = U,
$1 = T,
$1 = $0

@jroesch
Copy link
Member

jroesch commented Aug 12, 2015

I'm up late working on a paper and this thought came to mind, you can encode a cute little trait trick in order to apply a secondary default:

#![feature(default_type_parameter_fallback)]

enum Maybe<T> { Nothing, Just(T) }
use Maybe::*;

trait DefaultTo<Ty, Default> {}
impl<T, U=T> DefaultTo<U, T> for () {}

impl<U, T = U> PartialEq<Maybe<U>> for Maybe<T>
    where T: PartialEq<U>,
          (): DefaultTo<U, T>
{
    fn eq(&self, other: &Maybe<U>) -> bool {
        match (self, other) {
            (&Nothing, &Nothing) => true,
            (&Just(ref a), &Just(ref b)) => a == b,
            _ => false,
        }
    }
}

fn main() {
    println!("{}", PartialEq::eq(&Nothing, &Just("foo")));

    println!("{}", PartialEq::eq(&Just("foo"), &Nothing));
}

@brson brson removed the E-tedious label Jun 27, 2016
@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@m-ou-se
Copy link
Member

m-ou-se commented Jan 5, 2022

@rust-lang/lang How realistic would it be to for a solution for the problem here to be designed and accepted any time soon? Ideally we'd make Some("1".to_string()) == Some("1") work, but right now we can't do that without breaking Some(1) == None and None == Some(1).

@m-ou-se
Copy link
Member

m-ou-se commented Jan 5, 2022

Note that it wouldn't need to have T = U and U = T defaults on the PartialEq impl. Default of T = ! and U = ! (or some other dummy type) would also work, as long as ! implements PartialEq<T> for all T, and all T implement PartialEq<!> (which overlap, but that should be fine for !).

@fdecode
Copy link

fdecode commented Nov 22, 2024

Some languages are able to do point-to-point vectorial comparisons:

[1, 2, 3, 4, 5] <= [5, 4, 3, 2, 1] will give [true, true, true, false, false]

Such overloading would be possible, for example, if trait PartialOrd had associated output type, as (this is only a sketchy suggestion):

pub trait PartialOrd<Rhs = Self>: PartialEq<Rhs>
where
    Rhs: ?Sized,
{
    type Ordering;
    type Bool;
    
    fn partial_cmp(&self, other: &Rhs) -> Option<Self::Ordering>;

    fn lt(&self, other: &Rhs) -> Self::Bool { ... }
    fn le(&self, other: &Rhs) -> Self::Bool { ... }
    fn gt(&self, other: &Rhs) -> Self::Bool { ... }
    fn ge(&self, other: &Rhs) -> Self::Bool { ... }
}

In whatever case, is there an RFC aimed at enabling point-to-point vector comparisons (or even more general comparisons such as functional point-to-point comparisons, etc.)?

@scottmcm
Copy link
Member

@fdecode That's https://doc.rust-lang.org/std/simd/cmp/trait.SimdPartialOrd.html#tymethod.simd_le

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests