From 20e4c7452135275ad69f412d59848239f8006931 Mon Sep 17 00:00:00 2001 From: maekawatoshiki Date: Tue, 13 Feb 2024 03:01:56 +0900 Subject: [PATCH 1/4] Handle false positive with map_clone --- clippy_lints/src/methods/map_clone.rs | 10 ++++++++-- tests/ui/map_clone.fixed | 16 +++++++++++++++- tests/ui/map_clone.rs | 16 +++++++++++++++- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 27e17b43b01c..0378c8171c07 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -86,8 +86,11 @@ pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_ } } }, - hir::ExprKind::Call(call, [_]) => { - if let hir::ExprKind::Path(qpath) = call.kind { + hir::ExprKind::Call(call, args) => { + if let hir::ExprKind::Path(qpath) = call.kind + && let [arg] = args + && ident_eq(name, arg) + { handle_path(cx, call, &qpath, e, recv); } }, @@ -119,6 +122,9 @@ fn handle_path( && let args = args.as_slice() && let Some(ty) = args.iter().find_map(|generic_arg| generic_arg.as_type()) && ty.is_ref() + && 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)) { lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs())); } diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index 395eea69294d..e58b6b2f19ea 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -6,7 +6,8 @@ clippy::redundant_clone, clippy::redundant_closure, clippy::useless_asref, - clippy::useless_vec + clippy::useless_vec, + clippy::empty_loop )] fn main() { @@ -117,4 +118,17 @@ fn main() { let y = x.as_ref().map(|x| String::clone(x)); let x: Result = Ok(String::new()); let y = x.as_ref().map(|x| String::clone(x)); + + // Issue #12271 + { + // Don't lint these + let x: Option<&u8> = None; + let y = x.map(|x| String::clone(loop {})); + let x: Option<&u8> = None; + let y = x.map(|x| u8::clone(loop {})); + let x: Vec<&u8> = vec![]; + let y = x.into_iter().map(|x| String::clone(loop {})); + let x: Vec<&u8> = vec![]; + let y = x.into_iter().map(|x| u8::clone(loop {})); + } } diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index 82a103edf5a9..e642e4046f8b 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -6,7 +6,8 @@ clippy::redundant_clone, clippy::redundant_closure, clippy::useless_asref, - clippy::useless_vec + clippy::useless_vec, + clippy::empty_loop )] fn main() { @@ -117,4 +118,17 @@ fn main() { let y = x.as_ref().map(|x| String::clone(x)); let x: Result = Ok(String::new()); let y = x.as_ref().map(|x| String::clone(x)); + + // Issue #12271 + { + // Don't lint these + let x: Option<&u8> = None; + let y = x.map(|x| String::clone(loop {})); + let x: Option<&u8> = None; + let y = x.map(|x| u8::clone(loop {})); + let x: Vec<&u8> = vec![]; + let y = x.into_iter().map(|x| String::clone(loop {})); + let x: Vec<&u8> = vec![]; + let y = x.into_iter().map(|x| u8::clone(loop {})); + } } From 560a5a8cd1f93c0fd258e3c984ce957c80d9de8f Mon Sep 17 00:00:00 2001 From: maekawatoshiki Date: Tue, 13 Feb 2024 11:07:06 +0900 Subject: [PATCH 2/4] Fix logic --- clippy_lints/src/methods/map_clone.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 0378c8171c07..8d2a2410ed42 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -121,10 +121,10 @@ fn handle_path( if 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()) - && ty.is_ref() && 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)) + && let ty::Adt(_, args) = cx.typeck_results().expr_ty(e).kind() + && let args = args.as_slice() + && args.iter().find_map(|generic_arg| generic_arg.as_type()) == Some(*ty) { lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs())); } From c5d3b62cfc0cc4b743e27d03b5532c9aa0321db1 Mon Sep 17 00:00:00 2001 From: maekawatoshiki Date: Thu, 14 Mar 2024 12:40:33 +0900 Subject: [PATCH 3/4] Fix conflict --- tests/ui/map_clone.stderr | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr index 1a26a26a4cac..d9e025de4abe 100644 --- a/tests/ui/map_clone.stderr +++ b/tests/ui/map_clone.stderr @@ -1,5 +1,5 @@ error: you are using an explicit closure for copying elements - --> tests/ui/map_clone.rs:13:22 + --> tests/ui/map_clone.rs:14:22 | LL | let _: Vec = vec![5_i8; 6].iter().map(|x| *x).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![5_i8; 6].iter().copied()` @@ -8,85 +8,85 @@ LL | let _: Vec = vec![5_i8; 6].iter().map(|x| *x).collect(); = help: to override `-D warnings` add `#[allow(clippy::map_clone)]` error: you are using an explicit closure for cloning elements - --> tests/ui/map_clone.rs:14:26 + --> tests/ui/map_clone.rs:15:26 | LL | let _: Vec = vec![String::new()].iter().map(|x| x.clone()).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `vec![String::new()].iter().cloned()` error: you are using an explicit closure for copying elements - --> tests/ui/map_clone.rs:15:23 + --> tests/ui/map_clone.rs:16:23 | LL | let _: Vec = vec![42, 43].iter().map(|&x| x).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `vec![42, 43].iter().copied()` error: you are using an explicit closure for copying elements - --> tests/ui/map_clone.rs:17:26 + --> tests/ui/map_clone.rs:18:26 | LL | let _: Option = Some(&16).map(|b| *b); | ^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&16).copied()` error: you are using an explicit closure for copying elements - --> tests/ui/map_clone.rs:18:25 + --> tests/ui/map_clone.rs:19:25 | LL | let _: Option = Some(&1).map(|x| x.clone()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `Some(&1).copied()` error: you are needlessly cloning iterator elements - --> tests/ui/map_clone.rs:29:29 + --> tests/ui/map_clone.rs:30:29 | LL | let _ = std::env::args().map(|v| v.clone()); | ^^^^^^^^^^^^^^^^^^^ help: remove the `map` call error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:68:13 + --> tests/ui/map_clone.rs:69:13 | LL | let y = x.map(|x| String::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:70:13 + --> tests/ui/map_clone.rs:71:13 | LL | let y = x.map(Clone::clone); | ^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:73:13 + --> tests/ui/map_clone.rs:74:13 | LL | let y = x.map(String::clone); | ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:79:13 + --> tests/ui/map_clone.rs:80:13 | LL | let y = x.map(|x| u32::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:82:13 + --> tests/ui/map_clone.rs:83:13 | LL | let y = x.map(|x| Clone::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:94:13 + --> tests/ui/map_clone.rs:95:13 | LL | let y = x.map(|x| String::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:97:13 + --> tests/ui/map_clone.rs:98:13 | LL | let y = x.map(|x| Clone::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:103:13 + --> tests/ui/map_clone.rs:104:13 | LL | let y = x.map(|x| u32::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()` error: you are explicitly cloning with `.map()` - --> tests/ui/map_clone.rs:106:13 + --> tests/ui/map_clone.rs:107:13 | LL | let y = x.map(|x| Clone::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `copied` method: `x.copied()` From 5f8d8f165606775fcf81dfb27ac9de19f60a7bee Mon Sep 17 00:00:00 2001 From: maekawatoshiki Date: Thu, 14 Mar 2024 12:48:34 +0900 Subject: [PATCH 4/4] Simplify logic --- clippy_lints/src/methods/map_clone.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 8d2a2410ed42..c3c7a3a00330 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -122,9 +122,8 @@ fn handle_path( && 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::Adt(_, args) = cx.typeck_results().expr_ty(e).kind() - && let args = args.as_slice() - && args.iter().find_map(|generic_arg| generic_arg.as_type()) == Some(*ty) + && let ty::FnDef(_, lst) = cx.typeck_results().expr_ty(arg).kind() + && lst.iter().all(|l| l.as_type() == Some(*ty)) { lint_path(cx, e.span, recv.span, is_copy(cx, ty.peel_refs())); }