From cb32cfb729654560c9f1761db8902e78446d6cb8 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Sat, 30 Jan 2021 14:45:46 +0100 Subject: [PATCH 1/3] Remove `Iterator::is_sorted_by_key` This method can be trivially replaced by `iter.map(...).is_sorted()`. There has been some discussion about this, but the only reason for having that method that has been brought up is to have a more symmetry between the methods of `[T]` and `Iterator`. `[T]::is_sorted_by_key` is necessary, as there is not a simple replacement for it. But I don't think API symmetry is a strong enough argument for adding methods that are essentially superfluous or "a tiny bit convenient" at most. Finally, this method can always be added in the future. For the initial stabilization of the `is_sorted` API, it is not necessary. --- compiler/rustc_typeck/src/astconv/generics.rs | 17 ++++++----- library/core/src/iter/traits/iterator.rs | 28 ------------------- library/core/src/slice/mod.rs | 2 +- library/core/tests/iter/traits/iterator.rs | 2 -- .../feature-gates/feature-gate-is_sorted.rs | 2 -- .../feature-gate-is_sorted.stderr | 15 ++-------- 6 files changed, 14 insertions(+), 52 deletions(-) diff --git a/compiler/rustc_typeck/src/astconv/generics.rs b/compiler/rustc_typeck/src/astconv/generics.rs index 0797c95636260..b53edebc7c685 100644 --- a/compiler/rustc_typeck/src/astconv/generics.rs +++ b/compiler/rustc_typeck/src/astconv/generics.rs @@ -270,13 +270,16 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { tcx, arg, param, - !args_iter.clone().is_sorted_by_key(|arg| match arg { - GenericArg::Lifetime(_) => ParamKindOrd::Lifetime, - GenericArg::Type(_) => ParamKindOrd::Type, - GenericArg::Const(_) => ParamKindOrd::Const { - unordered: tcx.features().const_generics, - }, - }), + !args_iter + .clone() + .map(|arg| match arg { + GenericArg::Lifetime(_) => ParamKindOrd::Lifetime, + GenericArg::Type(_) => ParamKindOrd::Type, + GenericArg::Const(_) => ParamKindOrd::Const { + unordered: tcx.features().const_generics, + }, + }) + .is_sorted(), Some(&format!( "reorder the arguments: {}: `<{}>`", param_types_present diff --git a/library/core/src/iter/traits/iterator.rs b/library/core/src/iter/traits/iterator.rs index 9f7ced829b0ac..7f379df9e603e 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -3337,34 +3337,6 @@ pub trait Iterator { true } - /// Checks if the elements of this iterator are sorted using the given key extraction - /// function. - /// - /// Instead of comparing the iterator's elements directly, this function compares the keys of - /// the elements, as determined by `f`. Apart from that, it's equivalent to [`is_sorted`]; see - /// its documentation for more information. - /// - /// [`is_sorted`]: Iterator::is_sorted - /// - /// # Examples - /// - /// ``` - /// #![feature(is_sorted)] - /// - /// assert!(["c", "bb", "aaa"].iter().is_sorted_by_key(|s| s.len())); - /// assert!(![-2i32, -1, 0, 3].iter().is_sorted_by_key(|n| n.abs())); - /// ``` - #[inline] - #[unstable(feature = "is_sorted", reason = "new API", issue = "53485")] - fn is_sorted_by_key(self, f: F) -> bool - where - Self: Sized, - F: FnMut(Self::Item) -> K, - K: PartialOrd, - { - self.map(f).is_sorted() - } - /// See [TrustedRandomAccess] // The unusual name is to avoid name collisions in method resolution // see #76479. diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index b06b6e93373f3..bb889fe1d1c45 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -3385,7 +3385,7 @@ impl [T] { F: FnMut(&T) -> K, K: PartialOrd, { - self.iter().is_sorted_by_key(f) + self.iter().map(f).is_sorted() } /// Returns the index of the partition point according to the given predicate diff --git a/library/core/tests/iter/traits/iterator.rs b/library/core/tests/iter/traits/iterator.rs index 422e389e38017..228d7d711857e 100644 --- a/library/core/tests/iter/traits/iterator.rs +++ b/library/core/tests/iter/traits/iterator.rs @@ -375,9 +375,7 @@ fn test_is_sorted() { assert!(std::iter::empty::().is_sorted()); assert!(![0.0, 1.0, f32::NAN].iter().is_sorted()); assert!([-2, -1, 0, 3].iter().is_sorted()); - assert!(![-2i32, -1, 0, 3].iter().is_sorted_by_key(|n| n.abs())); assert!(!["c", "bb", "aaa"].iter().is_sorted()); - assert!(["c", "bb", "aaa"].iter().is_sorted_by_key(|s| s.len())); } #[test] diff --git a/src/test/ui/feature-gates/feature-gate-is_sorted.rs b/src/test/ui/feature-gates/feature-gate-is_sorted.rs index 359ed835bcbb2..b20062de1932d 100644 --- a/src/test/ui/feature-gates/feature-gate-is_sorted.rs +++ b/src/test/ui/feature-gates/feature-gate-is_sorted.rs @@ -2,8 +2,6 @@ fn main() { // Assert `Iterator` methods are unstable assert!([1, 2, 2, 9].iter().is_sorted()); //~^ ERROR: use of unstable library feature 'is_sorted': new API - assert!(![-2i32, -1, 0, 3].iter().is_sorted_by_key(|n| n.abs())); - //~^ ERROR: use of unstable library feature 'is_sorted': new API // Assert `[T]` methods are unstable assert!([1, 2, 2, 9].is_sorted()); diff --git a/src/test/ui/feature-gates/feature-gate-is_sorted.stderr b/src/test/ui/feature-gates/feature-gate-is_sorted.stderr index ccac827076bc8..2f2b89410da21 100644 --- a/src/test/ui/feature-gates/feature-gate-is_sorted.stderr +++ b/src/test/ui/feature-gates/feature-gate-is_sorted.stderr @@ -8,16 +8,7 @@ LL | assert!([1, 2, 2, 9].iter().is_sorted()); = help: add `#![feature(is_sorted)]` to the crate attributes to enable error[E0658]: use of unstable library feature 'is_sorted': new API - --> $DIR/feature-gate-is_sorted.rs:5:39 - | -LL | assert!(![-2i32, -1, 0, 3].iter().is_sorted_by_key(|n| n.abs())); - | ^^^^^^^^^^^^^^^^ - | - = note: see issue #53485 for more information - = help: add `#![feature(is_sorted)]` to the crate attributes to enable - -error[E0658]: use of unstable library feature 'is_sorted': new API - --> $DIR/feature-gate-is_sorted.rs:9:26 + --> $DIR/feature-gate-is_sorted.rs:7:26 | LL | assert!([1, 2, 2, 9].is_sorted()); | ^^^^^^^^^ @@ -26,7 +17,7 @@ LL | assert!([1, 2, 2, 9].is_sorted()); = help: add `#![feature(is_sorted)]` to the crate attributes to enable error[E0658]: use of unstable library feature 'is_sorted': new API - --> $DIR/feature-gate-is_sorted.rs:11:32 + --> $DIR/feature-gate-is_sorted.rs:9:32 | LL | assert!(![-2i32, -1, 0, 3].is_sorted_by_key(|n| n.abs())); | ^^^^^^^^^^^^^^^^ @@ -34,6 +25,6 @@ LL | assert!(![-2i32, -1, 0, 3].is_sorted_by_key(|n| n.abs())); = note: see issue #53485 for more information = help: add `#![feature(is_sorted)]` to the crate attributes to enable -error: aborting due to 4 previous errors +error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0658`. From 3c047a2e86795b0205a0ad80328c2a4d68268ae5 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Sat, 30 Jan 2021 14:50:14 +0100 Subject: [PATCH 2/3] Change `is_sorted[_by_key]` methods to require `Ord` instead of `PartialOrd` With `f32::total_cmp` and `f64::total_cmp`, the main reason for having an `PartialOrd` bound on these methods is mostly gone. By requiring `Ord`, we have no more edges cases with non-comparable values that need to be explained in the docs. These methods are now also in line with the `sort*` methods. Also note that `is_sorted` is mostly for convenience; if it is not powerful enough, there are more powerful tools that are only slightly less convenient. Finally, the trait bound can be relaxed in the future. --- library/core/src/iter/traits/iterator.rs | 8 ++------ library/core/src/slice/mod.rs | 10 +++------- library/core/tests/iter/traits/iterator.rs | 1 - library/core/tests/slice.rs | 1 - 4 files changed, 5 insertions(+), 15 deletions(-) diff --git a/library/core/src/iter/traits/iterator.rs b/library/core/src/iter/traits/iterator.rs index 7f379df9e603e..be54438b7c207 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -3272,9 +3272,6 @@ pub trait Iterator { /// That is, for each element `a` and its following element `b`, `a <= b` must hold. If the /// iterator yields exactly zero or one element, `true` is returned. /// - /// Note that if `Self::Item` is only `PartialOrd`, but not `Ord`, the above definition - /// implies that this function returns `false` if any two consecutive items are not - /// comparable. /// /// # Examples /// @@ -3285,16 +3282,15 @@ pub trait Iterator { /// assert!(![1, 3, 2, 4].iter().is_sorted()); /// assert!([0].iter().is_sorted()); /// assert!(std::iter::empty::().is_sorted()); - /// assert!(![0.0, 1.0, f32::NAN].iter().is_sorted()); /// ``` #[inline] #[unstable(feature = "is_sorted", reason = "new API", issue = "53485")] fn is_sorted(self) -> bool where Self: Sized, - Self::Item: PartialOrd, + Self::Item: Ord, { - self.is_sorted_by(PartialOrd::partial_cmp) + self.is_sorted_by(|a, b| Some(a.cmp(b))) } /// Checks if the elements of this iterator are sorted using the given comparator function. diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs index bb889fe1d1c45..63d1b7e54cf47 100644 --- a/library/core/src/slice/mod.rs +++ b/library/core/src/slice/mod.rs @@ -3322,9 +3322,6 @@ impl [T] { /// That is, for each element `a` and its following element `b`, `a <= b` must hold. If the /// slice yields exactly zero or one element, `true` is returned. /// - /// Note that if `Self::Item` is only `PartialOrd`, but not `Ord`, the above definition - /// implies that this function returns `false` if any two consecutive items are not - /// comparable. /// /// # Examples /// @@ -3336,15 +3333,14 @@ impl [T] { /// assert!(![1, 3, 2, 4].is_sorted()); /// assert!([0].is_sorted()); /// assert!(empty.is_sorted()); - /// assert!(![0.0, 1.0, f32::NAN].is_sorted()); /// ``` #[inline] #[unstable(feature = "is_sorted", reason = "new API", issue = "53485")] pub fn is_sorted(&self) -> bool where - T: PartialOrd, + T: Ord, { - self.is_sorted_by(|a, b| a.partial_cmp(b)) + self.is_sorted_by(|a, b| Some(a.cmp(b))) } /// Checks if the elements of this slice are sorted using the given comparator function. @@ -3383,7 +3379,7 @@ impl [T] { pub fn is_sorted_by_key(&self, f: F) -> bool where F: FnMut(&T) -> K, - K: PartialOrd, + K: Ord, { self.iter().map(f).is_sorted() } diff --git a/library/core/tests/iter/traits/iterator.rs b/library/core/tests/iter/traits/iterator.rs index 228d7d711857e..a09c02c7c2c70 100644 --- a/library/core/tests/iter/traits/iterator.rs +++ b/library/core/tests/iter/traits/iterator.rs @@ -373,7 +373,6 @@ fn test_is_sorted() { assert!(![1, 3, 2].iter().is_sorted()); assert!([0].iter().is_sorted()); assert!(std::iter::empty::().is_sorted()); - assert!(![0.0, 1.0, f32::NAN].iter().is_sorted()); assert!([-2, -1, 0, 3].iter().is_sorted()); assert!(!["c", "bb", "aaa"].iter().is_sorted()); } diff --git a/library/core/tests/slice.rs b/library/core/tests/slice.rs index 9ccc5a08dcbea..b493defe2ca38 100644 --- a/library/core/tests/slice.rs +++ b/library/core/tests/slice.rs @@ -2005,7 +2005,6 @@ fn test_is_sorted() { assert!(![1, 3, 2].is_sorted()); assert!([0].is_sorted()); assert!(empty.is_sorted()); - assert!(![0.0, 1.0, f32::NAN].is_sorted()); assert!([-2, -1, 0, 3].is_sorted()); assert!(![-2i32, -1, 0, 3].is_sorted_by_key(|n| n.abs())); assert!(!["c", "bb", "aaa"].is_sorted()); From 324d5e9fc1d10d50f54d3e4df210f88d97ea985a Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Mon, 25 Jan 2021 13:53:31 +0100 Subject: [PATCH 3/3] Slightly improve doc examples on `Iterator::is_sorted[_by]` --- library/core/src/iter/traits/iterator.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/core/src/iter/traits/iterator.rs b/library/core/src/iter/traits/iterator.rs index be54438b7c207..9965f58f752a9 100644 --- a/library/core/src/iter/traits/iterator.rs +++ b/library/core/src/iter/traits/iterator.rs @@ -3280,7 +3280,7 @@ pub trait Iterator { /// /// assert!([1, 2, 2, 9].iter().is_sorted()); /// assert!(![1, 3, 2, 4].iter().is_sorted()); - /// assert!([0].iter().is_sorted()); + /// assert!(std::iter::once("ferris").is_sorted()); /// assert!(std::iter::empty::().is_sorted()); /// ``` #[inline] @@ -3304,9 +3304,9 @@ pub trait Iterator { /// ``` /// #![feature(is_sorted)] /// - /// assert!([1, 2, 2, 9].iter().is_sorted_by(|a, b| a.partial_cmp(b))); - /// assert!(![1, 3, 2, 4].iter().is_sorted_by(|a, b| a.partial_cmp(b))); - /// assert!([0].iter().is_sorted_by(|a, b| a.partial_cmp(b))); + /// assert!([8, 5, 4, 1].iter().is_sorted_by(|a, b| a.partial_cmp(b).map(|o| o.reverse()))); + /// assert!(![1, 3, 2, 4].iter().is_sorted_by(|a, b| a.partial_cmp(b).map(|o| o.reverse()))); + /// assert!(std::iter::once("ferris").is_sorted_by(|a, b| a.partial_cmp(b))); /// assert!(std::iter::empty::().is_sorted_by(|a, b| a.partial_cmp(b))); /// assert!(![0.0, 1.0, f32::NAN].iter().is_sorted_by(|a, b| a.partial_cmp(b))); /// ```