Add heuristic to filter_map and find_map

clippy_dev/src/ra_setup.rs

@@ -1,5 +1,3 @@
-#![allow(clippy::filter_map)]
-
 use std::fs;
 use std::fs::File;
 use std::io::prelude::*;

clippy_lints/src/loops.rs

@@ -1061,7 +1061,6 @@ fn get_assignments<'a: 'c, 'tcx: 'c, 'c>(
 ) -> impl Iterator<Item = Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)>> + 'c {
     // As the `filter` and `map` below do different things, I think putting together
     // just increases complexity. (cc #3188 and #4193)
-    #[allow(clippy::filter_map)]
     stmts
         .iter()
         .filter_map(move |stmt| match stmt.kind {

clippy_lints/src/methods/mod.rs

@@ -14,18 +14,20 @@ use if_chain::if_chain;
 use rustc_ast::ast;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::{TraitItem, TraitItemKind};
+use rustc_hir::{ExprKind, TraitItem, TraitItemKind};
+use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_middle::ty::{self, TraitRef, Ty, TyS};
 use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::Span;
 use rustc_span::symbol::{sym, SymbolStr};
+use rustc_typeck::expr_use_visitor::ExprUseVisitor;
 
 use crate::consts::{constant, Constant};
 use crate::utils::eager_or_lazy::is_lazyness_candidate;
-use crate::utils::usage::mutated_variables;
+use crate::utils::usage::{mutated_variables, UsesOptionOrResult};
 use crate::utils::{
     contains_return, contains_ty, get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher,
     implements_trait, in_macro, is_copy, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment,
@@ -2927,17 +2929,41 @@ fn lint_skip_while_next<'tcx>(
 fn lint_filter_map<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx hir::Expr<'_>,
-    _filter_args: &'tcx [hir::Expr<'_>],
+    filter_args: &'tcx [hir::Expr<'_>],
     _map_args: &'tcx [hir::Expr<'_>],
 ) {
-    // lint if caller of `.filter().map()` is an Iterator
-    if match_trait_method(cx, expr, &paths::ITERATOR) {
-        let msg = "called `filter(..).map(..)` on an `Iterator`";
-        let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead";
-        span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
+    if_chain! {
+        // lint if caller of `.filter().map()` is an Iterator
+        if match_trait_method(cx, expr, &paths::ITERATOR);
+        if let [_, filter_arg] = filter_args;
+        if filter_map_heuristic(filter_arg, cx);
+        then {
+            let msg = "called `filter(..).map(..)` on an `Iterator`";
+            let hint = "this is more succinctly expressed by calling `.filter_map(..)` instead";
+            span_lint_and_help(cx, FILTER_MAP, expr.span, msg, None, hint);
+        }
     }
 }
 
+/// Checks the argument to `filter` to see if it is a candidate for replacement with `filter_map`.
+/// Or similarly check `find` for replacement with `find_map`.
+/// It simply checks if `Option` or `Result` is used at all.
+fn filter_map_heuristic(filter_arg: &hir::Expr<'_>, cx: &LateContext<'_>) -> bool {
+    if let ExprKind::Closure(_, _, body_id, ..) = filter_arg.kind {
+        let map = cx.tcx.hir();
+        let fn_def_id = map.local_def_id(filter_arg.hir_id);
+        let body = map.body(body_id);
+        let mut delegate = UsesOptionOrResult::new(cx);
+        cx.tcx.infer_ctxt().enter(|infcx| {
+            ExprUseVisitor::new(&mut delegate, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body);
+        });
+        if delegate.found {
+            return true;
+        }
+    }
+    false
+}
+
 const FILTER_MAP_NEXT_MSRV: RustcVersion = RustcVersion::new(1, 30, 0);
 
 /// lint use of `filter_map().next()` for `Iterators`
@@ -2976,14 +3002,19 @@ fn lint_filter_map_next<'tcx>(
 fn lint_find_map<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx hir::Expr<'_>,
-    _find_args: &'tcx [hir::Expr<'_>],
+    find_args: &'tcx [hir::Expr<'_>],
     map_args: &'tcx [hir::Expr<'_>],
 ) {
-    // lint if caller of `.filter().map()` is an Iterator
-    if match_trait_method(cx, &map_args[0], &paths::ITERATOR) {
-        let msg = "called `find(..).map(..)` on an `Iterator`";
-        let hint = "this is more succinctly expressed by calling `.find_map(..)` instead";
-        span_lint_and_help(cx, FIND_MAP, expr.span, msg, None, hint);
+    if_chain! {
+        // lint if caller of `.filter().map()` is an Iterator
+        if match_trait_method(cx, &map_args[0], &paths::ITERATOR);
+        if let [_, find_arg] = find_args;
+        if filter_map_heuristic(find_arg, cx);
+        then {
+            let msg = "called `find(..).map(..)` on an `Iterator`";
+            let hint = "this is more succinctly expressed by calling `.find_map(..)` instead";
+            span_lint_and_help(cx, FIND_MAP, expr.span, msg, None, hint);
+        }
     }
 }
 
@@ -2994,7 +3025,7 @@ fn lint_filter_map_map<'tcx>(
     _filter_args: &'tcx [hir::Expr<'_>],
     _map_args: &'tcx [hir::Expr<'_>],
 ) {
-    // lint if caller of `.filter().map()` is an Iterator
+    // lint if caller of `.filter_map().map()` is an Iterator
     if match_trait_method(cx, expr, &paths::ITERATOR) {
         let msg = "called `filter_map(..).map(..)` on an `Iterator`";
         let hint = "this is more succinctly expressed by only calling `.filter_map(..)` instead";

clippy_lints/src/multiple_crate_versions.rs

@@ -51,7 +51,8 @@ impl LateLintPass<'_> for MultipleCrateVersions {
             if let Some(resolve) = &metadata.resolve;
             if let Some(local_id) = packages
                 .iter()
-                .find_map(|p| if p.name == *local_name { Some(&p.id) } else { None });
+                .find(|p| p.name == *local_name)
+                .map(|p| &p.id);
             then {
                 for (name, group) in &packages.iter().group_by(|p| p.name.clone()) {
                     let group: Vec<&Package> = group.collect();

clippy_lints/src/shadow.rs

@@ -203,9 +203,7 @@ fn check_pat<'tcx>(
                 if let ExprKind::Struct(_, ref efields, _) = init_struct.kind {
                     for field in pfields {
                         let name = field.ident.name;
-                        let efield = efields
-                            .iter()
-                            .find_map(|f| if f.ident.name == name { Some(&*f.expr) } else { None });
+                        let efield = efields.iter().find(|f| f.ident.name == name).map(|f| f.expr);
                         check_pat(cx, &field.pat, efield, span, bindings);
                     }
                 } else {

clippy_lints/src/suspicious_operation_groupings.rs

@@ -688,6 +688,5 @@ fn skip_index<A, Iter>(iter: Iter, index: usize) -> impl Iterator<Item = A>
 where
     Iter: Iterator<Item = A>,
 {
-    iter.enumerate()
-        .filter_map(move |(i, a)| if i == index { None } else { Some(a) })
+    iter.enumerate().filter(move |&(i, _)| i != index).map(|(_, a)| a)
 }

clippy_lints/src/utils/mod.rs

@@ -356,8 +356,6 @@ pub fn path_to_res(cx: &LateContext<'_>, path: &[&str]) -> Option<def::Res> {
                 let item_def_id = current_item.res.def_id();
                 if cx.tcx.def_kind(item_def_id) == DefKind::Struct;
                 then {
-                    // Bad `find_map` suggestion. See #4193.
-                    #[allow(clippy::find_map)]
                     return cx.tcx.inherent_impls(item_def_id).iter()
                         .flat_map(|&impl_def_id| cx.tcx.item_children(impl_def_id))
                         .find(|item| item.ident.name.as_str() == *segment)

clippy_lints/src/utils/usage.rs

@@ -10,6 +10,7 @@ use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
 use rustc_middle::hir::map::Map;
 use rustc_middle::ty;
+use rustc_span::sym;
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 
@@ -228,3 +229,40 @@ pub fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool {
     recursive_visitor.visit_expr(expression);
     recursive_visitor.seen_return_break_continue
 }
+
+pub struct UsesOptionOrResult<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    pub found: bool,
+}
+
+impl<'a, 'tcx> UsesOptionOrResult<'a, 'tcx> {
+    pub fn new(cx: &'a LateContext<'tcx>) -> Self {
+        Self { cx, found: false }
+    }
+
+    fn check_place_with_id(&mut self, place_with_id: &PlaceWithHirId<'tcx>) {
+        if !self.found {
+            if let Some(adt_def) = place_with_id.place.ty().peel_refs().ty_adt_def() {
+                if self.cx.tcx.is_diagnostic_item(sym::option_type, adt_def.did)
+                    || self.cx.tcx.is_diagnostic_item(sym::result_type, adt_def.did)
+                {
+                    self.found = true;
+                }
+            }
+        }
+    }
+}
+
+impl<'a, 'tcx> Delegate<'tcx> for UsesOptionOrResult<'a, 'tcx> {
+    fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId, _: ConsumeMode) {
+        self.check_place_with_id(place_with_id);
+    }
+
+    fn borrow(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) {
+        self.check_place_with_id(place_with_id);
+    }
+
+    fn mutate(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) {
+        self.check_place_with_id(place_with_id);
+    }
+}

tests/ui/filter_methods.rs

@@ -3,8 +3,6 @@
 #![allow(clippy::missing_docs_in_private_items)]
 
 fn main() {
-    let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
-
     let _: Vec<_> = vec![5_i8; 6]
         .into_iter()
         .filter(|&x| x == 0)
@@ -23,3 +21,7 @@ fn main() {
         .map(|x| x.checked_mul(2))
         .collect();
 }
+
+fn no_lint() {
+    let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
+}

tests/ui/filter_methods.stderr

@@ -1,14 +1,5 @@
-error: called `filter(..).map(..)` on an `Iterator`
-  --> $DIR/filter_methods.rs:6:21
-   |
-LL |     let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = note: `-D clippy::filter-map` implied by `-D warnings`
-   = help: this is more succinctly expressed by calling `.filter_map(..)` instead
-
 error: called `filter(..).flat_map(..)` on an `Iterator`
-  --> $DIR/filter_methods.rs:8:21
+  --> $DIR/filter_methods.rs:6:21
    |
 LL |       let _: Vec<_> = vec![5_i8; 6]
    |  _____________________^
@@ -17,10 +8,11 @@ LL | |         .filter(|&x| x == 0)
 LL | |         .flat_map(|x| x.checked_mul(2))
    | |_______________________________________^
    |
+   = note: `-D clippy::filter-map` implied by `-D warnings`
    = help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`
 
 error: called `filter_map(..).flat_map(..)` on an `Iterator`
-  --> $DIR/filter_methods.rs:14:21
+  --> $DIR/filter_methods.rs:12:21
    |
 LL |       let _: Vec<_> = vec![5_i8; 6]
    |  _____________________^
@@ -32,7 +24,7 @@ LL | |         .flat_map(|x| x.checked_mul(2))
    = help: this is more succinctly expressed by calling `.flat_map(..)` and filtering by returning `iter::empty()`
 
 error: called `filter_map(..).map(..)` on an `Iterator`
-  --> $DIR/filter_methods.rs:20:21
+  --> $DIR/filter_methods.rs:18:21
    |
 LL |       let _: Vec<_> = vec![5_i8; 6]
    |  _____________________^
@@ -43,5 +35,5 @@ LL | |         .map(|x| x.checked_mul(2))
    |
    = help: this is more succinctly expressed by only calling `.filter_map(..)` instead
 
-error: aborting due to 4 previous errors
+error: aborting due to 3 previous errors
 

tests/ui/find_map.rs

@@ -18,16 +18,8 @@ fn main() {
     let a = ["lol", "NaN", "2", "5", "Xunda"];
 
     let _: Option<i32> = a.iter().find(|s| s.parse::<i32>().is_ok()).map(|s| s.parse().unwrap());
+}
 
-    #[allow(clippy::match_like_matches_macro)]
-    let _: Option<Flavor> = desserts_of_the_week
-        .iter()
-        .find(|dessert| match *dessert {
-            Dessert::Cake(_) => true,
-            _ => false,
-        })
-        .map(|dessert| match *dessert {
-            Dessert::Cake(ref flavor) => *flavor,
-            _ => unreachable!(),
-        });
+fn no_lint() {
+    let _ = vec![1].into_iter().find(|n| *n > 5).map(|n| n + 7);
 }

tests/ui/find_map.stderr

@@ -7,20 +7,5 @@ LL |     let _: Option<i32> = a.iter().find(|s| s.parse::<i32>().is_ok()).map(|s
    = note: `-D clippy::find-map` implied by `-D warnings`
    = help: this is more succinctly expressed by calling `.find_map(..)` instead
 
-error: called `find(..).map(..)` on an `Iterator`
-  --> $DIR/find_map.rs:23:29
-   |
-LL |       let _: Option<Flavor> = desserts_of_the_week
-   |  _____________________________^
-LL | |         .iter()
-LL | |         .find(|dessert| match *dessert {
-LL | |             Dessert::Cake(_) => true,
-...  |
-LL | |             _ => unreachable!(),
-LL | |         });
-   | |__________^
-   |
-   = help: this is more succinctly expressed by calling `.find_map(..)` instead
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error