From 790e611c9ce9478c94e5103afb0506ab5c9fcd5d Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Sun, 9 Dec 2018 19:18:35 +0900 Subject: [PATCH 1/6] Cleanup --- clippy_lints/src/redundant_clone.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index c61608e1c1b3..843f1cbd36ea 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -12,7 +12,7 @@ use crate::rustc::hir::{def_id, Body, FnDecl}; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::mir::{ self, traversal, - visit::{MutatingUseContext, NonUseContext, PlaceContext, Visitor}, + visit::{MutatingUseContext, PlaceContext, Visitor}, TerminatorKind, }; use crate::rustc::ty; @@ -279,10 +279,8 @@ 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(NonUseContext::StorageDead) => { - return; - }, - _ => {}, + PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return, + _ => {} } if *local == self.local { From a4fe5676022d66ad157b4b4238d035d6035dc31e Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Sun, 9 Dec 2018 19:18:44 +0900 Subject: [PATCH 2/6] Fix test `if true` is recognized by MIR optimization. --- tests/ui/redundant_clone.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index deedde382316..e5c5528e4faa 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -38,8 +38,8 @@ fn main() { #[derive(Clone)] struct Alpha; -fn double(a: Alpha) -> (Alpha, Alpha) { - if true { +fn with_branch(a: Alpha, b: bool) -> (Alpha, Alpha) { + if b { (a.clone(), a.clone()) } else { (Alpha, a) From 109d4b1ab3ef4823b3767acde6301e609535cb70 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Sun, 9 Dec 2018 20:19:21 +0900 Subject: [PATCH 3/6] Lint redundant clone of projection --- clippy_lints/src/no_effect.rs | 10 ++-- clippy_lints/src/redundant_clone.rs | 71 +++++++++++++++++++++++++---- clippy_lints/src/utils/mod.rs | 5 +- tests/ui/redundant_clone.rs | 21 +++++++++ tests/ui/redundant_clone.stderr | 22 +++++++-- 5 files changed, 106 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index d39c13621ffc..f30da9c909d9 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 843f1cbd36ea..2c1173bf10e0 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 68357b08d6ca..aa6b3305a49a 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 e5c5528e4faa..71f83e155f3c 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 db452822f891..4d1c7aa26009 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 From 22f396a1c173d3431849f1fca8df8f570ca8248c Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Sun, 9 Dec 2018 22:02:23 +0900 Subject: [PATCH 4/6] Apply redundant_clone on clippy --- clippy_lints/src/attrs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 9f8cc76c5aac..a8b03d214823 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -332,7 +332,7 @@ fn check_clippy_lint_names(cx: &LateContext<'_, '_>, items: &[NestedMetaItem]) { &format!("unknown clippy lint: clippy::{}", name), |db| { if name.as_str().chars().any(|c| c.is_uppercase()) { - let name_lower = name.as_str().to_lowercase().to_string(); + let name_lower = name.as_str().to_lowercase(); match lint_store.check_lint_name( &name_lower, Some(tool_name.as_str()) From fd9f5df36ca934405183ee8e7ba96b46b1f645f2 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Mon, 10 Dec 2018 15:48:34 +0900 Subject: [PATCH 5/6] Add comment and rename --- clippy_lints/src/redundant_clone.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 2c1173bf10e0..68b79869718d 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -250,7 +250,8 @@ fn is_call_with_ref_arg<'tcx>( type CannotMoveOut = bool; -/// Finds the first `to = (&)from`, and returns `Some(from)`. +/// Finds the first `to = (&)from`, and returns +/// ``Some((from, [`true` if `from` cannot be moved out]))``. fn find_stmt_assigns_to<'a, 'tcx: 'a>( cx: &LateContext<'_, 'tcx>, mir: &mir::Mir<'tcx>, @@ -263,10 +264,10 @@ fn find_stmt_assigns_to<'a, 'tcx: 'a>( if *local == to { if by_ref { if let mir::Rvalue::Ref(_, _, ref place) = **v { - return base_local(cx, mir, place); + return base_local_and_movability(cx, mir, place); } } else if let mir::Rvalue::Use(mir::Operand::Copy(ref place)) = **v { - return base_local(cx, mir, place); + return base_local_and_movability(cx, mir, place); } } } @@ -275,15 +276,20 @@ fn find_stmt_assigns_to<'a, 'tcx: 'a>( }) } -fn base_local<'tcx>( +/// Extracts and returns the undermost base `Local` of given `place`. Returns `place` itself +/// if it is already a `Local`. +/// +/// Also reports whether given `place` cannot be moved out. +fn base_local_and_movability<'tcx>( cx: &LateContext<'_, 'tcx>, mir: &mir::Mir<'tcx>, mut place: &mir::Place<'tcx>, ) -> Option<(mir::Local, CannotMoveOut)> { use rustc::mir::Place::*; + // Dereference. You cannot move things out from a borrowed value. let mut deref = false; - // Accessing a field of an ADT that has `Drop` + // Accessing a field of an ADT that has `Drop`. Moving the field out will cause E0509. let mut field = false; loop { From e7d18084fb8e6646489be545d20d050623d8d45d Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Mon, 10 Dec 2018 15:59:21 +0900 Subject: [PATCH 6/6] Only check the assignment found at last If there are more than one such assignment, the last one may be the one supplied to `clone` method. Makes `find_stmt_assigns_to` internally reverses the iterator to make the intent to "iterate statements backward" clear. --- clippy_lints/src/redundant_clone.rs | 36 +++++++++++++++++------------ 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 68b79869718d..0d31129f30a0 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -132,7 +132,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { mir, arg, from_borrow, - bbdata.statements.iter().rev() + bbdata.statements.iter() )); if from_borrow && cannot_move_out { @@ -166,7 +166,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { mir, pred_arg, true, - mir[ps[0]].statements.iter().rev() + mir[ps[0]].statements.iter() )); if cannot_move_out { continue; @@ -257,23 +257,29 @@ fn find_stmt_assigns_to<'a, 'tcx: 'a>( mir: &mir::Mir<'tcx>, to: mir::Local, by_ref: bool, - mut stmts: impl Iterator>, + stmts: impl DoubleEndedIterator>, ) -> 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(_, _, ref place) = **v { - return base_local_and_movability(cx, mir, place); - } - } else if let mir::Rvalue::Use(mir::Operand::Copy(ref place)) = **v { - return base_local_and_movability(cx, mir, place); + stmts + .rev() + .find_map(|stmt| { + if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind { + if *local == to { + return Some(v); } } - } - None - }) + None + }) + .and_then(|v| { + if by_ref { + if let mir::Rvalue::Ref(_, _, ref place) = **v { + return base_local_and_movability(cx, mir, place); + } + } else if let mir::Rvalue::Use(mir::Operand::Copy(ref place)) = **v { + return base_local_and_movability(cx, mir, place); + } + None + }) } /// Extracts and returns the undermost base `Local` of given `place`. Returns `place` itself