Redesign the std::iter::Step trait, tweak related iterator impls for ranges

[SimonSapin]

CC #42168

The trait is now:

/// Objects that have a notion of *successor* and *predecessor*
/// for the purpose of range iterators.
///
/// This trait is `unsafe` because implementations of the `unsafe` trait `TrustedLen`
/// depend on its implementations being correct.
#[unstable(feature = "step_trait",
           reason = "recently redesigned",
           issue = "42168")]
pub unsafe trait Step: Clone + PartialOrd + Sized {
    /// Returns the number of *successor* steps needed to get from `start` to `end`.
    ///
    /// Returns `None` if that number would overflow `usize`
    /// (or is infinite, if `end` would never be reached).
    ///
    /// This must hold for any `a`, `b`, and `n`:
    ///
    /// * `steps_between(&a, &b) == Some(0)` if and only if `a >= b`.
    /// * `steps_between(&a, &b) == Some(n)` if and only if `a.forward(n) == Some(b)`
    /// * `steps_between(&a, &b) == Some(n)` if and only if `b.backward(n) == Some(a)`
    fn steps_between(start: &Self, end: &Self) -> Option<usize>;

    /// Returns the value that would be obtained by taking the *successor* of `self`,
    /// `step_count` times.
    ///
    /// Returns `None` if this would overflow the range of values supported by the type `Self`.
    ///
    /// Note: `step_count == 1` is a common case,
    /// used for example in `Iterator::next` for ranges.
    ///
    /// This must hold for any `a`, `n`, and `m` where `n + m` doesn’t overflow:
    ///
    /// * `a.forward(n).and_then(|x| x.forward(m)) == a.forward(n + m)`
    fn forward(&self, step_count: usize) -> Option<Self>;

    /// Returns the value that would be obtained by taking the *predecessor* of `self`,
    /// `step_count` times.
    ///
    /// Returns `None` if this would overflow the range of values supported by the type `Self`.
    ///
    /// Note: `step_count == 1` is a common case,
    /// used for example in `Iterator::next_back` for ranges.
    ///
    /// This must hold for any `a`, `n`, and `m` where `n + m` doesn’t overflow:
    ///
    /// * `a.backward(n).and_then(|x| x.backward(m)) == a.backward(n + m)`
    fn backward(&self, step_count: usize) -> Option<Self>;
}

Arithmetic and overflow handling with multiple integer types of different widths and signedness is tricky, careful review would be appreciated.

Other changes:

• The unsafe trait TrustedLen is now implemented for ranges of all integer types all T: Step types. Step is now an unsafe trait as a consequence.
• Check for overflow everywhere. If it turns out the optimizer doesn’t manage to elide some of the checks that could be removed I’ll leave it to someone else to carefully optimize this, preferably without changing behavior. CC #43064
• Remove ExactSizeIterator impls for RangeInclusive<u16> and RangeInclusive<i16> as they are incorrect on 16-bit platforms. CC #43086 (comment)
• Don’t reset inclusive ranges to 1...0 after the last iteration. Instead increment or decrement similarly to other iterations. If that would cause overflow, decrement/increment the other bound instead.
• Return (usize::MAX, None) instead of (0, None) in size_hint when the exact result would overflow usize.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[SimonSapin]

https://travis-ci.org/rust-lang/rust/jobs/251678510 "env": "IMAGE=x86_64-gnu-llvm-3.7 ALLOW_PR=1 RUST_BACKTRACE=1"

error: linking with `cc` failed: exit code: 1
[…]
undefined reference to `core::option::expect_failed::h6f9984c44d33d89c

🤔

[kennytm]

Merging this as-is would make the situation in #43124 even worse — the speed of (0 .. u16::MAX).collect::<Vec<_>>() is reduced from ~110µs/iter to ~170µs/iter.

---

Edit: However, changing the implementation of forward() from:

                #[inline]
                fn forward(&self, n: usize) -> Option<Self> {
                    match Self::try_from(n) {
                        Ok(n_converted) => self.checked_add(n_converted),
                        Err(_) => None,  // if n is out of range, `something_unsigned + n` is too
                    }
                }

to:

    #[inline]
    fn forward(&self, n: usize) -> Option<Self> {
        if n <= u16::MAX as usize {
            self.checked_add(n as u16)
        } else {
            None
        }
    }

would immediately fix #43124 (speed becomes ~4µs/iter). I don't understand what LLVM is thinking 🤔

---

Edit²: Putting the TryFrom definition directly inside the user crate also fixes the issue. Some inlining magic?

[oyvindln]

I don't understand what LLVM is thinking

The impl for TryFrom casts the value to i/u128 to check if it's in range, maybe that is something that could cause issues as the 128-bit types are somewhat special.

[SimonSapin]

I think the current state of this PR is about the amount of effort I’m willing to put into this at the moment. If anyone would like to pick it up and push on the optimization front, feel free.

(I think going through 128-bit ints in TryFrom is unnecessary if we use separate code for size_of::<$Source>() < size_of::<$Destination>() vs size_of::<$Source>() > size_of::<$Destination>().)

[carols10cents]

o.O I'm not sure why travis is failing like this with linking problems: https://travis-ci.org/rust-lang/rust/jobs/251678510#L1044

I tried restarting that job to make sure it wasn't something intermittent but it just failed in the same way again...

do you have any ideas why this might be happening for this PR @SimonSapin ?

[ExpHP]

/// Returns the number of *successor* steps needed to get from `start` to `end`.
///
/// Returns `None` if that number would overflow `usize`
/// (or is infinite, if `end` would never be reached).
///
/// This must hold for any `a`, `b`, and `n`:
///
/// * `steps_between(&a, &b) == Some(0)` if and only if `a >= b`.
/// * `steps_between(&a, &b) == Some(n)` if and only if `a.forward(n) == Some(b)`
/// * `steps_between(&a, &b) == Some(n)` if and only if `b.backward(n) == Some(a)`

Surely this couId be better worded. I had to read this docstring very carefully to convince myself that it does indeed follow the typical rust semantics of half-open ranges, and that e.g. steps_between(&0, &n) == Some(n)

[SimonSapin]

@carols10cents On IRC @eddyb said it’s probably because intrinsics for u128 divison use a range iterator but are not allowed to link to panic code, and this PR adds calls to Option::expect in range iterator implementations. (Even if these "should" never panic, merely linking causes issues if that code is not optimized away.)

So it sounds like this needs more work to have correct overflow semantics without using panicking code.

@ExpHP I don’t mind rewriting this if you have a suggestion how. I avoided talking about addition and subtraction because Step can be implemented for things that are not necessarily numbers.

[ExpHP]

@ExpHP I don’t mind rewriting this if you have a suggestion how. I avoided talking about addition and subtraction because Step can be implemented for things that are not necessarily numbers.

I would suggest making reference to the "length of the range a..b," which should already be readily familiar to almost any reader, and also places the trait in context of where it is used. As far as I can tell, the only difference is that sometimes this function returns None.

[scottmcm]

Should steps_between perhaps instead return something like Result<Option<usize>, Unreachable>? Might make the documentation for the method easier to write, too.

[scottmcm]

Would it be worth a fn forward_one(&self) -> Option<Self> { self.forward(1) } (or call it successor, perhaps, like the comment here does) so the common case is overridable if needed? I suppose it's always addable later if needed.

[scottmcm]

IIRC this used to only panic in debug, right?

I like the change, but it might need to be called out in documentation somewhere.

[scottmcm]

Thanks for picking this up, @SimonSapin! The trait looks way better now.

Some general musings on the trait:

• steps_between is very similar to PartialOrd::cmp, and could be a superset. It might be interesting to remove the need for PartialOrd using that fact.
• Some of the range RFCs talked about ranges of slightly weird things like singly-linked list or DAG nodes which don't have computable or unique predecessors (respectively). Would it be worth splitting the trait in two, like iterator? (Now, I have no idea how to efficiently implement steps_between for those things, so it might be a moot point.)

[SimonSapin]

Some of the range RFCs talked about ranges of slightly weird things like singly-linked list or DAG nodes which don't have computable or unique predecessors (respectively). Would it be worth splitting the trait in two, like iterator?

So things that have a successor but no predecessor? Yes, this would require a trait separate from Step for the DoubleEndedIterator impl. But honestly this sounds so niche that I’m not convinced std should "go out of its way" to support it. Libraries with such types can define their own ranges and iterators.

[carols10cents]

friendly ping to keep this on your radar @SimonSapin !

[SimonSapin]

My understanding is that the main concern at this point is performance, and I wrote:

#43127 (comment)

I think the current state of this PR is about the amount of effort I’m willing to put into this at the moment. If anyone would like to pick it up and push on the optimization front, feel free.

As to the CI failure, it might be fixed by #43258. I’ve pushed a rebase to trigger a new build.

[SimonSapin]

Perf might be worth re-evaluating once #43248 or #43194 lands.

[alexcrichton]

ping @aturon for a review!

[SimonSapin]

This needs some more work that I’m not planning to do soon, closing to make that clear. Hopefully someone else can take over.

• In one Travis-CI configuration, "Building stage0 compiler artifacts" is failing at link-time with errors like undefined reference to `core::option::expect_failed::h6f9984c44d33d89c'. Eddyb said this might be because u128 division intrinsics use range iterators, and intrinsics are not allowed to link to panicking code. This PR introduces usage of Option::expect in range iterators, although for primitive integers types these should never panic (unless libcore is buggy). I think removing usage of Option::expect would require making the Step trait more complex, effectively undoing some of what this PR does. Maybe the intrinsics can be changed to not use range iterators instead?
• Several people have expressed concerns about performance. This needs benchmarking to see if this PR regresses it, maybe analyzing assembly code to figure out why and what can be done about it. If it turns out that overflow checking is responsible, consider how much overflow checking is desirable.