Default `PartialOrd` impl for non-matching `Option` variants

help
1

AIUI, the PartialOrd implementation for Option uses the declaration order in the enum for non-matching variants. None being declared before Some, None is always less than Some(_).

Consider the following example (try it on the playground):

use std::time::Duration;

fn main() {
    let d0 = Duration::from_millis(0);

    assert!(Some(d0) != None);
    assert_eq!(Some(d0) < None, false);
    // Fails    
    assert_eq!(Some(d0) > None, false);
}

IMO, any Some(duration) is neither greater nor less than None, they just can't be compared. The only solution I found to work this around was to use constructs such as:

(d0 and d1 as Option<Duration> from now on)

    if d0.zip(d1).map_or(false, |(d0, d1)| do > d1) {
        ...
    }

Which feels a bit verbose to me! Besides, using if d0 > d1 { ... } just compiles even though it doesn't lead to the behavior I would expect.

Anything better I could use to express this?

Is the current PartialOrd implementation for Option actually correct?

Edit: added a mention about d0 and d1 type.

1 Like
2

It provides a total ordering1, which allows it to be used in more places. Note however that it's not the only possible ordering, like the one you want.

Another implementation could be

if d0.is_some() == d1.is_some() && d0 > d1 {
    ...
}

1 every value can be compared to every other value, comparison is transitive, etc. See the docs of Ord for more details.

3

Yes that would probably be easier to read.

Any example in mind for which comparing None to Some could be true?

4

I do not think it makes sense to talk about whether the PartialOrd impl is correct. There are several different equally valid orderings that Option might have. Rust has chosen one of them, and it is right for some use-cases, and wrong for others.

If there is a "right" choice, then it would be to not have an PartialOrd at all due to this ambiguity.

1 Like
5

Yes, I think it should be left to the user to decide. Something like:

    if d0.partial_cmp(d1).unwrap_or(false) {
        ...
    }

I just can't find use cases for which we want current behavior, so I tend to believe it's more the exception than the norm.

Edit: not that comparison, but something in the line...

6

If you are able to write that if, then it has not been left up to the user to decide. The existence of a partial_cmp function implies that you have a PartialOrd impl, and in this case that you have the one that you suggested.

7

Indeed

8

I have previously used the current Ord implementation by defining a struct like this:

#[derive(PartialOrd, Ord, PartialEq, Eq)]
struct MyHeapKey {
    distance: std::cmp::Reverse<u64>,
    node: usize,
    prev_node: Option<usize>,
}

I have used this as the value in a BinaryHeap where I want the values to be yielded by increasing distance. I did not care about the relative ordering of values with the same distance.

If Option had the PartialOrd implementation that you suggested, then it would not be able to implement the Ord trait. That would prevent me from using a #[derive(...)] in the above example, and I would have to manually write the impl block for all four traits (well maybe I could still derive two of them).

So in this case, I care about Option implementing PartialOrd. I did not care about which way it was implemented, except that it be literally any other choice than your suggestion.

9

Good point! Thanks for the example.

10

OTHO, don't you feel we are sacrificing explicitness for usability in this case? In my example, I can get an unexpected behavior without the compiler complaining, which is not like Rust :grinning:

11

This is a feature I find to be mildly footgunnish, and is somewhat at odds with the policy of providing only PartialOrd but not Ord for floating-point types. We could apply the same logic there: it's definitely possible to provide a total ordering for f32 (IEEE 754 even defines one), and it might make working with f32 a lot easier in the cases where you don't care about the relative ordering of NaNs with everything else (which is not infrequent). But instead we use "logical" (partial) ordering for f32, which we don't do with Option.

It's academic, at this point, because Ord will not be removed from Option (and I think adding Ord to floating-point numbers would be an even greater mistake).

Bear in mind, though: while Rust's standard library is IMO pretty well-designed, "preventing all bugs" is not a goal of Rust. There's no way to use Option's total ordering to cause undefined behavior in safe code, so any bugs that result from this quirk are "logic bugs", not "soundness bugs".

1 Like
12

Thanks for the insights!

13

I don't like it, but we're stuck with it.

(Personally, I think Option ought to have been !Ord so that comparing Some(_) to None is always None from partial_cmp, because I've never seen a convincing reason why the current None < Some(_) is more correct than None > Some(_). And ditto for Result.)

3 Likes
14

I'd just use a newtype wrapper around Option and impl Deref, DerefMut, PartialOrd and Ord for that, then you can derive PartialOrd/Ord for your MyHeapKey struct without a questionable decision every user of Option (which I'd guess to be roughly 99% of all Rust users) is stuck with for eternity.

15

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.