Make is_sorted[_by_key] require Ord instead of PartialOrd and remove Iterator::is_sorted_by_key

[LukasKalbertodt]

See #53485 and the RFC thread for discussion about this. One of the last comments by @KodrAus:

With #72568 I think an Ord bound is quite compelling.

And I agree. We should not longer block is_sorted in this discussion. It's better to have a stable is_sorted that doesn't work with PartialOrd + !Ord types than to have an forever unstable function. A few more arguments in favor of Ord:

• No more edges cases with non-comparable values that need to be explained in the docs.
• The is_sorted_by closure can now just return Ordering instead of Option<Ordering>.
• Ord is more in line with sort* methods.

This PR does not stabilize anything yet, but I hope that with this PR we can stabilize these methods soon.

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[m-ou-se]

Hm, which bound is more useful here is an interesting question. Ord seems tot make more sense at first, but I'm not conviced it's necessarily better than PartialOrd. The current documentation says:

    /// 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.

And this also makes sense to me. Partially orderable values can also be sorted, as long as every two consecutive elements are in the right order: [1, 2, 3] is sorted, [1, 2, NaN] is not.

The is_sorted_by closure can now just return Ordering instead of Option<Ordering>.

Making the return type of the is_sorted_by method simpler than Option<Ordering> sounds good, but #53485 (comment) makes a compelling argument for making it a bool instead, as Equal and Less are treated equally anyway.

Ord is more in line with sort* methods.

Ord already implies PartialOrd, so everything that you can .sort() you can also call .is_sorted() on. Making that promise the other way around might just be an unnecessary restriction.

[m-ou-se]

Oh and I forgot to add: I don't think this new total_cmp feature is a good alternative if is_sorted would require Ord, because it wouldn't do the same thing for NaNs. With total_cmp, [1, 2, NaN] might be considered sorted (depending on the type of NaN), wheras I'd expect something containing NaN to never be considered sorted.

[LukasKalbertodt]

The is_sorted_by closure can now just return Ordering instead of Option<Ordering>.

Making the return type of the is_sorted_by method simpler than Option<Ordering> sounds good, but #53485 (comment) makes a compelling argument for making it a bool instead, as Equal and Less are treated equally anyway.

That's a good point. I totally forgot bool was brought up as return type. With bool, the method can be used in lots of other use-cases that don't have anything to do with sorting, as mentioned by the comment you linked. And then another name might be more appropriate, e.g. all_pairwise (should we then also add any_pairwise?) or the maximally verbose is_true_for_all_pairs. I think such a method would be useful, but in that case, is_sorted_by is a tough sell as it offers basically zero benefit over the more general method.

I also thought about whether const-generics and array can make an all_pairwise method unnecessary. E.g. something like iter.array_windows().any(|[a, b]| a <= b). A similar idea (tuple_windows) came up in the RFC thread as well. However, such a method would only be possible for Item: Clone iterators :/
A method like is_true_for_all_array_windows<const N: usize>(self, f: impl FnMut(&[Self::Item; N]) -> bool) would work without Clone bound.

[1, 2, NaN] might be considered sorted (depending on the type of NaN), wheras I'd expect something containing NaN to never be considered sorted.

I think I disagree and that there is no "correct" behavior here. There are useful behaviors, and the current way is_sorted works is a useful behavior in some situations, but the statement "[1, 2, NAN] is not sorted" is not an obvious fact to me. If I read some code that calls is_sorted in a float array, I would probably always have to look up the semantics for NaN.

There was some very in-depth discussion back in the RFC thread about the exact semantics of PartialOrd and its behavior with is_sorted. The RFC was merged without clear solution/decision, but with a "we will figure it out before stabilization" approach. In the end, I don't care much about Ord vs. PartialOrd for is_sorted, but I'd like to have is_sorted stabilized and as far as I can tell, we can still relax the bound from Ord to PartialOrd after stabilization. (Thinking of all strange backwards-compatibility hazard I know, at least I couldn't come up with a way that relaxation would be able to break any code.)

---

As an alternative way to stabilization, what do you think about this?

• Make Iterator::is_sorted and [T]::is_sorted required Ord instead of PartialOrd
• Either add Iterator::array_windows or Iterator::all_pairwise (or even the more general is_true_for_all_array_windows) to make is_sorted_by redundant. Or just let is_sorted_by's closure return bool.
• Remove Iterator::is_sorted_by_key: it can trivially be replaced by .map(...).is_sorted() and we might want to add a more powerful GAT-powered version of this later.
• Stabilize [T]::is_sorted, [T]::is_sorted_by_key and Iterator::is_sorted.

[m-ou-se]

That's a good point. I totally forgot bool was brought up as return type. With bool, the method can be used in lots of other use-cases that don't have anything to do with sorting, as mentioned by the comment you linked. And then another name might be more appropriate, e.g. all_pairwise (should we then also add any_pairwise?) or the maximally verbose is_true_for_all_pairs. I think such a method would be useful, but in that case, is_sorted_by is a tough sell as it offers basically zero benefit over the more general method.

Yeah, exactly. Maybe is_sorted_by is still the best name, as that's probably the most common use case. But that's a separate discussion. ^^

I also thought about whether const-generics and array can make an all_pairwise method unnecessary. E.g. something like iter.array_windows().any(|[a, b]| a <= b). A similar idea (tuple_windows) came up in the RFC thread as well. However, such a method would only be possible for Item: Clone iterators :/
A method like is_true_for_all_array_windows<const N: usize>(self, f: impl FnMut(&[Self::Item; N]) -> bool) would work without Clone bound.

Sounds like we should investigate this.

I think I disagree and that there is no "correct" behavior here. There are useful behaviors, and the current way is_sorted works is a useful behavior in some situations, but the statement "[1, 2, NAN] is not sorted" is not an obvious fact to me.

Yes, I agree that it's only one useful behaviour, and not necessarily the one you'd always want. But the point still stands that total_cmp will often not be the right replacement.

As an alternative way to stabilization, what do you think about this?

(...)

That sounds reasonable. We should make sure that a potential Ord -> PartialOrd change later is indeed non-breaking then, if we want to avoid that decision now.

[KodrAus]

We should make sure that a potential Ord -> PartialOrd change later is indeed non-breaking then

cc @rust-lang/libs nominating for discussion at our next meeting if we haven't convinced ourselves that this is breaking or not by then.

[KodrAus]

I think it should be non-breaking if all we're doing is simply "downgrading" a full Ord to a PartialOrd<Self>, since Ord is a sub-trait of PartialOrd<Self>.

[LukasKalbertodt]

Sounds like we should investigate this.

I created a Zulip stream about this.

---

I just force pushed so that this PR now removes Iterator::is_sorted_by_key and changes the bound from PartialOrd to Ord for is_sorted and [T]::is_sorted_by_key. The is_sorted_by methods are untouched.

If this PR is merged, I would create another PR stabilizing is_sorted for slices and iterators (maybe also [T]::is_sorted_by_key, but I'm not sure about that yet).

We should make sure that a potential Ord -> PartialOrd change later is indeed non-breaking

As I said, I couldn't come up with a scenario where something could break. The "sneaky backwards-compatibility breaking things" I considered were: inference breaking and deref coercion breaking. Both of these seem to deal with specific types though.

Consider: FooOrd and FooPartialOrd which both implement the traits in their names. Also, FooPartialOrd implements Deref<Target = FooOrd>. Now imagine vec![&FooPartialOrd].into_iter().is_sorted(). Luckily, Ord does not trigger a deref coercion here: this errors instead. Similarly, if we have another type Baz which impls Into<FooOrd> and Into<FooPartialOrd>. The strict Ord bound does not help inference here and the compiler says that type annotations are needed. See both examples on the playground.

Finally, I tried what would happen for something like vec![&0u32 as &dyn Ord].into_iter().is_sorted(). But Ord cannot be made into a trait object, so that approach doesn't cause any breakage as well.

So yeah, I'm pretty sure by now that changing Ord to PartialOrd in the future would be completely backwards-compatible. But I'm interested in what the libs team thinks.

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking rustc_traits v0.0.0 (/checkout/compiler/rustc_traits)
    Checking rustc_mir_build v0.0.0 (/checkout/compiler/rustc_mir_build)
    Checking rustc_passes v0.0.0 (/checkout/compiler/rustc_passes)
    Checking rustc_plugin_impl v0.0.0 (/checkout/compiler/rustc_plugin_impl)
error[E0599]: no method named `is_sorted_by_key` found for struct `FlatMap<std::option::Iter<'_, &rustc_hir::GenericArgs<'_>>, std::slice::Iter<'_, rustc_hir::GenericArg<'_>>, [closure@compiler/rustc_typeck/src/astconv/generics.rs:185:58: 185:97]>` in the current scope
   --> compiler/rustc_typeck/src/astconv/generics.rs:273:60
    |
273 | ...                   !args_iter.clone().is_sorted_by_key(|arg| match arg {
    |                                          ^^^^^^^^^^^^^^^^ method not found in `FlatMap<std::option::Iter<'_, &rustc_hir::GenericArgs<'_>>, std::slice::Iter<'_, rustc_hir::GenericArg<'_>>, [closure@compiler/rustc_typeck/src/astconv/generics.rs:185:58: 185:97]>`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: could not compile `rustc_typeck`
error: could not compile `rustc_typeck`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" " llvm max_level_info" "--manifest-path" "/checkout/compiler/rustc/Cargo.toml" "-p" "rustc-main" "-p" "rustc_driver" "-p" "rustc_span" "-p" "rustc_macros" "-p" "rustc_arena" "-p" "rustc_index" "-p" "rustc_hir_pretty" "-p" "rustc_data_structures" "-p" "rustc_graphviz" "-p" "rustc_errors" "-p" "rustc_lint_defs" "-p" "rustc_lint" "-p" "rustc_attr" "-p" "rustc_lexer" "-p" "rustc_parse_format" "-p" "rustc_trait_selection" "-p" "rustc_infer" "-p" "rustc_error_codes" "-p" "rustc_hir" "-p" "rustc_plugin_impl" "-p" "rustc_parse" "-p" "rustc_target" "-p" "rustc_serialize" "-p" "rustc_feature" "-p" "rustc_ast" "-p" "rustc_mir" "-p" "coverage_test_macros" "-p" "rustc_apfloat" "-p" "rustc_save_analysis" "-p" "rustc_ast_pretty" "-p" "rustc_session" "-p" "rustc_fs_util" "-p" "rustc_interface" "-p" "rustc_mir_build" "-p" "rustc_ast_lowering" "-p" "rustc_ast_passes" "-p" "rustc_passes" "-p" "rustc_traits" "-p" "rustc_codegen_llvm" "-p" "rustc_llvm" "-p" "rustc_privacy" "-p" "rustc_expand" "-p" "rustc_builtin_macros" "-p" "rustc_ty_utils" "-p" "rustc_symbol_mangling" "-p" "rustc_resolve" "-p" "rustc_incremental" "-p" "rustc_typeck" "-p" "rustc_middle" "-p" "rustc_type_ir" "-p" "rustc_query_system" "-p" "rustc_metadata" "-p" "rustc_codegen_ssa" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:02:30

[m-ou-se]

This was discussed in the library team meeting last week, in which we agreed that PartialOrd is the right bound for this function, as that matches what the <= operator does.

[LukasKalbertodt]

I see. So then I'd probably close this PR and open a PR that stabilizes [T]::is_sorted and Iterator::is_sorted as is. Does this sound good?

[LukasKalbertodt]

In any case, this PR has become useless now basically, so I will close it.

As far as I see it, the open question "Should is_sorted require Ord instead of PartialOrd?" is now answered by the libs team and the only thing blocking stabilization of is_sorted (not the by* variants) is "more tests". I might tackle that at some point, but probably not in the near future.