From d2657769a22f15098343f9272f52a01145698786 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 29 Mar 2021 13:29:58 -0400 Subject: [PATCH] Improve `clone_on_copy` Lint on `_.clone().method()` when method takes self by value Set applicability correctly Correct suggestion when the cloned value is a macro call. e.g. `m!(x).clone()` Don't lint when not using the `Clone` trait --- clippy_lints/src/methods/clone_on_copy.rs | 118 ++++++++++++---------- tests/ui/clone_on_copy.fixed | 34 ++++++- tests/ui/clone_on_copy.rs | 34 ++++++- tests/ui/clone_on_copy.stderr | 30 ++++-- tests/ui/clone_on_copy_mut.rs | 18 ---- 5 files changed, 157 insertions(+), 77 deletions(-) delete mode 100644 tests/ui/clone_on_copy_mut.rs diff --git a/clippy_lints/src/methods/clone_on_copy.rs b/clippy_lints/src/methods/clone_on_copy.rs index edb6649b87b..ce2e8fa8b10 100644 --- a/clippy_lints/src/methods/clone_on_copy.rs +++ b/clippy_lints/src/methods/clone_on_copy.rs @@ -1,10 +1,12 @@ -use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::get_parent_node; +use clippy_utils::source::snippet_with_context; use clippy_utils::sugg; use clippy_utils::ty::is_copy; use rustc_errors::Applicability; -use rustc_hir as hir; +use rustc_hir::{BindingAnnotation, Expr, ExprKind, MatchSource, Node, PatKind}; use rustc_lint::LateContext; -use rustc_middle::ty; +use rustc_middle::ty::{self, adjustment::Adjust}; use rustc_span::symbol::{sym, Symbol}; use std::iter; @@ -12,12 +14,26 @@ use super::CLONE_DOUBLE_REF; use super::CLONE_ON_COPY; /// Checks for the `CLONE_ON_COPY` lint. -pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol, args: &[hir::Expr<'_>]) { - if !(args.len() == 1 && method_name == sym::clone) { +#[allow(clippy::too_many_lines)] +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: Symbol, args: &[Expr<'_>]) { + let arg = match args { + [arg] if method_name == sym::clone => arg, + _ => return, + }; + if cx + .typeck_results() + .type_dependent_def_id(expr.hir_id) + .and_then(|id| cx.tcx.trait_of_item(id)) + .zip(cx.tcx.lang_items().clone_trait()) + .map_or(true, |(x, y)| x != y) + { return; } - let arg = &args[0]; - let arg_ty = cx.typeck_results().expr_ty_adjusted(&args[0]); + let arg_adjustments = cx.typeck_results().expr_adjustments(arg); + let arg_ty = arg_adjustments + .last() + .map_or_else(|| cx.typeck_results().expr_ty(arg), |a| a.target); + let ty = cx.typeck_results().expr_ty(expr); if let ty::Ref(_, inner, _) = arg_ty.kind() { if let ty::Ref(_, innermost, _) = inner.kind() { @@ -61,57 +77,57 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Sym } if is_copy(cx, ty) { - let snip; - if let Some(snippet) = sugg::Sugg::hir_opt(cx, arg) { - let parent = cx.tcx.hir().get_parent_node(expr.hir_id); - match &cx.tcx.hir().get(parent) { - hir::Node::Expr(parent) => match parent.kind { - // &*x is a nop, &x.clone() is not - hir::ExprKind::AddrOf(..) => return, - // (*x).func() is useless, x.clone().func() can work in case func borrows mutably - hir::ExprKind::MethodCall(_, _, parent_args, _) if expr.hir_id == parent_args[0].hir_id => { - return; - }, - - _ => {}, - }, - hir::Node::Stmt(stmt) => { - if let hir::StmtKind::Local(ref loc) = stmt.kind { - if let hir::PatKind::Ref(..) = loc.pat.kind { - // let ref y = *x borrows x, let ref y = x.clone() does not - return; - } - } - }, - _ => {}, + let parent_is_suffix_expr = match get_parent_node(cx.tcx, expr.hir_id) { + Some(Node::Expr(parent)) => match parent.kind { + // &*x is a nop, &x.clone() is not + ExprKind::AddrOf(..) => return, + // (*x).func() is useless, x.clone().func() can work in case func borrows self + ExprKind::MethodCall(_, _, [self_arg, ..], _) + if expr.hir_id == self_arg.hir_id && ty != cx.typeck_results().expr_ty_adjusted(expr) => + { + return; + } + ExprKind::MethodCall(_, _, [self_arg, ..], _) if expr.hir_id == self_arg.hir_id => true, + ExprKind::Match(_, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar) + | ExprKind::Field(..) + | ExprKind::Index(..) => true, + _ => false, + }, + // local binding capturing a reference + Some(Node::Local(l)) + if matches!( + l.pat.kind, + PatKind::Binding(BindingAnnotation::Ref | BindingAnnotation::RefMut, ..) + ) => + { + return; } + _ => false, + }; - // x.clone() might have dereferenced x, possibly through Deref impls - if cx.typeck_results().expr_ty(arg) == ty { - snip = Some(("try removing the `clone` call", format!("{}", snippet))); - } else { - let deref_count = cx - .typeck_results() - .expr_adjustments(arg) - .iter() - .filter(|adj| matches!(adj.kind, ty::adjustment::Adjust::Deref(_))) - .count(); - let derefs: String = iter::repeat('*').take(deref_count).collect(); - snip = Some(("try dereferencing it", format!("{}{}", derefs, snippet))); - } + let mut app = Applicability::MachineApplicable; + let snip = snippet_with_context(cx, arg.span, expr.span.ctxt(), "_", &mut app).0; + + let deref_count = arg_adjustments + .iter() + .take_while(|adj| matches!(adj.kind, Adjust::Deref(_))) + .count(); + let (help, sugg) = if deref_count == 0 { + ("try removing the `clone` call", snip.into()) + } else if parent_is_suffix_expr { + ("try dereferencing it", format!("({}{})", "*".repeat(deref_count), snip)) } else { - snip = None; - } - span_lint_and_then( + ("try dereferencing it", format!("{}{}", "*".repeat(deref_count), snip)) + }; + + span_lint_and_sugg( cx, CLONE_ON_COPY, expr.span, &format!("using `clone` on type `{}` which implements the `Copy` trait", ty), - |diag| { - if let Some((text, snip)) = snip { - diag.span_suggestion(expr.span, text, snip, Applicability::MachineApplicable); - } - }, + help, + sugg, + app, ); } } diff --git a/tests/ui/clone_on_copy.fixed b/tests/ui/clone_on_copy.fixed index d924625132e..8d43f64768d 100644 --- a/tests/ui/clone_on_copy.fixed +++ b/tests/ui/clone_on_copy.fixed @@ -6,7 +6,8 @@ clippy::deref_addrof, clippy::no_effect, clippy::unnecessary_operation, - clippy::vec_init_then_push + clippy::vec_init_then_push, + clippy::toplevel_ref_arg )] use std::cell::RefCell; @@ -29,6 +30,37 @@ fn clone_on_copy() { let rc = RefCell::new(0); *rc.borrow(); + let x = 0u32; + x.rotate_left(1); + + #[derive(Clone, Copy)] + struct Foo; + impl Foo { + fn clone(&self) -> u32 { + 0 + } + } + Foo.clone(); // ok, this is not the clone trait + + macro_rules! m { + ($e:expr) => {{ $e }}; + } + m!(42); + + struct Wrap([u32; 2]); + impl core::ops::Deref for Wrap { + type Target = [u32; 2]; + fn deref(&self) -> &[u32; 2] { + &self.0 + } + } + let x = Wrap([0, 0]); + (*x)[0]; + + let x = 42; + let ref y = x.clone(); // ok, binds by reference + let ref mut y = x.clone(); // ok, binds by reference + // Issue #4348 let mut x = 43; let _ = &x.clone(); // ok, getting a ref diff --git a/tests/ui/clone_on_copy.rs b/tests/ui/clone_on_copy.rs index 97f49467244..f15501f7184 100644 --- a/tests/ui/clone_on_copy.rs +++ b/tests/ui/clone_on_copy.rs @@ -6,7 +6,8 @@ clippy::deref_addrof, clippy::no_effect, clippy::unnecessary_operation, - clippy::vec_init_then_push + clippy::vec_init_then_push, + clippy::toplevel_ref_arg )] use std::cell::RefCell; @@ -29,6 +30,37 @@ fn clone_on_copy() { let rc = RefCell::new(0); rc.borrow().clone(); + let x = 0u32; + x.clone().rotate_left(1); + + #[derive(Clone, Copy)] + struct Foo; + impl Foo { + fn clone(&self) -> u32 { + 0 + } + } + Foo.clone(); // ok, this is not the clone trait + + macro_rules! m { + ($e:expr) => {{ $e }}; + } + m!(42).clone(); + + struct Wrap([u32; 2]); + impl core::ops::Deref for Wrap { + type Target = [u32; 2]; + fn deref(&self) -> &[u32; 2] { + &self.0 + } + } + let x = Wrap([0, 0]); + x.clone()[0]; + + let x = 42; + let ref y = x.clone(); // ok, binds by reference + let ref mut y = x.clone(); // ok, binds by reference + // Issue #4348 let mut x = 43; let _ = &x.clone(); // ok, getting a ref diff --git a/tests/ui/clone_on_copy.stderr b/tests/ui/clone_on_copy.stderr index 7a706884fb0..e7d28b4320b 100644 --- a/tests/ui/clone_on_copy.stderr +++ b/tests/ui/clone_on_copy.stderr @@ -1,5 +1,5 @@ error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:23:5 + --> $DIR/clone_on_copy.rs:24:5 | LL | 42.clone(); | ^^^^^^^^^^ help: try removing the `clone` call: `42` @@ -7,28 +7,46 @@ LL | 42.clone(); = note: `-D clippy::clone-on-copy` implied by `-D warnings` error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:27:5 + --> $DIR/clone_on_copy.rs:28:5 | LL | (&42).clone(); | ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)` error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:30:5 + --> $DIR/clone_on_copy.rs:31:5 | LL | rc.borrow().clone(); | ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()` +error: using `clone` on type `u32` which implements the `Copy` trait + --> $DIR/clone_on_copy.rs:34:5 + | +LL | x.clone().rotate_left(1); + | ^^^^^^^^^ help: try removing the `clone` call: `x` + +error: using `clone` on type `i32` which implements the `Copy` trait + --> $DIR/clone_on_copy.rs:48:5 + | +LL | m!(42).clone(); + | ^^^^^^^^^^^^^^ help: try removing the `clone` call: `m!(42)` + +error: using `clone` on type `[u32; 2]` which implements the `Copy` trait + --> $DIR/clone_on_copy.rs:58:5 + | +LL | x.clone()[0]; + | ^^^^^^^^^ help: try dereferencing it: `(*x)` + error: using `clone` on type `char` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:36:14 + --> $DIR/clone_on_copy.rs:68:14 | LL | is_ascii('z'.clone()); | ^^^^^^^^^^^ help: try removing the `clone` call: `'z'` error: using `clone` on type `i32` which implements the `Copy` trait - --> $DIR/clone_on_copy.rs:40:14 + --> $DIR/clone_on_copy.rs:72:14 | LL | vec.push(42.clone()); | ^^^^^^^^^^ help: try removing the `clone` call: `42` -error: aborting due to 5 previous errors +error: aborting due to 8 previous errors diff --git a/tests/ui/clone_on_copy_mut.rs b/tests/ui/clone_on_copy_mut.rs deleted file mode 100644 index 5bfa256623b..00000000000 --- a/tests/ui/clone_on_copy_mut.rs +++ /dev/null @@ -1,18 +0,0 @@ -pub fn dec_read_dec(i: &mut i32) -> i32 { - *i -= 1; - let ret = *i; - *i -= 1; - ret -} - -pub fn minus_1(i: &i32) -> i32 { - dec_read_dec(&mut i.clone()) -} - -fn main() { - let mut i = 10; - assert_eq!(minus_1(&i), 9); - assert_eq!(i, 10); - assert_eq!(dec_read_dec(&mut i), 9); - assert_eq!(i, 8); -}