False positive for unnecessary_filter_map

[emabee]

cargo clippy -V
clippy 0.0.212 (e3cb40e 2019-06-25)

A warning is produced from

#[warn(clippy::unnecessary_filter_map)]
let errors: Vec<ServerError> = server_errors
    .into_iter()
    .filter_map(|se| match se.severity() {
        Severity::Warning => {
            self.warnings.push(se);
            None
        }
        _ => Some(se),
    })
    .collect();

The code splits moves some items from one collection into a second. If I'd use filter, as recommended, then I would have to clone the ses, and discard the original instances.

[emabee]

Sorry for the duplications - I have no clue how this happened!

[phansch]

It was probably due to the problems GitHub had earlier today, no worries!

[MightyPork]

Here is another example from my code, just to add some test-case diversity (yeah it's ugly, refactor tips are very welcome :p)

// "drain_filter"
self.pending_requests = self.pending_requests.drain(..)
    .filter_map(|pending| {
        if pending.expires <= now {
            return None; // Expired, remove
        }

        if pending.group_id == group_id {
            // Matched - reuse strings and remove
            node.send_response(pending.reply_to, pending.token, Value::Null)
                .on_error_warn();
            None
        } else {
            // Keep waiting
            Some(pending)
        }
    })
    .collect();

[drahnr]

Another sample point of an enumartion extraction

            .filter_map(|res| match res {
                Ok(::pgp::packet::Packet::Signature(sig_packet)) => Some(sig_packet),
                _ => None,
            })

[BartMassey]

I just ran into this issue as well. The lint needs to check that all uses in the closure body can be by shared reference.

Another reduced test case:

pub struct S(u8);

impl S {
    pub fn consume(self) {
        println!("yum");
    }
}

pub fn filter_owned() -> impl Iterator<Item = S> {
    (0..10)
        .map(|i| S(i))
        .filter_map(|s| {
            if s.0 & 1 == 0 {
                s.consume();
                None
            } else {
                Some(s)
            }
        })
}

If this lint can't be fixed, then I'd like to see Clippy default to allow: it's quite annoying.

[smoelius]

If this lint can't be fixed, then I'd like to see Clippy default to allow: it's quite annoying.

These are good opportunities to thank the Clippy maintainers. That we're willing to put up with a quirk every so often is a testament to Clippy's profound usefulness!

[BartMassey]

@smoelius Yeah, Clippy is an amazing tool — I only expect so much of it because I know how good its developers are. 🙂