From 28c133b4bc2795d78c0bc15d99af5e5f44093a2d Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 6 Jan 2024 16:56:24 +0100 Subject: [PATCH 1/3] Extend `map_clone` lint to also work on non-explicit closures --- clippy_lints/src/methods/map_clone.rs | 98 +++++++++++++++++---------- 1 file changed, 64 insertions(+), 34 deletions(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index cc6eeaa86e5a..c8d5a358098b 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -2,7 +2,7 @@ use clippy_config::msrvs::{self, Msrv}; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::{is_copy, is_type_diagnostic_item}; -use clippy_utils::{is_diag_trait_item, peel_blocks}; +use clippy_utils::{is_diag_trait_item, match_def_path, paths, peel_blocks}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; @@ -19,50 +19,63 @@ pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_ && (cx.tcx.impl_of_method(method_id).map_or(false, |id| { is_type_diagnostic_item(cx, cx.tcx.type_of(id).instantiate_identity(), sym::Option) }) || is_diag_trait_item(cx, method_id, sym::Iterator)) - && let hir::ExprKind::Closure(&hir::Closure { body, .. }) = arg.kind { - let closure_body = cx.tcx.hir().body(body); - let closure_expr = peel_blocks(closure_body.value); - match closure_body.params[0].pat.kind { - hir::PatKind::Ref(inner, hir::Mutability::Not) => { - if let hir::PatKind::Binding(hir::BindingAnnotation::NONE, .., name, None) = inner.kind { - if ident_eq(name, closure_expr) { - lint_explicit_closure(cx, e.span, recv.span, true, msrv); - } - } - }, - hir::PatKind::Binding(hir::BindingAnnotation::NONE, .., name, None) => { - match closure_expr.kind { - hir::ExprKind::Unary(hir::UnOp::Deref, inner) => { - if ident_eq(name, inner) { - if let ty::Ref(.., Mutability::Not) = cx.typeck_results().expr_ty(inner).kind() { + match arg.kind { + hir::ExprKind::Closure(&hir::Closure { body, .. }) => { + let closure_body = cx.tcx.hir().body(body); + let closure_expr = peel_blocks(closure_body.value); + match closure_body.params[0].pat.kind { + hir::PatKind::Ref(inner, hir::Mutability::Not) => { + if let hir::PatKind::Binding(hir::BindingAnnotation::NONE, .., name, None) = inner.kind { + if ident_eq(name, closure_expr) { lint_explicit_closure(cx, e.span, recv.span, true, msrv); } } }, - hir::ExprKind::MethodCall(method, obj, [], _) => { - if ident_eq(name, obj) && method.ident.name == sym::clone - && 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().map_or(false, |id| id == trait_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(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); + hir::PatKind::Binding(hir::BindingAnnotation::NONE, .., name, None) => { + match closure_expr.kind { + hir::ExprKind::Unary(hir::UnOp::Deref, inner) => { + if ident_eq(name, inner) { + if let ty::Ref(.., Mutability::Not) = cx.typeck_results().expr_ty(inner).kind() { + lint_explicit_closure(cx, e.span, recv.span, true, msrv); + } } - } else { - lint_needless_cloning(cx, e.span, recv.span); - } + }, + hir::ExprKind::MethodCall(method, obj, [], _) => { + if ident_eq(name, obj) && method.ident.name == sym::clone + && 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().map_or(false, |id| id == trait_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(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); + } + } else { + lint_needless_cloning(cx, e.span, recv.span); + } + } + }, + _ => {}, } }, _ => {}, } }, + hir::ExprKind::Path(qpath) => { + if let Some(path_def_id) = cx.qpath_res(&qpath, arg.hir_id).opt_def_id() + && match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD) + { + // FIXME: It would be better to infer the type to check if it's copyable or not + // to suggest to use `.copied()` instead of `.cloned()` where applicable. + lint_path(cx, e.span, recv.span); + } + }, _ => {}, } } @@ -88,6 +101,23 @@ fn lint_needless_cloning(cx: &LateContext<'_>, root: Span, receiver: Span) { ); } +fn lint_path(cx: &LateContext<'_>, replace: Span, root: Span) { + let mut applicability = Applicability::MachineApplicable; + + span_lint_and_sugg( + cx, + MAP_CLONE, + replace, + "you are explicitly cloning with `.map()`", + "consider calling the dedicated `cloned` method", + format!( + "{}.cloned()", + snippet_with_applicability(cx, root, "..", &mut applicability), + ), + applicability, + ); +} + fn lint_explicit_closure(cx: &LateContext<'_>, replace: Span, root: Span, is_copy: bool, msrv: &Msrv) { let mut applicability = Applicability::MachineApplicable; From 6410606815c64946d93ad51d5de968a960364961 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 6 Jan 2024 16:56:38 +0100 Subject: [PATCH 2/3] Update `map_clone` lint ui test --- tests/ui/useless_asref.fixed | 8 ++++++++ tests/ui/useless_asref.rs | 8 ++++++++ tests/ui/useless_asref.stderr | 17 ++++++++++++++++- 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/tests/ui/useless_asref.fixed b/tests/ui/useless_asref.fixed index f6770558bd80..9083ae2d0b86 100644 --- a/tests/ui/useless_asref.fixed +++ b/tests/ui/useless_asref.fixed @@ -132,6 +132,14 @@ fn generic_ok + AsRef + ?Sized, T: Debug + ?Sized>(mru: &mut U) { foo_rt(mru.as_ref()); } +fn foo() { + let x = Some(String::new()); + let y = x.as_ref().cloned(); + //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.as_ref().cloned(); + //~^ ERROR: you are explicitly cloning with `.map()` +} + fn main() { not_ok(); ok(); diff --git a/tests/ui/useless_asref.rs b/tests/ui/useless_asref.rs index 0996218076b8..65f361dc2aeb 100644 --- a/tests/ui/useless_asref.rs +++ b/tests/ui/useless_asref.rs @@ -132,6 +132,14 @@ fn generic_ok + AsRef + ?Sized, T: Debug + ?Sized>(mru: &mut U) { foo_rt(mru.as_ref()); } +fn foo() { + let x = Some(String::new()); + let y = x.as_ref().map(Clone::clone); + //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.as_ref().map(String::clone); + //~^ ERROR: you are explicitly cloning with `.map()` +} + fn main() { not_ok(); ok(); diff --git a/tests/ui/useless_asref.stderr b/tests/ui/useless_asref.stderr index 163eb7b14374..e91a9db4e3db 100644 --- a/tests/ui/useless_asref.stderr +++ b/tests/ui/useless_asref.stderr @@ -70,5 +70,20 @@ error: this call to `as_ref` does nothing LL | foo_rt(mrt.as_ref()); | ^^^^^^^^^^^^ help: try: `mrt` -error: aborting due to 11 previous errors +error: you are explicitly cloning with `.map()` + --> $DIR/useless_asref.rs:137:13 + | +LL | let y = x.as_ref().map(Clone::clone); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.as_ref().cloned()` + | + = note: `-D clippy::map-clone` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::map_clone)]` + +error: you are explicitly cloning with `.map()` + --> $DIR/useless_asref.rs:139:13 + | +LL | let y = x.as_ref().map(String::clone); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.as_ref().cloned()` + +error: aborting due to 13 previous errors From af35d37749166b550e18a65325deb76cc05a343a Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 6 Jan 2024 17:16:34 +0100 Subject: [PATCH 3/3] Fix new dogfood issue --- clippy_utils/src/macros.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs index 46ce4ffdce5d..c475e7b7c435 100644 --- a/clippy_utils/src/macros.rs +++ b/clippy_utils/src/macros.rs @@ -420,7 +420,7 @@ pub fn find_format_args(cx: &LateContext<'_>, start: &Expr<'_>, expn_id: ExpnId) ast_format_args .get()? .get(&format_args_expr.span.with_parent(None)) - .map(Rc::clone) + .cloned() }) }