Check clone_or_copy_needed in unnecessary_filter_map::check

smoelius · smoelius · commit d123ffc6c754 · 2022-02-28T05:24:00.000-05:00
diff --git 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
@@ -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
@@ -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))
+    }
+}
