suggest &mut for redundant FnMut closures

clippy_lints/src/eta_reduction.rs

@@ -3,13 +3,14 @@ use clippy_utils::higher;
 use clippy_utils::higher::VecArgs;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{implements_trait, type_is_unsafe_function};
-use clippy_utils::{is_adjusted, iter_input_pats};
+use clippy_utils::usage::UsedAfterExprVisitor;
+use clippy_utils::{is_adjusted, iter_input_pats, path_to_local};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{def_id, Expr, ExprKind, Param, PatKind, QPath};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::{self, Ty};
+use rustc_middle::ty::{self, ClosureKind, Ty};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
@@ -86,7 +87,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction {
     }
 }
 
-fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
+fn check_closure<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
     if let ExprKind::Closure(_, decl, eid, _, _) = expr.kind {
         let body = cx.tcx.hir().body(eid);
         let ex = &body.value;
@@ -131,7 +132,18 @@ fn check_closure(cx: &LateContext<'_>, expr: &Expr<'_>) {
 
             then {
                 span_lint_and_then(cx, REDUNDANT_CLOSURE, expr.span, "redundant closure", |diag| {
-                    if let Some(snippet) = snippet_opt(cx, caller.span) {
+                    if let Some(mut snippet) = snippet_opt(cx, caller.span) {
+                        if_chain! {
+                            if let ty::Closure(_, substs) = fn_ty.kind();
+                            if let ClosureKind::FnMut = substs.as_closure().kind();
+                            if let Some(closure_def) = path_to_local(caller);
+                            if UsedAfterExprVisitor::is_found(cx, closure_def, expr);
+
+                            then {
+                                // Mutable closure is used after current expr; we cannot consume it.
+                                snippet = format!("&mut {}", snippet);
+                            }
+                        }
                         diag.span_suggestion(
                             expr.span,
                             "replace the closure with the function itself",

clippy_utils/src/usage.rs

@@ -199,3 +199,59 @@ pub fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool {
     recursive_visitor.visit_expr(expression);
     recursive_visitor.seen_return_break_continue
 }
+
+pub struct UsedAfterExprVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    definition: HirId,
+    past_self: bool,
+    used_after_self: bool,
+}
+impl<'a, 'tcx> UsedAfterExprVisitor<'a, 'tcx> {
+    pub fn is_found(cx: &'a LateContext<'tcx>, definition: HirId, expr: &'tcx Expr<'_>) -> bool {
+        let mut visitor = UsedAfterExprVisitor {
+            cx,
+            expr,
+            definition,
+            past_self: false,
+            used_after_self: false,
+        };
+        if let Some(block) = utils::get_enclosing_block(cx, definition) {
+            visitor.visit_block(block);
+
+            visitor.used_after_self
+        } else {
+            false
+        }
+    }
+}
+
+impl<'a, 'tcx> intravisit::Visitor<'tcx> for UsedAfterExprVisitor<'a, 'tcx> {
+    type Map = Map<'tcx>;
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        if self.used_after_self {
+            return;
+        }
+
+        if expr.hir_id == self.expr.hir_id {
+            self.past_self = true;
+
+            if let Some(Expr {
+                kind: ExprKind::Loop(..),
+                ..
+            }) = utils::get_enclosing_loop_or_closure(self.cx.tcx, expr)
+            {
+                self.used_after_self = true;
+            }
+        } else if self.past_self && utils::path_to_local_id(expr, self.definition) {
+            self.used_after_self = true;
+        } else {
+            intravisit::walk_expr(self, expr);
+        }
+    }
+}

tests/ui/eta.fixed

@@ -220,3 +220,18 @@ impl std::ops::Deref for Bar {
 fn test_deref_with_trait_method() {
     let _ = [Bar].iter().map(|s| s.to_string()).collect::<Vec<_>>();
 }
+
+fn mutable_closure_used_twice(x: Vec<i32>, y: Vec<i32>) {
+    let mut res = Vec::new();
+    let mut add_to_res = |n| res.push(n);
+    x.into_iter().for_each(&mut add_to_res);
+    y.into_iter().for_each(add_to_res);
+}
+
+fn mutable_closure_in_loop() {
+    let mut value = 0;
+    let mut closure = |n| value += n;
+    for _ in 0..5 {
+        Some(1).map(&mut closure);
+    }
+}

tests/ui/eta.rs

@@ -220,3 +220,18 @@ impl std::ops::Deref for Bar {
 fn test_deref_with_trait_method() {
     let _ = [Bar].iter().map(|s| s.to_string()).collect::<Vec<_>>();
 }
+
+fn mutable_closure_used_twice(x: Vec<i32>, y: Vec<i32>) {
+    let mut res = Vec::new();
+    let mut add_to_res = |n| res.push(n);
+    x.into_iter().for_each(|x| add_to_res(x));
+    y.into_iter().for_each(|x| add_to_res(x));
+}
+
+fn mutable_closure_in_loop() {
+    let mut value = 0;
+    let mut closure = |n| value += n;
+    for _ in 0..5 {
+        Some(1).map(|n| closure(n));
+    }
+}

tests/ui/eta.stderr

@@ -82,5 +82,23 @@ error: redundant closure
 LL |     let a = Some(1u8).map(|a| closure(a));
    |                           ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `closure`
 
-error: aborting due to 13 previous errors
+error: redundant closure
+  --> $DIR/eta.rs:227:28
+   |
+LL |     x.into_iter().for_each(|x| add_to_res(x));
+   |                            ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut add_to_res`
+
+error: redundant closure
+  --> $DIR/eta.rs:228:28
+   |
+LL |     y.into_iter().for_each(|x| add_to_res(x));
+   |                            ^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `add_to_res`
+
+error: redundant closure
+  --> $DIR/eta.rs:235:21
+   |
+LL |         Some(1).map(|n| closure(n));
+   |                     ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut closure`
+
+error: aborting due to 16 previous errors