Skip to content

Extend the documentation of PartialOrd #53386

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

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 116 additions & 15 deletions src/libcore/cmp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,16 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

//! Functionality for ordering and comparison.
//! Functionality for ordering and relational operations.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be just relational operations ? This is what comparisons are.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't mind keeping the old version ("ordering and comparisons"). I don't think we gain a lot from being precise here.

//!
//! This module defines both [`PartialOrd`] and [`PartialEq`] traits which are used
//! by the compiler to implement comparison operators. Rust programs may
//! implement [`PartialOrd`] to overload the `<`, `<=`, `>`, and `>=` operators,
//! and may implement [`PartialEq`] to overload the `==` and `!=` operators.
//! This module defines four traits related to ordering and comparisons:
//!
//! [`PartialOrd`]: trait.PartialOrd.html
//! [`PartialEq`]: trait.PartialEq.html
//! * [`PartialEq`]: which overloads the `==` and `!=` operators,
//! * [`PartialOrd`] (requires `PartialEq`): which overloads the `<`, `<=`, `>`,
//! and `>=` operators,
//!
//! # Examples
//! state that values of a type can be compared for equality, or ordered, but that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this hard to parse, where does the sentence even begin and what is the subject matching this verb?

//! not all values of the type can be compared to all other values (more on this below).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can be removed as it appears below already?

//!
//! ```
//! let x: u32 = 0;
Expand All @@ -32,6 +31,66 @@
//! assert_eq!(x == y, false);
//! assert_eq!(x.eq(&y), false);
//! ```
//!
//! This module also provides the following two traits:
//!
//! * [`Eq`]: which requires `PartialEq`, states that all values of a type can be
//! compared to all others,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative:

Eq: marker trait which states that all values of a type can be compared to all other values of the same type (requires PartialEq<Self>)

//! * [`Ord`]: which requires `PartialOrd` and `Eq`, states that the ordering
//! relations expressed by the comparison operators form all total ordering relations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to form "all total ordering relations"?

Or do you mean, "all the comparison operators form total ordering relations"?

//! (more on this below).
//!
//! The distinction between these traits is important: it tell us what it _means_ for a
//! sequence of values to be ordered.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the pragmatic way I choose to put it, might not be the best one.

@LukasKalbertodt I basically decided to put the "intro docs" to PartialOrd at the top level of the module. Otherwise the docs of PatialOrd and other traits end up restating the same things over and over again and is hard to keep them in sync.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "it tellS us"

I think putting the text describing the differences in the mod documentation is a good idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second part of this sentence does not talk about a distinction, so I am confused what this is even telling us.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I am not even sure which distinction you mean: PartialEq vs Eq, or Eq vs Ord...

//!
//! Two commonly encountered examples are integers and floating-point numbers. For example, every [`i32`]
//! value is comparable to every other. If you can tell that two `i32` values are distinc, then these two
//! values never compare equal, and if you sorte a sequence of `i32` values like `[1, 1, 0, -5]` using `<=`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small nit while reading the new docs: sort

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "distinc" => "distinct"

//! doing so will always produce the same unique result: `[-5, 0, 1, 1]`. We say that `<=` imposes a _total_
//! order, and denote this by implementing `Ord` for `i32`.
//!
//! However, none of this holds for IEEE-754 floating-point numbers like `f32`. There is a special `f32` value
//! called "Not a Number" (`NaN`) for which any relational operation alwys return false, that is:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always

//! `a == NaN` is always `false`, even when `a` is `NaN`. For this reason `f32` only implements `PartialOrd`, and it
//! does not implement neither `Eq` nor `Ord`. What does this mean in practice? It means two things:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence is wrong somehow. Maybe put it like this:

For this reason, f32 implements PartialOrd and PartialEq, but neither Ord nor Eq.

//!
//! * there are sequences of `f32`s that we can order using `<=` and will always produce the same unique
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"There"

//! result: sorting `[3.0, 5.0, 1.0]` into `[1.0, 3.0, 5.0]`.
//!
//! * there are also sequences of `f32`s that when ordered using `<=` will produce a different result depending
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"There"

//! on which algorithm we use to sort them: we can sort `[-0.0, NaN, +0.0] into all its permutations
//! (`[-0.0, +0.0, NaN]`, `[+0.0, -0.0, NaN]`, `[NaN, +0.0, -0.0]`, etc. are all "sorted" under `<=`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this part (the two bullet points) :/

I think I would replace everything from "What does this mean in practice?" to "are all "sorted" under <=)." with something like:

While this is no problem for sequences like [3.0, 5.0, 1.0], there is no clear order for sequences containing NaN: what is the correct way to sort [-1.0, NaN, 4.0]? The order [NaN, -1.0, 4.0] would be exactly as correct as [-1.0, 4.0, NaN]. So there is no unique ordering for floats.

Also: "are all "sorted" under <=" -> is that even correct? I'm not quite sure what you mean, but NaN <= +0.0 is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I wasn't fully happy with this either. For me, all are sorted under <= means that !(a > b) is true for all adjacent pairs in the sequence. When either a or b are NaN that's automatically satisfied. Sadly for floats that's not exactly the same as a <= b being true for all adjacent pairs...

Copy link
Member

@RalfJung RalfJung Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be "sorted := a <= b holds for all adjacent pairs"?

EDIT: Oh I see. That's not clear at all. Either remove the example entirely or expand to explain these things.

Maybe it is enough to instead just say that !(a > b) and a <= b are not the same any more, and not go into sorting?

//!
//! Another example is the "subset relation" on sets in which, for sets `a` and `b`, `a < b` is true if and only
//! if `a` is a proper subset of `b` (e.g. `{4, 9} < {1, 4, 5, 9}` is true). The problem is that when neither
//! `a` is a subset of `b` nor `b` is a subset of `a` nor `a = b` (e.g. `{1, 3}` and `{1, 4}`), we cannot sensible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"sensibly"

//! compare `a` and `b`, so that `a < b`, `a == b`, and `a > b` all return false.
//!
//! The distinction between a _total_ order and a _partial_ order is important, and impacts what it means for
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refers to the distinction made above, right? It should be "This distinction" then.

Generally, I feel this needs more structure. All the text explaining partial vs total order should be in some kind of appropriately titeled subsection.

//! the values of a type to be "sorted". This impacts how it makes sense to use these values. For example,
//! if we sort a vector of `f32`s containing `NaN`s, does it make sense to use binary search to try to find a value
//! in that vector? Will it always work? Does it make sense to put `f32`s containing `NaN`s into a set? a hash table?
//! etc. Algorithms and data-structures that require a total order or total equality to work correctly can constrain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would replace "into a set? a hash table? etc." with "into a set or hash table?"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should actually answer all those questions eventually or people will leave this documentation will more questions than they had to begin with. ;)

//! the types they accept with the [`Eq`] and [`Ord`] traits, while those for which a partial order or partial equality
//! is enough can use the `PartialEq` and `PartialOrd` traits.
//!
//! This leads us to the final question: which traits should a type implement ? There is no easy answer: a type should only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space before ?. Also: the lines are pretty long here.

//! implement the traits that make sense for the type, but this answer does not help. The different traits have
//! different semantic requirements in their operations, and if a type cannot satisfy these requirements then it probably
//! should not implement the trait. However, there are types that _could_ satisfy the requirements but for which it
//! still might not make sense to implement the trait. For example, a `Point(i32,i32)` could implement `Ord` by using
//! a lexicographical-order, but whether this might make sense would depend on the application. Another example is
//! floating-point numbers: the IEEE754 standard provides _multiple_ different orderings for floating-point numbers. The one
//! we have seen above is cheap to compute, but it is only a partial order. The IEEE754 standard also provides a
//! total order for floating point numbers that is more expensive to compute. So which ordering should they implement? Arguably,
//! they should offer both orders and let the user choose, but they can only implement the trait once. For these kind of types,
/// for which multiple orderings exist, it might make sense to not implement any order at all, but use another mechanism
//! (e.g. a wrapper type) to allow users to select the order they want.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see what's so hard about this TBH. Once you have a type and a relation (or two, = and <) fixed, there is no doubt any more about which traits to implement.

//!
//! [`PartialOrd`]: trait.PartialOrd.html
//! [`PartialEq`]: trait.PartialEq.html
//! [`Ord`]: trait.Ord.html
//! [`Eq`]: trait.Eq.html

#![stable(feature = "rust1", since = "1.0.0")]

Expand Down Expand Up @@ -512,16 +571,56 @@ impl PartialOrd for Ordering {
}
}

/// Trait for values that can be compared for a sort-order.
/// Trait for values that can be partially ordered.
///
/// This traits defines the operators `<`, `>`, `<=`, and `>=` which compare two values and return `bool`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This trait"

/// where `<` and `>` form [_strict_ partial ordering relations][wiki_porder].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split this sentence. "[...] return bool. The operators < and > form [...]"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move the remark about strict ordering relations far down to all the corollaries.

///
/// The `PartialOrd` trait has fewer requirements of the ordering relation than Ord. In particular, `partial_cmp`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits: two spaces after trait and Ord not in monofont

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also "requirements on the ordering". I think.

/// can return `None` if two elements cannot be compared. That means that `PartialOrd` can be implemented
/// for types with incomparable values like floating-point numbers (`NaN`) and for ordering relations like
/// subset relations.
///
/// It extends the partial equivalence relation provided by `PartialEq` (`==`) with `partial_cmp(a, b) -> Option<Ordering>`,
/// which is a [trichotomy](https://en.wikipedia.org/wiki/Trichotomy_(mathematics)) of the ordering relation when
/// its result is `Some`:
///
/// * `a < b` if and only if `partial_cmp(a, b) == Some(Less)`
/// * `a > b` if and only if `partial_cmp(a, b) == Some(Greater)`
/// * `a == b` if and only if `partial_cmp(a, b) == Some(Equal)`
///
/// The comparison must satisfy, for all `a`, `b` and `c`:
/// and the absence of an ordering between `a` and `b` if and only if `partial_cmp(a, b) == None`.
/// Furthermore, this trait defines `<=` as `a < b || a == b` and `>=` as `a > b || a == b`.
///
/// - antisymmetry: if `a < b` then `!(a > b)`, as well as `a > b` implying `!(a < b)`; and
/// - transitivity: `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
/// The comparisons must satisfy, for all `a`, `b`, and `c`:
///
/// Note that these requirements mean that the trait itself must be implemented symmetrically and
/// transitively: if `T: PartialOrd<U>` and `U: PartialOrd<V>` then `U: PartialOrd<T>` and `T:
/// PartialOrd<V>`.
/// * _transitivity_: if `a < b` and `b < c` then `a < c`
/// * _duality_: if `a < b` then `b > a`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it say somewhere that consistency with == from PartialEq is required?

Basically, this trait contains strictly more information than PartialEq, and it must be the case that == behaves exactly as if implemented via partial_cmp == Some(Equal).

///
/// The `lt` (`<`), `le` (`<=`), `gt` (`>`), and `ge` (`>=`) methods are implemented in terms of `partial_cmp`
/// according to these rules. The default implementations can be overridden for performance reasons, but manual
/// implementations must satisfy the rules above.
///
/// From these rules it follows that `PartialOrd` must be implemented symmetrically and transitively.
/// That is, for two distinct types `T` and `U`, `T: PartialOrd<U>` requires the following trait
/// implementations to exist and satisfy these rules: `T: PartialOrd<T>`, `U: PartialOrd<T>`,
/// and `U: PartialOrd<U>`. If for a third distinct type `V` `T: PartialOrd<V>` is provided, then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhhh I don't quite get why T: PartialOrd<T> and U: PartialOrd<U> are required from the rules above. I am probably just missing something obvious...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required by transitivity (if a < b and b < c then a < c).

Suppose that a: T, b: U, and c: T., and that a < b and b < c are both true. For a < c to also be true it would need to be well-typed, and that requires T: PartialOrd<T>.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, that makes sense. Thanks :)

/// the following implementations should be implemented accordingly: `V: PartialOrd<V>`,
/// `V: PartialOrd<T>`, `U: PartialOrd<V>`, and `V: PartialOrd<U>`.
///
/// The following corollaries follow from transitivity of `<`, duality, and from the definition of `<` et al.
/// in terms of the same `partial_cmp`:
///
/// * irreflexivity of `<`: `!(a < a)`
/// * transitivity of `>`: if `a > b` and `b > c` then `a > c`
/// * asymmetry of `<`: if `a < b` then `!(b < a)`
///
/// Stronger ordering relations can be expressed by using the `Eq` and `Ord` traits, where the `PartialOrd`
/// methods provide:
///
/// * a [_non-strict_ partial ordering relation](https://en.wikipedia.org/wiki/Partially_ordered_set#Strict_and_non-strict_partial_orders)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to use [wiki_porder] here? Since you added the link at the bottom...

/// if `T: PartialOrd + Eq`
/// * a [total ordering relation](https://en.wikipedia.org/wiki/Total_order) if `T: Ord`
///
/// ## Derivable
///
Expand Down Expand Up @@ -610,6 +709,8 @@ impl PartialOrd for Ordering {
/// assert_eq!(x < y, true);
/// assert_eq!(x.lt(&y), true);
/// ```
///
/// [wiki_porder]: https://en.wikipedia.org/wiki/Partially_ordered_set#Strict_and_non-strict_partial_orders
#[lang = "partial_ord"]
#[stable(feature = "rust1", since = "1.0.0")]
#[doc(alias = ">")]
Expand Down