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.

Sign up for GitHub

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

document PartialEq, PartialOrd, Ord requirements more explicitly #85637

Merged
merged 3 commits into from
Jun 21, 2021
Merged

document PartialEq, PartialOrd, Ord requirements more explicitly #85637

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1cfc187
document PartialEq, PartialOrd, Ord requirements more explicitly
RalfJung May 24, 2021
a7abd13
make Ord doc style consistent with the other two; explain which prope…
RalfJung May 27, 2021
45675f3
wording
RalfJung Jun 15, 2021
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
89 changes: 64 additions & 25 deletions library/core/src/cmp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,25 @@ use self::Ordering::*;
/// Trait for equality comparisons which are [partial equivalence
/// relations](https://en.wikipedia.org/wiki/Partial_equivalence_relation).
///
/// `x.eq(y)` can also be written `x == y`, and `x.ne(y)` can be written `x != y`.
/// We use the easier-to-read infix notation in the remainder of this documentation.
///
/// This trait allows for partial equality, for types that do not have a full
/// equivalence relation. For example, in floating point numbers `NaN != NaN`,
/// so floating point types implement `PartialEq` but not [`trait@Eq`].
///
/// Formally, the equality must be (for all `a`, `b`, `c` of type `A`, `B`,
/// `C`):
/// Implementations must ensure that `eq` and `ne` are consistent with each other:
///
/// - `a != b` if and only if `!(a == b)`
/// (ensured by the default implementation).
///
/// If [`PartialOrd`] or [`Ord`] are also implemented for `Self` and `Rhs`, their methods must also
/// be consistent with `PartialEq` (see the documentation of those traits for the exact
/// requirememts). It's easy to accidentally make them disagree by deriving some of the traits and
/// manually implementing others.
Copy link
Member

Choose a reason for hiding this comment

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

I think this sentence is begging for a code example. I was saying to myself, "Oh wow, I should watch out for that, I'm hoping an example comes next" while reading it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I did not write this sentence, I just moved it around. I don't know what the original author had in their mind when writing this.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what I am saying is, this issue is pre-existing, and I hope my PR doesn't have to fix it. :) It seems largely orthogonal to what this PR is about.

///
/// The equality relation `==` must satisfy the following conditions
/// (for all `a`, `b`, `c` of type `A`, `B`, `C`):
///
/// - **Symmetric**: if `A: PartialEq<B>` and `B: PartialEq<A>`, then **`a == b`
/// implies `b == a`**; and
Expand All @@ -53,15 +66,6 @@ use self::Ordering::*;
///
/// ## How can I implement `PartialEq`?
///
/// `PartialEq` only requires the [`eq`] method to be implemented; [`ne`] is defined
/// in terms of it by default. Any manual implementation of [`ne`] *must* respect
/// the rule that [`eq`] is a strict inverse of [`ne`]; that is, `!(a == b)` if and
/// only if `a != b`.
///
/// Implementations of `PartialEq`, [`PartialOrd`], and [`Ord`] *must* agree with
/// each other. It's easy to accidentally make them disagree by deriving some
/// of the traits and manually implementing others.
///
/// An example implementation for a domain in which two books are considered
/// the same book if their ISBN matches, even if the formats differ:
///
Expand Down Expand Up @@ -631,10 +635,25 @@ impl<T: Clone> Clone for Reverse<T> {

/// Trait for types that form a [total order](https://en.wikipedia.org/wiki/Total_order).
///
/// An order is a total order if it is (for all `a`, `b` and `c`):
/// Implementations must be consistent with the [`PartialOrd`] implementation, and ensure
/// `max`, `min`, and `clamp` are consistent with `cmp`:
///
/// - `partial_cmp(a, b) == Some(cmp(a, b))`.
/// - `max(a, b) == max_by(a, b, cmp)` (ensured by the default implementation).
/// - `min(a, b) == min_by(a, b, cmp)` (ensured by the default implementation).
/// - For `a.clamp(min, max)`, see the [method docs](#method.clamp)
/// (ensured by the default implementation).
///
/// It's easy to accidentally make `cmp` and `partial_cmp` disagree by
/// deriving some of the traits and manually implementing others.
///
/// ## Corollaries
///
/// From the above and the requirements of `PartialOrd`, it follows that `<` defines a strict total order.
/// This means that for all `a`, `b` and `c`:
///
/// - total and asymmetric: exactly one of `a < b`, `a == b` or `a > b` is true; and
/// - transitive, `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
/// - exactly one of `a < b`, `a == b` or `a > b` is true; and
/// - `<` is transitive: `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
///
/// ## Derivable
///
Expand All @@ -659,12 +678,6 @@ impl<T: Clone> Clone for Reverse<T> {
/// Then you must define an implementation for [`cmp`]. You may find it useful to use
/// [`cmp`] on your type's fields.
///
/// Implementations of [`PartialEq`], [`PartialOrd`], and `Ord` *must*
/// agree with each other. That is, `a.cmp(b) == Ordering::Equal` if
/// and only if `a == b` and `Some(a.cmp(b)) == a.partial_cmp(b)` for
/// all `a` and `b`. It's easy to accidentally make them disagree by
/// deriving some of the traits and manually implementing others.
///
/// Here's an example where you want to sort people by height only, disregarding `id`
/// and `name`:
///
Expand Down Expand Up @@ -824,15 +837,45 @@ impl PartialOrd for Ordering {

/// Trait for values that can be compared for a sort-order.
///
/// The `lt`, `le`, `gt`, and `ge` methods of this trait can be called using
/// the `<`, `<=`, `>`, and `>=` operators, respectively.
///
/// The methods of this trait must be consistent with each other and with those of `PartialEq` in
/// the following sense:
///
/// - `a == b` if and only if `partial_cmp(a, b) == Some(Equal)`.
/// - `a < b` if and only if `partial_cmp(a, b) == Some(Less)`
/// (ensured by the default implementation).
/// - `a > b` if and only if `partial_cmp(a, b) == Some(Greater)`
/// (ensured by the default implementation).
/// - `a <= b` if and only if `a < b || a == b`
/// (ensured by the default implementation).
/// - `a >= b` if and only if `a > b || a == b`
/// (ensured by the default implementation).
/// - `a != b` if and only if `!(a == b)` (already part of `PartialEq`).
///
/// If [`Ord`] is also implemented for `Self` and `Rhs`, it must also be consistent with
/// `partial_cmp` (see the documentation of that trait for the exact requirements). It's
/// easy to accidentally make them disagree by deriving some of the traits and manually
/// implementing others.
Copy link
Member

Choose a reason for hiding this comment

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

Same thought as above on an example.

///
/// The comparison must satisfy, for all `a`, `b` and `c`:
///
/// - asymmetry: if `a < b` then `!(a > b)`, as well as `a > b` implying `!(a < b)`; and
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this line because:

  • It is redundant: it already follows from a < b being defined as partial_cmp(a, b) == Some(Less), which implies !(a > b) (defined as partial_cmp(a, b) != Some(Greater)).
  • "asymmetry" is the wrong term, an "asymmetric" relation is a relation that satisfies "if a < b then !(b < a)".

asymmtery (in the correct sense of the word) is a consequence of duality, so we could state it in the corollary section if you wish. antisymmetry is more closely related to what the docs are currently stating, but it is defined for <=-style relations: R is antisymmetric if R(a, b) && R(b, a) implies a == b.

/// - transitivity: `a < b` and `b < c` implies `a < c`. The same must hold for both `==` and `>`.
/// - duality: `a < b` if and only if `b > a`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably the most questionable part of this PR -- it could be considered a new requirement. On the other hand, given that we demand symmetry for ==, it would seem really strange to not also demand duality here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably the most questionable part of this PR -- it could be considered a new requirement. On the other hand, given that we demand symmetry for ==, it would seem really strange to not also demand duality here.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is another property here that we might want to add: compatibility of partial_cmp with ==:
when a1 == a2 and b1 == b2, then partial_cmp(a1, b1) == partial_cmp(a2, b2).

///
/// 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>`.
///
/// ## Corollaries
///
/// The following corollaries follow from the above requirements:
///
/// - irreflexivity of `<` and `>`: `!(a < a)`, `!(a > a)`
/// - transitivity of `>`: if `a > b` and `b > c` then `a > c`
/// - duality of `partial_cmp`: `partial_cmp(a, b) == partial_cmp(b, a).map(Ordering::reverse)`
///
/// ## Derivable
///
/// This trait can be used with `#[derive]`. When `derive`d on structs, it will produce a
Expand All @@ -850,10 +893,6 @@ impl PartialOrd for Ordering {
///
/// `PartialOrd` requires your type to be [`PartialEq`].
///
/// Implementations of [`PartialEq`], `PartialOrd`, and [`Ord`] *must* agree with each other. It's
/// easy to accidentally make them disagree by deriving some of the traits and manually
/// implementing others.
///
/// If your type is [`Ord`], you can implement [`partial_cmp`] by using [`cmp`]:
///
/// ```
Expand Down