Understand iterators & reverse iterator

1

I have this simple function that looks at a boolean flag and reverses the iterator if it is set. I want to return the new iterator or the old one if it is not set.

The new clippy is telling me to not use 'collect'. But I think that I need collect to create a new iterator. Otherwise it won't compile.

I'm clearly doing something slightly wrong here. Can I change the return type to something that implements Iterator or Reverse Iterator ? Can I change how my iterator is constructed?

Please can someone tell me how to tidy up the below function. Thanks!

 impl DisplayNode {

    pub fn get_children_from_node(&self, is_reversed: bool) -> impl Iterator<Item = DisplayNode> {
        if is_reversed {
            let children: Vec<DisplayNode> = self.children.clone().into_iter().rev().collect();
            children.into_iter()
        } else {
            self.children.clone().into_iter()
        }
    }
}
1 Like
2

The reason it won't compile is because there's a subtle gotcha to returning an impl Trait, which is that you can still only return one type, even though it allows you to leave it to the compiler to work out what concrete type that actually is. This causes a problem because a forward iterator, and the rev call on that same iterator, produce different concrete types even though they're both Iterator implementations.

The reason the collect appears to fix this is because, by collecting, you produce a seperate collection and the forward iterator of that has the same type as your original forward iterator, so now all branches return the same concrete type and the compiler's happy. (It is only paying attention to the fact that your elements are iterated forward relative to the container they're sitting in - that that just happens to be opposite your original order doesn't change anything)

There's unfortunately no easy built-in way to achieve the effect you actually want, and the branching type means that even inlining this function into its caller wouldn't help if you wanted to do anything afterwards that builds on the iterator type, like further maps or other iterator operations besides collect. If this was a more complicated example, you could use something like enum_dispatch to do the boilerplate for an enum for you, but IMO that'd be overkill here.

The easiest way to hide the concrete type and so make the two match up is to just stuff them into a Box<dyn Iterator> like so:

fn get_children(children: Vec<u32>, reverse: bool) -> impl Iterator<Item=u32> {
    // This type needs to be explicit so the compiler's not upset that 
    // these boxes are different concrete types
    let out : Box<dyn Iterator<Item=u32>> = if reverse {
        Box::new(children.into_iter().rev())
    }
    else {
        Box::new(children.into_iter())
    };
    return out;
}

(I left out the DisplayNode just for checking that this compiles on playground)

Also, as a matter of style, it might be worth knowing that in Rust, it's unusual to clone incoming parameters - instead, if you need to own it, simply consume it rather than borrow in the first place. This gives the caller control over if a new object needs to be created and avoids inefficiency if it doesn't.

1 Like
3

Slightly less overkill, Either also implements Iterator when both of its variant types do. That crate is popular enough that it might be in your dependency tree already.

5 Likes
4

It looks like a clippy bug. You can report it.

5

Looks like this is something already raised to the clippy team.
https://github.com/rust-lang/rust-clippy/issues/7512
https://github.com/rust-lang/rust-clippy/issues/7526

Thanks for your suggestion with the boxing @garagedragon

6

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.