-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redesign the std::iter::Step trait #68807
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@@ -311,7 +549,13 @@ impl<A: Step> Iterator for ops::RangeFrom<A> { | |||
|
|||
#[inline] | |||
fn next(&mut self) -> Option<A> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RangeFrom
is a very tricky case (cc #25708, @ollie27, #62886 (comment)).
Because of the stable declaration of the type, it is physically impossible to store the e.g. 257 states for RangeFrom
to yield 0, 1, 2, ..., 254, 255, panic, ...
. The current implementation (and the one here) in debug mode yield 253, 254, panic, ...
and in release mode yield 254, 255, 0, 1, ...
. Mixing in nth
with next
calls makes it even more interesting: nth
always panics if it would overflow u8
, and when it should return 255, in release mode sets the range to 0..
, and in debug mode panics. So nth
is not consistent with n
calls to next
due to this behavior of only sometimes panicking when overflowing.
This PR shouldn't change the observable behavior of RangeFrom
, but it's worth discussing where it is desired to end up with RangeFrom
, as that will shape what behaviors Step
needs to provide. But no matter what we choose, n..
must behave differently from n..MAX
.
The four consistent potential solutions are:
- Unconditional saturation:
0, 1, ..., 254, 255, 255, ...
, - Unconditional wrapping:
0, 1, ..., 254, 255, 0, 1, ...
, - Unconditional panic "off-by-one":
0, 1, ..., 254, panic, ...
- Debug-conditional panic "off-by-one" or wrapping, applied identically to
nth
as tonext
, or - Through some amazing hack, add an extra bit of state to
RangeFrom
without breaking public API.
The last is impossible without drastically changing how struct initialization works in Rust, so is a no-go.
The unconditional "off-by-one" panic is consistent and simple, and is consistent with interpreting n..
as n..MAX
(not n..=MAX
). Still, this feels like an incorrect choice semantically to me. This also makes e.g. (0u8..).take(256)
not work, even though iteration is capped before overflow, requiring instead use of 0u8..=255u8
(which to be fair is actually less state, 2*u8
rather than u8+usize
).
Unconditional saturation, unconditional wrapping, and debug-conditional are "just" a choice of semantics, though the former two would break the debug-checked custom of Rust's integer types. However, it's actually a bit more complicated than that, because of nth
. successor_wrapping
, forward_wrapping
, and forward_saturating
aren't (efficiently) implementable by this minimum required API surface (of forward_checked(n)
and backward_checked(n)
). If we want either of these semantics, we'd have to add a batch operation for it to Step
in order to keep RangeFrom::nth
O(1).
Writing all of this out, I think I'm now leaning towards renaming forward
to forward_checked
, and adding a forward
that has the same "panic or wrap" semantics as successor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, equivalently, giving successor
(and friends) a step parameter as well.
Remove the unused replace_one and replace_zero, and rename methods to make Step about succ/pred operations. The primary api is still forward (succ_n) and backward (pred_n), but we provide stepwise methods for optimization purposes.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Regarding the use of chalk -- we are planning on upgrading to newer versions but there is some work required for that. I have been debating if we should simply remove the existing integration in the meantime, maybe this is a motivator. |
☔ The latest upstream changes (presumably #68835) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage: @CAD97 - can you address the merge conflicts? |
I think I'm just going to close this for now. It's still blocked on chalk being upgraded or removed, even if I do keep up with rebasing. I think once the irlo discussion of RangeFrom died down I'll come back with a fresh proposal that makes sure to support whatever consensus is there. |
Remove experimental chalk option As suggested by @nikomatsakis [here](#68807 (comment)). The current version of chalk used by the experimental `-Zchalk` flag is [v0.9.0, which is over a year old](https://crates.io/crates/chalk-engine). Since v0.9.0, chalk has seen [a lot of further development](rust-lang/chalk@41dfe13...master), and the intent is to eventually upgrade rustc to use a more recent chalk. However, it will take a decent chunk of effort to upgrade the current experimental chalk support, and it is currently [blocking at least some PRs](#68807) due to chalk:0.9.0's use of unstable features. So for the interim until the next chalk release and experimental rustc integration, we remove the chalk-specific code from rustc.
Rework the std::iter::Step trait Previous attempts: rust-lang#43127 rust-lang#62886 rust-lang#68807 Tracking issue: rust-lang#42168 This PR reworks the `Step` trait to be phrased in terms of the *successor* and *predecessor* operations. With this, `Step` hopefully has a consistent identity that can have a path towards stabilization. The proposed trait: ```rust /// Objects that have a notion of *successor* and *predecessor* operations. /// /// The *successor* operation moves towards values that compare greater. /// The *predecessor* operation moves towards values that compare lesser. /// /// # Safety /// /// This trait is `unsafe` because its implementation must be correct for /// the safety of `unsafe trait TrustedLen` implementations, and the results /// of using this trait can otherwise be trusted by `unsafe` code to be correct /// and fulful the listed obligations. pub unsafe trait Step: Clone + PartialOrd + Sized { /// Returns the number of *successor* steps required to get from `start` to `end`. /// /// Returns `None` if the number of steps would overflow `usize` /// (or is infinite, or if `end` would never be reached). /// /// # Invariants /// /// For any `a`, `b`, and `n`: /// /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::forward(&a, n) == Some(b)` /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::backward(&a, n) == Some(a)` /// * `steps_between(&a, &b) == Some(n)` only if `a <= b` /// * Corollary: `steps_between(&a, &b) == Some(0)` if and only if `a == b` /// * Note that `a <= b` does _not_ imply `steps_between(&a, &b) != None`; /// this is the case wheen it would require more than `usize::MAX` steps to get to `b` /// * `steps_between(&a, &b) == None` if `a > b` fn steps_between(start: &Self, end: &Self) -> Option<usize>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, m).and_then(|x| Step::forward_checked(x, n))` /// /// For any `a`, `n`, and `m` where `n + m` does not overflow: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, n + m)` /// /// For any `a` and `n`: /// /// * `Step::forward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::forward_checked(&x, 1))` /// * Corollary: `Step::forward_checked(&a, 0) == Some(a)` fn forward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::forward(Step::forward(a, n), m) == Step::forward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_checked(a, n) == Some(Step::forward(a, n))` /// * `Step::forward(a, n) == (0..n).fold(a, |x, _| Step::forward(x, 1))` /// * Corollary: `Step::forward(a, 0) == a` /// * `Step::forward(a, n) >= a` /// * `Step::backward(Step::forward(a, n), n) == a` fn forward(start: Self, count: usize) -> Self { Step::forward_checked(start, count).expect("overflow in `Step::forward`") } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `forward` or `forward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b > a`, it is safe to call `Step::forward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&a, &b) == Some(n)`, /// it is safe to call `Step::forward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_unchecked(a, n)` is equivalent to `Step::forward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn forward_unchecked(start: Self, count: usize) -> Self { Step::forward(start, count) } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == n.checked_add(m).and_then(|x| Step::backward_checked(a, x))` /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == try { Step::backward_checked(a, n.checked_add(m)?) }` /// /// For any `a` and `n`: /// /// * `Step::backward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::backward_checked(&x, 1))` /// * Corollary: `Step::backward_checked(&a, 0) == Some(a)` fn backward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::backward(Step::backward(a, n), m) == Step::backward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_checked(a, n) == Some(Step::backward(a, n))` /// * `Step::backward(a, n) == (0..n).fold(a, |x, _| Step::backward(x, 1))` /// * Corollary: `Step::backward(a, 0) == a` /// * `Step::backward(a, n) <= a` /// * `Step::forward(Step::backward(a, n), n) == a` fn backward(start: Self, count: usize) -> Self { Step::backward_checked(start, count).expect("overflow in `Step::backward`") } /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `backward` or `backward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b < a`, it is safe to call `Step::backward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&b, &a) == Some(n)`, /// it is safe to call `Step::backward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_unchecked(a, n)` is equivalent to `Step::backward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn backward_unchecked(start: Self, count: usize) -> Self { Step::backward(start, count) } } ``` Note that all of these are associated functions and not callable via method syntax; the calling syntax is always `Step::forward(start, n)`. This version of the trait additionally changes the stepping functions to talk their arguments by value. As opposed to previous attempts which provided a "step by one" method directly, this version of the trait only exposes "step by n". There are a few reasons for this: - `Range*`, the primary consumer of `Step`, assumes that the "step by n" operation is cheap. If a single step function is provided, it will be a lot more enticing to implement "step by n" as n repeated calls to "step by one". While this is not strictly incorrect, this behavior would be surprising for anyone used to using `Range<{primitive integer}>`. - With a trivial default impl, this can be easily added backwards-compatibly later. - The debug-wrapping "step by n" needs to exist for `RangeFrom` to be consistent between "step by n" and "step by one" operation. (Note: the behavior is not changed by this PR, but making the behavior consistent is made tenable by this PR.) Three "kinds" of step are provided: `_checked`, which returns an `Option` indicating attempted overflow; (unsuffixed), which provides "safe overflow" behavior (is allowed to panic, wrap, or saturate, depending on what is most convenient for a given type); and `_unchecked`, which is a version which assumes overflow does not happen. Review is appreciated to check that: - The invariants as described on the `Step` functions are enough to specify the "common sense" consistency for successor/predecessor. - Implementation of `Step` functions is correct in the face of overflow and the edges of representable integers. - Added tests of `Step` functions are asserting the correct behavior (and not just the implemented behavior).
Rework the std::iter::Step trait Previous attempts: rust-lang#43127 rust-lang#62886 rust-lang#68807 Tracking issue: rust-lang#42168 This PR reworks the `Step` trait to be phrased in terms of the *successor* and *predecessor* operations. With this, `Step` hopefully has a consistent identity that can have a path towards stabilization. The proposed trait: ```rust /// Objects that have a notion of *successor* and *predecessor* operations. /// /// The *successor* operation moves towards values that compare greater. /// The *predecessor* operation moves towards values that compare lesser. /// /// # Safety /// /// This trait is `unsafe` because its implementation must be correct for /// the safety of `unsafe trait TrustedLen` implementations, and the results /// of using this trait can otherwise be trusted by `unsafe` code to be correct /// and fulful the listed obligations. pub unsafe trait Step: Clone + PartialOrd + Sized { /// Returns the number of *successor* steps required to get from `start` to `end`. /// /// Returns `None` if the number of steps would overflow `usize` /// (or is infinite, or if `end` would never be reached). /// /// # Invariants /// /// For any `a`, `b`, and `n`: /// /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::forward(&a, n) == Some(b)` /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::backward(&a, n) == Some(a)` /// * `steps_between(&a, &b) == Some(n)` only if `a <= b` /// * Corollary: `steps_between(&a, &b) == Some(0)` if and only if `a == b` /// * Note that `a <= b` does _not_ imply `steps_between(&a, &b) != None`; /// this is the case wheen it would require more than `usize::MAX` steps to get to `b` /// * `steps_between(&a, &b) == None` if `a > b` fn steps_between(start: &Self, end: &Self) -> Option<usize>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, m).and_then(|x| Step::forward_checked(x, n))` /// /// For any `a`, `n`, and `m` where `n + m` does not overflow: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, n + m)` /// /// For any `a` and `n`: /// /// * `Step::forward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::forward_checked(&x, 1))` /// * Corollary: `Step::forward_checked(&a, 0) == Some(a)` fn forward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::forward(Step::forward(a, n), m) == Step::forward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_checked(a, n) == Some(Step::forward(a, n))` /// * `Step::forward(a, n) == (0..n).fold(a, |x, _| Step::forward(x, 1))` /// * Corollary: `Step::forward(a, 0) == a` /// * `Step::forward(a, n) >= a` /// * `Step::backward(Step::forward(a, n), n) == a` fn forward(start: Self, count: usize) -> Self { Step::forward_checked(start, count).expect("overflow in `Step::forward`") } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `forward` or `forward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b > a`, it is safe to call `Step::forward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&a, &b) == Some(n)`, /// it is safe to call `Step::forward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_unchecked(a, n)` is equivalent to `Step::forward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn forward_unchecked(start: Self, count: usize) -> Self { Step::forward(start, count) } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == n.checked_add(m).and_then(|x| Step::backward_checked(a, x))` /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == try { Step::backward_checked(a, n.checked_add(m)?) }` /// /// For any `a` and `n`: /// /// * `Step::backward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::backward_checked(&x, 1))` /// * Corollary: `Step::backward_checked(&a, 0) == Some(a)` fn backward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::backward(Step::backward(a, n), m) == Step::backward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_checked(a, n) == Some(Step::backward(a, n))` /// * `Step::backward(a, n) == (0..n).fold(a, |x, _| Step::backward(x, 1))` /// * Corollary: `Step::backward(a, 0) == a` /// * `Step::backward(a, n) <= a` /// * `Step::forward(Step::backward(a, n), n) == a` fn backward(start: Self, count: usize) -> Self { Step::backward_checked(start, count).expect("overflow in `Step::backward`") } /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `backward` or `backward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b < a`, it is safe to call `Step::backward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&b, &a) == Some(n)`, /// it is safe to call `Step::backward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_unchecked(a, n)` is equivalent to `Step::backward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn backward_unchecked(start: Self, count: usize) -> Self { Step::backward(start, count) } } ``` Note that all of these are associated functions and not callable via method syntax; the calling syntax is always `Step::forward(start, n)`. This version of the trait additionally changes the stepping functions to talk their arguments by value. As opposed to previous attempts which provided a "step by one" method directly, this version of the trait only exposes "step by n". There are a few reasons for this: - `Range*`, the primary consumer of `Step`, assumes that the "step by n" operation is cheap. If a single step function is provided, it will be a lot more enticing to implement "step by n" as n repeated calls to "step by one". While this is not strictly incorrect, this behavior would be surprising for anyone used to using `Range<{primitive integer}>`. - With a trivial default impl, this can be easily added backwards-compatibly later. - The debug-wrapping "step by n" needs to exist for `RangeFrom` to be consistent between "step by n" and "step by one" operation. (Note: the behavior is not changed by this PR, but making the behavior consistent is made tenable by this PR.) Three "kinds" of step are provided: `_checked`, which returns an `Option` indicating attempted overflow; (unsuffixed), which provides "safe overflow" behavior (is allowed to panic, wrap, or saturate, depending on what is most convenient for a given type); and `_unchecked`, which is a version which assumes overflow does not happen. Review is appreciated to check that: - The invariants as described on the `Step` functions are enough to specify the "common sense" consistency for successor/predecessor. - Implementation of `Step` functions is correct in the face of overflow and the edges of representable integers. - Added tests of `Step` functions are asserting the correct behavior (and not just the implemented behavior).
Rework the std::iter::Step trait Previous attempts: rust-lang#43127 rust-lang#62886 rust-lang#68807 Tracking issue: rust-lang#42168 This PR reworks the `Step` trait to be phrased in terms of the *successor* and *predecessor* operations. With this, `Step` hopefully has a consistent identity that can have a path towards stabilization. The proposed trait: ```rust /// Objects that have a notion of *successor* and *predecessor* operations. /// /// The *successor* operation moves towards values that compare greater. /// The *predecessor* operation moves towards values that compare lesser. /// /// # Safety /// /// This trait is `unsafe` because its implementation must be correct for /// the safety of `unsafe trait TrustedLen` implementations, and the results /// of using this trait can otherwise be trusted by `unsafe` code to be correct /// and fulful the listed obligations. pub unsafe trait Step: Clone + PartialOrd + Sized { /// Returns the number of *successor* steps required to get from `start` to `end`. /// /// Returns `None` if the number of steps would overflow `usize` /// (or is infinite, or if `end` would never be reached). /// /// # Invariants /// /// For any `a`, `b`, and `n`: /// /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::forward(&a, n) == Some(b)` /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::backward(&a, n) == Some(a)` /// * `steps_between(&a, &b) == Some(n)` only if `a <= b` /// * Corollary: `steps_between(&a, &b) == Some(0)` if and only if `a == b` /// * Note that `a <= b` does _not_ imply `steps_between(&a, &b) != None`; /// this is the case wheen it would require more than `usize::MAX` steps to get to `b` /// * `steps_between(&a, &b) == None` if `a > b` fn steps_between(start: &Self, end: &Self) -> Option<usize>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, m).and_then(|x| Step::forward_checked(x, n))` /// /// For any `a`, `n`, and `m` where `n + m` does not overflow: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, n + m)` /// /// For any `a` and `n`: /// /// * `Step::forward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::forward_checked(&x, 1))` /// * Corollary: `Step::forward_checked(&a, 0) == Some(a)` fn forward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::forward(Step::forward(a, n), m) == Step::forward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_checked(a, n) == Some(Step::forward(a, n))` /// * `Step::forward(a, n) == (0..n).fold(a, |x, _| Step::forward(x, 1))` /// * Corollary: `Step::forward(a, 0) == a` /// * `Step::forward(a, n) >= a` /// * `Step::backward(Step::forward(a, n), n) == a` fn forward(start: Self, count: usize) -> Self { Step::forward_checked(start, count).expect("overflow in `Step::forward`") } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `forward` or `forward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b > a`, it is safe to call `Step::forward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&a, &b) == Some(n)`, /// it is safe to call `Step::forward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_unchecked(a, n)` is equivalent to `Step::forward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn forward_unchecked(start: Self, count: usize) -> Self { Step::forward(start, count) } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == n.checked_add(m).and_then(|x| Step::backward_checked(a, x))` /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == try { Step::backward_checked(a, n.checked_add(m)?) }` /// /// For any `a` and `n`: /// /// * `Step::backward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::backward_checked(&x, 1))` /// * Corollary: `Step::backward_checked(&a, 0) == Some(a)` fn backward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::backward(Step::backward(a, n), m) == Step::backward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_checked(a, n) == Some(Step::backward(a, n))` /// * `Step::backward(a, n) == (0..n).fold(a, |x, _| Step::backward(x, 1))` /// * Corollary: `Step::backward(a, 0) == a` /// * `Step::backward(a, n) <= a` /// * `Step::forward(Step::backward(a, n), n) == a` fn backward(start: Self, count: usize) -> Self { Step::backward_checked(start, count).expect("overflow in `Step::backward`") } /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `backward` or `backward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b < a`, it is safe to call `Step::backward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&b, &a) == Some(n)`, /// it is safe to call `Step::backward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_unchecked(a, n)` is equivalent to `Step::backward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn backward_unchecked(start: Self, count: usize) -> Self { Step::backward(start, count) } } ``` Note that all of these are associated functions and not callable via method syntax; the calling syntax is always `Step::forward(start, n)`. This version of the trait additionally changes the stepping functions to talk their arguments by value. As opposed to previous attempts which provided a "step by one" method directly, this version of the trait only exposes "step by n". There are a few reasons for this: - `Range*`, the primary consumer of `Step`, assumes that the "step by n" operation is cheap. If a single step function is provided, it will be a lot more enticing to implement "step by n" as n repeated calls to "step by one". While this is not strictly incorrect, this behavior would be surprising for anyone used to using `Range<{primitive integer}>`. - With a trivial default impl, this can be easily added backwards-compatibly later. - The debug-wrapping "step by n" needs to exist for `RangeFrom` to be consistent between "step by n" and "step by one" operation. (Note: the behavior is not changed by this PR, but making the behavior consistent is made tenable by this PR.) Three "kinds" of step are provided: `_checked`, which returns an `Option` indicating attempted overflow; (unsuffixed), which provides "safe overflow" behavior (is allowed to panic, wrap, or saturate, depending on what is most convenient for a given type); and `_unchecked`, which is a version which assumes overflow does not happen. Review is appreciated to check that: - The invariants as described on the `Step` functions are enough to specify the "common sense" consistency for successor/predecessor. - Implementation of `Step` functions is correct in the face of overflow and the edges of representable integers. - Added tests of `Step` functions are asserting the correct behavior (and not just the implemented behavior).
Rework the std::iter::Step trait Previous attempts: rust-lang#43127 rust-lang#62886 rust-lang#68807 Tracking issue: rust-lang#42168 This PR reworks the `Step` trait to be phrased in terms of the *successor* and *predecessor* operations. With this, `Step` hopefully has a consistent identity that can have a path towards stabilization. The proposed trait: ```rust /// Objects that have a notion of *successor* and *predecessor* operations. /// /// The *successor* operation moves towards values that compare greater. /// The *predecessor* operation moves towards values that compare lesser. /// /// # Safety /// /// This trait is `unsafe` because its implementation must be correct for /// the safety of `unsafe trait TrustedLen` implementations, and the results /// of using this trait can otherwise be trusted by `unsafe` code to be correct /// and fulful the listed obligations. pub unsafe trait Step: Clone + PartialOrd + Sized { /// Returns the number of *successor* steps required to get from `start` to `end`. /// /// Returns `None` if the number of steps would overflow `usize` /// (or is infinite, or if `end` would never be reached). /// /// # Invariants /// /// For any `a`, `b`, and `n`: /// /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::forward(&a, n) == Some(b)` /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::backward(&a, n) == Some(a)` /// * `steps_between(&a, &b) == Some(n)` only if `a <= b` /// * Corollary: `steps_between(&a, &b) == Some(0)` if and only if `a == b` /// * Note that `a <= b` does _not_ imply `steps_between(&a, &b) != None`; /// this is the case wheen it would require more than `usize::MAX` steps to get to `b` /// * `steps_between(&a, &b) == None` if `a > b` fn steps_between(start: &Self, end: &Self) -> Option<usize>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, m).and_then(|x| Step::forward_checked(x, n))` /// /// For any `a`, `n`, and `m` where `n + m` does not overflow: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, n + m)` /// /// For any `a` and `n`: /// /// * `Step::forward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::forward_checked(&x, 1))` /// * Corollary: `Step::forward_checked(&a, 0) == Some(a)` fn forward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::forward(Step::forward(a, n), m) == Step::forward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_checked(a, n) == Some(Step::forward(a, n))` /// * `Step::forward(a, n) == (0..n).fold(a, |x, _| Step::forward(x, 1))` /// * Corollary: `Step::forward(a, 0) == a` /// * `Step::forward(a, n) >= a` /// * `Step::backward(Step::forward(a, n), n) == a` fn forward(start: Self, count: usize) -> Self { Step::forward_checked(start, count).expect("overflow in `Step::forward`") } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `forward` or `forward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b > a`, it is safe to call `Step::forward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&a, &b) == Some(n)`, /// it is safe to call `Step::forward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_unchecked(a, n)` is equivalent to `Step::forward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn forward_unchecked(start: Self, count: usize) -> Self { Step::forward(start, count) } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == n.checked_add(m).and_then(|x| Step::backward_checked(a, x))` /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == try { Step::backward_checked(a, n.checked_add(m)?) }` /// /// For any `a` and `n`: /// /// * `Step::backward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::backward_checked(&x, 1))` /// * Corollary: `Step::backward_checked(&a, 0) == Some(a)` fn backward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::backward(Step::backward(a, n), m) == Step::backward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_checked(a, n) == Some(Step::backward(a, n))` /// * `Step::backward(a, n) == (0..n).fold(a, |x, _| Step::backward(x, 1))` /// * Corollary: `Step::backward(a, 0) == a` /// * `Step::backward(a, n) <= a` /// * `Step::forward(Step::backward(a, n), n) == a` fn backward(start: Self, count: usize) -> Self { Step::backward_checked(start, count).expect("overflow in `Step::backward`") } /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `backward` or `backward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b < a`, it is safe to call `Step::backward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&b, &a) == Some(n)`, /// it is safe to call `Step::backward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_unchecked(a, n)` is equivalent to `Step::backward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn backward_unchecked(start: Self, count: usize) -> Self { Step::backward(start, count) } } ``` Note that all of these are associated functions and not callable via method syntax; the calling syntax is always `Step::forward(start, n)`. This version of the trait additionally changes the stepping functions to talk their arguments by value. As opposed to previous attempts which provided a "step by one" method directly, this version of the trait only exposes "step by n". There are a few reasons for this: - `Range*`, the primary consumer of `Step`, assumes that the "step by n" operation is cheap. If a single step function is provided, it will be a lot more enticing to implement "step by n" as n repeated calls to "step by one". While this is not strictly incorrect, this behavior would be surprising for anyone used to using `Range<{primitive integer}>`. - With a trivial default impl, this can be easily added backwards-compatibly later. - The debug-wrapping "step by n" needs to exist for `RangeFrom` to be consistent between "step by n" and "step by one" operation. (Note: the behavior is not changed by this PR, but making the behavior consistent is made tenable by this PR.) Three "kinds" of step are provided: `_checked`, which returns an `Option` indicating attempted overflow; (unsuffixed), which provides "safe overflow" behavior (is allowed to panic, wrap, or saturate, depending on what is most convenient for a given type); and `_unchecked`, which is a version which assumes overflow does not happen. Review is appreciated to check that: - The invariants as described on the `Step` functions are enough to specify the "common sense" consistency for successor/predecessor. - Implementation of `Step` functions is correct in the face of overflow and the edges of representable integers. - Added tests of `Step` functions are asserting the correct behavior (and not just the implemented behavior).
CC #42168, @SimonSapin, r? @scottmcm. Revival of #43127, re-opened #62886 (with manual rebase and no longer a PR from my master).
This is still blocked on the use of published chalk 0.9.0, unfortunately. cc @rust-lang/wg-traits: is there some way we could get rustc updated to use a version of chalk that no longer implements
Step
? Should I create a minimal patch for published chalk 0.9.0 that uses the newStep
trait? Cherry pick the commit that stops usingStep
?The new
Step
trait is defined as follows:Most of the implementation is taken from @SimonSapin's original PR, just updated for current master. Again, arithmetic and overflow handling with multiple integer types of different widths and signedness is tricky, careful review would be appreciated. I had to change a few of of Simon's originally added tests to get them to pass, but I believe that their formulation is correct now and they were incorrect before.
This could use a perf test to see how it impacts performance. Hopefully, due to the fact that this version of the redesign really is pretty much just removing
replace_one
/replace_zero
and renaming the other functions to fit the description as successor/predecessor rather than add/sub, it shouldn't impact much. This also makes a potential impl to makeRangeInclusive<char>
work much more practical.