Tracking issue for RFC 2351, "Add is_sorted to the standard library"

[Centril]

Feature gate: #![feature(is_sorted)]

This is a tracking issue for is_sorted{_by,_by_key} functions on [T] and Iterator (rust-lang/rfcs#2351).

Public API

impl<T> [T] {
    pub fn is_sorted(&self) -> bool
    where
        T: PartialOrd;

    pub fn is_sorted_by<'a, F>(&'a self, mut compare: F) -> bool
    where
        F: FnMut(&'a T, &'a T) -> bool;

    pub fn is_sorted_by_key<'a, F, K>(&'a self, f: F) -> bool
    where
        F: FnMut(&'a T) -> K,
        K: PartialOrd;
}

// core::iter

pub trait Iterator {
    // all the other methods omitted

    fn is_sorted(self) -> bool
    where
        Self: Sized,
        Self::Item: PartialOrd;

    fn is_sorted_by<F>(mut self, compare: F) -> bool
    where
        Self: Sized,
        F: FnMut(&Self::Item, &Self::Item) -> bool;

    fn is_sorted_by_key<F, K>(self, f: F) -> bool
    where
        Self: Sized,
        F: FnMut(Self::Item) -> K,
        K: PartialOrd;
}

Steps / History

• Implementation: Add is_sorted to Iterator and [T] #55045
• Only call the closure parameter of Iterator::is_sorted_by_key once per item #62473
• remove HRTB from [T]::is_sorted_by{,_key} #102977
• Add more comprehensive tests for is_sorted and friends #112699
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• Require Ord instead of only PartialOrd?
  • Answered by Make is_sorted[_by_key] require Ord instead of PartialOrd and remove Iterator::is_sorted_by_key #81382 (comment) (no, PartialOrd is the right bound)
• Should Iterator::is_sorted_by_key be added as well?
  • It was added in the original implementation PR
• Add std::cmp::is_sorted instead?
• Should is_sorted_by take a closure returning bool instead of Option<Ordering>?
  • Added in Use bool instead of PartiolOrd as return value of the comparison closure in {slice,Iteraotr}::is_sorted_by #118811

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[LukasKalbertodt]

@gnzlbg Would you be interested in implementing this? You basically already wrote the code. I think it would be fancy to have this SIMD code in the standard library. (Or should we start with only the fully generic algorithm?). PS: Sorry for not having reviewed #53386 yet -- I hope I'll find the time and mental capacity to do that in the near future!

@Centril The links for the unresolved questions are dead (still pointing to the 0000 file). Could you or anyone update them? :)

---

Regarding the "Should Iterator::is_sorted_by_key be added as well?" question: this last minute comment in the RFC thread argues that this method should be added:

I feel like it is not a question about how easy it is to do it yourself using map but more about completeness of the api.

And people seem to agree. So I'd also say that this method should be added.

[Centril]

@LukasKalbertodt done :)

[Centril]

cc @SimonSapin re. the unresolved questions (particularly around is_sorted_by_key).

[gnzlbg]

I don't know when I can work on this, but I can mentor anyone wanting to implement this or wanting to port the implementation of the is_sorted crate.

[SimonSapin]

Oh, I didn’t realized that Iterator::is_sorted_by_key was not part of the RFC when I proposed FCP. I thought there was consensus to include it for consistence, both internal and with the existing sort_by_key/min_by_key/max_by_key methods which are also redundant with their respective method like sort_by.

We do have precedent of adding some APIs that can be expressed as a terse combination of existing APIs: #49098

@clarcharr, you expressed the opposite in rust-lang/rfcs#2351 (comment). Do you still think it should not be included?

[clarfonthey]

At the time I made that comment, I didn't realise that max_by_key and min_by_key were already stable (as of 1.6, which is a while ago). Basically, there are two options:

1. Deprecate max_by_key and min_by_key and don't add is_sorted_by_key.
2. Add is_sorted_by_key.

I feel like the usefulness of these should be re-evaluated, as it forces the key to outlive any reference to self rather than just the current reference to self. Basically, we'd need something similar to GAT for it to be useful, and map already exists.

Personally, I'd prefer option 1. But option 2 is also acceptable and there may be a way to make these functions work with GAT in the future.

[gnzlbg]

@SimonSapin would you like to be the reviewer of a PR implementing this ? If so, I'd like to discuss with somebody first how the is_sorted crate is structure, and how to best implement this functionality in the std library.

[gnzlbg]

So I don't know how to properly implement this in core.

The is_sorted functionality should use intrinsics when available at run-time. The iter and slice modules in core do not have access to run-time detection (that requires std), but e.g. the Iterator trait exported by core and std has to be the same, so we can't override the core implementation in std to add run-time detection on top.

After talking with @SimonSapin on IRC, I am just going to add the naive versions to core which get picked up by std and call it a day. Those who need more performance can just use is_sorted from crates.io. Adding vectorized versions of the is_sorted functionality isn't worth it either, because one would need to re-compile core to profit from it (using crates.io is just easier). Maybe if we get newer targets in the future with more features enabled this might become worth it.

[kleimkuhler]

I have interest in working on this RFC. This would be a significant learning experience for me, but I don't see any reason why I should not be able to complete it.

@gnzlbg If your offer for being available as a mentor is still open, I'd like to take you up on that!

If there is any concern in timeline (I should be able to begin implementing this week) or preference for someone who may have more experience, I have no problem with that; just let me know!

[gnzlbg]

An efficient implementation of this is probably blocked on rust-lang/rfcs#2492 (e.g. see rust-lang/rfcs#2492 (comment)) , since until then we cannot really use SIMD in libcore effectively.

My recommendation is that if you want to implement this now, you should just adapt rust-itertools/itertools#261 which is a straight-forward plain Rust implementation. Maybe with some tweaks one can get it to auto-vectorize in some cases. This would allow us to get experience with the API until the other problems are solved.

[kleimkuhler]

Thanks for the links; it makes sense that a more efficient implementation is blocked on that right now.

To clarify on my initial comment (and what you picked up on in your last sentence), is the "learning" part of this would be more related to familiarizing myself with the API and codebase.

I think an initial implementation based off rust-itertools/itertools#261 could be a good first step and we'll keep an eye on the progress of rust-lang/rfcs#2492?

[kleimkuhler]

@gnzlbg You'll find my initial implementation in the PR above (#55045). I'm not sure who should be officially reviewing this, but I can update the PR if need be!

[kleimkuhler]

@SimonSapin It was asked a few comments up if you'd like to be the reviewer of this PR? I still need to assign someone to review it, but I'm just not sure who.

[mtak-]

This has broken the is_sorted crate on nightly. Is the only workaround to turn the feature flag on or use an older nightly?

   --> ~/.cargo/registry/src/github.com-1ecc6299db9ec823/is_sorted-0.1.0/src/lib.rs:119:31
    |
119 |         self.map(|v| key(&v)).is_sorted()
    |                               ^^^^^^^^^
    |
    = help: add #![feature(is_sorted)] to the crate attributes to enable

[clarfonthey]

Call IsSorted::is_sorted directly.

[mtak-]

@clarcharr The above error happens when adding is_sorted as a dependency (without using it). The is_sorted crate itself no longer builds.

[CryZe]

You can send a PR to the is_sorted crate and use the patch section in your Cargo.toml in the meantime until it is merged and published.

[gnzlbg]

@mtak- thank you for the PR, i've merged it already, will fix the doc tests and do a release so that the world keeps going.

The collision between the is_sorted crate and this PR was expected, I guess I could have fixed that a priori. Sorry about that. Keep in mind that if you are using the is_sorted crate, if you don't disambiguate your code the implementations of libcore will be picked after a re-compile.

[BurntSushi]

My first reaction to these APIs was to find it odd that Ordering wasn't used. I immediately wondered why it was inconsistent with the existing sort_by APIs. So I read up and found this:

• Using bool rather than Option<Ordering> for the return value of the closure to is_sorted_by seems clearer and more flexible. E.g. then you can use a < b or a <= b depending on whether it should be strictly increasing or not, etc.

The Option<Ordering> made me realize that the is_sorted_by requires PartialOrd and not Ord. Indeed, if we want to use PartialOrd here, then returning an Option<Ordering> is pretty annoying. But this bugs me a little bit, because AFAIK, we use Ordering everywhere in the standard library whenever we do anything oriented around comparisons. Is this the first API where we shed that and use bool instead? And I don't think there's anything in the name of these functions that points callers toward what the bool is supposed to mean. Is it less than? Or greater than? ¯_(ツ)_/¯

If we used Ord instead of PartialOrd, then my feeling is that returning Ordering is ergonomic enough to go with it instead of a bool here. But I assume the main practical benefit of using PartialOrd is that this will work on floating point types. But I'm not sure that's the right trade-off given the existence of f64::total_cmp.

What pieces am I missing here? Is there something more definitive pushing us toward PartialOrd here?

[Amanieu]

If you have PartialOrd types then you should probably be using is_sorted or is_sorted_by_key. is_sorted_by is intended for cases where you want full control to answer the question "are there 2 elements in order?". It makes sense to return a bool here.

[robjtede]

On is_sorted_by:

And I don't think there's anything in the name of these functions that points callers toward what the bool is supposed to mean.

I read it as "is it sorted how you want", asc, desc, in a circle, etc.

I agree the API inconsistency should be explicitly addressed as ok or not.

Addendum (I know tracking tickets aren't the right place for this):

Leaving the meaning blank actually feels more flexible; you get to use .partial_cmp or .cmp in your closure and give it readable semantics rather than the tricky-to-skim-read Ordering return value (IMO just as hard as -1,0,+1 returns in the JS' optional Array.sort argument). E.g., returning Ordering::Greater for lower numbers when trying to sort in a descending fashion is weird.

[BurntSushi]

@rfcbot concern inconsistent-with-sort

I'm registering this concern because I'm not a fan of the inconsistency between these APIs and our existing sort APIs. I'm not saying they have to be consistent, but I don't think I'm seeing a compelling justification for doing something different here.

If you have PartialOrd types then you should probably be using is_sorted or is_sorted_by_key. is_sorted_by is intended for cases where you want full control to answer the question "are there 2 elements in order?". It makes sense to return a bool here.

Yeah but as #34162 demonstrates, you often need to use is_sorted_by because is_sorted_by_key doesn't work due to a limitation in its type signature and/or Rust's expressiveness (depending on how you view it I suppose). This happens a lot in practice for me where the natural thing to use is indeed sort_by_key, but I can't because of the lifetime issue. So I end up needing to use sort_by. I assume the same will be true for is_sorted_by and is_sorted_by_key.

I read it as "is it sorted how you want", asc, desc, in a circle, etc.

I guess this makes sense. It wasn't what I saw on first impression, but I suppose that can be fixed with docs.

Leaving the meaning blank actually feels more flexible; you get to use .partial_cmp or .cmp in your closure and give it readable semantics rather than the tricky-to-skim-read Ordering return value (IMO just as hard as -1,0,+1 returns in the JS' optional Array.sort argument). E.g., returning Ordering::Greater for lower numbers when trying to sort in a descending fashion is weird.

I don't think I want to get into a "what's easier to read as a comparator, bool or Ordering" discussion here. Rust's standard library uses the latter pretty much everywhere as far as I can tell whenever comparisons come up. Why are we doing it differently here? Are we going to commit to the abdication of Ordering in favor of bool for any future comparator oriented APIs?

I think I overall need this inconsistency spelled out a bit more for me. Like I said, I'm not trying to say we should be consistent for consistency's sake. ("A foolish consistency is the hobgoblin of little minds.") But that consistency has value and I'm not sure we're gaining enough to justify throwing away that value here.

[joshtriplett]

I do personally think that the function returning bool is easier to write (you can write a closure that just uses <= or <, rather than having to translate to an Ordering), but I think the consistency is important as well.

Suggestion: could we go ahead and do an FCP to stabilize is_sorted and is_sorted_by_key, which aren't blocked by continuing to debate either the name or signature of is_sorted_by?

[joshtriplett]

Nominating for us to sort this out.

[BurntSushi]

Suggestion: could we go ahead and do an FCP to stabilize is_sorted and is_sorted_by_key, which aren't blocked by continuing to debate either the name or signature of is_sorted_by?

This could lead to a frustrating state of affairs ("I can almost use is_sorted_by_key but actually need is_sorted_by which isn't stable") but I'm fine with it.

[m-ou-se]

I don't think I want to get into a "what's easier to read as a comparator, bool or Ordering" discussion here. Rust's standard library uses the latter pretty much everywhere as far as I can tell whenever comparisons come up. Why are we doing it differently here? Are we going to commit to the abdication of Ordering in favor of bool for any future comparator oriented APIs?

Responding to the "why are we doing it differently here?" part:

We use Ordering or Option<Ordering> when all three/four possibilities (less, equal, greater, unordered) have some meaning. In this case, we only check whether two elements are "sorted" yes or no, which is a boolean decision. If we used a Ordering API here, then switching between < and <= gets really awkward, having to swap Less for Equal or the other way around..

[m-ou-se]

Perhaps we should instead call it .pairwise().all(|a, b| a > b) rather than .is_sorted_by(|a, b| a > b), to avoid the inconsistency. (But the return type of pairwise for Iterator is a good question. It's only obvious for slices.)

That might hurt discoverability, but it also might make it much clearer that this is useful for other (non sorting) use cases too.

[Amanieu]

Additionally, there is precedent for comparison closures returning a bool, such as Vec::dedup_by and [T]::partition_point. We discussed this again at the @rust-lang/libs-api meeting and everyone there agreed that returning bool in this case is better overall than returning an Ordering for the reasons previously listed.

[SimonSapin]

We already have slice.array_windows().all(|[a, b]| a <= b) but that’s even less discoverable

[BurntSushi]

In this case, we only check whether two elements are "sorted" yes or no, which is a boolean decision.

I found this phrasing compelling. Thank you!

@rfcbot resolve inconsistent-with-sort

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[ahakkar]

As a late note, I find it greatly infuriating that sorted_by requires a F: FnMut(&T, &T) -> Ordering, while is_sorted_by requires a F: FnMut(&'a T, &'a T) -> bool

It makes no sense at all for the "same" functions to require completely different input. Similar functions should definitely always require similar input for consistency.

Edit: I forgot to mention that at least it can be circumvented by using != Ordering::Greater as a kludge, so at least you don't have to implement multiple comparators for the essentially same functionality.