Handle additional cases from #14396

Author: smoelius

clippy_lints/src/methods/implicit_clone.rs

@@ -1,11 +1,12 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_with_context;
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{is_diag_item_method, is_diag_trait_item, peel_middle_ty_refs};
+use clippy_utils::{get_parent_expr, is_diag_item_method, is_diag_trait_item, peel_middle_ty_refs};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
 use rustc_span::sym;
+use std::ops::ControlFlow;
 
 use super::IMPLICIT_CLONE;
 
@@ -20,6 +21,7 @@ pub fn check(cx: &LateContext<'_>, method_name: &str, expr: &hir::Expr<'_>, recv
         && return_type == input_type
         && let Some(clone_trait) = cx.tcx.lang_items().clone_trait()
         && implements_trait(cx, return_type, clone_trait, &[])
+        && is_called_from_map_like(cx, expr).is_none()
     {
         let mut app = Applicability::MachineApplicable;
         let recv_snip = snippet_with_context(cx, recv.span, expr.span.ctxt(), "..", &mut app).0;
@@ -56,3 +58,38 @@ pub fn is_clone_like(cx: &LateContext<'_>, method_name: &str, method_def_id: hir
         _ => false,
     }
 }
+
+fn is_called_from_map_like(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<rustc_span::Span> {
+    // Look for a closure as parent of `expr`, discarding simple blocks
+    let parent_closure = cx
+        .tcx
+        .hir_parent_iter(expr.hir_id)
+        .try_fold(expr.hir_id, |child_hir_id, (_, node)| match node {
+            // Check that the child expression is the only expression in the block
+            hir::Node::Block(block) if block.stmts.is_empty() && block.expr.map(|e| e.hir_id) == Some(child_hir_id) => {
+                ControlFlow::Continue(block.hir_id)
+            },
+            hir::Node::Expr(expr) if matches!(expr.kind, hir::ExprKind::Block(..)) => {
+                ControlFlow::Continue(expr.hir_id)
+            },
+            hir::Node::Expr(expr) if matches!(expr.kind, hir::ExprKind::Closure(_)) => ControlFlow::Break(Some(expr)),
+            _ => ControlFlow::Break(None),
+        })
+        .break_value()?;
+    is_parent_map_like(cx, parent_closure?)
+}
+
+fn is_parent_map_like(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<rustc_span::Span> {
+    if let Some(parent_expr) = get_parent_expr(cx, expr)
+        && let hir::ExprKind::MethodCall(name, _, _, parent_span) = parent_expr.kind
+        && name.ident.name == sym::map
+        && let Some(caller_def_id) = cx.typeck_results().type_dependent_def_id(parent_expr.hir_id)
+        && (is_diag_item_method(cx, caller_def_id, sym::Result)
+            || is_diag_item_method(cx, caller_def_id, sym::Option)
+            || is_diag_trait_item(cx, caller_def_id, sym::Iterator))
+    {
+        Some(parent_span)
+    } else {
+        None
+    }
+}

clippy_lints/src/methods/map_clone.rs

@@ -1,3 +1,4 @@
+use super::implicit_clone::is_clone_like;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::snippet_with_applicability;
@@ -8,8 +9,8 @@ use rustc_hir::def_id::DefId;
 use rustc_hir::{self as hir, LangItem};
 use rustc_lint::LateContext;
 use rustc_middle::mir::Mutability;
-use rustc_middle::ty;
 use rustc_middle::ty::adjustment::Adjust;
+use rustc_middle::ty::{self, Ty};
 use rustc_span::symbol::Ident;
 use rustc_span::{Span, sym};
 
@@ -67,22 +68,29 @@ pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_
                                 }
                             },
                             hir::ExprKind::MethodCall(method, obj, [], _) => {
-                                if ident_eq(name, obj) && method.ident.name == sym::clone
+                                if ident_eq(name, obj)
                                 && let Some(fn_id) = cx.typeck_results().type_dependent_def_id(closure_expr.hir_id)
-                                && let Some(trait_id) = cx.tcx.trait_of_item(fn_id)
-                                && cx.tcx.lang_items().clone_trait() == Some(trait_id)
+                                && (is_clone(cx, method.ident.name, fn_id)
+                                    || is_clone_like(cx, method.ident.as_str(), fn_id))
                                 // no autoderefs
                                 && !cx.typeck_results().expr_adjustments(obj).iter()
                                     .any(|a| matches!(a.kind, Adjust::Deref(Some(..))))
+                                && let obj_ty = cx.typeck_results().expr_ty_adjusted(obj)
+                                && let ty::Ref(_, ty, Mutability::Not) = obj_ty.kind()
+                                // Verify that the method call's output type is the same as its input type. This is to
+                                // avoid cases like `to_string` being called on a `&str`, or `to_vec` being called on a
+                                // slice.
+                                && *ty == cx.typeck_results().expr_ty_adjusted(closure_expr)
                                 {
-                                    let obj_ty = cx.typeck_results().expr_ty(obj);
-                                    if let ty::Ref(_, ty, mutability) = obj_ty.kind() {
-                                        if matches!(mutability, Mutability::Not) {
-                                            let copy = is_copy(cx, *ty);
-                                            lint_explicit_closure(cx, e.span, recv.span, copy, msrv);
-                                        }
+                                    let obj_ty_unadjusted = cx.typeck_results().expr_ty(obj);
+                                    if obj_ty == obj_ty_unadjusted {
+                                        let copy = is_copy(cx, *ty);
+                                        lint_explicit_closure(cx, e.span, recv.span, copy, msrv);
                                     } else {
-                                        lint_needless_cloning(cx, e.span, recv.span);
+                                        // To avoid issue #6299, ensure `obj` is not a mutable reference.
+                                        if !matches!(obj_ty_unadjusted.kind(), ty::Ref(_, _, Mutability::Mut)) {
+                                            lint_needless_cloning(cx, e.span, recv.span);
+                                        }
                                     }
                                 }
                             },
@@ -105,29 +113,66 @@ pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_
     }
 }
 
+fn is_clone(cx: &LateContext<'_>, method: rustc_span::Symbol, fn_id: DefId) -> bool {
+    if method == sym::clone
+        && let Some(trait_id) = cx.tcx.trait_of_item(fn_id)
+        && cx.tcx.lang_items().clone_trait() == Some(trait_id)
+    {
+        true
+    } else {
+        false
+    }
+}
+
 fn handle_path(
     cx: &LateContext<'_>,
     arg: &hir::Expr<'_>,
     qpath: &hir::QPath<'_>,
     e: &hir::Expr<'_>,
     recv: &hir::Expr<'_>,
 ) {
+    let method = || match qpath {
+        hir::QPath::TypeRelative(_, method) => Some(method),
+        _ => None,
+    };
     if let Some(path_def_id) = cx.qpath_res(qpath, arg.hir_id).opt_def_id()
-        && cx.tcx.lang_items().get(LangItem::CloneFn) == Some(path_def_id)
+        && (cx.tcx.lang_items().get(LangItem::CloneFn) == Some(path_def_id)
+            || method().is_some_and(|method| is_clone_like(cx, method.ident.name.as_str(), path_def_id)))
         // The `copied` and `cloned` methods are only available on `&T` and `&mut T` in `Option`
         // and `Result`.
-        && let ty::Adt(_, args) = cx.typeck_results().expr_ty(recv).kind()
-        && let args = args.as_slice()
-        && let Some(ty) = args.iter().find_map(|generic_arg| generic_arg.as_type())
-        && let ty::Ref(_, ty, Mutability::Not) = ty.kind()
-        && let ty::FnDef(_, lst) = cx.typeck_results().expr_ty(arg).kind()
-        && lst.iter().all(|l| l.as_type() == Some(*ty))
-        && !should_call_clone_as_function(cx, *ty)
+        // Previously, `handle_path` would make this determination by checking the type of `recv`.
+        // Specifically, `handle_path` would check whether `recv`'s type was instantiated with a
+        // reference. However, that approach can produce false negatives. Consider
+        // `std::slice::Iter`, for example. Even if it is instantiated with a non-reference type,
+        // its `map` method will expect a function that operates on references.
+        && let Some(ty) = clone_like_referent(cx, arg)
+        && !should_call_clone_as_function(cx, ty)
     {
         lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs()));
     }
 }
 
+/// Determine whether `expr` is function whose input type is `&T` and whose output type is `T`. If
+/// so, return `T`.
+fn clone_like_referent<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option<Ty<'tcx>> {
+    let ty = cx.typeck_results().expr_ty_adjusted(expr);
+    if let ty::FnDef(def_id, generic_args) = ty.kind()
+        && let tys = cx
+            .tcx
+            .fn_sig(def_id)
+            .instantiate(cx.tcx, generic_args)
+            .skip_binder()
+            .inputs_and_output
+        && let [input_ty, output_ty] = tys.as_slice()
+        && let ty::Ref(_, referent_ty, Mutability::Not) = input_ty.kind()
+        && referent_ty == output_ty
+    {
+        Some(*referent_ty)
+    } else {
+        None
+    }
+}
+
 fn ident_eq(name: Ident, path: &hir::Expr<'_>) -> bool {
     if let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = path.kind {
         path.segments.len() == 1 && path.segments[0].ident == name

tests/ui/map_clone.fixed

@@ -158,4 +158,20 @@ fn main() {
         let y = Some(&x);
         let _z = y.map(RcWeak::clone);
     }
+
+    let variable1 = String::new();
+    let v = &variable1;
+    let variable2 = Some(v);
+    let _ = variable2.cloned();
+    //~^ map_clone
+    let _ = variable2.cloned();
+    //~^ map_clone
+    #[rustfmt::skip]
+    let _ = variable2.cloned();
+    //~^ map_clone
+
+    let _ = vec![String::new()].iter().cloned().collect::<Vec<_>>();
+    //~^ map_clone
+    let _ = vec![String::new()].iter().cloned().collect::<Vec<_>>();
+    //~^ map_clone
 }

tests/ui/map_clone.rs

@@ -158,4 +158,20 @@ fn main() {
         let y = Some(&x);
         let _z = y.map(RcWeak::clone);
     }
+
+    let variable1 = String::new();
+    let v = &variable1;
+    let variable2 = Some(v);
+    let _ = variable2.map(String::to_string);
+    //~^ map_clone
+    let _ = variable2.map(|x| x.to_string());
+    //~^ map_clone
+    #[rustfmt::skip]
+    let _ = variable2.map(|x| { x.to_string() });
+    //~^ map_clone
+
+    let _ = vec![String::new()].iter().map(String::to_string).collect::<Vec<_>>();
+    //~^ map_clone
+    let _ = vec![String::new()].iter().map(|x| x.to_string()).collect::<Vec<_>>();
+    //~^ map_clone
 }

tests/ui/map_clone.stderr

@@ -91,5 +91,35 @@ error: you are explicitly cloning with `.map()`
 LL |     let y = x.map(|x| Clone::clone(x));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()`
 
-error: aborting due to 15 previous errors
+error: you are explicitly cloning with `.map()`
+  --> tests/ui/map_clone.rs:165:13
+   |
+LL |     let _ = variable2.map(String::to_string);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `variable2.cloned()`
+
+error: you are using an explicit closure for cloning elements
+  --> tests/ui/map_clone.rs:167:13
+   |
+LL |     let _ = variable2.map(|x| x.to_string());
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `variable2.cloned()`
+
+error: you are using an explicit closure for cloning elements
+  --> tests/ui/map_clone.rs:170:13
+   |
+LL |     let _ = variable2.map(|x| { x.to_string() });
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `variable2.cloned()`
+
+error: you are explicitly cloning with `.map()`
+  --> tests/ui/map_clone.rs:173:13
+   |
+LL |     let _ = vec![String::new()].iter().map(String::to_string).collect::<Vec<_>>();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()`
+
+error: you are using an explicit closure for cloning elements
+  --> tests/ui/map_clone.rs:175:13
+   |
+LL |     let _ = vec![String::new()].iter().map(|x| x.to_string()).collect::<Vec<_>>();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()`
+
+error: aborting due to 20 previous errors