From 9de642190eef4e9c0cb9aac8195d602ca514952d Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 13 Mar 2020 00:31:09 +0900 Subject: [PATCH 1/4] Extend `redundant_clone` to the case that cloned value is not consumed --- clippy_lints/src/redundant_clone.rs | 95 ++++++++++++++++------------- tests/ui/format.fixed | 2 +- tests/ui/format.rs | 2 +- tests/ui/redundant_clone.fixed | 10 +++ tests/ui/redundant_clone.rs | 10 +++ tests/ui/redundant_clone.stderr | 30 ++++++--- 6 files changed, 96 insertions(+), 53 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 103c063aef4b..b30d29116cc7 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -6,7 +6,7 @@ use if_chain::if_chain; use matches::matches; use rustc::mir::{ self, traversal, - visit::{MutatingUseContext, PlaceContext, Visitor as _}, + visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor as _}, }; use rustc::ty::{self, fold::TypeVisitor, Ty}; use rustc_data_structures::{fx::FxHashMap, transitive_relation::TransitiveRelation}; @@ -110,7 +110,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { continue; } - let (fn_def_id, arg, arg_ty, _) = unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind)); + let (fn_def_id, arg, arg_ty, clone_ret) = + unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind)); let from_borrow = match_def_path(cx, fn_def_id, &paths::CLONE_TRAIT_METHOD) || match_def_path(cx, fn_def_id, &paths::TO_OWNED_METHOD) @@ -132,8 +133,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { statement_index: bbdata.statements.len(), }; - // Cloned local - let local = if from_borrow { + // `Local` to be cloned, and a local of `clone` call's destination + let (local, ret_local) = if from_borrow { // `res = clone(arg)` can be turned into `res = move arg;` // if `arg` is the only borrow of `cloned` at this point. @@ -141,7 +142,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { continue; } - cloned + (cloned, clone_ret) } else { // `arg` is a reference as it is `.deref()`ed in the previous block. // Look into the predecessor block and find out the source of deref. @@ -153,15 +154,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let pred_terminator = mir[ps[0]].terminator(); // receiver of the `deref()` call - let pred_arg = if_chain! { - if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, Some(res))) = + let (pred_arg, deref_clone_ret) = if_chain! { + if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, res)) = is_call_with_ref_arg(cx, mir, &pred_terminator.kind); - if res.local == cloned; + if res == cloned; if match_def_path(cx, pred_fn_def_id, &paths::DEREF_TRAIT_METHOD); if match_type(cx, pred_arg_ty, &paths::PATH_BUF) || match_type(cx, pred_arg_ty, &paths::OS_STRING); then { - pred_arg + (pred_arg, res) } else { continue; } @@ -188,25 +189,32 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { continue; } - local + (local, deref_clone_ret) }; - // `local` cannot be moved out if it is used later - let used_later = traversal::ReversePostorder::new(&mir, bb).skip(1).any(|(tbb, tdata)| { - // Give up on loops - if tdata.terminator().successors().any(|s| *s == bb) { - return true; - } + // 1. `local` cannot be moved out if it is used later. + // 2. If `ret_local` is not consumed, we can remove this `clone` call anyway. + let (used, consumed) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold( + (false, false), + |(used, consumed), (tbb, tdata)| { + // Short-circuit + if (used && consumed) || + // Give up on loops + tdata.terminator().successors().any(|s| *s == bb) + { + return (true, true); + } - let mut vis = LocalUseVisitor { - local, - used_other_than_drop: false, - }; - vis.visit_basic_block_data(tbb, tdata); - vis.used_other_than_drop - }); + let mut vis = LocalUseVisitor { + used: (local, false), + consumed: (ret_local, false), + }; + vis.visit_basic_block_data(tbb, tdata); + (used || vis.used.1, consumed || vis.consumed.1) + }, + ); - if !used_later { + if !used || !consumed { let span = terminator.source_info.span; let scope = terminator.source_info.scope; let node = mir.source_scopes[scope] @@ -240,10 +248,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { String::new(), app, ); - db.span_note( - span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())), - "this value is dropped without further use", - ); + if used { + db.span_note( + span, + "cloned value is not consumed", + ); + } else { + db.span_note( + span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())), + "this value is dropped without further use", + ); + } }); } else { span_lint_hir(cx, REDUNDANT_CLONE, node, span, "redundant clone"); @@ -259,7 +274,7 @@ fn is_call_with_ref_arg<'tcx>( cx: &LateContext<'_, 'tcx>, mir: &'tcx mir::Body<'tcx>, kind: &'tcx mir::TerminatorKind<'tcx>, -) -> Option<(def_id::DefId, mir::Local, Ty<'tcx>, Option<&'tcx mir::Place<'tcx>>)> { +) -> Option<(def_id::DefId, mir::Local, Ty<'tcx>, mir::Local)> { if_chain! { if let mir::TerminatorKind::Call { func, args, destination, .. } = kind; if args.len() == 1; @@ -268,7 +283,7 @@ fn is_call_with_ref_arg<'tcx>( if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(&*mir, cx.tcx)); if !is_copy(cx, inner_ty); then { - Some((def_id, *local, inner_ty, destination.as_ref().map(|(dest, _)| dest))) + Some((def_id, *local, inner_ty, destination.as_ref().map(|(dest, _)| dest)?.as_local()?)) } else { None } @@ -337,8 +352,8 @@ fn base_local_and_movability<'tcx>( } struct LocalUseVisitor { - local: mir::Local, - used_other_than_drop: bool, + used: (mir::Local, bool), + consumed: (mir::Local, bool), } impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { @@ -346,11 +361,6 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { let statements = &data.statements; for (statement_index, statement) in statements.iter().enumerate() { self.visit_statement(statement, mir::Location { block, statement_index }); - - // Once flagged, skip remaining statements - if self.used_other_than_drop { - return; - } } self.visit_terminator( @@ -363,13 +373,14 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { } fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext, _: mir::Location) { - match ctx { - PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return, - _ => {}, + if *local == self.used.0 + && !matches!(ctx, PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_)) + { + self.used.1 = true; } - if *local == self.local { - self.used_other_than_drop = true; + if *local == self.consumed.0 && matches!(ctx, PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)) { + self.consumed.1 = true; } } } diff --git a/tests/ui/format.fixed b/tests/ui/format.fixed index 6e100230a3ad..306514769990 100644 --- a/tests/ui/format.fixed +++ b/tests/ui/format.fixed @@ -1,6 +1,6 @@ // run-rustfix -#![allow(clippy::print_literal)] +#![allow(clippy::print_literal, clippy::redundant_clone)] #![warn(clippy::useless_format)] struct Foo(pub String); diff --git a/tests/ui/format.rs b/tests/ui/format.rs index 1fae6603ac09..b604d79cca37 100644 --- a/tests/ui/format.rs +++ b/tests/ui/format.rs @@ -1,6 +1,6 @@ // run-rustfix -#![allow(clippy::print_literal)] +#![allow(clippy::print_literal, clippy::redundant_clone)] #![warn(clippy::useless_format)] struct Foo(pub String); diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index 84931f66fa8d..99d97f9cc3ce 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -50,6 +50,7 @@ fn main() { cannot_double_move(Alpha); cannot_move_from_type_with_drop(); borrower_propagation(); + not_consumed(); } #[derive(Clone)] @@ -136,3 +137,12 @@ fn borrower_propagation() { let _f = f.clone(); // ok } } + +fn not_consumed() { + let x = std::path::PathBuf::from("home"); + let y = x.join("matthias"); + // join() creates a new owned PathBuf, does not take a &mut to x variable, thus the .clone() is + // redundant. (It also does not consume the PathBuf) + + println!("x: {:?}, y: {:?}", x, y); +} diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index 9ea2de9a3daa..58cd05c67cf0 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -50,6 +50,7 @@ fn main() { cannot_double_move(Alpha); cannot_move_from_type_with_drop(); borrower_propagation(); + not_consumed(); } #[derive(Clone)] @@ -136,3 +137,12 @@ fn borrower_propagation() { let _f = f.clone(); // ok } } + +fn not_consumed() { + let x = std::path::PathBuf::from("home"); + let y = x.clone().join("matthias"); + // join() creates a new owned PathBuf, does not take a &mut to x variable, thus the .clone() is + // redundant. (It also does not consume the PathBuf) + + println!("x: {:?}, y: {:?}", x, y); +} diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index 0f185cda0197..04fe74e9175b 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -108,52 +108,64 @@ LL | let _t = tup.0.clone(); | ^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:59:22 + --> $DIR/redundant_clone.rs:60:22 | LL | (a.clone(), a.clone()) | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:59:21 + --> $DIR/redundant_clone.rs:60:21 | LL | (a.clone(), a.clone()) | ^ error: redundant clone - --> $DIR/redundant_clone.rs:119:15 + --> $DIR/redundant_clone.rs:120:15 | LL | let _s = s.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:119:14 + --> $DIR/redundant_clone.rs:120:14 | LL | let _s = s.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:120:15 + --> $DIR/redundant_clone.rs:121:15 | LL | let _t = t.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:120:14 + --> $DIR/redundant_clone.rs:121:14 | LL | let _t = t.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:130:19 + --> $DIR/redundant_clone.rs:131:19 | LL | let _f = f.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:130:18 + --> $DIR/redundant_clone.rs:131:18 | LL | let _f = f.clone(); | ^ -error: aborting due to 13 previous errors +error: redundant clone + --> $DIR/redundant_clone.rs:143:14 + | +LL | let y = x.clone().join("matthias"); + | ^^^^^^^^ help: remove this + | +note: cloned value is not consumed + --> $DIR/redundant_clone.rs:143:13 + | +LL | let y = x.clone().join("matthias"); + | ^^^^^^^^^ + +error: aborting due to 14 previous errors From a3773785288e71fde264b9264a142a662aace5ee Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 13 Mar 2020 00:54:40 +0900 Subject: [PATCH 2/4] Only fires on temporaries `let y = x.clone()` cannot be turned into `let y = x` without moving x, regardless of whether `y` is consumed or not. --- clippy_lints/src/redundant_clone.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index b30d29116cc7..7c1a70408465 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -192,10 +192,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { (local, deref_clone_ret) }; - // 1. `local` cannot be moved out if it is used later. - // 2. If `ret_local` is not consumed, we can remove this `clone` call anyway. + let is_temp = mir_read_only.local_kind(ret_local) == mir::LocalKind::Temp; + + // 1. `local` can be moved out if it is not used later. + // 2. If `ret_local` is a temporary and is not consumed, we can remove this `clone` call anyway. let (used, consumed) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold( - (false, false), + (false, !is_temp), |(used, consumed), (tbb, tdata)| { // Short-circuit if (used && consumed) || From aca64b8df77670c6661b7174d7edc16133442162 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 13 Mar 2020 01:25:18 +0900 Subject: [PATCH 3/4] Check for mutation --- clippy_lints/src/redundant_clone.rs | 25 ++++++++++++++++--------- tests/ui/redundant_clone.fixed | 5 +++++ tests/ui/redundant_clone.rs | 5 +++++ tests/ui/redundant_clone.stderr | 2 +- 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 7c1a70408465..624fe227ca3d 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -195,8 +195,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let is_temp = mir_read_only.local_kind(ret_local) == mir::LocalKind::Temp; // 1. `local` can be moved out if it is not used later. - // 2. If `ret_local` is a temporary and is not consumed, we can remove this `clone` call anyway. - let (used, consumed) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold( + // 2. If `ret_local` is a temporary and is neither consumed nor mutated, we can remove this `clone` + // call anyway. + let (used, consumed_or_mutated) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold( (false, !is_temp), |(used, consumed), (tbb, tdata)| { // Short-circuit @@ -209,14 +210,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { let mut vis = LocalUseVisitor { used: (local, false), - consumed: (ret_local, false), + consumed_or_mutated: (ret_local, false), }; vis.visit_basic_block_data(tbb, tdata); - (used || vis.used.1, consumed || vis.consumed.1) + (used || vis.used.1, consumed || vis.consumed_or_mutated.1) }, ); - if !used || !consumed { + if !used || !consumed_or_mutated { let span = terminator.source_info.span; let scope = terminator.source_info.scope; let node = mir.source_scopes[scope] @@ -253,7 +254,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone { if used { db.span_note( span, - "cloned value is not consumed", + "cloned value is neither consumed nor mutated", ); } else { db.span_note( @@ -355,7 +356,7 @@ fn base_local_and_movability<'tcx>( struct LocalUseVisitor { used: (mir::Local, bool), - consumed: (mir::Local, bool), + consumed_or_mutated: (mir::Local, bool), } impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { @@ -381,8 +382,14 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { self.used.1 = true; } - if *local == self.consumed.0 && matches!(ctx, PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)) { - self.consumed.1 = true; + if *local == self.consumed_or_mutated.0 { + match ctx { + PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) + | PlaceContext::MutatingUse(MutatingUseContext::Borrow) => { + self.consumed_or_mutated.1 = true; + }, + _ => {}, + } } } } diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index 99d97f9cc3ce..17734b04aba4 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -145,4 +145,9 @@ fn not_consumed() { // redundant. (It also does not consume the PathBuf) println!("x: {:?}, y: {:?}", x, y); + + let mut s = String::new(); + s.clone().push_str("foo"); // OK, removing this `clone()` will change the behavior. + s.push_str("bar"); + assert_eq!(s, "bar"); } diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index 58cd05c67cf0..aee6855eea94 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -145,4 +145,9 @@ fn not_consumed() { // redundant. (It also does not consume the PathBuf) println!("x: {:?}, y: {:?}", x, y); + + let mut s = String::new(); + s.clone().push_str("foo"); // OK, removing this `clone()` will change the behavior. + s.push_str("bar"); + assert_eq!(s, "bar"); } diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index 04fe74e9175b..9c27812b9cdc 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -161,7 +161,7 @@ error: redundant clone LL | let y = x.clone().join("matthias"); | ^^^^^^^^ help: remove this | -note: cloned value is not consumed +note: cloned value is neither consumed nor mutated --> $DIR/redundant_clone.rs:143:13 | LL | let y = x.clone().join("matthias"); From d9ad33852c1e0179fc9b22ac2324ab18455879f1 Mon Sep 17 00:00:00 2001 From: Shotaro Yamada Date: Fri, 13 Mar 2020 02:06:47 +0900 Subject: [PATCH 4/4] Use visit_place --- clippy_lints/src/redundant_clone.rs | 8 +++++--- tests/ui/redundant_clone.fixed | 9 +++++++++ tests/ui/redundant_clone.rs | 9 +++++++++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 624fe227ca3d..b63bb371c4f3 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -375,14 +375,16 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor { ); } - fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext, _: mir::Location) { - if *local == self.used.0 + fn visit_place(&mut self, place: &mir::Place<'tcx>, ctx: PlaceContext, _: mir::Location) { + let local = place.local; + + if local == self.used.0 && !matches!(ctx, PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_)) { self.used.1 = true; } - if *local == self.consumed_or_mutated.0 { + if local == self.consumed_or_mutated.0 { match ctx { PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) | PlaceContext::MutatingUse(MutatingUseContext::Borrow) => { diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index 17734b04aba4..54815603c6de 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -150,4 +150,13 @@ fn not_consumed() { s.clone().push_str("foo"); // OK, removing this `clone()` will change the behavior. s.push_str("bar"); assert_eq!(s, "bar"); + + let t = Some(s); + // OK + if let Some(x) = t.clone() { + println!("{}", x); + } + if let Some(x) = t { + println!("{}", x); + } } diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index aee6855eea94..a9b31183adc8 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -150,4 +150,13 @@ fn not_consumed() { s.clone().push_str("foo"); // OK, removing this `clone()` will change the behavior. s.push_str("bar"); assert_eq!(s, "bar"); + + let t = Some(s); + // OK + if let Some(x) = t.clone() { + println!("{}", x); + } + if let Some(x) = t { + println!("{}", x); + } }