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

[theemathas]

Suggested change

	self.exhausted |= o;
	self.exhausted = o;

self.exhausted is always false before this line.

View changes since the review

[theemathas]

Suggested change

	self.exhausted |= on | o1;
	self.exhausted = on | o1;

Same thing here

View changes since the review

[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

[Amanieu]

FCP specifically for the changes to RangeInclusive exhaustion. This is allowed according to the docs, which say that the start and end of an inclusive range are unspecified after exhaustion. Everything else is backwards-compatible.

@rfcbot merge libs-api

[rust-rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @the8472

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[the8472]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 1c26bd2 (1c26bd2d5f051797bd192abf038975cfe05d3ce0, parent: 2aabf3ce0569dd7c8c2d2a27944850056dcf2566)

[rust-timer]

Finished benchmarking commit (1c26bd2): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-1.0%, -0.8%]	6
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.3%, secondary -0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

Results (secondary -3.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-3.2%, -3.2%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	4
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	4

Bootstrap: 510.527s -> 508.082s (-0.48%)
Artifact size: 400.06 MiB -> 400.12 MiB (0.02%)

[rust-rfcbot]

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

[rust-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.

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