Feedback for Iterator Adapter

code review
1

Motivated by the below topic I tried to write an iterator adapter that would take an "Iterator<Iterator<U>>" and intersperse it with elements of type U returning a flatened "Iterator<U>". I would appreciate some feedback :slight_smile:

This is my implementation:

struct IntersperseIter<I, U>
where
    I: Iterator,
    I::Item: Iterator<Item = U>,
    U: Clone,
{
    element: U,
    iter: I,
    peek: Option<I::Item>,
}

impl<I, U> Iterator for IntersperseIter<I, U>
where
    I: Iterator,
    I::Item: Iterator<Item = U>,
    U: Clone,
{
    type Item = U;

    fn next(&mut self) -> Option<Self::Item> {
        if self.peek.is_some() {
            if let Some(el) = self.peek.as_mut().and_then(|inner| inner.next()) {
                return Some(el);
            }
            self.peek.replace(self.iter.next()?);
            if self.peek.is_some() {
                return Some(self.element.clone());
            } else {
                return None;
            }
        } else {
            return None;
        }
    }
}

trait IterExt<U>: Iterator {
    fn intersperse_iter(mut self, element: U) -> IntersperseIter<Self, U>
    where
        Self: Sized,
        Self::Item: Iterator<Item = U>,
        U: Clone,
    {
        IntersperseIter {
            element,
            peek: self.next(),
            iter: self,
        }
    }
}

impl<I, U> IterExt<U> for I
where
    I: Iterator,
    I::Item: Iterator<Item = U>,
    U: Clone,
{
}

fn main() {
    let a = vec!["hello", "world", "you", "are", "my", "favourite!"];
    let b: Vec<_> = a
        .into_iter()
        .map(|word| word.chars())
        .intersperse_iter(' ')
        .collect::<Vec<_>>();
    dbg!(b);
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 0.45s
     Running `target/debug/playground`
[src/main.rs:67] b = [
    'h',
    'e',
    'l',
    'l',
    'o',
    ' ',
    'w',
    'o',
    'r',
    'l',
    'd',
    ' ',
    'y',
    'o',
    'u',
    ' ',
    'a',
    'r',
    'e',
    ' ',
    'm',
    'y',
    ' ',
    'f',
    'a',
    'v',
    'o',
    'u',
    'r',
    'i',
    't',
    'e',
    '!',
]
1 Like
2

The more general bound one would usually use is I::Item : IntoIterator<…>. You can find that kind of bound for example on Flatten in std::iter - Rust

Also, if you like, you can get rid of the U from IterExt and from IntersperseIter, at the cost of spelling out the lengthy name <I::Item as IntoIterator>::Item in the signature of intersperse_iter, and in the element field's type. For the Iterator implementation you may keep it if you like - that should work even when it's no longer a parameter of IntersperseIter.

6 Likes
3

Yeah, the U is definitely redundant. It's best to temove it, because additional free variables can cause a lot of headache for downstream users (usually the compiler will complain that the U type parameter is not constrained if you are holding it wrong).

Requiring trait bounds in a type definition is in general bad practice, because it makes construction unnecessarily constrained. In your case, none of the bounds are actually required on the struct itself.

As for the implementation, all but one of your explicit returns are not needed. The peeking mechanism would be better done using Peekable, too.

4

Some of them should be required in order to name the I::Item type used in a field. If changed to an IntoIterator bound, the situation will still be similar. If U is removed even more bounds (essentially all besides the : Clone one, I believe) are required to be able to name the type that replaces U in the element field.

5

Accordingly, I'd like to offer a simplified implementation:

struct IntersperseIter<I>
where
    I: Iterator,
    I::Item: IntoIterator,
{
    iter: I,
    next_part: Option<<I::Item as IntoIterator>::IntoIter>,
    delimiter: <I::Item as IntoIterator>::Item,
}

impl<I> Iterator for IntersperseIter<I>
where
    I: Iterator,
    I::Item: IntoIterator,
    <I::Item as IntoIterator>::Item: Clone,
{
    type Item = <I::Item as IntoIterator>::Item;

    fn next(&mut self) -> Option<Self::Item> {
        let Some(next_part) = self.next_part.as_mut() else { return None };
        let item = next_part.next();

        if item.is_some() {
            item
        } else {
            self.next_part = self.iter.next().map(IntoIterator::into_iter);
            self.next_part.is_some().then(|| self.delimiter.clone())
        }
    }
}
6

Thanks for all the feedback. Question for @H2CO3, why did you add an explicit lifetime to the trait?

7

That's just an oversight. I was experimenting with returning a trait object, and the lifetime was needed for that, but it's no longer necessary.

1 Like
8

I don't really understand why you say that this makes construction unnecessarily constrained. Why would I want to be able to construct a IntersperseIter that does not satisfy <I::Item as IntoIterator>::Item: Clone> when it is needed for it's next() implementation. The struct doesn't serve any purpose but to be an Iterator. So where do I get constrained where it would not be necessary?

9

Usually, generics.