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

Author: the8472

library/alloc/tests/vec.rs

@@ -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);
 }
 

library/core/src/iter/adapters/skip.rs

@@ -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;
@@ -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) };
             }
         }
 

library/core/src/iter/adapters/take.rs

@@ -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(()) };
     }
 }
 

library/core/src/iter/traits/double_ended.rs

@@ -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

library/core/src/iter/traits/iterator.rs

@@ -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

library/core/tests/iter/adapters/chain.rs

@@ -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(&[], &[]);
@@ -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(&[], &[]);

library/core/tests/iter/adapters/flatten.rs

@@ -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();
@@ -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)));
 }
 

library/core/tests/iter/adapters/skip.rs

@@ -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];

library/core/tests/iter/adapters/take.rs

@@ -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];

library/core/tests/iter/range.rs

@@ -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();