Can't solve this clippy warning: "avoid using `collect()` when not needed"

Consider this working example:

use std::boxed::Box;

#[derive(Debug)]
pub enum Value {
    Char(char),
    // Int(i64),
    // ...
}

pub trait ToValueIter {
    /// Creates a new, cloned iterator over the naturally iterable values of `&self`.
    fn to_value_iter(&self) -> ValueIter;
}

pub struct ValueIter(
    pub Box<dyn Iterator<Item = Value>>
);

// The issue is here:
impl ToValueIter for &str {
    fn to_value_iter(&self) -> ValueIter {
        let values = self
            .chars()
            .map(Value::Char)
            .collect::<Vec<_>>();
        ValueIter(Box::new(values.into_iter()))
    }
}

fn main() {
    let it = "Hello".to_value_iter();
    for v in it.0 {
        println!("{:?}", v);
    }
}

This is working fine, but when I run cargo clippy I get this warning:

warning: avoid using `collect()` when not needed
  --> src/main.rs:25:14
   |
25 |             .collect::<Vec<_>>();
   |              ^^^^^^^
26 |         ValueIter(Box::new(values.into_iter()))
   |                            ------------------ the iterator could be used here instead
   |
   = note: `#[warn(clippy::needless_collect)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_collect
help: use the original Iterator instead of collecting it and then producing a new one
   |
22 |
23 |         ValueIter(Box::new(self
24 |             .chars()
25 |             .map(Value::Char)))
   |

I tried to understand and solve the problem, but after a lot of tries and searches, I didn't manage to solve it. Furthermore, I'm not sure if there is really a problem : this iterator data needs to be cloned in a way or another, and collect() seems to be a reasonnable solution for this.

Is there really a problem here, and if so, can you help me to solve it?

Thanks in advance

Clippy is trying to tell you that it is not necessary to convert an iterator into a vector if you are going to convert the vector into an iterator later on.

Yes, this warning is spurious/incorrect. Clippy doesn't see that

(because self.chars() borrows from self and ValueIter has a 'static requirement). You should check the Clippy issue tracker for previous reports of this problem and open a new issue if there are none. Static analysis is hard!

Admittedly going through Vec<char> like that is a bit of a strange/inefficient way to get a 'static iterator for boxing. I thought String would have some kind of into_chars method so you could do self.to_owned().into_chars(), but surprisingly it doesn't.

3 Likes

To be honnest, I don't understand what does this mean:

ValueIter has a 'static requirement

I know what 'static lifetime means, but I don't understand what static lifetimes are dealing about here...

It helps to see what everything looks like without lifetimes elided. This:

pub struct ValueIter(pub Box<dyn Iterator<Item = Value>>);

is actually shorthand for

pub struct ValueIter(pub Box<dyn Iterator<Item = Value> + 'static>);

where Box<dyn Iterator<Item = Value> + 'static> means "a Box containing an iterator over Values that doesn't hold any non-'static references". But you want to do

impl ToValueIter for &str {
    fn to_value_iter(&self) -> ValueIter {
        // ...
    }
}

which is shorthand for

impl<'a> ToValueIter for &'a str {
    fn to_value_iter(&self) -> ValueIter {
        // ...
    }
}

So inside the body of to_value_iter, self has the type &&'a str, where 'a can be any lifetime. You can't do what Clippy suggests because self.chars().map(Value::Char) has a type like

std::iter::Map<std::str::Chars<'a>, /* unnameable function type */>

This type has an 'a in it, and 'a may not equal 'static, so we can't put a value of it into a Box<dyn Iterator<Item = Value> + 'static>, as the compiler will tell us (albeit not so clearly):

error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
  --> src/main.rs:22:33
   |
22 |         ValueIter(Box::new(self.chars().map(Value::Char)))
   |                                 ^^^^^
   |
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the impl at 20:6...
  --> src/main.rs:20:6
   |
20 | impl<'a> ToValueIter for &'a str {
   |      ^^
note: ...so that reference does not outlive borrowed content
  --> src/main.rs:22:28
   |
22 |         ValueIter(Box::new(self.chars().map(Value::Char)))
   |                            ^^^^
   = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the expression is assignable
  --> src/main.rs:22:19
   |
22 |         ValueIter(Box::new(self.chars().map(Value::Char)))
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: expected `Box<(dyn Iterator<Item = Value> + 'static)>`
              found `Box<dyn Iterator<Item = Value>>`

(imo allowing + 'static to be elided in trait object types and allowing impl ToValueIter for &str without naming the generic lifetime are both unfortunate "features" of the language, and hopefully rustc will start warning against them at some point. Trait object + 'static elision in particular confuses people on this forum all the time.)

4 Likes

Thank you very much for this very precise answer; I will look at it more seriously tomorrow, night is coming here, life time is important even in the real life:) I guess I will have some questions about it, thank you again!

1 Like

If you do want to allow non-'static iterators inside ValueIter, you can get it working without the intermediary vector:

// Note the 'a:
pub struct ValueIter<'a>(
    pub Box<dyn Iterator<Item = Value> + 'a>
);

impl ToValueIter for &str {
    fn to_value_iter(&self) -> ValueIter<'_> {
        let values = self
            .chars()
            .map(Value::Char);
        ValueIter(Box::new(values))
    }
}

playground