Skip to content

Commit

Permalink
Auto merge of #5304 - sinkuu:redundant_clone_not_consumed, r=flip1995
Browse files Browse the repository at this point in the history 
Extend `redundant_clone` to the case that cloned value is not consumed

Fixes #5303.

---

changelog: Extend `redundant_clone` to the case that cloned value is not consumed
  • Loading branch information
@bors
bors committed Mar 12, 2020
2 parents b064ea8 + d9ad338 commit 8485d40
Show file tree
Hide file tree
Showing 6 changed files with 136 additions and 54 deletions.
108 changes: 65 additions & 43 deletions clippy_lints/src/redundant_clone.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)
Expand All @@ -132,16 +133,16 @@ 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.

if cannot_move_out || !possible_borrower.only_borrowers(&[arg], cloned, loc) {
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.
Expand All @@ -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;
}
Expand All @@ -188,25 +189,35 @@ 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;
}
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 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
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_or_mutated: (ret_local, false),
};
vis.visit_basic_block_data(tbb, tdata);
(used || vis.used.1, consumed || vis.consumed_or_mutated.1)
},
);

if !used_later {
if !used || !consumed_or_mutated {
let span = terminator.source_info.span;
let scope = terminator.source_info.scope;
let node = mir.source_scopes[scope]
Expand Down Expand Up @@ -240,10 +251,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 neither consumed nor mutated",
);
} 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");
Expand All @@ -259,7 +277,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;
Expand All @@ -268,7 +286,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
}
Expand Down Expand Up @@ -337,20 +355,15 @@ fn base_local_and_movability<'tcx>(
}

struct LocalUseVisitor {
local: mir::Local,
used_other_than_drop: bool,
used: (mir::Local, bool),
consumed_or_mutated: (mir::Local, bool),
}

impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
fn visit_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) {
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(
Expand All @@ -362,14 +375,23 @@ 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,
_ => {},
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.local {
self.used_other_than_drop = true;
if local == self.consumed_or_mutated.0 {
match ctx {
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)
| PlaceContext::MutatingUse(MutatingUseContext::Borrow) => {
self.consumed_or_mutated.1 = true;
},
_ => {},
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/format.fixed
View file
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/format.rs
View file
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/redundant_clone.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ fn main() {
cannot_double_move(Alpha);
cannot_move_from_type_with_drop();
borrower_propagation();
not_consumed();
}

#[derive(Clone)]
Expand Down Expand Up @@ -136,3 +137,26 @@ 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);

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");

let t = Some(s);
// OK
if let Some(x) = t.clone() {
println!("{}", x);
}
if let Some(x) = t {
println!("{}", x);
}
}
24 changes: 24 additions & 0 deletions tests/ui/redundant_clone.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ fn main() {
cannot_double_move(Alpha);
cannot_move_from_type_with_drop();
borrower_propagation();
not_consumed();
}

#[derive(Clone)]
Expand Down Expand Up @@ -136,3 +137,26 @@ 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);

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");

let t = Some(s);
// OK
if let Some(x) = t.clone() {
println!("{}", x);
}
if let Some(x) = t {
println!("{}", x);
}
}
30 changes: 21 additions & 9 deletions tests/ui/redundant_clone.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 neither consumed nor mutated
--> $DIR/redundant_clone.rs:143:13
|
LL | let y = x.clone().join("matthias");
| ^^^^^^^^^

error: aborting due to 14 previous errors

0 comments on commit 8485d40

Please sign in to comment.