Skip to content

Commit

Permalink
Fix Iterator::advance_by contract inconsistency
Browse files Browse the repository at this point in the history
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
the8472 committed Oct 15, 2021
1 parent eeb16a2 commit 17ce56f
Show file tree
Hide file tree
Showing 11 changed files with 58 additions and 20 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
26 changes: 14 additions & 12 deletions library/core/src/iter/adapters/take.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,21 +215,23 @@ 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) {
let trim_inner = self.iter.len().saturating_sub(self.n);
let advance_by = trim_inner.saturating_add(n);

return match self.iter.advance_back_by(advance_by) {
Ok(_) => {
self.n = remainder;
if n > len {
return Err(len);
}
return Ok(());
self.n -= n;
let advanced = advance_by - trim_inner;
if advanced < n { Err(advanced) } else { Ok(()) }
}
_ => panic!("ExactSizeIterator contract violation"),
}
Err(advanced) => {
let advanced = advanced - trim_inner;
self.n -= advanced;
Err(advanced)
}
};
}
}

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
12 changes: 12 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,18 @@ fn test_iterator_take_nth_back() {
assert_eq!(it.nth_back(1), None);
}

#[test]
fn test_take_advance_by() {
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));

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 17ce56f

Please sign in to comment.