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

Improve Ord violation help #128273

Merged
merged 6 commits into from
Aug 10, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
16 changes: 13 additions & 3 deletions library/alloc/src/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,9 @@ impl<T> [T] {
/// handled without allocation, medium sized slices allocate `self.len()` and beyond that it
/// clamps at `self.len() / 2`.
///
/// If `T: Ord` does not implement a total order, the implementation may panic.
/// # Panics
///
/// May panic if `T: Ord` does not implement a total order.
///
/// # Examples
///
Expand Down Expand Up @@ -258,7 +260,9 @@ impl<T> [T] {
/// handled without allocation, medium sized slices allocate `self.len()` and beyond that it
/// clamps at `self.len() / 2`.
///
/// If `T: Ord` does not implement a total order, the implementation may panic.
/// # Panics
///
/// May panic if `compare` does not implement a total order.
///
/// # Examples
///
Expand Down Expand Up @@ -304,7 +308,9 @@ impl<T> [T] {
/// handled without allocation, medium sized slices allocate `self.len()` and beyond that it
/// clamps at `self.len() / 2`.
///
/// If `K: Ord` does not implement a total order, the implementation may panic.
/// # Panics
///
/// May panic if `K: Ord` does not implement a total order.
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Examples
///
Expand Down Expand Up @@ -356,6 +362,10 @@ impl<T> [T] {
/// In the worst case, the algorithm allocates temporary storage in a `Vec<(K, usize)>` the
/// length of the slice.
///
/// # Panics
///
/// May panic if `K: Ord` does not implement a total order.
///
/// # Examples
///
/// ```
Expand Down
27 changes: 15 additions & 12 deletions library/core/src/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2898,7 +2898,9 @@ impl<T> [T] {
/// It is typically faster than stable sorting, except in a few special cases, e.g., when the
/// slice is partially sorted.
///
/// If `T: Ord` does not implement a total order, the implementation may panic.
/// # Panics
///
/// May panic if `T: Ord` does not implement a total order.
///
/// # Examples
///
Expand Down Expand Up @@ -2955,7 +2957,9 @@ impl<T> [T] {
/// It is typically faster than stable sorting, except in a few special cases, e.g., when the
/// slice is partially sorted.
///
/// If `T: Ord` does not implement a total order, the implementation may panic.
/// # Panics
///
/// May panic if `compare` does not implement a total order.
///
/// # Examples
///
Expand Down Expand Up @@ -2999,7 +3003,9 @@ impl<T> [T] {
/// It is typically faster than stable sorting, except in a few special cases, e.g., when the
/// slice is partially sorted.
///
/// If `K: Ord` does not implement a total order, the implementation may panic.
/// # Panics
///
/// May panic if `K: Ord` does not implement a total order.
///
/// # Examples
///
Expand Down Expand Up @@ -3042,15 +3048,14 @@ impl<T> [T] {
/// Median of Medians using Tukey's Ninther for pivot selection, which guarantees linear runtime
/// for all inputs.
///
/// It is typically faster than stable sorting, except in a few special cases, e.g., when the
/// slice is nearly fully sorted, where `slice::sort` may be faster.
///
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
/// [`sort_unstable`]: slice::sort_unstable
///
/// # Panics
///
/// Panics when `index >= len()`, meaning it always panics on empty slices.
///
/// May panic if `T: Ord` does not implement a total order.
///
/// # Examples
///
/// ```
Expand Down Expand Up @@ -3103,15 +3108,14 @@ impl<T> [T] {
/// Median of Medians using Tukey's Ninther for pivot selection, which guarantees linear runtime
/// for all inputs.
///
/// It is typically faster than stable sorting, except in a few special cases, e.g., when the
/// slice is nearly fully sorted, where `slice::sort` may be faster.
///
/// [`sort_unstable`]: slice::sort_unstable
///
/// # Panics
///
/// Panics when `index >= len()`, meaning it always panics on empty slices.
///
/// May panic if `compare` does not implement a total order.
///
/// # Examples
///
/// ```
Expand Down Expand Up @@ -3168,15 +3172,14 @@ impl<T> [T] {
/// Median of Medians using Tukey's Ninther for pivot selection, which guarantees linear runtime
/// for all inputs.
///
/// It is typically faster than stable sorting, except in a few special cases, e.g., when the
/// slice is nearly fully sorted, where `slice::sort` may be faster.
///
/// [`sort_unstable`]: slice::sort_unstable
///
/// # Panics
///
/// Panics when `index >= len()`, meaning it always panics on empty slices.
///
/// May panic if `K: Ord` does not implement a total order.
///
/// # Examples
///
/// ```
Expand Down
23 changes: 19 additions & 4 deletions library/core/src/slice/sort/shared/smallsort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -834,9 +834,10 @@ unsafe fn bidirectional_merge<T: FreezeMarker, F: FnMut(&T, &T) -> bool>(
right = right.add((!left_nonempty) as usize);
}

// We now should have consumed the full input exactly once. This can
// only fail if the comparison operator fails to be Ord, in which case
// we will panic and never access the inconsistent state in dst.
// We now should have consumed the full input exactly once. This can only fail if the
// user-provided comparison operator fails implements a strict weak ordering as required by
// Ord incorrectly, in which case we will panic and never access the inconsistent state in
// dst.
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
if left != left_end || right != right_end {
panic_on_ord_violation();
}
Expand All @@ -845,7 +846,21 @@ unsafe fn bidirectional_merge<T: FreezeMarker, F: FnMut(&T, &T) -> bool>(

#[inline(never)]
fn panic_on_ord_violation() -> ! {
panic!("Ord violation");
// This is indicative of a logic bug in the user-provided comparison function or Ord
// implementation. They are expected to implement a total order as explained in the Ord
// documentation.
//
// By raising this panic we inform the user, that they have a logic bug in their program. If a
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
// strict weak ordering is not given, the concept of comparison based sorting makes no sense.
// E.g.: a < b < c < a
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
//
// The Ord documentation requires users to implement a total order, arguably that's
// unnecessarily strict in the context of sorting. Issues only arise if the weaker requirement
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
// of a strict weak ordering is violated.
//
// The panic message talks about a total order because that's what the Ord documentation talks
// about and requires, so as to not confuse users.
panic!("user-provided comparison function does not correctly implement a total order");
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
}

#[must_use]
Expand Down
Loading