From 4ab6924bca42b5d6c5753dd3d3a9e5a2b9eae258 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 6 Jan 2024 19:32:52 +0100 Subject: [PATCH 1/7] Extend `useless_asref` lint on `map(clone)` --- clippy_lints/src/methods/useless_asref.rs | 135 +++++++++++++++++++++- 1 file changed, 132 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/useless_asref.rs b/clippy_lints/src/methods/useless_asref.rs index 84ee64e88a6..f3aa8858250 100644 --- a/clippy_lints/src/methods/useless_asref.rs +++ b/clippy_lints/src/methods/useless_asref.rs @@ -1,19 +1,52 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::walk_ptrs_ty_depth; -use clippy_utils::{get_parent_expr, is_trait_method}; +use clippy_utils::{get_parent_expr, is_diag_trait_item, match_def_path, paths, peel_blocks}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; -use rustc_span::sym; +use rustc_middle::ty::adjustment::Adjust; +use rustc_middle::ty::{Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor}; +use rustc_span::{sym, Span}; + +use core::ops::ControlFlow; use super::USELESS_ASREF; +/// Returns the first type inside the `Option`/`Result` type passed as argument. +fn get_enum_ty(enum_ty: Ty<'_>) -> Option> { + struct ContainsTyVisitor { + level: usize, + } + + impl<'tcx> TypeVisitor> for ContainsTyVisitor { + type BreakTy = Ty<'tcx>; + + fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow { + self.level += 1; + if self.level == 1 { + t.super_visit_with(self) + } else { + ControlFlow::Break(t) + } + } + } + + match enum_ty.visit_with(&mut ContainsTyVisitor { level: 0 }) { + ControlFlow::Break(ty) => Some(ty), + ControlFlow::Continue(()) => None, + } +} + /// Checks for the `USELESS_ASREF` lint. pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, recvr: &hir::Expr<'_>) { // when we get here, we've already checked that the call name is "as_ref" or "as_mut" // check if the call is to the actual `AsRef` or `AsMut` trait - if is_trait_method(cx, expr, sym::AsRef) || is_trait_method(cx, expr, sym::AsMut) { + let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else { + return; + }; + + if is_diag_trait_item(cx, def_id, sym::AsRef) || is_diag_trait_item(cx, def_id, sym::AsMut) { // check if the type after `as_ref` or `as_mut` is the same as before let rcv_ty = cx.typeck_results().expr_ty(recvr); let res_ty = cx.typeck_results().expr_ty(expr); @@ -39,5 +72,101 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, applicability, ); } + } else if match_def_path(cx, def_id, &["core", "option", "Option", call_name]) + || match_def_path(cx, def_id, &["core", "result", "Result", call_name]) + { + let rcv_ty = cx.typeck_results().expr_ty(recvr).peel_refs(); + let res_ty = cx.typeck_results().expr_ty(expr).peel_refs(); + + if let Some(rcv_ty) = get_enum_ty(rcv_ty) + && let Some(res_ty) = get_enum_ty(res_ty) + // If the only thing the `as_mut`/`as_ref` call is doing is adding references and not + // changing the type, then we can move forward. + && rcv_ty.peel_refs() == res_ty.peel_refs() + && let Some(parent) = get_parent_expr(cx, expr) + && let hir::ExprKind::MethodCall(segment, _, args, _) = parent.kind + && segment.ident.span != expr.span + // We check that the called method name is `map`. + && segment.ident.name == sym::map + // And that it only has one argument. + && let [arg] = args + { + match arg.kind { + hir::ExprKind::Closure(&hir::Closure { body, .. }) => { + // If it's a closure, we need to check what is called. + let closure_body = cx.tcx.hir().body(body); + let closure_expr = peel_blocks(closure_body.value); + match closure_expr.kind { + hir::ExprKind::MethodCall(method, obj, [], _) => { + if 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) + // We check it's the `Clone` trait. + && 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(..)))) + { + lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name); + } + }, + hir::ExprKind::Call(call, [_]) => { + if let hir::ExprKind::Path(qpath) = call.kind { + check_qpath( + cx, + expr.span.with_hi(parent.span.hi()), + recvr, + call_name, + qpath, + call.hir_id, + ); + } + }, + _ => {}, + } + }, + hir::ExprKind::Path(qpath) => check_qpath( + cx, + expr.span.with_hi(parent.span.hi()), + recvr, + call_name, + qpath, + arg.hir_id, + ), + _ => {}, + } + } } } + +fn check_qpath( + cx: &LateContext<'_>, + span: Span, + recvr: &hir::Expr<'_>, + call_name: &str, + qpath: hir::QPath<'_>, + hir_id: hir::HirId, +) { + // We check it's calling the `clone` method of the `Clone` trait. + if let Some(path_def_id) = cx.qpath_res(&qpath, hir_id).opt_def_id() + && match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD) + { + lint_as_ref_clone(cx, span, recvr, call_name); + } +} + +fn lint_as_ref_clone(cx: &LateContext<'_>, span: Span, recvr: &hir::Expr<'_>, call_name: &str) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + USELESS_ASREF, + span, + &format!("this call to `{call_name}.map(...)` does nothing"), + "try", + format!( + "{}.clone()", + snippet_with_applicability(cx, recvr.span, "..", &mut applicability) + ), + applicability, + ); +} From cdd96bc6626c342528b6f6578de176fd6c09f381 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Sat, 6 Jan 2024 19:33:09 +0100 Subject: [PATCH 2/7] Update ui tests for `useless_asref` lint extension --- tests/ui/map_clone.fixed | 7 ++++- tests/ui/map_clone.rs | 7 ++++- tests/ui/map_clone.stderr | 20 +++++++++++--- tests/ui/useless_asref.fixed | 14 ++++++---- tests/ui/useless_asref.rs | 14 ++++++---- tests/ui/useless_asref.stderr | 51 ++++++++++++++++++----------------- 6 files changed, 73 insertions(+), 40 deletions(-) diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index b7cf12c1a84..74e4ea5ce01 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -63,6 +63,11 @@ fn main() { } let x = Some(String::new()); - let y = x.as_ref().cloned(); + let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint. + let y = x.cloned(); + //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.cloned(); + //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.cloned(); //~^ ERROR: you are explicitly cloning with `.map()` } diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index 693914ec30a..0aac8024f67 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -63,6 +63,11 @@ fn main() { } let x = Some(String::new()); - let y = x.as_ref().map(|x| String::clone(x)); + let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint. + let y = x.map(|x| String::clone(x)); + //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.map(Clone::clone); + //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.map(String::clone); //~^ ERROR: you are explicitly cloning with `.map()` } diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr index e6192a487ab..aae4dae634a 100644 --- a/tests/ui/map_clone.stderr +++ b/tests/ui/map_clone.stderr @@ -38,10 +38,22 @@ LL | let _ = std::env::args().map(|v| v.clone()); | ^^^^^^^^^^^^^^^^^^^ help: remove the `map` call error: you are explicitly cloning with `.map()` - --> $DIR/map_clone.rs:66:13 + --> $DIR/map_clone.rs:67:13 | -LL | let y = x.as_ref().map(|x| String::clone(x)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.as_ref().cloned()` +LL | let y = x.map(|x| String::clone(x)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` -error: aborting due to 7 previous errors +error: you are explicitly cloning with `.map()` + --> $DIR/map_clone.rs:69:13 + | +LL | let y = x.map(Clone::clone); + | ^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` + +error: you are explicitly cloning with `.map()` + --> $DIR/map_clone.rs:71:13 + | +LL | let y = x.map(String::clone); + | ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` + +error: aborting due to 9 previous errors diff --git a/tests/ui/useless_asref.fixed b/tests/ui/useless_asref.fixed index 9083ae2d0b8..88b95095bc0 100644 --- a/tests/ui/useless_asref.fixed +++ b/tests/ui/useless_asref.fixed @@ -2,7 +2,9 @@ #![allow( clippy::explicit_auto_deref, clippy::uninlined_format_args, - clippy::needless_pass_by_ref_mut + clippy::map_clone, + clippy::needless_pass_by_ref_mut, + clippy::redundant_closure )] use std::fmt::Debug; @@ -134,10 +136,12 @@ fn generic_ok + AsRef + ?Sized, T: Debug + ?Sized>(mru: &mut U) { 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()` + let z = x.clone(); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + let z = x.clone(); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + let z = x.clone(); + //~^ ERROR: this call to `as_ref.map(...)` does nothing } fn main() { diff --git a/tests/ui/useless_asref.rs b/tests/ui/useless_asref.rs index 65f361dc2ae..504dc1f5cbf 100644 --- a/tests/ui/useless_asref.rs +++ b/tests/ui/useless_asref.rs @@ -2,7 +2,9 @@ #![allow( clippy::explicit_auto_deref, clippy::uninlined_format_args, - clippy::needless_pass_by_ref_mut + clippy::map_clone, + clippy::needless_pass_by_ref_mut, + clippy::redundant_closure )] use std::fmt::Debug; @@ -134,10 +136,12 @@ fn generic_ok + AsRef + ?Sized, T: Debug + ?Sized>(mru: &mut U) { 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()` + let z = x.as_ref().map(String::clone); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + let z = x.as_ref().map(|z| z.clone()); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + let z = x.as_ref().map(|z| String::clone(z)); + //~^ ERROR: this call to `as_ref.map(...)` does nothing } fn main() { diff --git a/tests/ui/useless_asref.stderr b/tests/ui/useless_asref.stderr index e91a9db4e3d..deb5d90f2f6 100644 --- a/tests/ui/useless_asref.stderr +++ b/tests/ui/useless_asref.stderr @@ -1,5 +1,5 @@ error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:46:18 + --> $DIR/useless_asref.rs:48:18 | LL | foo_rstr(rstr.as_ref()); | ^^^^^^^^^^^^^ help: try: `rstr` @@ -11,79 +11,82 @@ LL | #![deny(clippy::useless_asref)] | ^^^^^^^^^^^^^^^^^^^^^ error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:48:20 + --> $DIR/useless_asref.rs:50:20 | LL | foo_rslice(rslice.as_ref()); | ^^^^^^^^^^^^^^^ help: try: `rslice` error: this call to `as_mut` does nothing - --> $DIR/useless_asref.rs:52:21 + --> $DIR/useless_asref.rs:54:21 | LL | foo_mrslice(mrslice.as_mut()); | ^^^^^^^^^^^^^^^^ help: try: `mrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:54:20 + --> $DIR/useless_asref.rs:56:20 | LL | foo_rslice(mrslice.as_ref()); | ^^^^^^^^^^^^^^^^ help: try: `mrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:61:20 + --> $DIR/useless_asref.rs:63:20 | LL | foo_rslice(rrrrrslice.as_ref()); | ^^^^^^^^^^^^^^^^^^^ help: try: `rrrrrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:63:18 + --> $DIR/useless_asref.rs:65:18 | LL | foo_rstr(rrrrrstr.as_ref()); | ^^^^^^^^^^^^^^^^^ help: try: `rrrrrstr` error: this call to `as_mut` does nothing - --> $DIR/useless_asref.rs:68:21 + --> $DIR/useless_asref.rs:70:21 | LL | foo_mrslice(mrrrrrslice.as_mut()); | ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:70:20 + --> $DIR/useless_asref.rs:72:20 | LL | foo_rslice(mrrrrrslice.as_ref()); | ^^^^^^^^^^^^^^^^^^^^ help: try: `mrrrrrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:74:16 + --> $DIR/useless_asref.rs:76:16 | LL | foo_rrrrmr((&&&&MoreRef).as_ref()); | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `(&&&&MoreRef)` error: this call to `as_mut` does nothing - --> $DIR/useless_asref.rs:124:13 + --> $DIR/useless_asref.rs:126:13 | LL | foo_mrt(mrt.as_mut()); | ^^^^^^^^^^^^ help: try: `mrt` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:126:12 + --> $DIR/useless_asref.rs:128:12 | LL | foo_rt(mrt.as_ref()); | ^^^^^^^^^^^^ help: try: `mrt` -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()` +error: this call to `as_ref.map(...)` does nothing --> $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()` +LL | let z = x.as_ref().map(String::clone); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()` -error: aborting due to 13 previous errors +error: this call to `as_ref.map(...)` does nothing + --> $DIR/useless_asref.rs:141:13 + | +LL | let z = x.as_ref().map(|z| z.clone()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()` + +error: this call to `as_ref.map(...)` does nothing + --> $DIR/useless_asref.rs:143:13 + | +LL | let z = x.as_ref().map(|z| String::clone(z)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()` + +error: aborting due to 14 previous errors From 8791a28c4ab4d6b5d4f20c6785e8d65ff02c30cc Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 9 Jan 2024 16:35:10 +0100 Subject: [PATCH 3/7] Also handle `Result` type for `map_clone` lint --- clippy_lints/src/methods/map_clone.rs | 1 + tests/ui/map_clone.fixed | 6 ++++++ tests/ui/map_clone.rs | 6 ++++++ tests/ui/map_clone.stderr | 8 +++++++- 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 652d2b80081..33b77cdc33d 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -18,6 +18,7 @@ pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_ if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id) && (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_type_diagnostic_item(cx, cx.tcx.type_of(id).instantiate_identity(), sym::Result) }) || is_diag_trait_item(cx, method_id, sym::Iterator)) { match arg.kind { diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index 74e4ea5ce01..144c42a1cb6 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -70,4 +70,10 @@ fn main() { //~^ ERROR: you are explicitly cloning with `.map()` let y = x.cloned(); //~^ ERROR: you are explicitly cloning with `.map()` + + // Testing with `Result` now. + let x: Result = Ok(String::new()); + let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint. + let y = x.cloned(); + //~^ ERROR: you are explicitly cloning with `.map()` } diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index 0aac8024f67..e264d8ffc53 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -70,4 +70,10 @@ fn main() { //~^ ERROR: you are explicitly cloning with `.map()` let y = x.map(String::clone); //~^ ERROR: you are explicitly cloning with `.map()` + + // Testing with `Result` now. + let x: Result = Ok(String::new()); + let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint. + let y = x.map(|x| String::clone(x)); + //~^ ERROR: you are explicitly cloning with `.map()` } diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr index aae4dae634a..7c6dc08f604 100644 --- a/tests/ui/map_clone.stderr +++ b/tests/ui/map_clone.stderr @@ -55,5 +55,11 @@ error: you are explicitly cloning with `.map()` LL | let y = x.map(String::clone); | ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` -error: aborting due to 9 previous errors +error: you are explicitly cloning with `.map()` + --> $DIR/map_clone.rs:77:13 + | +LL | let y = x.map(|x| String::clone(x)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` + +error: aborting due to 10 previous errors From e90eea78948a13a810a4c6268f85670fdb4533e3 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 9 Jan 2024 16:57:54 +0100 Subject: [PATCH 4/7] Clean up code in `map_clone` and `useless_asref` lints --- clippy_lints/src/methods/map_clone.rs | 19 ++++- clippy_lints/src/methods/useless_asref.rs | 99 ++++++++++------------- 2 files changed, 59 insertions(+), 59 deletions(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 33b77cdc33d..527e557987d 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -5,6 +5,7 @@ use clippy_utils::ty::{is_copy, is_type_diagnostic_item}; use clippy_utils::{is_diag_trait_item, match_def_path, paths, peel_blocks}; use rustc_errors::Applicability; use rustc_hir as hir; +use rustc_hir::def_id::DefId; use rustc_lint::LateContext; use rustc_middle::mir::Mutability; use rustc_middle::ty; @@ -14,12 +15,22 @@ use rustc_span::{sym, Span}; use super::MAP_CLONE; +fn should_run_lint(cx: &LateContext<'_>, method_id: DefId) -> bool { + if is_diag_trait_item(cx, method_id, sym::Iterator) { + return true; + } + if !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_type_diagnostic_item(cx, cx.tcx.type_of(id).instantiate_identity(), sym::Result) + }) { + return false; + } + return true; +} + pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>, msrv: &Msrv) { if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id) - && (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_type_diagnostic_item(cx, cx.tcx.type_of(id).instantiate_identity(), sym::Result) - }) || is_diag_trait_item(cx, method_id, sym::Iterator)) + && should_run_lint(cx, method_id) { match arg.kind { hir::ExprKind::Closure(&hir::Closure { body, .. }) => { diff --git a/clippy_lints/src/methods/useless_asref.rs b/clippy_lints/src/methods/useless_asref.rs index f3aa8858250..c15ca40e8d8 100644 --- a/clippy_lints/src/methods/useless_asref.rs +++ b/clippy_lints/src/methods/useless_asref.rs @@ -91,67 +91,56 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, // And that it only has one argument. && let [arg] = args { - match arg.kind { - hir::ExprKind::Closure(&hir::Closure { body, .. }) => { - // If it's a closure, we need to check what is called. - let closure_body = cx.tcx.hir().body(body); - let closure_expr = peel_blocks(closure_body.value); - match closure_expr.kind { - hir::ExprKind::MethodCall(method, obj, [], _) => { - if 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) - // We check it's the `Clone` trait. - && 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(..)))) - { - lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name); - } - }, - hir::ExprKind::Call(call, [_]) => { - if let hir::ExprKind::Path(qpath) = call.kind { - check_qpath( - cx, - expr.span.with_hi(parent.span.hi()), - recvr, - call_name, - qpath, - call.hir_id, - ); - } - }, - _ => {}, - } - }, - hir::ExprKind::Path(qpath) => check_qpath( - cx, - expr.span.with_hi(parent.span.hi()), - recvr, - call_name, - qpath, - arg.hir_id, - ), - _ => {}, + if is_calling_clone(cx, arg) { + lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name); } } } } -fn check_qpath( - cx: &LateContext<'_>, - span: Span, - recvr: &hir::Expr<'_>, - call_name: &str, - qpath: hir::QPath<'_>, - hir_id: hir::HirId, -) { +fn check_qpath(cx: &LateContext<'_>, qpath: hir::QPath<'_>, hir_id: hir::HirId) -> bool { // We check it's calling the `clone` method of the `Clone` trait. - if let Some(path_def_id) = cx.qpath_res(&qpath, hir_id).opt_def_id() - && match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD) - { - lint_as_ref_clone(cx, span, recvr, call_name); + if let Some(path_def_id) = cx.qpath_res(&qpath, hir_id).opt_def_id() { + match_def_path(cx, path_def_id, &paths::CLONE_TRAIT_METHOD) + } else { + false + } +} + +fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool { + match arg.kind { + hir::ExprKind::Closure(&hir::Closure { body, .. }) => { + // If it's a closure, we need to check what is called. + let closure_body = cx.tcx.hir().body(body); + let closure_expr = peel_blocks(closure_body.value); + match closure_expr.kind { + hir::ExprKind::MethodCall(method, obj, [], _) => { + if 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) + // We check it's the `Clone` trait. + && 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(..)))) + { + true + } else { + false + } + }, + hir::ExprKind::Call(call, [_]) => { + if let hir::ExprKind::Path(qpath) = call.kind { + check_qpath(cx, qpath, call.hir_id) + } else { + false + } + }, + _ => false, + } + }, + hir::ExprKind::Path(qpath) => check_qpath(cx, qpath, arg.hir_id), + _ => false, } } From 83ae5edd50bf81810821fa9ceaf256036305aed9 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 9 Jan 2024 17:31:59 +0100 Subject: [PATCH 5/7] Don't lint with `map_clone` if current type is `Option` or `Result` and method call is `as_ref`. --- clippy_lints/src/methods/map_clone.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 527e557987d..19eea00d545 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -15,22 +15,29 @@ use rustc_span::{sym, Span}; use super::MAP_CLONE; -fn should_run_lint(cx: &LateContext<'_>, method_id: DefId) -> bool { +fn should_run_lint(cx: &LateContext<'_>, e: &hir::Expr<'_>, method_id: DefId) -> bool { if is_diag_trait_item(cx, method_id, sym::Iterator) { return true; } if !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_type_diagnostic_item(cx, cx.tcx.type_of(id).instantiate_identity(), sym::Result) + let identity = cx.tcx.type_of(id).instantiate_identity(); + is_type_diagnostic_item(cx, identity, sym::Option) || is_type_diagnostic_item(cx, identity, sym::Result) }) { return false; } + // We check if the previous method call is `as_ref`. + if let hir::ExprKind::MethodCall(path1, receiver, _, _) = &e.kind + && let hir::ExprKind::MethodCall(path2, _, _, _) = &receiver.kind + { + return path2.ident.name != sym::as_ref || path1.ident.name != sym::map; + } + return true; } pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>, msrv: &Msrv) { if let Some(method_id) = cx.typeck_results().type_dependent_def_id(e.hir_id) - && should_run_lint(cx, method_id) + && should_run_lint(cx, e, method_id) { match arg.kind { hir::ExprKind::Closure(&hir::Closure { body, .. }) => { From 103e8881c6d157d95fe1d95bbdb6feb6828307ad Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 9 Jan 2024 17:39:13 +0100 Subject: [PATCH 6/7] Add tests to ensure that `map_clone` is not emitted if `as_ref().clone()` is present --- tests/ui/map_clone.fixed | 8 ++++++++ tests/ui/map_clone.rs | 8 ++++++++ tests/ui/map_clone.stderr | 28 +++++++++++++++++----------- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/tests/ui/map_clone.fixed b/tests/ui/map_clone.fixed index 144c42a1cb6..08b155a1aea 100644 --- a/tests/ui/map_clone.fixed +++ b/tests/ui/map_clone.fixed @@ -5,6 +5,7 @@ clippy::many_single_char_names, clippy::redundant_clone, clippy::redundant_closure, + clippy::useless_asref, clippy::useless_vec )] @@ -76,4 +77,11 @@ fn main() { let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint. let y = x.cloned(); //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.cloned(); + + // We ensure that no warning is emitted here because `useless_asref` is taking over. + let x = Some(String::new()); + 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)); } diff --git a/tests/ui/map_clone.rs b/tests/ui/map_clone.rs index e264d8ffc53..901d9b278b4 100644 --- a/tests/ui/map_clone.rs +++ b/tests/ui/map_clone.rs @@ -5,6 +5,7 @@ clippy::many_single_char_names, clippy::redundant_clone, clippy::redundant_closure, + clippy::useless_asref, clippy::useless_vec )] @@ -76,4 +77,11 @@ fn main() { let x = x.as_ref(); // We do this to prevent triggering the `useless_asref` lint. let y = x.map(|x| String::clone(x)); //~^ ERROR: you are explicitly cloning with `.map()` + let y = x.map(|x| String::clone(x)); + + // We ensure that no warning is emitted here because `useless_asref` is taking over. + let x = Some(String::new()); + 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)); } diff --git a/tests/ui/map_clone.stderr b/tests/ui/map_clone.stderr index 7c6dc08f604..9d7e9317b58 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 - --> $DIR/map_clone.rs:12:22 + --> $DIR/map_clone.rs:13: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,58 +8,64 @@ 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 - --> $DIR/map_clone.rs:13:26 + --> $DIR/map_clone.rs:14: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 - --> $DIR/map_clone.rs:14:23 + --> $DIR/map_clone.rs:15: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 - --> $DIR/map_clone.rs:16:26 + --> $DIR/map_clone.rs:17: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 - --> $DIR/map_clone.rs:17:25 + --> $DIR/map_clone.rs:18: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 - --> $DIR/map_clone.rs:28:29 + --> $DIR/map_clone.rs:29:29 | LL | let _ = std::env::args().map(|v| v.clone()); | ^^^^^^^^^^^^^^^^^^^ help: remove the `map` call error: you are explicitly cloning with `.map()` - --> $DIR/map_clone.rs:67:13 + --> $DIR/map_clone.rs:68: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()` - --> $DIR/map_clone.rs:69:13 + --> $DIR/map_clone.rs:70:13 | LL | let y = x.map(Clone::clone); | ^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` error: you are explicitly cloning with `.map()` - --> $DIR/map_clone.rs:71:13 + --> $DIR/map_clone.rs:72:13 | LL | let y = x.map(String::clone); | ^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` error: you are explicitly cloning with `.map()` - --> $DIR/map_clone.rs:77:13 + --> $DIR/map_clone.rs:78:13 | LL | let y = x.map(|x| String::clone(x)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` -error: aborting due to 10 previous errors +error: you are explicitly cloning with `.map()` + --> $DIR/map_clone.rs:80:13 + | +LL | let y = x.map(|x| String::clone(x)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `x.cloned()` + +error: aborting due to 11 previous errors From f7ef9a6f1fd1ea4126ec1e9173b23baa8449906c Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 9 Jan 2024 17:58:09 +0100 Subject: [PATCH 7/7] Fix dogfood and add code comments --- clippy_lints/src/methods/map_clone.rs | 14 ++++++++++---- clippy_lints/src/methods/useless_asref.rs | 5 ++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods/map_clone.rs b/clippy_lints/src/methods/map_clone.rs index 19eea00d545..f9f636bbbf7 100644 --- a/clippy_lints/src/methods/map_clone.rs +++ b/clippy_lints/src/methods/map_clone.rs @@ -15,14 +15,20 @@ use rustc_span::{sym, Span}; use super::MAP_CLONE; +// If this `map` is called on an `Option` or a `Result` and the previous call is `as_ref`, we don't +// run this lint because it would overlap with `useless_asref` which provides a better suggestion +// in this case. fn should_run_lint(cx: &LateContext<'_>, e: &hir::Expr<'_>, method_id: DefId) -> bool { if is_diag_trait_item(cx, method_id, sym::Iterator) { return true; } - if !cx.tcx.impl_of_method(method_id).map_or(false, |id| { + // We check if it's an `Option` or a `Result`. + if let Some(id) = cx.tcx.impl_of_method(method_id) { let identity = cx.tcx.type_of(id).instantiate_identity(); - is_type_diagnostic_item(cx, identity, sym::Option) || is_type_diagnostic_item(cx, identity, sym::Result) - }) { + if !is_type_diagnostic_item(cx, identity, sym::Option) && !is_type_diagnostic_item(cx, identity, sym::Result) { + return false; + } + } else { return false; } // We check if the previous method call is `as_ref`. @@ -32,7 +38,7 @@ fn should_run_lint(cx: &LateContext<'_>, e: &hir::Expr<'_>, method_id: DefId) -> return path2.ident.name != sym::as_ref || path1.ident.name != sym::map; } - return true; + true } pub(super) fn check(cx: &LateContext<'_>, e: &hir::Expr<'_>, recv: &hir::Expr<'_>, arg: &hir::Expr<'_>, msrv: &Msrv) { diff --git a/clippy_lints/src/methods/useless_asref.rs b/clippy_lints/src/methods/useless_asref.rs index c15ca40e8d8..66727e5a29d 100644 --- a/clippy_lints/src/methods/useless_asref.rs +++ b/clippy_lints/src/methods/useless_asref.rs @@ -90,10 +90,9 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, && segment.ident.name == sym::map // And that it only has one argument. && let [arg] = args + && is_calling_clone(cx, arg) { - if is_calling_clone(cx, arg) { - lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name); - } + lint_as_ref_clone(cx, expr.span.with_hi(parent.span.hi()), recvr, call_name); } } }