Merge #449 #461

449: Improve `tuple_windows()` doc (add a reference to `pairwise`) r=jswrenn a=robinmoussu

As shown in #388, tuple_windows can be hard to find if you come from other
environment (python, RxJs, …) since people may expect to find a `pairwise()`
method instead.

461: Clean up specializations tests r=jswrenn a=phimuemue

In response to this comment #413 (comment):

> If the contribution is an optimized specialization of an `Iterator` method (such as `nth` or `fold`), the specialization needs to be very thoroughly tested (preferably with quickcheck tests) to ensure it behaves identically (_especially_ on edge cases, and with when it panics).

I thought about having a simple way to test specializations: If we had *one* test function testing all specializations, we would have an easy way to quickcheck specializations for our iterators.

We already had quite a bit in place, so I took it and streamlined it a bit.

* `fold` is tested by remembering all elements provided to the closure (instead of only XORing elements).
* cleaned up some things (unused specialization of `size_hint`, some redundant lifetimes, superfluous arguments)
* test all specializations in one function (instead of only some specializations in two functions)
* remove manual special case tests in favor of quickcheck'd ones

If we decide to do it like this, we can, in a future commit, just do the following for all iterators (possibly via a macro) to test specializations uniformly (and hopefully reliably):

```
quickcheck! {
    fn test_specializations_qc(params_for_iterator) -> () {
        test_specializations(&make_iterator_to_be_tested(params_for_iterator));
    }
}
```

This way, we would also test methods that are not even specialized, but for simplicity, I'd be willing to accept this. If we see that it slows things down considerably, we could still devise a simple solution that would allow to specify the methods to be tested.

Co-authored-by: Robin Moussu <[email protected]>
Co-authored-by: philipp <[email protected]>

Author: 3 people

src/lib.rs

@@ -584,6 +584,8 @@ pub trait Itertools : Iterator {
     /// ```
     /// use itertools::Itertools;
     /// let mut v = Vec::new();
+    ///
+    /// // pairwise iteration
     /// for (a, b) in (1..5).tuple_windows() {
     ///     v.push((a, b));
     /// }

tests/specializations.rs

@@ -1,6 +1,5 @@
-use itertools::{EitherOrBoth, Itertools};
+use itertools::Itertools;
 use std::fmt::Debug;
-use std::ops::BitXor;
 use quickcheck::quickcheck;
 
 struct Unspecialized<I>(I);
@@ -14,17 +13,11 @@ where
     fn next(&mut self) -> Option<Self::Item> {
         self.0.next()
     }
-
-    #[inline(always)]
-    fn size_hint(&self) -> (usize, Option<usize>) {
-        self.0.size_hint()
-    }
 }
 
 fn check_specialized<'a, V, IterItem, Iter, F>(iterator: &Iter, mapper: F)
 where
     V: Eq + Debug,
-    IterItem: 'a,
     Iter: Iterator<Item = IterItem> + Clone + 'a,
     F: Fn(Box<dyn Iterator<Item = IterItem> + 'a>) -> V,
 {
@@ -34,27 +27,43 @@ where
     )
 }
 
-fn check_specialized_count_last_nth_sizeh<'a, IterItem, Iter>(
+fn test_specializations<IterItem, Iter>(
     it: &Iter,
-    known_expected_size: Option<usize>,
 ) where
-    IterItem: 'a + Eq + Debug,
-    Iter: Iterator<Item = IterItem> + Clone + 'a,
+    IterItem: Eq + Debug + Clone,
+    Iter: Iterator<Item = IterItem> + Clone,
 {
-    let size = it.clone().count();
-    if let Some(expected_size) = known_expected_size {
-        assert_eq!(size, expected_size);
-    }
     check_specialized(it, |i| i.count());
     check_specialized(it, |i| i.last());
+    check_specialized(it, |i| i.collect::<Vec<_>>());
+    check_specialized(it, |i| {
+        let mut parameters_from_fold = vec![];
+        let fold_result = i.fold(vec![], |mut acc, v: IterItem| {
+            parameters_from_fold.push((acc.clone(), v.clone()));
+            acc.push(v);
+            acc
+        });
+        (parameters_from_fold, fold_result)
+    });
+    check_specialized(it, |mut i| {
+        let mut parameters_from_all = vec![];
+        let first = i.next();
+        let all_result = i.all(|x| {
+            parameters_from_all.push(x.clone());
+            Some(x)==first
+        });
+        (parameters_from_all, all_result)
+    });
+    let size = it.clone().count();
     for n in 0..size + 2 {
         check_specialized(it, |mut i| i.nth(n));
     }
+    // size_hint is a bit harder to check
     let mut it_sh = it.clone();
     for n in 0..size + 2 {
         let len = it_sh.clone().count();
         let (min, max) = it_sh.size_hint();
-        assert_eq!((size - n.min(size)), len);
+        assert_eq!(size - n.min(size), len);
         assert!(min <= len);
         if let Some(max) = max {
             assert!(len <= max);
@@ -63,87 +72,17 @@ fn check_specialized_count_last_nth_sizeh<'a, IterItem, Iter>(
     }
 }
 
-fn check_specialized_fold_xor<'a, IterItem, Iter>(it: &Iter)
-where
-    IterItem: 'a
-        + BitXor
-        + Eq
-        + Debug
-        + BitXor<<IterItem as BitXor>::Output, Output = <IterItem as BitXor>::Output>
-        + Clone,
-    <IterItem as BitXor>::Output:
-        BitXor<Output = <IterItem as BitXor>::Output> + Eq + Debug + Clone,
-    Iter: Iterator<Item = IterItem> + Clone + 'a,
-{
-    check_specialized(it, |mut i| {
-        let first = i.next().map(|f| f.clone() ^ (f.clone() ^ f));
-        i.fold(first, |acc, v: IterItem| acc.map(move |a| v ^ a))
-    });
-}
-
-fn put_back_test(test_vec: Vec<i32>, known_expected_size: Option<usize>) {
-    {
-        // Lexical lifetimes support
-        let pb = itertools::put_back(test_vec.iter());
-        check_specialized_count_last_nth_sizeh(&pb, known_expected_size);
-        check_specialized_fold_xor(&pb);
-    }
-
-    let mut pb = itertools::put_back(test_vec.into_iter());
-    pb.put_back(1);
-    check_specialized_count_last_nth_sizeh(&pb, known_expected_size.map(|x| x + 1));
-    check_specialized_fold_xor(&pb)
-}
-
-#[test]
-fn put_back() {
-    put_back_test(vec![7, 4, 1], Some(3));
-}
-
 quickcheck! {
     fn put_back_qc(test_vec: Vec<i32>) -> () {
-        put_back_test(test_vec, None)
+        test_specializations(&itertools::put_back(test_vec.iter()));
+        let mut pb = itertools::put_back(test_vec.into_iter());
+        pb.put_back(1);
+        test_specializations(&pb);
     }
 }
 
-fn merge_join_by_test(i1: Vec<usize>, i2: Vec<usize>, known_expected_size: Option<usize>) {
-    let i1 = i1.into_iter();
-    let i2 = i2.into_iter();
-    let mjb = i1.clone().merge_join_by(i2.clone(), std::cmp::Ord::cmp);
-    check_specialized_count_last_nth_sizeh(&mjb, known_expected_size);
-    // Rust 1.24 compatibility:
-    fn eob_left_z(eob: EitherOrBoth<usize, usize>) -> usize {
-        eob.left().unwrap_or(0)
-    }
-    fn eob_right_z(eob: EitherOrBoth<usize, usize>) -> usize {
-        eob.right().unwrap_or(0)
-    }
-    fn eob_both_z(eob: EitherOrBoth<usize, usize>) -> usize {
-        let (a, b) = eob.both().unwrap_or((0, 0));
-        assert_eq!(a, b);
-        a
-    }
-    check_specialized_fold_xor(&mjb.clone().map(eob_left_z));
-    check_specialized_fold_xor(&mjb.clone().map(eob_right_z));
-    check_specialized_fold_xor(&mjb.clone().map(eob_both_z));
-
-    // And the other way around
-    let mjb = i2.merge_join_by(i1, std::cmp::Ord::cmp);
-    check_specialized_count_last_nth_sizeh(&mjb, known_expected_size);
-    check_specialized_fold_xor(&mjb.clone().map(eob_left_z));
-    check_specialized_fold_xor(&mjb.clone().map(eob_right_z));
-    check_specialized_fold_xor(&mjb.clone().map(eob_both_z));
-}
-
-#[test]
-fn merge_join_by() {
-    let i1 = vec![1, 3, 5, 7, 8, 9];
-    let i2 = vec![0, 3, 4, 5];
-    merge_join_by_test(i1, i2, Some(8));
-}
-
 quickcheck! {
     fn merge_join_by_qc(i1: Vec<usize>, i2: Vec<usize>) -> () {
-        merge_join_by_test(i1, i2, None)
+        test_specializations(&i1.into_iter().merge_join_by(i2.into_iter(), std::cmp::Ord::cmp));
     }
 }