Is this a correct implementation of PartialEq/Eq/PartialOrd/Ord?

[upper pilot]

TeamMemberRole comes from an external API and said API does not guarantee that there will not be new variants, therefore I have to define it as so: ```rs
#[derive(serde::Deserialize)]
#[non_exhaustive]
pub enum TeamMemberRole {
Admin,
Developer,
ReadOnly,
#[serde(untagged)]
Other(String),
}

I would like to implement ordering on this, so users can use it to check permissions, and make sure that all `Other` values are the same ordering value to prevent weirdness where a new role is added and the checks start to act eratically, so just checking that ```rs
impl TeamMemberRole {
    fn discriminant(&self) -> u8 {
        match self {
            Self::Admin => 3,
            Self::Developer => 2,
            Self::ReadOnly => 1,
            Self::Other(_) => 0,
        }
    }
}

impl PartialEq for TeamMemberRole {
    fn eq(&self, other: &Self) -> bool {
        self.discriminant() == other.discriminant()
    }
}

impl Eq for TeamMemberRole {}

impl PartialOrd for TeamMemberRole {
    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
        Some(self.cmp(other))
    }
}

impl Ord for TeamMemberRole {
    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
        self.discriminant().cmp(&other.discriminant())
    }
}
``` does not break the contract of Ord/Eq.

keep in mind I have to have a manual discriminant method as std::mem::Discriminant does not implement ordering

[vapid zephyr]

this seems like a valid implementation (all four traits are consistently delegating to discriminant()) but one which may be very surprising to users

How about defining a second, exhaustive enum and offering fallible conversion to it?

Another thing to consider is that if the set of roles is expanded, in the future there might not be any single linear ordering of all of them that agrees with permission checks, so it won't be possible to continue using Ord for this purpose.

[upper pilot]

I really don't think that "unknown enum variants are treated as having the least permissions" is suprising to users.

[vapid zephyr]

Sorry, I don't mean the "least permissions" part

I mean that it is surprising for Eq to treat all unknown roles as identical, even though they clearly have different data

[upper pilot]

Do Eq and Ord have to agree, like Eq and Hash for types used in HashMaps?

[vapid zephyr]

Yes. Unfortunately.

(I think this is a design mistake; I think that "equally ordered/ranked" should be a distinct concept from "equal value". But Rust, and most languages, don't make that distinction, and it's infeasible to change.)

Oh, another thing you could do is define a struct which wraps a TeamMemberRole and provides the special Eq/Ord behavior — kinda like std::cmp::Reverse is also a wrapper that changes Ord behavior. That way it's opt-in and you can still get at the data if you want.

[cerulean nexus]

In fairness, it's extremely weird for a.cmp(&b) == Ordering::Equal to do something different from a == b.

Like, I understand the idea. If you order people by their position in a set of rankings, and compare them by personhood, both of which are fairly natural, two people may be different but have identical rankings, but I'd argue what happened here is you chose two different notions of equality/ordering and conflated them

Newtype them up, that seems more reasonable to me

[vapid zephyr]

Right, it should be rarely used, and perhaps this example is a bad case to apply it, but I do in fact mean that there should be and aren't separate "equal-values" and "equally-ordered" operators (or functions, whatever). More to the point, There should be an operation which says NaN == NaN because they are equivalent answers to a question.

[upper pilot]

I just needed to check if someone was developer or admin, so I just swapped to that check and gave up on the Eq/Ord impls because it seems like a mess

[cerulean nexus]

(there kinda is such a function, it's f{32, 64}::total_cmp. Though it splits NaNs into four different brackets of equality)

[vapid zephyr]

Sorry, right, I mean, there should be a default operation.

memo_of_expensive_function: HashMap<f64, f64> should just work.