Add Step::forward/backward_overflowing to enable RangeInclusive loop optimizations

[pitaj]

View all comments

ACP: rust-lang/libs-team#767

This adds new required methods to the Step trait:

trait Step: ... {
    // ... existing functions

    // New required functions
    fn forward_overflowing(start: Self, count: usize) -> (Self, bool);
    fn backward_overflowing(start: Self, count: usize) -> (Self, bool);
}

It was found that using these to implement RangeInclusive's Iterator impl enabled optimizations previously only applicable to the exclusive-ended Range.

This required changing how "exhaustion" works for RangeInclusive. I've nominated this for libs-api discussion because of one insta-stable change:

The new implementations now only set exhausted when overflow occurs, and start is now advanced past end otherwise. I doubt anyone depends on the prior behavior, but it's probably worth a crater run.

The exhaustion changes also affect Debug but my understanding is that debug formatting is never guaranteed stable.

I have now changed the nth impls to use the new functions as well.

r? libs

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting.

The PartialEq implementation as proposed violates the reflexivity requirement of the Eq trait which states that a == a must always be true. Instead, could equality be computed by ignoring the start field if the range is exhausted? This would treat the combination of start and exhausted as an Option<Idx> for the purposes of comparison.

If the PartialEq implementation can be fixed, we are happy to accept this after a crater run.

[pitaj]

Right, I forgot this implements Eq even though I implemented it manually 🤦‍♂️. exhausted.then_some(self.start) could work though it's less meaningful if the range was exhausted by reverse iteration.

@Amanieu I'm not sure it gains much over just going back to the derive, would that be acceptable?

[Amanieu]

RangeInclusive doesn't support reverse iteration.

The derive is also acceptable. We explicitly document that start has an unspecified value after exhaustion, so it makes sense for equality to also be unspecified.

[pitaj]

RangeInclusive doesn't support reverse iteration.

It implements DoubleEndedIterator but regardless yeah I'll change it back to the derive

[pitaj]

@rustbot ready

[theemathas]

I found a bug with this PR.

fn main() {
    let mut range = 255_u8..=255_u8;
    let _ = range.next();
    println!("{range:?}");
    println!("{}", range.contains(&100_u8));
}

Output of above code on nightly:

255..=255 (exhausted)
false

Output of above code with this PR:

0..=255 (exhausted)
true

(The desired behavior of the RangeBounds methods also need to be figured out.)

[theemathas]

And here's some strange behavior that's possibly a bug:

fn main() {
    let mut range = 0_usize..=0_usize;
    let _ = range.next_back();
    println!("{range:?}");
    let data = [0; 1000];
    println!("{:?}", &data[range]);
}

Output of above code on nightly:

0..=0 (exhausted)
[]

Output of above code with this PR:

0..=18446744073709551615 (exhausted)

thread 'main' (188575) panicked at src/main.rs:6:27:
range end index 18446744073709551615 out of range for slice of length 1000
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[pitaj]

@theemathas

I found a bug with this PR.

Good find on contains, that should be fixed now. Added some tests as well.

And here's some strange behavior that's possibly a bug

This is expected behavior from the new overflowing behavior. Given that the values of start and end have always been unspecified when exhausted, the indexing behavior is also unspecified - so I don't think this is an issue.

(The desired behavior of the RangeBounds methods also need to be figured out.)

I think the end_bound change I just made should resolve this. Now end_bound will return Excluded(start) when exhausted, which ensures (start_bound, end_bound) is always empty. This is fine since the values are unspecified anyways. Excluded(end) no longer works since end is no longer held equal to start on exhaustion.

I also changed into_bounds to panic when converting if exhausted. This matches the behavior of the conversion to the new RangeInclusive type, and is the only viable way of implementing IntoBounds unrestricted with the change. IntoBounds is unstable, so this can still be changed.

Nominating for libs-api approval of those changes

[Amanieu]

This was discussed the @rust-lang/libs-api meeting. We're happy with the end_bound change. Let's get a crater run started.

@bors try

[jhpratt]

@bors r-

https://triage.rust-lang.org/gha-logs/rust-lang/rust/83976413749#L2026-06-29T04:53:02.5947801Z

---- library/core/src/range.rs - range::RangeInclusive<T>::from (line 422) stdout ----
---- library/core/src/range.rs - range::RangeInclusive<T>::from (line 422) stdout end ----
NOTE: test did not panic as expected at library/core/src/range.rs:422:0

failures:
    library/core/src/range.rs - range::RangeInclusive<T>::from (line 422)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#158538), which was unapproved.

View changes since this unapproval

[theemathas]

The failed test was added in #155421

[theemathas]

I believe that guaranteeing (as stated in #155421) that the impl<T> From<legacy::RangeInclusive<T>> for RangeInclusive<T> implementation always panics on exhausted iterators is impossible with this PR's implementation.

This is because, with this PR, checking whether a legacy::RangeInclusive has already been exhausted or not requires inspecting the T values. This cannot be done in the From impl, since there is no bounds such as T: PartialEq or anything similar.

Nominating to decide how to resolve this dilemma, possibly reverting #155421.

[pitaj]

This PR changed when RangeInclusive becomes exhausted (now happens only when the bound overflows). 0..=0.next() no longer exhausts the iterator. So all that needs to be done is change that doctest to exercise the new exhausted behavior, like so:

let mut exhausted: legacy::RangeInclusive<u8> = 255..=255;
exhausted.next();

So I don't think this needs to be looked at by libs-api again, there already okayed the exhausted change.

Your concern seems to be confusing the exhausted state with being empty.

[theemathas]

@pitaj I am interpreting "exhaust" to mean the operation of "calling .next() until the iterator has produced all elements it could ever produce". I think of it as a publicly visible property, not a private implementation detail stored in the field named exhausted. As an example of the term "exhaust" used in the way I think of it, see https://doc.rust-lang.org/1.96.0/src/core/iter/traits/iterator.rs.html#2076.

At the very least, I think we should let libs-api decide what "exhaust" in the documentation should mean.

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting. We think that the best solution here is to relax the guarantees around panicking. Instead of guaranteeing a panic if the range is exhausted, we would instead only guarantee that the result is either a panic or an empty range.

[theemathas]

Any ideas on how the doctests on the From impl should be changed?

cc @tbu-, the author of #155421, which added the doctest.

[tbu-]

Changing this to panic only sometimes feels like a serious footgun. Perhaps it could be changed to always return an empty range instead?

[Amanieu]

I don't think always returning an empty range is possible in a generic context.

I'm not too concerned. Realistically, nobody is going to ever call .into() on an exhausted iterator. The From impl is mainly going to be used immediately after a range is constructed.

[tbu-]

I don't think always returning an empty range is possible in a generic context.

True.

Any ideas on how the doctests on the From impl should be changed?

You can construct such a test with catch_unwind, I guess.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[pitaj]

@rustbot ready

[pitaj]

@Mark-Simulacrum

Looking through the implementation, it seems like it would be a good idea to have coverage directly targeting the conditions on forward/backward overflowing are met for each impl -- should be possible to do exhaustively for the smaller integer types. I think we have coverage indirectly so going to go ahead and approve, but if you have some time to follow up that would be good.

I added these - turns out it was a very good idea! Found a few copypaste bugs that slipped past us, which have now been fixed.

[theemathas]

I made a PR #158770, which only touches the From docs and doctest. It also updates the test in a different way. I think that it would be a good idea to have the From change as a separate change anyway, since it would need to be backported if it merges after the 1.98 beta branch promotion.

[tgross35]

Did you happen to test whether something along the lines of @matthieu-m's proposal at rust-lang/rfcs#3550 (comment) happens to get the desired optimization?

Sorry if this has been discussed already, I haven't been keeping up with this thread but noticed #45222 didn't seem to be linked anywhere.

[pitaj]

@tgross35

That optimization would only be applicable to the new RangeInclusive iterator, whereas this one applies to both legacy and new.