Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend redundant_clone to the case that cloned value is not consumed #5304

Merged
merged 4 commits into from
Mar 12, 2020
+136 −54
Merged

Extend redundant_clone to the case that cloned value is not consumed #5304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9de6421
Extend `redundant_clone` to the case that cloned value is not consumed
sinkuu Mar 12, 2020
a377378
Only fires on temporaries
sinkuu Mar 12, 2020
aca64b8
Check for mutation
sinkuu Mar 12, 2020
d9ad338
Use visit_place
sinkuu Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 65 additions & 43 deletions clippy_lints/src/redundant_clone.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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