New API: Range::cmp_scalar; comparison (less/equal/greater) to a primitive of the Range

[golddranks]

ACP: rust-lang/libs-team#115

This PR adds a method, cmp_scalar to Range, RangeFrom, RangeTo, RangeInclusive and RangeToInclusive.

The point of the method is to compare the range to a single Ord -comparable element of the range (I call it "scalar" here, because I think that makes sense for an element that implements Ord.); the method returns Ordering::{Less, Greater, Equal}, depending on whether the range is below or above the element, or contains it.

Justification

Simple and obvious operation to do

Given a range and a number (or anything that implements Ord), beyond the "is contained" check, comparing whether the number is above or below the range is a simple and obvious operation to do, and possibly the only operation that makes sense in this generic form of having a range of T: Ords.

Useful when searching amongst multiple ranges

The example given in the doctests gives a clear motivation for this API; a search through multiple ranges with a specified target:

let files = vec![
    File { seqnum: 0, range: 0..1000 },
    File { seqnum: 1, range: 1000..2200 },
    File { seqnum: 2, range: 2200..3900 },
    File { seqnum: 3, range: 3900..5000 },
];
let target = 1600;
let index = files.binary_search_by(|f| f.range.cmp_scalar(target))?;
assert_eq!(files[index].seqnum, 1);

Why compare a range to a scalar, and not scalar to range?

I.e. why i.e. (3..9).cmp_scalar(5) instead of 5.cmp_range(3..9)?

Originally, I thought that an API to compare a number to a range would make more sense intuitively, and set out to implement it. However, it turned out that there were two kinds of problems:

1. The order in the original motivation gets reversed, and reverse() must be called after this operation to fix it:
   files.binary_search_by(|f| target.cmp_range(f.range)).reverse()?;
   Of course, cmp_range would make sense, if searching for a single number among of many, that fits the range. However, I had hard time imagining a common case where this would be the requirement, whereas the motivation for cmp_scalar was clear from the case introduced above.

2. The implementation gets gnarly, as the API must take the range as a generic parameter, and the different kind of Ranges are different types. I tried to implement it as generic T: Into<RangeBounds>, but the ergonomics kind of sucked. Also, the problem of ranges not being Copy could be mitigated when the range is passed as the &self parameter because auto-ref works in that position.

Points for discussion

• Does the name cmp_scalar make sense? To me it kind of does, but I see no precedent with calling stuff "scalars" in the stdlib. I wonder if there are better alternatives.
• Ord vs PartialOrd? With contains, PartialOrd makes sense, but with a proper ordering returned by this API, I think that Ord is the only sensible choice.
• The method currently takes the scalar by copy. It might make sense to take it by reference to support textual ranges? That worsens the ergonomics for numbers though.
• Does it make sense to make it more generic (to support &str vs String comparisons, for example)? That might worsen the ergonomics because it makes type inference harder.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @thomcc

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

[thomcc]

Note: I'm not entirely sure what procedures adding new APIs require these days, but I have the impression that an RFC isn't require for simple and straightforward cases.

An ACP (API change proposal) is required. See https://std-dev-guide.rust-lang.org/feature-lifecycle/api-change-proposals.html.

@rustbot label +T-libs-api -T-libs +S-waiting-on-ACP

[golddranks]

Oh, there was the "API change proposal" thing. Thanks for the pointer. I'll send one later this week!

[golddranks]

I mentioned this in the ACP, but apparently PartialOrd supports having a different type for the RHS operand, which means that this functionality could be plausibly implemented as a PartialOrd impl. I checked only Ord before, and that requires the operands to be of the same type, so I didn't notice the added flexibility of PartialOrd before.

[bors]

☔ The latest upstream changes (presumably #102632) made this pull request unmergeable. Please resolve the merge conflicts.

[thomcc]

I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks.

r? libs

[Dylan-DPC]

@golddranks if you can resolve the conflicts, we can push this forward

also reässigning
r? thomcc

[golddranks]

I'm trying to reactivate on this!

[alex-semenyuk]

@golddranks
From wg-triage. Any chance to take a look this?

[alex-semenyuk]

@golddranks
From wg-triage. Closed this PR due to inactivity. Feel free to reopen or raised new one. Thanks for your efforts.