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

Allow adding ordering without adding PartialEq #19

Open
jayvdb opened this issue Aug 7, 2023 · 4 comments
Open

Allow adding ordering without adding PartialEq #19

jayvdb opened this issue Aug 7, 2023 · 4 comments

Comments

@jayvdb
Copy link

jayvdb commented Aug 7, 2023

This macro adds shallow equality based on the sort key.

In my case I would like shallow ordering based on the key, but retain deep equality provided by #[derive(Eq, PartialEq)].

Would this be possible?

@valsteen
Copy link
Owner

valsteen commented Aug 7, 2023

Out-of-the-box there is a problem indeed, a PartialEq implementation is generated and considers that a == b is the same as a.cmp(&b).is_eq()

impl #generics core::cmp::PartialEq<Self> for #struct_name <#(#generics_params),*> #where_clause {
fn eq(&self, other: &Self) -> bool {
self.cmp(other).is_eq()
}

So here would be an example that doesn't work:

use sort_by_derive::{EnumAccessor, SortBy};


#[derive(Eq, PartialEq)]
struct Details {
    intensity: u32,
}

#[derive(SortBy, EnumAccessor)]
#[sort_by(intensity())]
#[accessor(intensity: u32)]
// We'd like to provide PartialEq instead of considering that 'a == b' === 'a.cmp(&b).is_eq()'
// #[derive(Eq, PartialEq)]
enum State {
    Up(Details),
    Down(Details),
}

#[derive(SortBy)]
// #[derive(Eq, PartialEq)]
struct Point {
    #[sort_by]
    state: State,
    id: usize,
}


fn main() {
    let p1 = Point { id: 1, state: State::Up(Details { intensity: 1 }) };
    let p2 = Point { id: 2, state: State::Up(Details { intensity: 1 }) };

    assert!(p1.cmp(&p2).is_eq());
    assert!(p1 != p2);
}

We want Point::id to contribute to PartialEq, but not to PartialOrd, but #[derive(SortBy)] produces a conflicting trait implementation of PartialOrd.

It's done this way so there is no boilerplate to provide when PartialEq is just a special case of PartialCmp but I reckon there is a lack of flexibility.

It would be nice if the macro knew that an impl for PartialEq already exists, unfortunately proc macros just see the block of code it decorates. So we need some kind of marker.

It's not complicated to implement, but there are several options.

Here is a possible implementation where #[no_eq] tells the macro that PartialEq must not be generated:

#[derive(SortBy, Eq, PartialEq)]
#[no_eq]
struct Point {
    #[sort_by]
    state: State,
    id: usize,
}

another option, have a different derive macro name SortByNoEq for instance.

#[derive(SortByNoEq, Eq, PartialEq)]
struct Point {
    #[sort_by]
    state: State,
    id: usize,
}

It could also be some additional parameter to the struct-level #[sort_by(...)].

Not sure what's the best ; there are certainly also other ways.

I'm willing to add this feature but I hesitate about the syntax to introduce, any preference ?

@jayvdb
Copy link
Author

jayvdb commented Aug 7, 2023

What about #[sort_by_no_eq]. Otherwise #[sort_by(...)] would be my preference as it would allow for other flags to be added over time.

@valsteen
Copy link
Owner

valsteen commented Aug 7, 2023

I guess I'd better follow some de-facto convention.

Serde for instance has a top-level #[serde(option=value, option2=value)

#[derive(Serialize, Deserialize)]
#[serde(tag = "t", content = "c")]
enum Block {
    Para(Vec<Inline>),
    Str(String),
}

so indeed it looks like it belongs to the top-level #[sort_by(..)]. It now takes a list of fields but I can detect occurrences of option=...

#[derive(SortBy, Eq, PartialEq)]
#[sort_by(state, x, y, derive_eq=false)]
struct Point {
    state: State,
    x: u8,
    y: u8,
    id: usize,
}

wdyt ?

@jayvdb
Copy link
Author

jayvdb commented Aug 7, 2023

lgtm

valsteen added a commit that referenced this issue Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants