Auto merge of #6831 - camsteffen:suspicious-map, r=Y-Nak,flip1995

Fix suspicious_map false positives

changelog: Fix suspicious_map false positives

Fixes #5253
Replaces #5375

Author: bors

clippy_lints/src/methods/mod.rs

@@ -1739,7 +1739,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                 unnecessary_filter_map::check(cx, expr, arg_lists[0]);
                 filter_map_identity::check(cx, expr, arg_lists[0], method_spans[0]);
             },
-            ["count", "map"] => suspicious_map::check(cx, expr),
+            ["count", "map"] => suspicious_map::check(cx, expr, arg_lists[1], arg_lists[0]),
             ["assume_init"] => uninit_assumed_init::check(cx, &arg_lists[0][0], expr),
             ["unwrap_or", arith @ ("checked_add" | "checked_sub" | "checked_mul")] => {
                 manual_saturating_arithmetic::check(cx, expr, &arg_lists, &arith["checked_".len()..])

clippy_lints/src/methods/suspicious_map.rs

@@ -1,16 +1,41 @@
-use crate::utils::span_lint_and_help;
+use crate::utils::usage::mutated_variables;
+use crate::utils::{expr_or_init, is_trait_method, span_lint_and_help};
+use if_chain::if_chain;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
+use rustc_span::sym;
 
 use super::SUSPICIOUS_MAP;
 
-pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>) {
-    span_lint_and_help(
-        cx,
-        SUSPICIOUS_MAP,
-        expr.span,
-        "this call to `map()` won't have an effect on the call to `count()`",
-        None,
-        "make sure you did not confuse `map` with `filter` or `for_each`",
-    );
+pub fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &hir::Expr<'_>,
+    map_args: &[hir::Expr<'_>],
+    count_args: &[hir::Expr<'_>],
+) {
+    if_chain! {
+        if let [count_recv] = count_args;
+        if let [_, map_arg] = map_args;
+        if is_trait_method(cx, count_recv, sym::Iterator);
+        let closure = expr_or_init(cx, map_arg);
+        if let Some(body_id) = cx.tcx.hir().maybe_body_owned_by(closure.hir_id);
+        let closure_body = cx.tcx.hir().body(body_id);
+        if !cx.typeck_results().expr_ty(&closure_body.value).is_unit();
+        then {
+            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,
+                expr.span,
+                "this call to `map()` won't have an effect on the call to `count()`",
+                None,
+                "make sure you did not confuse `map` with `filter` or `for_each`",
+            );
+        }
+    }
 }

clippy_utils/src/lib.rs

@@ -62,10 +62,10 @@ use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
 use rustc_hir::{
-    def, Arm, Block, Body, Constness, CrateItem, Expr, ExprKind, FnDecl, ForeignItem, GenericArgs, GenericParam, HirId,
-    Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local, MacroDef, MatchSource, Node, Param, Pat,
-    PatKind, Path, PathSegment, QPath, Stmt, StructField, TraitItem, TraitItemKind, TraitRef, TyKind, Unsafety,
-    Variant, Visibility,
+    def, Arm, BindingAnnotation, Block, Body, Constness, CrateItem, Expr, ExprKind, FnDecl, ForeignItem, GenericArgs,
+    GenericParam, HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Lifetime, Local, MacroDef,
+    MatchSource, Node, Param, Pat, PatKind, Path, PathSegment, QPath, Stmt, StructField, TraitItem, TraitItemKind,
+    TraitRef, TyKind, Unsafety, Variant, Visibility,
 };
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::{LateContext, Level, Lint, LintContext};
@@ -138,6 +138,58 @@ pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool {
     rhs.ctxt() != lhs.ctxt()
 }
 
+/// If the given expression is a local binding, find the initializer expression.
+/// If that initializer expression is another local binding, find its initializer again.
+/// This process repeats as long as possible (but usually no more than once). Initializer
+/// expressions with adjustments are ignored. If this is not desired, use [`find_binding_init`]
+/// instead.
+///
+/// Examples:
+/// ```ignore
+/// let abc = 1;
+/// //        ^ output
+/// let def = abc;
+/// dbg!(def)
+/// //   ^^^ input
+///
+/// // or...
+/// let abc = 1;
+/// let def = abc + 2;
+/// //        ^^^^^^^ output
+/// dbg!(def)
+/// //   ^^^ input
+/// ```
+pub fn expr_or_init<'a, 'b, 'tcx: 'b>(cx: &LateContext<'tcx>, mut expr: &'a Expr<'b>) -> &'a Expr<'b> {
+    while let Some(init) = path_to_local(expr)
+        .and_then(|id| find_binding_init(cx, id))
+        .filter(|init| cx.typeck_results().expr_adjustments(init).is_empty())
+    {
+        expr = init;
+    }
+    expr
+}
+
+/// Finds the initializer expression for a local binding. Returns `None` if the binding is mutable.
+/// By only considering immutable bindings, we guarantee that the returned expression represents the
+/// value of the binding wherever it is referenced.
+///
+/// Example: For `let x = 1`, if the `HirId` of `x` is provided, the `Expr` `1` is returned.
+/// Note: If you have an expression that references a binding `x`, use `path_to_local` to get the
+/// canonical binding `HirId`.
+pub fn find_binding_init<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Option<&'tcx Expr<'tcx>> {
+    let hir = cx.tcx.hir();
+    if_chain! {
+        if let Some(Node::Binding(pat)) = hir.find(hir_id);
+        if matches!(pat.kind, PatKind::Binding(BindingAnnotation::Unannotated, ..));
+        let parent = hir.get_parent_node(hir_id);
+        if let Some(Node::Local(local)) = hir.find(parent);
+        then {
+            return local.init;
+        }
+    }
+    None
+}
+
 /// Returns `true` if the given `NodeId` is inside a constant context
 ///
 /// # Example

tests/ui/suspicious_map.rs

@@ -2,4 +2,31 @@
 
 fn main() {
     let _ = (0..3).map(|x| x + 2).count();
+
+    let f = |x| x + 1;
+    let _ = (0..3).map(f).count();
+}
+
+fn negative() {
+    // closure with side effects
+    let mut sum = 0;
+    let _ = (0..3).map(|x| sum += x).count();
+
+    // closure variable with side effects
+    let ext_closure = |x| sum += x;
+    let _ = (0..3).map(ext_closure).count();
+
+    // closure that returns unit
+    let _ = (0..3)
+        .map(|x| {
+            // do nothing
+        })
+        .count();
+
+    // external function
+    let _ = (0..3).map(do_something).count();
+}
+
+fn do_something<T>(t: T) -> String {
+    unimplemented!()
 }

tests/ui/suspicious_map.stderr

@@ -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:7:13
+   |
+LL |     let _ = (0..3).map(f).count();
+   |             ^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: make sure you did not confuse `map` with `filter` or `for_each`
+
+error: aborting due to 2 previous errors