From 77d47e1872bd71ff7133955a91a2594833e307cc Mon Sep 17 00:00:00 2001
From: Ben Boeckel <mathstuf@gmail.com>
Date: Wed, 25 Mar 2020 21:13:24 -0400
Subject: [PATCH] suspicious_map: accept mutation in `map().count()`

If the closure in the `map` call ends up mutating a variable, the call
is assumed to no longer be suspicious.

Given just a function name or path, there's no way to detect that there
is interiour mutability, so the lint still fires in that case. However,
it is now documented as a known problem.

Fixes #5253
---
 clippy_lints/src/methods/mod.rs | 17 ++++++++++++++---
 tests/ui/suspicious_map.rs      | 14 ++++++++++++++
 tests/ui/suspicious_map.stderr  | 12 ++++++++++--
 3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
index 8bba56c675ff..1e3f1b50084c 100644
--- a/clippy_lints/src/methods/mod.rs
+++ b/clippy_lints/src/methods/mod.rs
@@ -1084,7 +1084,8 @@ declare_clippy_lint! {
     /// If the `map` call is intentional, this should be rewritten. Or, if you intend to
     /// drive the iterator to completion, you can just use `for_each` instead.
     ///
-    /// **Known problems:** None
+    /// **Known problems:** Named functions which mutate other data internally are not
+    /// detected as non-suspicious.
     ///
     /// **Example:**
     ///
@@ -1341,7 +1342,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             ["as_mut"] => lint_asref(cx, expr, "as_mut", arg_lists[0]),
             ["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0], method_spans[0]),
             ["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]),
-            ["count", "map"] => lint_suspicious_map(cx, expr),
+            ["count", "map"] => lint_suspicious_map(cx, expr, &arg_lists[1][1]),
             ["assume_init"] => lint_maybe_uninit(cx, &arg_lists[0][0], expr),
             ["unwrap_or", arith @ "checked_add"]
             | ["unwrap_or", arith @ "checked_sub"]
@@ -3142,7 +3143,17 @@ fn is_maybe_uninit_ty_valid(cx: &LateContext<'_, '_>, ty: Ty<'_>) -> bool {
     }
 }
 
-fn lint_suspicious_map(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>) {
+fn lint_suspicious_map<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &hir::Expr<'tcx>, map_arg: &'tcx hir::Expr<'_>) {
+    if let Some(body_id) = cx.tcx.hir().maybe_body_owned_by(map_arg.hir_id) {
+        let closure_body = cx.tcx.hir().body(body_id);
+        if let Some(map_mutated_vars) = mutated_variables(&closure_body.value, cx) {
+            // A variable is used mutably inside of the closure. Suppress the lint.
+            if !map_mutated_vars.is_empty() {
+                return;
+            }
+        }
+    }
+
     span_lint_and_help(
         cx,
         SUSPICIOUS_MAP,
diff --git a/tests/ui/suspicious_map.rs b/tests/ui/suspicious_map.rs
index d838d8fde210..2bea017d67fc 100644
--- a/tests/ui/suspicious_map.rs
+++ b/tests/ui/suspicious_map.rs
@@ -1,5 +1,19 @@
 #![warn(clippy::suspicious_map)]
+#![allow(clippy::redundant_closure)]
 
 fn main() {
     let _ = (0..3).map(|x| x + 2).count();
+
+    // This usage is OK because the `sum` side effect makes the `map` useful.
+    let mut sum = 0;
+    let _ = (0..3).map(|x| sum += x).count();
+
+    // The linter is blind to path-based arguments however.
+    let mut ext_sum = 0;
+    let ext_closure = |x| ext_sum += x;
+    let _ = (0..3).map(ext_closure).count();
+
+    // The linter can see `FnMut` calls however.
+    let mut ext_closure_inner = |x| ext_sum += x;
+    let _ = (0..3).map(|x| ext_closure_inner(x)).count();
 }
diff --git a/tests/ui/suspicious_map.stderr b/tests/ui/suspicious_map.stderr
index e1b4ba40376f..9ea30f5d69b4 100644
--- a/tests/ui/suspicious_map.stderr
+++ b/tests/ui/suspicious_map.stderr
@@ -1,5 +1,5 @@
 error: this call to `map()` won't have an effect on the call to `count()`
-  --> $DIR/suspicious_map.rs:4:13
+  --> $DIR/suspicious_map.rs:5:13
    |
 LL |     let _ = (0..3).map(|x| x + 2).count();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -7,5 +7,13 @@ LL |     let _ = (0..3).map(|x| x + 2).count();
    = note: `-D clippy::suspicious-map` implied by `-D warnings`
    = help: make sure you did not confuse `map` with `filter` or `for_each`
 
-error: aborting due to previous error
+error: this call to `map()` won't have an effect on the call to `count()`
+  --> $DIR/suspicious_map.rs:14:13
+   |
+LL |     let _ = (0..3).map(ext_closure).count();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: make sure you did not confuse `map` with `filter` or `for_each`
+
+error: aborting due to 2 previous errors