Auto merge of #8479 - smoelius:unnecessary-filter-map, r=llogiq

Fix some `unnecessary_filter_map` false positives

This is a proposed fix for #4433.

It moves `clone_or_copy_needed` out of `unnecessary_iter_cloned.rs` and into `methods::utils`. It then adds a check of this function to `unnecessary_filter_map::check`.

Fixes #4433

changelog: none

5 files changed, 228 insertions, 94 deletions

diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs
index 12ad3d8d690..108e143f337 100644
--- a/clippy_lints/src/methods/unnecessary_filter_map.rs
+++ b/clippy_lints/src/methods/unnecessary_filter_map.rs
@@ -1,4 +1,6 @@
+use super::utils::clone_or_copy_needed;
 use clippy_utils::diagnostics::span_lint;
+use clippy_utils::ty::is_copy;
 use clippy_utils::usage::mutated_variables;
 use clippy_utils::{is_lang_ctor, is_trait_method, path_to_local_id};
 use rustc_hir as hir;
@@ -20,6 +22,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<
         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 (clone_or_copy_needed, _) = clone_or_copy_needed(cx, body.params[0].pat, &body.value);
 
         let (mut found_mapping, mut found_filtering) = check_expression(cx, arg_id, &body.value);
 
@@ -28,10 +31,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<
         found_mapping |= return_visitor.found_mapping;
         found_filtering |= return_visitor.found_filtering;
 
+        let in_ty = cx.typeck_results().node_type(body.params[0].hir_id);
         let sugg = if !found_filtering {
             "map"
-        } else if !found_mapping && !mutates_arg {
-            let in_ty = cx.typeck_results().node_type(body.params[0].hir_id);
+        } else if !found_mapping && !mutates_arg && (!clone_or_copy_needed || is_copy(cx, in_ty)) {
             match cx.typeck_results().expr_ty(&body.value).kind() {
                 ty::Adt(adt, subst) if cx.tcx.is_diagnostic_item(sym::Option, adt.did) && in_ty == subst.type_at(0) => {
                     "filter"
diff --git a/clippy_lints/src/methods/unnecessary_iter_cloned.rs b/clippy_lints/src/methods/unnecessary_iter_cloned.rs
index 65e94c5f44a..7a39557ad57 100644
--- a/clippy_lints/src/methods/unnecessary_iter_cloned.rs
+++ b/clippy_lints/src/methods/unnecessary_iter_cloned.rs
@@ -1,14 +1,12 @@
+use super::utils::clone_or_copy_needed;
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::higher::ForLoop;
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{get_associated_type, get_iterator_item_ty, implements_trait};
-use clippy_utils::{fn_def_id, get_parent_expr, path_to_local_id, usage};
+use clippy_utils::{fn_def_id, get_parent_expr};
 use rustc_errors::Applicability;
-use rustc_hir::intravisit::{walk_expr, Visitor};
-use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind, HirId, LangItem, Mutability, Pat};
+use rustc_hir::{def_id::DefId, Expr, ExprKind, LangItem};
 use rustc_lint::LateContext;
-use rustc_middle::hir::nested_filter;
-use rustc_middle::ty;
 use rustc_span::{sym, Symbol};
 
 use super::UNNECESSARY_TO_OWNED;
@@ -100,89 +98,6 @@ pub fn check_for_loop_iter(
     false
 }
 
-/// The core logic of `check_for_loop_iter` above, this function wraps a use of
-/// `CloneOrCopyVisitor`.
-fn clone_or_copy_needed<'tcx>(
-    cx: &LateContext<'tcx>,
-    pat: &Pat<'tcx>,
-    body: &'tcx Expr<'tcx>,
-) -> (bool, Vec<&'tcx Expr<'tcx>>) {
-    let mut visitor = CloneOrCopyVisitor {
-        cx,
-        binding_hir_ids: pat_bindings(pat),
-        clone_or_copy_needed: false,
-        addr_of_exprs: Vec::new(),
-    };
-    visitor.visit_expr(body);
-    (visitor.clone_or_copy_needed, visitor.addr_of_exprs)
-}
-
-/// Returns a vector of all `HirId`s bound by the pattern.
-fn pat_bindings(pat: &Pat<'_>) -> Vec<HirId> {
-    let mut collector = usage::ParamBindingIdCollector {
-        binding_hir_ids: Vec::new(),
-    };
-    collector.visit_pat(pat);
-    collector.binding_hir_ids
-}
-
-/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only
-/// operations performed on `binding_hir_ids` are:
-/// * to take non-mutable references to them
-/// * to use them as non-mutable `&self` in method calls
-/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true
-/// when `CloneOrCopyVisitor` is done visiting.
-struct CloneOrCopyVisitor<'cx, 'tcx> {
-    cx: &'cx LateContext<'tcx>,
-    binding_hir_ids: Vec<HirId>,
-    clone_or_copy_needed: bool,
-    addr_of_exprs: Vec<&'tcx Expr<'tcx>>,
-}
-
-impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> {
-    type NestedFilter = nested_filter::OnlyBodies;
-
-    fn nested_visit_map(&mut self) -> Self::Map {
-        self.cx.tcx.hir()
-    }
-
-    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
-        walk_expr(self, expr);
-        if self.is_binding(expr) {
-            if let Some(parent) = get_parent_expr(self.cx, expr) {
-                match parent.kind {
-                    ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => {
-                        self.addr_of_exprs.push(parent);
-                        return;
-                    },
-                    ExprKind::MethodCall(_, args, _) => {
-                        if_chain! {
-                            if args.iter().skip(1).all(|arg| !self.is_binding(arg));
-                            if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id);
-                            let method_ty = self.cx.tcx.type_of(method_def_id);
-                            let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder();
-                            if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not));
-                            then {
-                                return;
-                            }
-                        }
-                    },
-                    _ => {},
-                }
-            }
-            self.clone_or_copy_needed = true;
-        }
-    }
-}
-
-impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> {
-    fn is_binding(&self, expr: &Expr<'tcx>) -> bool {
-        self.binding_hir_ids
-            .iter()
-            .any(|hir_id| path_to_local_id(expr, *hir_id))
-    }
-}
-
 /// Returns true if the named method is `IntoIterator::into_iter`.
 pub fn is_into_iter(cx: &LateContext<'_>, callee_def_id: DefId) -> bool {
     cx.tcx.lang_items().require(LangItem::IntoIterIntoIter) == Ok(callee_def_id)
diff --git a/clippy_lints/src/methods/utils.rs b/clippy_lints/src/methods/utils.rs
index 63c3273bd68..3015531e843 100644
--- a/clippy_lints/src/methods/utils.rs
+++ b/clippy_lints/src/methods/utils.rs
@@ -1,10 +1,14 @@
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{get_parent_expr, path_to_local_id, usage};
 use if_chain::if_chain;
 use rustc_ast::ast;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
+use rustc_hir::intravisit::{walk_expr, Visitor};
+use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Pat};
 use rustc_lint::LateContext;
+use rustc_middle::hir::nested_filter;
 use rustc_middle::ty::{self, Ty};
 use rustc_span::symbol::sym;
 
@@ -79,3 +83,86 @@ pub(super) fn get_hint_if_single_char_arg(
         }
     }
 }
+
+/// The core logic of `check_for_loop_iter` in `unnecessary_iter_cloned.rs`, this function wraps a
+/// use of `CloneOrCopyVisitor`.
+pub(super) fn clone_or_copy_needed<'tcx>(
+    cx: &LateContext<'tcx>,
+    pat: &Pat<'tcx>,
+    body: &'tcx Expr<'tcx>,
+) -> (bool, Vec<&'tcx Expr<'tcx>>) {
+    let mut visitor = CloneOrCopyVisitor {
+        cx,
+        binding_hir_ids: pat_bindings(pat),
+        clone_or_copy_needed: false,
+        addr_of_exprs: Vec::new(),
+    };
+    visitor.visit_expr(body);
+    (visitor.clone_or_copy_needed, visitor.addr_of_exprs)
+}
+
+/// Returns a vector of all `HirId`s bound by the pattern.
+fn pat_bindings(pat: &Pat<'_>) -> Vec<HirId> {
+    let mut collector = usage::ParamBindingIdCollector {
+        binding_hir_ids: Vec::new(),
+    };
+    collector.visit_pat(pat);
+    collector.binding_hir_ids
+}
+
+/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only
+/// operations performed on `binding_hir_ids` are:
+/// * to take non-mutable references to them
+/// * to use them as non-mutable `&self` in method calls
+/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true
+/// when `CloneOrCopyVisitor` is done visiting.
+struct CloneOrCopyVisitor<'cx, 'tcx> {
+    cx: &'cx LateContext<'tcx>,
+    binding_hir_ids: Vec<HirId>,
+    clone_or_copy_needed: bool,
+    addr_of_exprs: Vec<&'tcx Expr<'tcx>>,
+}
+
+impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> {
+    type NestedFilter = nested_filter::OnlyBodies;
+
+    fn nested_visit_map(&mut self) -> Self::Map {
+        self.cx.tcx.hir()
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        walk_expr(self, expr);
+        if self.is_binding(expr) {
+            if let Some(parent) = get_parent_expr(self.cx, expr) {
+                match parent.kind {
+                    ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => {
+                        self.addr_of_exprs.push(parent);
+                        return;
+                    },
+                    ExprKind::MethodCall(_, args, _) => {
+                        if_chain! {
+                            if args.iter().skip(1).all(|arg| !self.is_binding(arg));
+                            if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id);
+                            let method_ty = self.cx.tcx.type_of(method_def_id);
+                            let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder();
+                            if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not));
+                            then {
+                                return;
+                            }
+                        }
+                    },
+                    _ => {},
+                }
+            }
+            self.clone_or_copy_needed = true;
+        }
+    }
+}
+
+impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> {
+    fn is_binding(&self, expr: &Expr<'tcx>) -> bool {
+        self.binding_hir_ids
+            .iter()
+            .any(|hir_id| path_to_local_id(expr, *hir_id))
+    }
+}
diff --git a/tests/ui/unnecessary_filter_map.rs b/tests/ui/unnecessary_filter_map.rs
index c58181f518d..8e01c2674f1 100644
--- a/tests/ui/unnecessary_filter_map.rs
+++ b/tests/ui/unnecessary_filter_map.rs
@@ -1,3 +1,5 @@
+#![allow(dead_code)]
+
 fn main() {
     let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
     let _ = (0..4).filter_map(|x| {
@@ -19,3 +21,130 @@ fn main() {
 fn filter_map_none_changes_item_type() -> impl Iterator<Item = bool> {
     "".chars().filter_map(|_| None)
 }
+
+// https://github.com/rust-lang/rust-clippy/issues/4433#issue-483920107
+mod comment_483920107 {
+    enum Severity {
+        Warning,
+        Other,
+    }
+
+    struct ServerError;
+
+    impl ServerError {
+        fn severity(&self) -> Severity {
+            Severity::Warning
+        }
+    }
+
+    struct S {
+        warnings: Vec<ServerError>,
+    }
+
+    impl S {
+        fn foo(&mut self, server_errors: Vec<ServerError>) {
+            #[allow(unused_variables)]
+            let errors: Vec<ServerError> = server_errors
+                .into_iter()
+                .filter_map(|se| match se.severity() {
+                    Severity::Warning => {
+                        self.warnings.push(se);
+                        None
+                    },
+                    _ => Some(se),
+                })
+                .collect();
+        }
+    }
+}
+
+// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-611006622
+mod comment_611006622 {
+    struct PendingRequest {
+        reply_to: u8,
+        token: u8,
+        expires: u8,
+        group_id: u8,
+    }
+
+    enum Value {
+        Null,
+    }
+
+    struct Node;
+
+    impl Node {
+        fn send_response(&self, _reply_to: u8, _token: u8, _value: Value) -> &Self {
+            self
+        }
+        fn on_error_warn(&self) -> &Self {
+            self
+        }
+    }
+
+    struct S {
+        pending_requests: Vec<PendingRequest>,
+    }
+
+    impl S {
+        fn foo(&mut self, node: Node, now: u8, group_id: u8) {
+            // "drain_filter"
+            self.pending_requests = self
+                .pending_requests
+                .drain(..)
+                .filter_map(|pending| {
+                    if pending.expires <= now {
+                        return None; // Expired, remove
+                    }
+
+                    if pending.group_id == group_id {
+                        // Matched - reuse strings and remove
+                        node.send_response(pending.reply_to, pending.token, Value::Null)
+                            .on_error_warn();
+                        None
+                    } else {
+                        // Keep waiting
+                        Some(pending)
+                    }
+                })
+                .collect();
+        }
+    }
+}
+
+// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-621925270
+// This extrapolation doesn't reproduce the false positive. Additional context seems necessary.
+mod comment_621925270 {
+    struct Signature(u8);
+
+    fn foo(sig_packets: impl Iterator<Item = Result<Signature, ()>>) -> impl Iterator<Item = u8> {
+        sig_packets.filter_map(|res| match res {
+            Ok(Signature(sig_packet)) => Some(sig_packet),
+            _ => None,
+        })
+    }
+}
+
+// https://github.com/rust-lang/rust-clippy/issues/4433#issuecomment-1052978898
+mod comment_1052978898 {
+    #![allow(clippy::redundant_closure)]
+
+    pub struct S(u8);
+
+    impl S {
+        pub fn consume(self) {
+            println!("yum");
+        }
+    }
+
+    pub fn filter_owned() -> impl Iterator<Item = S> {
+        (0..10).map(|i| S(i)).filter_map(|s| {
+            if s.0 & 1 == 0 {
+                s.consume();
+                None
+            } else {
+                Some(s)
+            }
+        })
+    }
+}
diff --git a/tests/ui/unnecessary_filter_map.stderr b/tests/ui/unnecessary_filter_map.stderr
index 041829c3c78..5585b10ab90 100644
--- a/tests/ui/unnecessary_filter_map.stderr
+++ b/tests/ui/unnecessary_filter_map.stderr
@@ -1,5 +1,5 @@
 error: this `.filter_map` can be written more simply using `.filter`
-  --> $DIR/unnecessary_filter_map.rs:2:13
+  --> $DIR/unnecessary_filter_map.rs:4:13
    |
 LL |     let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -7,7 +7,7 @@ LL |     let _ = (0..4).filter_map(|x| if x > 1 { Some(x) } else { None });
    = note: `-D clippy::unnecessary-filter-map` implied by `-D warnings`
 
 error: this `.filter_map` can be written more simply using `.filter`
-  --> $DIR/unnecessary_filter_map.rs:3:13
+  --> $DIR/unnecessary_filter_map.rs:5:13
    |
 LL |       let _ = (0..4).filter_map(|x| {
    |  _____________^
@@ -19,7 +19,7 @@ LL | |     });
    | |______^
 
 error: this `.filter_map` can be written more simply using `.filter`
-  --> $DIR/unnecessary_filter_map.rs:9:13
+  --> $DIR/unnecessary_filter_map.rs:11:13
    |
 LL |       let _ = (0..4).filter_map(|x| match x {
    |  _____________^
@@ -29,7 +29,7 @@ LL | |     });
    | |______^
 
 error: this `.filter_map` can be written more simply using `.map`
-  --> $DIR/unnecessary_filter_map.rs:14:13
+  --> $DIR/unnecessary_filter_map.rs:16:13
    |
 LL |     let _ = (0..4).filter_map(|x| Some(x + 1));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^