diff options
| author | Guillaume Gomez <guillaume1.gomez@gmail.com> | 2024-08-10 16:23:51 +0200 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2024-08-10 16:23:51 +0200 |
| commit | 65875b2f5a48b6b6f8a4c49a73cae7e84c7e19e7 (patch) | |
| tree | becc91e62dd9a2c4af430037ac4f0ecac8abc689 | |
| parent | 48090b11b52bd841fa89082cdaa4c139f11995f7 (diff) | |
| parent | 1be60b5d2b6ac569d51abd376e6f04e2fc07692c (diff) | |
| download | rust-65875b2f5a48b6b6f8a4c49a73cae7e84c7e19e7.tar.gz rust-65875b2f5a48b6b6f8a4c49a73cae7e84c7e19e7.zip | |
Rollup merge of #128273 - Voultapher:improve-ord-violation-help, r=workingjubilee
Improve `Ord` violation help Recent experience in #128083 showed that the panic message when an Ord violation is detected by the new sort implementations can be confusing. So this PR aims to improve it, together with minor bug fixes in the doc comments for sort*, sort_unstable* and select_nth_unstable*. Is it possible to get these changes into the 1.81 release? It doesn't change behavior and would greatly help when users encounter this panic for the first time, which they may after upgrading to 1.81. Tagging `@orlp`
| -rw-r--r-- | library/alloc/src/slice.rs | 96 | ||||
| -rw-r--r-- | library/core/src/slice/mod.rs | 98 | ||||
| -rw-r--r-- | library/core/src/slice/sort/shared/smallsort.rs | 22 | ||||
| -rw-r--r-- | library/core/src/slice/sort/unstable/mod.rs | 2 |
4 files changed, 129 insertions, 89 deletions
diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index 7dcf344cdc5..9d704870326 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -178,15 +178,25 @@ impl<T> [T] { /// This sort is stable (i.e., does not reorder equal elements) and *O*(*n* \* log(*n*)) /// worst-case. /// - /// If `T: Ord` does not implement a total order the resulting order is unspecified. All - /// original elements will remain in the slice and any possible modifications via interior - /// mutability are observed in the input. Same is true if `T: Ord` panics. + /// If the implementation of [`Ord`] for `T` does not implement a [total order] the resulting + /// order of elements in the slice is unspecified. All original elements will remain in the + /// slice and any possible modifications via interior mutability are observed in the input. Same + /// is true if the implementation of [`Ord`] for `T` panics. /// /// When applicable, unstable sorting is preferred because it is generally faster than stable /// sorting and it doesn't allocate auxiliary memory. See /// [`sort_unstable`](slice::sort_unstable). The exception are partially sorted slices, which /// may be better served with `slice::sort`. /// + /// Sorting types that only implement [`PartialOrd`] such as [`f32`] and [`f64`] require + /// additional precautions. For example, `f32::NAN != f32::NAN`, which doesn't fulfill the + /// reflexivity requirement of [`Ord`]. By using an alternative comparison function with + /// `slice::sort_by` such as [`f32::total_cmp`] or [`f64::total_cmp`] that defines a [total + /// order] users can sort slices containing floating-point values. Alternatively, if all values + /// in the slice are guaranteed to be in a subset for which [`PartialOrd::partial_cmp`] forms a + /// [total order], it's possible to sort the slice with `sort_by(|a, b| + /// a.partial_cmp(b).unwrap())`. + /// /// # Current implementation /// /// The current implementation is based on [driftsort] by Orson Peters and Lukas Bergdoll, which @@ -198,18 +208,21 @@ 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 the implementation of [`Ord`] for `T` does not implement a [total order]. /// /// # Examples /// /// ``` - /// let mut v = [-5, 4, 1, -3, 2]; + /// let mut v = [4, -5, 1, -3, 2]; /// /// v.sort(); - /// assert!(v == [-5, -3, 1, 2, 4]); + /// assert_eq!(v, [-5, -3, 1, 2, 4]); /// ``` /// /// [driftsort]: https://github.com/Voultapher/driftsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[cfg(not(no_global_oom_handling))] #[rustc_allow_incoherent_impl] #[stable(feature = "rust1", since = "1.0.0")] @@ -221,30 +234,19 @@ impl<T> [T] { stable_sort(self, T::lt); } - /// Sorts the slice with a comparator function, preserving initial order of equal elements. + /// Sorts the slice with a comparison function, preserving initial order of equal elements. /// /// This sort is stable (i.e., does not reorder equal elements) and *O*(*n* \* log(*n*)) /// worst-case. /// - /// The comparator function should define a total ordering for the elements in the slice. If the - /// ordering is not total, the order of the elements is unspecified. - /// - /// If the comparator function does not implement a total order the resulting order is - /// unspecified. All original elements will remain in the slice and any possible modifications - /// via interior mutability are observed in the input. Same is true if the comparator function - /// panics. A total order (for all `a`, `b` and `c`): + /// If the comparison function `compare` does not implement a [total order] the resulting order + /// of elements in the slice is unspecified. All original elements will remain in the slice and + /// any possible modifications via interior mutability are observed in the input. Same is true + /// if `compare` panics. /// - /// * total and antisymmetric: 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 `>`. - /// - /// For example, while [`f64`] doesn't implement [`Ord`] because `NaN != NaN`, we can use - /// `partial_cmp` as our sort function when we know the slice doesn't contain a `NaN`. - /// - /// ``` - /// let mut floats = [5f64, 4.0, 1.0, 3.0, 2.0]; - /// floats.sort_unstable_by(|a, b| a.partial_cmp(b).unwrap()); - /// assert_eq!(floats, [1.0, 2.0, 3.0, 4.0, 5.0]); - /// ``` + /// For example `|a, b| (a - b).cmp(a)` is a comparison function that is neither transitive nor + /// reflexive nor total, `a < b < c < a` with `a = 1, b = 2, c = 3`. For more information and + /// examples see the [`Ord`] documentation. /// /// # Current implementation /// @@ -257,21 +259,24 @@ 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 /// /// ``` - /// let mut v = [5, 4, 1, 3, 2]; + /// let mut v = [4, -5, 1, -3, 2]; /// v.sort_by(|a, b| a.cmp(b)); - /// assert!(v == [1, 2, 3, 4, 5]); + /// assert_eq!(v, [-5, -3, 1, 2, 4]); /// /// // reverse sorting /// v.sort_by(|a, b| b.cmp(a)); - /// assert!(v == [5, 4, 3, 2, 1]); + /// assert_eq!(v, [4, 2, 1, -3, -5]); /// ``` /// /// [driftsort]: https://github.com/Voultapher/driftsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[cfg(not(no_global_oom_handling))] #[rustc_allow_incoherent_impl] #[stable(feature = "rust1", since = "1.0.0")] @@ -288,9 +293,10 @@ impl<T> [T] { /// This sort is stable (i.e., does not reorder equal elements) and *O*(*m* \* *n* \* log(*n*)) /// worst-case, where the key function is *O*(*m*). /// - /// If `K: Ord` does not implement a total order the resulting order is unspecified. - /// All original elements will remain in the slice and any possible modifications via interior - /// mutability are observed in the input. Same is true if `K: Ord` panics. + /// If the implementation of [`Ord`] for `K` does not implement a [total order] the resulting + /// order of elements in the slice is unspecified. All original elements will remain in the + /// slice and any possible modifications via interior mutability are observed in the input. Same + /// is true if the implementation of [`Ord`] for `K` panics. /// /// # Current implementation /// @@ -303,18 +309,21 @@ 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 the implementation of [`Ord`] for `K` does not implement a [total order]. /// /// # Examples /// /// ``` - /// let mut v = [-5i32, 4, 1, -3, 2]; + /// let mut v = [4i32, -5, 1, -3, 2]; /// /// v.sort_by_key(|k| k.abs()); - /// assert!(v == [1, 2, -3, 4, -5]); + /// assert_eq!(v, [1, 2, -3, 4, -5]); /// ``` /// /// [driftsort]: https://github.com/Voultapher/driftsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[cfg(not(no_global_oom_handling))] #[rustc_allow_incoherent_impl] #[stable(feature = "slice_sort_by_key", since = "1.7.0")] @@ -336,9 +345,10 @@ impl<T> [T] { /// storage to remember the results of key evaluation. The order of calls to the key function is /// unspecified and may change in future versions of the standard library. /// - /// If `K: Ord` does not implement a total order the resulting order is unspecified. - /// All original elements will remain in the slice and any possible modifications via interior - /// mutability are observed in the input. Same is true if `K: Ord` panics. + /// If the implementation of [`Ord`] for `K` does not implement a [total order] the resulting + /// order of elements in the slice is unspecified. All original elements will remain in the + /// slice and any possible modifications via interior mutability are observed in the input. Same + /// is true if the implementation of [`Ord`] for `K` panics. /// /// For simple key functions (e.g., functions that are property accesses or basic operations), /// [`sort_by_key`](slice::sort_by_key) is likely to be faster. @@ -355,16 +365,22 @@ 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 the implementation of [`Ord`] for `K` does not implement a [total order]. + /// /// # Examples /// /// ``` - /// let mut v = [-5i32, 4, 32, -3, 2]; + /// let mut v = [4i32, -5, 1, -3, 2, 10]; /// + /// // Strings are sorted by lexicographical order. /// v.sort_by_cached_key(|k| k.to_string()); - /// assert!(v == [-3, -5, 2, 32, 4]); + /// assert_eq!(v, [-3, -5, 1, 10, 2, 4]); /// ``` /// /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[cfg(not(no_global_oom_handling))] #[rustc_allow_incoherent_impl] #[stable(feature = "slice_sort_by_cached_key", since = "1.34.0")] diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index b1440214d79..c76157720b7 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -28,6 +28,7 @@ pub mod memchr; issue = "none", reason = "exposed from core to be reused in std;" )] +#[doc(hidden)] pub mod sort; mod ascii; @@ -2880,9 +2881,19 @@ impl<T> [T] { /// This sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not /// allocate), and *O*(*n* \* log(*n*)) worst-case. /// - /// If `T: Ord` does not implement a total order the resulting order is unspecified. All - /// original elements will remain in the slice and any possible modifications via interior - /// mutability are observed in the input. Same is true if `T: Ord` panics. + /// If the implementation of [`Ord`] for `T` does not implement a [total order] the resulting + /// order of elements in the slice is unspecified. All original elements will remain in the + /// slice and any possible modifications via interior mutability are observed in the input. Same + /// is true if the implementation of [`Ord`] for `T` panics. + /// + /// Sorting types that only implement [`PartialOrd`] such as [`f32`] and [`f64`] require + /// additional precautions. For example, `f32::NAN != f32::NAN`, which doesn't fulfill the + /// reflexivity requirement of [`Ord`]. By using an alternative comparison function with + /// `slice::sort_unstable_by` such as [`f32::total_cmp`] or [`f64::total_cmp`] that defines a + /// [total order] users can sort slices containing floating-point values. Alternatively, if all + /// values in the slice are guaranteed to be in a subset for which [`PartialOrd::partial_cmp`] + /// forms a [total order], it's possible to sort the slice with `sort_unstable_by(|a, b| + /// a.partial_cmp(b).unwrap())`. /// /// # Current implementation /// @@ -2894,18 +2905,21 @@ 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 the implementation of [`Ord`] for `T` does not implement a [total order]. /// /// # Examples /// /// ``` - /// let mut v = [-5, 4, 1, -3, 2]; + /// let mut v = [4, -5, 1, -3, 2]; /// /// v.sort_unstable(); - /// assert!(v == [-5, -3, 1, 2, 4]); + /// assert_eq!(v, [-5, -3, 1, 2, 4]); /// ``` /// /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[stable(feature = "sort_unstable", since = "1.20.0")] #[inline] pub fn sort_unstable(&mut self) @@ -2915,31 +2929,20 @@ impl<T> [T] { sort::unstable::sort(self, &mut T::lt); } - /// Sorts the slice with a comparator function, **without** preserving the initial order of + /// Sorts the slice with a comparison function, **without** preserving the initial order of /// equal elements. /// /// This sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not /// allocate), and *O*(*n* \* log(*n*)) worst-case. /// - /// The comparator function should define a total ordering for the elements in the slice. If the - /// ordering is not total, the order of the elements is unspecified. - /// - /// If the comparator function does not implement a total order the resulting order is - /// unspecified. All original elements will remain in the slice and any possible modifications - /// via interior mutability are observed in the input. Same is true if the comparator function - /// panics. A total order (for all `a`, `b` and `c`): - /// - /// * total and antisymmetric: 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 `>`. + /// If the comparison function `compare` does not implement a [total order] the resulting order + /// of elements in the slice is unspecified. All original elements will remain in the slice and + /// any possible modifications via interior mutability are observed in the input. Same is true + /// if `compare` panics. /// - /// For example, while [`f64`] doesn't implement [`Ord`] because `NaN != NaN`, we can use - /// `partial_cmp` as our sort function when we know the slice doesn't contain a `NaN`. - /// - /// ``` - /// let mut floats = [5f64, 4.0, 1.0, 3.0, 2.0]; - /// floats.sort_unstable_by(|a, b| a.partial_cmp(b).unwrap()); - /// assert_eq!(floats, [1.0, 2.0, 3.0, 4.0, 5.0]); - /// ``` + /// For example `|a, b| (a - b).cmp(a)` is a comparison function that is neither transitive nor + /// reflexive nor total, `a < b < c < a` with `a = 1, b = 2, c = 3`. For more information and + /// examples see the [`Ord`] documentation. /// /// # Current implementation /// @@ -2951,21 +2954,24 @@ 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 /// /// ``` - /// let mut v = [5, 4, 1, 3, 2]; + /// let mut v = [4, -5, 1, -3, 2]; /// v.sort_unstable_by(|a, b| a.cmp(b)); - /// assert!(v == [1, 2, 3, 4, 5]); + /// assert_eq!(v, [-5, -3, 1, 2, 4]); /// /// // reverse sorting /// v.sort_unstable_by(|a, b| b.cmp(a)); - /// assert!(v == [5, 4, 3, 2, 1]); + /// assert_eq!(v, [4, 2, 1, -3, -5]); /// ``` /// /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[stable(feature = "sort_unstable", since = "1.20.0")] #[inline] pub fn sort_unstable_by<F>(&mut self, mut compare: F) @@ -2981,9 +2987,10 @@ impl<T> [T] { /// This sort is unstable (i.e., may reorder equal elements), in-place (i.e., does not /// allocate), and *O*(*n* \* log(*n*)) worst-case. /// - /// If `K: Ord` does not implement a total order the resulting order is unspecified. - /// All original elements will remain in the slice and any possible modifications via interior - /// mutability are observed in the input. Same is true if `K: Ord` panics. + /// If the implementation of [`Ord`] for `K` does not implement a [total order] the resulting + /// order of elements in the slice is unspecified. All original elements will remain in the + /// slice and any possible modifications via interior mutability are observed in the input. Same + /// is true if the implementation of [`Ord`] for `K` panics. /// /// # Current implementation /// @@ -2995,18 +3002,21 @@ 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 the implementation of [`Ord`] for `K` does not implement a [total order]. /// /// # Examples /// /// ``` - /// let mut v = [-5i32, 4, 1, -3, 2]; + /// let mut v = [4i32, -5, 1, -3, 2]; /// /// v.sort_unstable_by_key(|k| k.abs()); - /// assert!(v == [1, 2, -3, 4, -5]); + /// assert_eq!(v, [1, 2, -3, 4, -5]); /// ``` /// /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[stable(feature = "sort_unstable", since = "1.20.0")] #[inline] pub fn sort_unstable_by_key<K, F>(&mut self, mut f: F) @@ -3038,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. - /// /// [`sort_unstable`]: slice::sort_unstable /// /// # Panics /// /// Panics when `index >= len()`, meaning it always panics on empty slices. /// + /// May panic if the implementation of [`Ord`] for `T` does not implement a [total order]. + /// /// # Examples /// /// ``` @@ -3069,6 +3078,7 @@ impl<T> [T] { /// ``` /// /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[stable(feature = "slice_select_nth_unstable", since = "1.49.0")] #[inline] pub fn select_nth_unstable(&mut self, index: usize) -> (&mut [T], &mut T, &mut [T]) @@ -3099,15 +3109,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 /// /// ``` @@ -3130,6 +3139,7 @@ impl<T> [T] { /// ``` /// /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[stable(feature = "slice_select_nth_unstable", since = "1.49.0")] #[inline] pub fn select_nth_unstable_by<F>( @@ -3164,15 +3174,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 /// /// ``` @@ -3195,6 +3204,7 @@ impl<T> [T] { /// ``` /// /// [ipnsort]: https://github.com/Voultapher/sort-research-rs/tree/main/ipnsort + /// [total order]: https://en.wikipedia.org/wiki/Total_order #[stable(feature = "slice_select_nth_unstable", since = "1.49.0")] #[inline] pub fn select_nth_unstable_by_key<K, F>( diff --git a/library/core/src/slice/sort/shared/smallsort.rs b/library/core/src/slice/sort/shared/smallsort.rs index 5064c5a0ae5..db0c5c72822 100644 --- a/library/core/src/slice/sort/shared/smallsort.rs +++ b/library/core/src/slice/sort/shared/smallsort.rs @@ -831,9 +831,9 @@ 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 function fails to implement a strict weak ordering. In that case + // we panic and never access the inconsistent state in dst. if left != left_end || right != right_end { panic_on_ord_violation(); } @@ -842,7 +842,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 panicking we inform the user, that they have a logic bug in their program. If a strict + // weak ordering is not given, the concept of comparison based sorting cannot yield a sorted + // result. E.g.: a < b < c < a + // + // 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 + // 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"); } #[must_use] diff --git a/library/core/src/slice/sort/unstable/mod.rs b/library/core/src/slice/sort/unstable/mod.rs index ed735e1ebfb..932e01f4401 100644 --- a/library/core/src/slice/sort/unstable/mod.rs +++ b/library/core/src/slice/sort/unstable/mod.rs @@ -8,7 +8,7 @@ use crate::slice::sort::shared::smallsort::insertion_sort_shift_left; pub(crate) mod heapsort; pub(crate) mod quicksort; -/// Unstable sort called ipnsort by Lukas Bergdoll. +/// Unstable sort called ipnsort by Lukas Bergdoll and Orson Peters. /// Design document: /// <https://github.com/Voultapher/sort-research-rs/blob/main/writeup/ipnsort_introduction/text.md> /// |
