diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index d39c13621ff..f30da9c909d 100644 --- a/clippy_lints/src/no_effect.rs +++ b/clippy_lints/src/no_effect.rs @@ -57,7 +57,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool { } match expr.node { ExprKind::Lit(..) | ExprKind::Closure(.., _) => true, - ExprKind::Path(..) => !has_drop(cx, expr), + ExprKind::Path(..) => !has_drop(cx, cx.tables.expr_ty(expr)), ExprKind::Index(ref a, ref b) | ExprKind::Binary(_, ref a, ref b) => { has_no_effect(cx, a) && has_no_effect(cx, b) }, @@ -70,7 +70,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool { | ExprKind::AddrOf(_, ref inner) | ExprKind::Box(ref inner) => has_no_effect(cx, inner), ExprKind::Struct(_, ref fields, ref base) => { - !has_drop(cx, expr) + !has_drop(cx, cx.tables.expr_ty(expr)) && fields.iter().all(|field| has_no_effect(cx, &field.expr)) && match *base { Some(ref base) => has_no_effect(cx, base), @@ -82,7 +82,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool { let def = cx.tables.qpath_def(qpath, callee.hir_id); match def { Def::Struct(..) | Def::Variant(..) | Def::StructCtor(..) | Def::VariantCtor(..) => { - !has_drop(cx, expr) && args.iter().all(|arg| has_no_effect(cx, arg)) + !has_drop(cx, cx.tables.expr_ty(expr)) && args.iter().all(|arg| has_no_effect(cx, arg)) }, _ => false, } @@ -161,7 +161,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option reduce_expression(cx, inner).or_else(|| Some(vec![inner])), ExprKind::Struct(_, ref fields, ref base) => { - if has_drop(cx, expr) { + if has_drop(cx, cx.tables.expr_ty(expr)) { None } else { Some(fields.iter().map(|f| &f.expr).chain(base).map(Deref::deref).collect()) @@ -172,7 +172,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option + if !has_drop(cx, cx.tables.expr_ty(expr)) => { Some(args.iter().collect()) }, diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 843f1cbd36e..2c1173bf10e 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -23,10 +23,11 @@ use crate::syntax::{ source_map::{BytePos, Span}, }; use crate::utils::{ - in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node, span_lint_node_and_then, - walk_ptrs_ty_depth, + has_drop, in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node, + span_lint_node_and_then, walk_ptrs_ty_depth, }; use if_chain::if_chain; +use matches::matches; use std::convert::TryFrom; macro_rules! unwrap_or_continue { @@ -126,7 +127,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { // _1 in MIR `{ _2 = &_1; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } (from_deref) // In case of `from_deref`, `arg` is already a reference since it is `deref`ed in the previous // block. - let cloned = unwrap_or_continue!(find_stmt_assigns_to(arg, from_borrow, bbdata.statements.iter().rev())); + let (cloned, cannot_move_out) = unwrap_or_continue!(find_stmt_assigns_to( + cx, + mir, + arg, + from_borrow, + bbdata.statements.iter().rev() + )); + + if from_borrow && cannot_move_out { + continue; + } // _1 in MIR `{ _2 = &_1; _3 = deref(move _2); } -> { _4 = _3; to_path_buf(move _4); }` let referent = if from_deref { @@ -150,7 +161,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { } }; - unwrap_or_continue!(find_stmt_assigns_to(pred_arg, true, mir[ps[0]].statements.iter().rev())) + let (local, cannot_move_out) = unwrap_or_continue!(find_stmt_assigns_to( + cx, + mir, + pred_arg, + true, + mir[ps[0]].statements.iter().rev() + )); + if cannot_move_out { + continue; + } + local } else { cloned }; @@ -227,21 +248,25 @@ fn is_call_with_ref_arg<'tcx>( } } +type CannotMoveOut = bool; + /// Finds the first `to = (&)from`, and returns `Some(from)`. fn find_stmt_assigns_to<'a, 'tcx: 'a>( + cx: &LateContext<'_, 'tcx>, + mir: &mir::Mir<'tcx>, to: mir::Local, by_ref: bool, mut stmts: impl Iterator>, -) -> Option { +) -> Option<(mir::Local, CannotMoveOut)> { stmts.find_map(|stmt| { if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind { if *local == to { if by_ref { - if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v { - return Some(r); + if let mir::Rvalue::Ref(_, _, ref place) = **v { + return base_local(cx, mir, place); } - } else if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v { - return Some(r); + } else if let mir::Rvalue::Use(mir::Operand::Copy(ref place)) = **v { + return base_local(cx, mir, place); } } } @@ -250,6 +275,32 @@ fn find_stmt_assigns_to<'a, 'tcx: 'a>( }) } +fn base_local<'tcx>( + cx: &LateContext<'_, 'tcx>, + mir: &mir::Mir<'tcx>, + mut place: &mir::Place<'tcx>, +) -> Option<(mir::Local, CannotMoveOut)> { + use rustc::mir::Place::*; + + let mut deref = false; + // Accessing a field of an ADT that has `Drop` + let mut field = false; + + loop { + match place { + Local(local) => return Some((*local, deref || field)), + Projection(proj) => { + place = &proj.base; + deref = deref || matches!(proj.elem, mir::ProjectionElem::Deref); + if !field && matches!(proj.elem, mir::ProjectionElem::Field(..)) { + field = has_drop(cx, place.ty(&mir.local_decls, cx.tcx).to_ty(cx.tcx)); + } + }, + _ => return None, + } + } +} + struct LocalUseVisitor { local: mir::Local, used_other_than_drop: bool, @@ -280,7 +331,7 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) { match ctx { PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return, - _ => {} + _ => {}, } if *local == self.local { diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 68357b08d6c..aa6b3305a49 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -266,9 +266,8 @@ pub fn implements_trait<'a, 'tcx>( } /// Check whether this type implements Drop. -pub fn has_drop(cx: &LateContext<'_, '_>, expr: &Expr) -> bool { - let struct_ty = cx.tables.expr_ty(expr); - match struct_ty.ty_adt_def() { +pub fn has_drop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool { + match ty.ty_adt_def() { Some(def) => def.has_dtor(cx.tcx), _ => false, } diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index e5c5528e4fa..71f83e155f3 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -34,6 +34,12 @@ fn main() { // Check that lint level works #[allow(clippy::redundant_clone)] let _ = String::new().to_string(); + + let tup = (String::from("foo"),); + let _ = tup.0.clone(); + + let tup_ref = &(String::from("foo"),); + let _s = tup_ref.0.clone(); // this `.clone()` cannot be removed } #[derive(Clone)] @@ -45,3 +51,18 @@ fn with_branch(a: Alpha, b: bool) -> (Alpha, Alpha) { (Alpha, a) } } + +struct TypeWithDrop { + x: String, +} + +impl Drop for TypeWithDrop { + fn drop(&mut self) {} +} + +fn cannot_move_from_type_with_drop() -> String { + let s = TypeWithDrop { + x: String::new() + }; + s.x.clone() // removing this `clone()` summons E0509 +} diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index db452822f89..4d1c7aa2600 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -96,16 +96,28 @@ note: this value is dropped without further use | ^^^^^^^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:43:22 + --> $DIR/redundant_clone.rs:39:18 | -43 | (a.clone(), a.clone()) +39 | let _ = tup.0.clone(); + | ^^^^^^^^ help: remove this + | +note: this value is dropped without further use + --> $DIR/redundant_clone.rs:39:13 + | +39 | let _ = tup.0.clone(); + | ^^^^^ + +error: redundant clone + --> $DIR/redundant_clone.rs:49:22 + | +49 | (a.clone(), a.clone()) | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:43:21 + --> $DIR/redundant_clone.rs:49:21 | -43 | (a.clone(), a.clone()) +49 | (a.clone(), a.clone()) | ^ -error: aborting due to 9 previous errors +error: aborting due to 10 previous errors