Make the filter_map lint more specific

[ghost]

The intent of the filter_map method is to unwrap element values and skip Nones when iterating over elements that are Options. It's not a universal shorthand for filter followed by a call tomap. The filter_map documentation indicates this.

The current implementation of the fliter_map lint triggers on every call to filter followed by a call tomap and basically notes that the suggestion can make the code more complicated as a known issue. It encourages changes like this (from #3186):

extra_args
    .split("__CLIPPY_HACKERY__")
    .filter(|s| !s.is_empty())
    .map(str::to_owned)

to

extra_args
   .split("__CLIPPY_HACKERY__")
   .filter_map(|s| if s.is_empty() {
       None
   } else {
       Some(s.to_string())
   })

I find the final code is less readable and more complicated than the code at the start. Although, there is one less function call, there is now an if statement and extra code for constructing an option.

Let's change the lint to only trigger in the following conditions. In the table below option_expr is an expression with type Option<_> and result_expr is an expression with type Result<_, _>.

Code	Suggestion
filter(|x| option_expr.is_some()).map(|x| x.unwrap())	filter_map(|x| option_expr)
filter(|x| option_expr.is_some()).flat_map(|x| x)	filter_map(|x| option_expr)
filter(|x| result_expr.is_ok()).map(|x| x.unwrap())	filter_map(|x| result_expr.ok())

[flip1995]

Yeah I also thought that the new code wasn't as readable as the original code. This would be a good change!

If this is fixed, the changes done in #3186 should also be reverted.

[waynr]

I'll give this issue a shot in #3461

[waynr]

filter(|x| option_expr.is_some()).flat_map(|x| x) -> filter_map(|x| option_expr)

This example doesn't seem to fit in with the other two unless flat_map is somehow implicitly unwraping the Some. @mikerite could you elaborate on how that is supposed to work?

[camsteffen]

I think lints from filter_map are so likely to be false positives that the lint does more harm than good (considering the bad form it suggests), even for a pedantic lint. The suggested improvement seems too specific, but I think it would still be an improvement. We can always add more positive cases to the lint in the future.

[vandenheuvel]

I agree with @camsteffen, in my project, this lint only yielded false positives. I'd even go as far as to say that it should be removed, even from pedantic, until it is specialized.