From 153b83f61bc064dd09d23927784bab2c18f7db54 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Fri, 12 Jan 2024 13:39:42 +0100 Subject: [PATCH 1/2] [`useless_asref`]: check that the clone receiver is the local --- clippy_lints/src/methods/useless_asref.rs | 26 ++++++++++++++++--- tests/ui/useless_asref.fixed | 31 +++++++++++++++++++++++ tests/ui/useless_asref.rs | 31 +++++++++++++++++++++++ tests/ui/useless_asref.stderr | 20 ++++++++++++++- 4 files changed, 104 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/useless_asref.rs b/clippy_lints/src/methods/useless_asref.rs index 66727e5a29d..d6c79db4d70 100644 --- a/clippy_lints/src/methods/useless_asref.rs +++ b/clippy_lints/src/methods/useless_asref.rs @@ -1,7 +1,7 @@ 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_diag_trait_item, match_def_path, paths, peel_blocks}; +use clippy_utils::{get_parent_expr, is_diag_trait_item, match_def_path, path_to_local_id, paths, peel_blocks}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; @@ -111,6 +111,23 @@ fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool { 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); + + // |x| ... + // ^ + let [ + hir::Param { + pat: + hir::Pat { + kind: hir::PatKind::Binding(_, local_id, ..), + .. + }, + .. + }, + ] = closure_body.params + else { + return false; + }; + let closure_expr = peel_blocks(closure_body.value); match closure_expr.kind { hir::ExprKind::MethodCall(method, obj, [], _) => { @@ -122,14 +139,17 @@ fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool { // no autoderefs && !cx.typeck_results().expr_adjustments(obj).iter() .any(|a| matches!(a.kind, Adjust::Deref(Some(..)))) + && path_to_local_id(obj, *local_id) { true } else { false } }, - hir::ExprKind::Call(call, [_]) => { - if let hir::ExprKind::Path(qpath) = call.kind { + hir::ExprKind::Call(call, [recv]) => { + if let hir::ExprKind::Path(qpath) = call.kind + && path_to_local_id(recv, *local_id) + { check_qpath(cx, qpath, call.hir_id) } else { false diff --git a/tests/ui/useless_asref.fixed b/tests/ui/useless_asref.fixed index 88b95095bc0..d39d8a74b26 100644 --- a/tests/ui/useless_asref.fixed +++ b/tests/ui/useless_asref.fixed @@ -144,6 +144,37 @@ fn foo() { //~^ ERROR: this call to `as_ref.map(...)` does nothing } +mod issue12135 { + pub struct Struct { + field: Option, + } + + #[derive(Clone)] + pub struct Foo; + + #[derive(Clone)] + struct InnerStruct { + x: Foo, + } + + impl InnerStruct { + fn method(&self) -> &Foo { + &self.x + } + } + + pub fn f(x: &Struct) -> Option { + x.field.clone(); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + x.field.clone(); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + x.field.clone(); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + + x.field.as_ref().map(|v| v.method().clone()) + } +} + fn main() { not_ok(); ok(); diff --git a/tests/ui/useless_asref.rs b/tests/ui/useless_asref.rs index 504dc1f5cbf..76277e7b7a4 100644 --- a/tests/ui/useless_asref.rs +++ b/tests/ui/useless_asref.rs @@ -144,6 +144,37 @@ fn foo() { //~^ ERROR: this call to `as_ref.map(...)` does nothing } +mod issue12135 { + pub struct Struct { + field: Option, + } + + #[derive(Clone)] + pub struct Foo; + + #[derive(Clone)] + struct InnerStruct { + x: Foo, + } + + impl InnerStruct { + fn method(&self) -> &Foo { + &self.x + } + } + + pub fn f(x: &Struct) -> Option { + x.field.as_ref().map(|v| v.clone()); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + x.field.as_ref().map(Clone::clone); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + x.field.as_ref().map(|v| Clone::clone(v)); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + + x.field.as_ref().map(|v| v.method().clone()) + } +} + fn main() { not_ok(); ok(); diff --git a/tests/ui/useless_asref.stderr b/tests/ui/useless_asref.stderr index deb5d90f2f6..583b48732d4 100644 --- a/tests/ui/useless_asref.stderr +++ b/tests/ui/useless_asref.stderr @@ -88,5 +88,23 @@ error: this call to `as_ref.map(...)` does nothing LL | let z = x.as_ref().map(|z| String::clone(z)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.clone()` -error: aborting due to 14 previous errors +error: this call to `as_ref.map(...)` does nothing + --> $DIR/useless_asref.rs:167:9 + | +LL | x.field.as_ref().map(|v| v.clone()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()` + +error: this call to `as_ref.map(...)` does nothing + --> $DIR/useless_asref.rs:169:9 + | +LL | x.field.as_ref().map(Clone::clone); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()` + +error: this call to `as_ref.map(...)` does nothing + --> $DIR/useless_asref.rs:171:9 + | +LL | x.field.as_ref().map(|v| Clone::clone(v)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()` + +error: aborting due to 17 previous errors From be5707ce1b608f607ca9327537076dc95869347b Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sat, 13 Jan 2024 17:46:46 +0100 Subject: [PATCH 2/2] lint on `.map(|&x| x.clone())` --- clippy_lints/src/methods/useless_asref.rs | 32 +++++++---------------- tests/ui/useless_asref.fixed | 5 ++++ tests/ui/useless_asref.rs | 5 ++++ tests/ui/useless_asref.stderr | 8 +++++- 4 files changed, 27 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/methods/useless_asref.rs b/clippy_lints/src/methods/useless_asref.rs index d6c79db4d70..514015af045 100644 --- a/clippy_lints/src/methods/useless_asref.rs +++ b/clippy_lints/src/methods/useless_asref.rs @@ -1,7 +1,9 @@ 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_diag_trait_item, match_def_path, path_to_local_id, paths, peel_blocks}; +use clippy_utils::{ + get_parent_expr, is_diag_trait_item, match_def_path, path_to_local_id, paths, peel_blocks, strip_pat_refs, +}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::LateContext; @@ -108,26 +110,12 @@ fn check_qpath(cx: &LateContext<'_>, qpath: hir::QPath<'_>, hir_id: hir::HirId) fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool { match arg.kind { - hir::ExprKind::Closure(&hir::Closure { body, .. }) => { + 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); - - // |x| ... - // ^ - let [ - hir::Param { - pat: - hir::Pat { - kind: hir::PatKind::Binding(_, local_id, ..), - .. - }, - .. - }, - ] = closure_body.params - else { - return false; - }; - + if let closure_body = cx.tcx.hir().body(body) + && let [param] = closure_body.params + && let hir::PatKind::Binding(_, local_id, ..) = strip_pat_refs(param.pat).kind => + { let closure_expr = peel_blocks(closure_body.value); match closure_expr.kind { hir::ExprKind::MethodCall(method, obj, [], _) => { @@ -139,7 +127,7 @@ fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool { // no autoderefs && !cx.typeck_results().expr_adjustments(obj).iter() .any(|a| matches!(a.kind, Adjust::Deref(Some(..)))) - && path_to_local_id(obj, *local_id) + && path_to_local_id(obj, local_id) { true } else { @@ -148,7 +136,7 @@ fn is_calling_clone(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool { }, hir::ExprKind::Call(call, [recv]) => { if let hir::ExprKind::Path(qpath) = call.kind - && path_to_local_id(recv, *local_id) + && path_to_local_id(recv, local_id) { check_qpath(cx, qpath, call.hir_id) } else { diff --git a/tests/ui/useless_asref.fixed b/tests/ui/useless_asref.fixed index d39d8a74b26..c98f2928e03 100644 --- a/tests/ui/useless_asref.fixed +++ b/tests/ui/useless_asref.fixed @@ -171,6 +171,11 @@ mod issue12135 { x.field.clone(); //~^ ERROR: this call to `as_ref.map(...)` does nothing + // https://github.com/rust-lang/rust-clippy/pull/12136#discussion_r1451565223 + #[allow(clippy::clone_on_copy)] + Some(1).clone(); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + x.field.as_ref().map(|v| v.method().clone()) } } diff --git a/tests/ui/useless_asref.rs b/tests/ui/useless_asref.rs index 76277e7b7a4..f9d603f116d 100644 --- a/tests/ui/useless_asref.rs +++ b/tests/ui/useless_asref.rs @@ -171,6 +171,11 @@ mod issue12135 { x.field.as_ref().map(|v| Clone::clone(v)); //~^ ERROR: this call to `as_ref.map(...)` does nothing + // https://github.com/rust-lang/rust-clippy/pull/12136#discussion_r1451565223 + #[allow(clippy::clone_on_copy)] + Some(1).as_ref().map(|&x| x.clone()); + //~^ ERROR: this call to `as_ref.map(...)` does nothing + x.field.as_ref().map(|v| v.method().clone()) } } diff --git a/tests/ui/useless_asref.stderr b/tests/ui/useless_asref.stderr index 583b48732d4..e158df2664d 100644 --- a/tests/ui/useless_asref.stderr +++ b/tests/ui/useless_asref.stderr @@ -106,5 +106,11 @@ error: this call to `as_ref.map(...)` does nothing LL | x.field.as_ref().map(|v| Clone::clone(v)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `x.field.clone()` -error: aborting due to 17 previous errors +error: this call to `as_ref.map(...)` does nothing + --> $DIR/useless_asref.rs:176:9 + | +LL | Some(1).as_ref().map(|&x| x.clone()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(1).clone()` + +error: aborting due to 18 previous errors