Skip to content

Commit

Permalink
Auto merge of rust-lang#89916 - the8472:advance_by-avoid-err-0, r=dto…
Browse files Browse the repository at this point in the history
…lnay

Fix Iterator::advance_by contract inconsistency

The `advance_by(n)` docs state that in the error case `Err(k)` that k is always less than n.
It also states that `advance_by(0)` may return `Err(0)` to indicate an exhausted iterator.
These statements are inconsistent.
Since only one implementation (Skip) actually made use of that I changed it to return Ok(()) in that case too.

While adding some tests I also found a bug in `Take::advance_back_by`.
  • Loading branch information
bors committed Nov 27, 2021
2 parents 0881b3a + 3f9b26d commit 5fd3a5c
Show file tree
Hide file tree
Showing 11 changed files with 69 additions and 22 deletions.
3 changes: 3 additions & 0 deletions library/alloc/tests/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,9 @@ fn test_into_iter_advance_by() {

assert_eq!(i.advance_by(usize::MAX), Err(0));

i.advance_by(0).unwrap();
i.advance_back_by(0).unwrap();

assert_eq!(i.len(), 0);
}

Expand Down
4 changes: 2 additions & 2 deletions library/core/src/iter/adapters/skip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,8 @@ where
#[rustc_inherit_overflow_checks]
fn advance_by(&mut self, n: usize) -> Result<(), usize> {
let mut rem = n;

let step_one = self.n.saturating_add(rem);

match self.iter.advance_by(step_one) {
Ok(_) => {
rem -= step_one - self.n;
Expand All @@ -129,7 +129,7 @@ where
Err(advanced) => {
let advanced_without_skip = advanced.saturating_sub(self.n);
self.n = self.n.saturating_sub(advanced);
return Err(advanced_without_skip);
return if n == 0 { Ok(()) } else { Err(advanced_without_skip) };
}
}

Expand Down
29 changes: 15 additions & 14 deletions library/core/src/iter/adapters/take.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,21 +215,22 @@ where
}

#[inline]
#[rustc_inherit_overflow_checks]
fn advance_back_by(&mut self, n: usize) -> Result<(), usize> {
let inner_len = self.iter.len();
let len = self.n;
let remainder = len.saturating_sub(n);
let to_advance = inner_len - remainder;
match self.iter.advance_back_by(to_advance) {
Ok(_) => {
self.n = remainder;
if n > len {
return Err(len);
}
return Ok(());
}
_ => panic!("ExactSizeIterator contract violation"),
}
// The amount by which the inner iterator needs to be shortened for it to be
// at most as long as the take() amount.
let trim_inner = self.iter.len().saturating_sub(self.n);
// The amount we need to advance inner to fulfill the caller's request.
// take(), advance_by() and len() all can be at most usize, so we don't have to worry
// about having to advance more than usize::MAX here.
let advance_by = trim_inner.saturating_add(n);

let advanced = match self.iter.advance_back_by(advance_by) {
Ok(_) => advance_by - trim_inner,
Err(advanced) => advanced - trim_inner,
};
self.n -= advanced;
return if advanced < n { Err(advanced) } else { Ok(()) };
}
}

Expand Down
3 changes: 0 additions & 3 deletions library/core/src/iter/traits/double_ended.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,6 @@ pub trait DoubleEndedIterator: Iterator {
/// Calling `advance_back_by(0)` can do meaningful work, for example [`Flatten`] can advance its
/// outer iterator until it finds an inner iterator that is not empty, which then often
/// allows it to return a more accurate `size_hint()` than in its initial state.
/// `advance_back_by(0)` may either return `Ok()` or `Err(0)`. The former conveys no information
/// whether the iterator is or is not exhausted, the latter can be treated as if [`next_back`]
/// had returned `None`. Replacing a `Err(0)` with `Ok` is only correct for `n = 0`.
///
/// [`advance_by`]: Iterator::advance_by
/// [`Flatten`]: crate::iter::Flatten
Expand Down
3 changes: 0 additions & 3 deletions library/core/src/iter/traits/iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,6 @@ pub trait Iterator {
/// Calling `advance_by(0)` can do meaningful work, for example [`Flatten`]
/// can advance its outer iterator until it finds an inner iterator that is not empty, which
/// then often allows it to return a more accurate `size_hint()` than in its initial state.
/// `advance_by(0)` may either return `Ok()` or `Err(0)`. The former conveys no information
/// whether the iterator is or is not exhausted, the latter can be treated as if [`next`]
/// had returned `None`. Replacing a `Err(0)` with `Ok` is only correct for `n = 0`.
///
/// [`Flatten`]: crate::iter::Flatten
/// [`next`]: Iterator::next
Expand Down
8 changes: 8 additions & 0 deletions library/core/tests/iter/adapters/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,25 @@ fn test_iterator_chain_advance_by() {
iter.advance_by(i).unwrap();
assert_eq!(iter.next(), Some(&xs[i]));
assert_eq!(iter.advance_by(100), Err(len - i - 1));
iter.advance_by(0).unwrap();
}

for i in 0..ys.len() {
let mut iter = Unfuse::new(xs).chain(Unfuse::new(ys));
iter.advance_by(xs.len() + i).unwrap();
assert_eq!(iter.next(), Some(&ys[i]));
assert_eq!(iter.advance_by(100), Err(ys.len() - i - 1));
iter.advance_by(0).unwrap();
}

let mut iter = xs.iter().chain(ys);
iter.advance_by(len).unwrap();
assert_eq!(iter.next(), None);
iter.advance_by(0).unwrap();

let mut iter = xs.iter().chain(ys);
assert_eq!(iter.advance_by(len + 1), Err(len));
iter.advance_by(0).unwrap();
}

test_chain(&[], &[]);
Expand All @@ -67,21 +71,25 @@ fn test_iterator_chain_advance_back_by() {
iter.advance_back_by(i).unwrap();
assert_eq!(iter.next_back(), Some(&ys[ys.len() - i - 1]));
assert_eq!(iter.advance_back_by(100), Err(len - i - 1));
iter.advance_back_by(0).unwrap();
}

for i in 0..xs.len() {
let mut iter = Unfuse::new(xs).chain(Unfuse::new(ys));
iter.advance_back_by(ys.len() + i).unwrap();
assert_eq!(iter.next_back(), Some(&xs[xs.len() - i - 1]));
assert_eq!(iter.advance_back_by(100), Err(xs.len() - i - 1));
iter.advance_back_by(0).unwrap();
}

let mut iter = xs.iter().chain(ys);
iter.advance_back_by(len).unwrap();
assert_eq!(iter.next_back(), None);
iter.advance_back_by(0).unwrap();

let mut iter = xs.iter().chain(ys);
assert_eq!(iter.advance_back_by(len + 1), Err(len));
iter.advance_back_by(0).unwrap();
}

test_chain(&[], &[]);
Expand Down
3 changes: 3 additions & 0 deletions library/core/tests/iter/adapters/flatten.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ fn test_flatten_try_folds() {
#[test]
fn test_flatten_advance_by() {
let mut it = once(0..10).chain(once(10..30)).chain(once(30..40)).flatten();

it.advance_by(5).unwrap();
assert_eq!(it.next(), Some(5));
it.advance_by(9).unwrap();
Expand All @@ -72,6 +73,8 @@ fn test_flatten_advance_by() {

assert_eq!(it.advance_by(usize::MAX), Err(9));
assert_eq!(it.advance_back_by(usize::MAX), Err(0));
it.advance_by(0).unwrap();
it.advance_back_by(0).unwrap();
assert_eq!(it.size_hint(), (0, Some(0)));
}

Expand Down
11 changes: 11 additions & 0 deletions library/core/tests/iter/adapters/skip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,17 @@ fn test_iterator_skip_nth() {
assert_eq!(it.nth(0), None);
}

#[test]
fn test_skip_advance_by() {
assert_eq!((0..0).skip(10).advance_by(0), Ok(()));
assert_eq!((0..0).skip(10).advance_by(1), Err(0));
assert_eq!((0u128..(usize::MAX as u128) + 1).skip(usize::MAX).advance_by(usize::MAX), Err(1));
assert_eq!((0u128..u128::MAX).skip(usize::MAX).advance_by(1), Ok(()));

assert_eq!((0..2).skip(1).advance_back_by(10), Err(1));
assert_eq!((0..0).skip(1).advance_back_by(0), Ok(()));
}

#[test]
fn test_iterator_skip_count() {
let xs = [0, 1, 2, 3, 5, 13, 15, 16, 17, 19, 20, 30];
Expand Down
22 changes: 22 additions & 0 deletions library/core/tests/iter/adapters/take.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,28 @@ fn test_iterator_take_nth_back() {
assert_eq!(it.nth_back(1), None);
}

#[test]
fn test_take_advance_by() {
let mut take = (0..10).take(3);
assert_eq!(take.advance_by(2), Ok(()));
assert_eq!(take.next(), Some(2));
assert_eq!(take.advance_by(1), Err(0));

assert_eq!((0..0).take(10).advance_by(0), Ok(()));
assert_eq!((0..0).take(10).advance_by(1), Err(0));
assert_eq!((0..10).take(4).advance_by(5), Err(4));

let mut take = (0..10).take(3);
assert_eq!(take.advance_back_by(2), Ok(()));
assert_eq!(take.next(), Some(0));
assert_eq!(take.advance_back_by(1), Err(0));

assert_eq!((0..2).take(1).advance_back_by(10), Err(1));
assert_eq!((0..0).take(1).advance_back_by(1), Err(0));
assert_eq!((0..0).take(1).advance_back_by(0), Ok(()));
assert_eq!((0..usize::MAX).take(100).advance_back_by(usize::MAX), Err(100));
}

#[test]
fn test_iterator_take_short() {
let xs = [0, 1, 2, 3];
Expand Down
3 changes: 3 additions & 0 deletions library/core/tests/iter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,9 @@ fn test_range_advance_by() {

assert_eq!(r.advance_by(usize::MAX), Err(usize::MAX - 2));

r.advance_by(0).unwrap();
r.advance_back_by(0).unwrap();

let mut r = 0u128..u128::MAX;

r.advance_by(usize::MAX).unwrap();
Expand Down
2 changes: 2 additions & 0 deletions library/core/tests/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ fn test_iterator_advance_by() {
assert_eq!(iter.as_slice(), &v[3..]);
iter.advance_by(2).unwrap();
assert_eq!(iter.as_slice(), &[]);
iter.advance_by(0).unwrap();
}

#[test]
Expand All @@ -175,6 +176,7 @@ fn test_iterator_advance_back_by() {
assert_eq!(iter.as_slice(), &v[..v.len() - 3]);
iter.advance_back_by(2).unwrap();
assert_eq!(iter.as_slice(), &[]);
iter.advance_back_by(0).unwrap();
}

#[test]
Expand Down

0 comments on commit 5fd3a5c

Please sign in to comment.