Fix false positive in unnecessary filter_map

clippy_lints/src/methods/unnecessary_filter_map.rs

@@ -1,25 +1,90 @@
 use crate::utils::paths;
-use crate::utils::usage::mutated_variables;
 use crate::utils::{match_qpath, match_trait_method, span_lint};
-use rustc::hir;
 use rustc::hir::def::Res;
 use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
+use rustc::hir::{Body, Expr, ExprKind, HirId, Pat};
 use rustc::lint::LateContext;
+use rustc::middle::expr_use_visitor::*;
+use rustc::middle::mem_categorization::{cmt_, Categorization};
+use rustc::ty;
+use syntax::source_map::Span;
 
 use if_chain::if_chain;
 
 use super::UNNECESSARY_FILTER_MAP;
 
-pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr]) {
+fn can_be_ref(cx: &LateContext<'_, '_>, body: &Body, arg_id: HirId) -> bool {
+    struct CanBeRefDelegate {
+        arg_id: HirId,
+        result: bool,
+    }
+
+    impl<'tcx> Delegate<'tcx> for CanBeRefDelegate {
+        fn consume(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, consume_mode: ConsumeMode) {
+            if_chain! {
+                if self.result;
+                if consume_mode != ConsumeMode::Copy;
+                if let Categorization::Local(id) = cmt.cat;
+                if self.arg_id == id;
+                then {
+                    self.result = false;
+                }
+            }
+        }
+        fn borrow(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, _: ty::Region<'_>, bk: ty::BorrowKind, _: LoanCause) {
+            if_chain! {
+                if self.result;
+                if bk != ty::BorrowKind::ImmBorrow;
+                if let Categorization::Local(id) = cmt.cat;
+                if self.arg_id == id;
+                then {
+                    self.result = false;
+                }
+            }
+        }
+        fn mutate(&mut self, _: HirId, _: Span, cmt: &cmt_<'tcx>, mutate_mode: MutateMode) {
+            if_chain! {
+                if self.result;
+                if mutate_mode != MutateMode::Init;
+                if let Categorization::Local(id) = cmt.cat;
+                if self.arg_id == id;
+                then {
+                    self.result = false;
+                }
+            }
+        }
+
+        fn matched_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: MatchMode) {}
+        fn consume_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: ConsumeMode) {}
+        fn decl_without_init(&mut self, _: HirId, _: Span) {}
+    }
+
+    let mut delegate = CanBeRefDelegate { arg_id, result: true };
+
+    let closure_def_id = body.id().hir_id.owner_def_id();
+    let region_scope_tree = &cx.tcx.region_scope_tree(closure_def_id);
+    ExprUseVisitor::new(
+        &mut delegate,
+        cx.tcx,
+        closure_def_id,
+        cx.param_env,
+        region_scope_tree,
+        cx.tables,
+        None,
+    )
+    .consume_body(body);
+
+    delegate.result
+}
+
+pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &Expr, args: &[Expr]) {
     if !match_trait_method(cx, expr, &paths::ITERATOR) {
         return;
     }
 
-    if let hir::ExprKind::Closure(_, _, body_id, ..) = args[1].node {
+    if let ExprKind::Closure(_, _, body_id, ..) = args[1].node {
         let body = cx.tcx.hir().body(body_id);
         let arg_id = body.params[0].pat.hir_id;
-        let mutates_arg =
-            mutated_variables(&body.value, cx).map_or(true, |used_mutably| used_mutably.contains(&arg_id));
 
         let (mut found_mapping, mut found_filtering) = check_expression(&cx, arg_id, &body.value);
 
@@ -38,7 +103,7 @@ pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr
             return;
         }
 
-        if !found_mapping && !mutates_arg {
+        if !found_mapping && can_be_ref(cx, body, arg_id) {
             span_lint(
                 cx,
                 UNNECESSARY_FILTER_MAP,
@@ -51,19 +116,15 @@ pub(super) fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[hir::Expr
 }
 
 // returns (found_mapping, found_filtering)
-fn check_expression<'a, 'tcx>(
-    cx: &'a LateContext<'a, 'tcx>,
-    arg_id: hir::HirId,
-    expr: &'tcx hir::Expr,
-) -> (bool, bool) {
+fn check_expression<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, arg_id: HirId, expr: &'tcx Expr) -> (bool, bool) {
     match &expr.node {
-        hir::ExprKind::Call(ref func, ref args) => {
+        ExprKind::Call(ref func, ref args) => {
             if_chain! {
-                if let hir::ExprKind::Path(ref path) = func.node;
+                if let ExprKind::Path(ref path) = func.node;
                 then {
                     if match_qpath(path, &paths::OPTION_SOME) {
                         if_chain! {
-                            if let hir::ExprKind::Path(path) = &args[0].node;
+                            if let ExprKind::Path(path) = &args[0].node;
                             if let Res::Local(ref local) = cx.tables.qpath_res(path, args[0].hir_id);
                             then {
                                 if arg_id == *local {
@@ -80,14 +141,14 @@ fn check_expression<'a, 'tcx>(
             }
             (true, true)
         },
-        hir::ExprKind::Block(ref block, _) => {
+        ExprKind::Block(ref block, _) => {
             if let Some(expr) = &block.expr {
                 check_expression(cx, arg_id, &expr)
             } else {
                 (false, false)
             }
         },
-        hir::ExprKind::Match(_, ref arms, _) => {
+        ExprKind::Match(_, ref arms, _) => {
             let mut found_mapping = false;
             let mut found_filtering = false;
             for arm in arms {
@@ -97,22 +158,22 @@ fn check_expression<'a, 'tcx>(
             }
             (found_mapping, found_filtering)
         },
-        hir::ExprKind::Path(path) if match_qpath(path, &paths::OPTION_NONE) => (false, true),
+        ExprKind::Path(path) if match_qpath(path, &paths::OPTION_NONE) => (false, true),
         _ => (true, true),
     }
 }
 
 struct ReturnVisitor<'a, 'tcx> {
     cx: &'a LateContext<'a, 'tcx>,
-    arg_id: hir::HirId,
+    arg_id: HirId,
     // Found a non-None return that isn't Some(input)
     found_mapping: bool,
     // Found a return that isn't Some
     found_filtering: bool,
 }
 
 impl<'a, 'tcx> ReturnVisitor<'a, 'tcx> {
-    fn new(cx: &'a LateContext<'a, 'tcx>, arg_id: hir::HirId) -> ReturnVisitor<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'a, 'tcx>, arg_id: HirId) -> ReturnVisitor<'a, 'tcx> {
         ReturnVisitor {
             cx,
             arg_id,
@@ -123,8 +184,8 @@ impl<'a, 'tcx> ReturnVisitor<'a, 'tcx> {
 }
 
 impl<'a, 'tcx> Visitor<'tcx> for ReturnVisitor<'a, 'tcx> {
-    fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
-        if let hir::ExprKind::Ret(Some(expr)) = &expr.node {
+    fn visit_expr(&mut self, expr: &'tcx Expr) {
+        if let ExprKind::Ret(Some(expr)) = &expr.node {
             let (found_mapping, found_filtering) = check_expression(self.cx, self.arg_id, expr);
             self.found_mapping |= found_mapping;
             self.found_filtering |= found_filtering;

tests/ui/unnecessary_filter_map.rs

@@ -14,4 +14,77 @@ fn main() {
     let _ = (0..4).filter_map(|x| Some(x + 1));
 
     let _ = (0..4).filter_map(i32::checked_abs);
+
+    struct S(u32);
+    impl S {
+        fn mutate(&mut self) {}
+    }
+
+    // Mutating
+    {
+        let _: Vec<_> = vec![S(0), S(1), S(2)]
+            .into_iter()
+            .filter_map(|mut x| {
+                x.mutate();
+                if x.0 % 2 == 0 {
+                    Some(x)
+                } else {
+                    None
+                }
+            })
+            .collect();
+    }
+
+    // Moving
+    {
+        let mut v = vec![];
+        let _: Vec<_> = vec![S(0), S(1), S(2)]
+            .into_iter()
+            .filter_map(|x| {
+                if x.0 % 2 == 0 {
+                    Some(x)
+                } else {
+                    v.push(x);
+                    None
+                }
+            })
+            .collect();
+    }
+
+    enum E {
+        A,
+        B(S),
+    }
+
+    // Mutating with pattern
+    {
+        let _: Vec<_> = vec![E::A, E::B(S(0))]
+            .into_iter()
+            .filter_map(|mut x| {
+                if let E::B(ref mut y) = x {
+                    y.mutate();
+                    None
+                } else {
+                    Some(x)
+                }
+            })
+            .collect();
+    }
+
+    // Moving with pattern
+    {
+        let mut v = vec![];
+        let _: Vec<_> = vec![E::A, E::B(S(0))]
+            .into_iter()
+            .filter_map(|x| {
+                if let E::B(y) = x {
+                    if y.0 % 2 == 0 {
+                        v.push(y);
+                    }
+                    return None;
+                }
+                Some(x)
+            })
+            .collect();
+    }
 }