Skip to content

Commit 9de6421

Browse files
sinkuusinkuu
committed
Extend redundant_clone to the case that cloned value is not consumed
1 parent fdce47b commit 9de6421

File tree

6 files changed

+96
-53
lines changed

6 files changed

+96
-53
lines changed

‎clippy_lints/src/redundant_clone.rs

Lines changed: 53 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use if_chain::if_chain;
66
use matches::matches;
77
use rustc::mir::{
88
self, traversal,
9-
visit::{MutatingUseContext, PlaceContext, Visitor as _},
9+
visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor as _},
1010
};
1111
use rustc::ty::{self, fold::TypeVisitor, Ty};
1212
use rustc_data_structures::{fx::FxHashMap, transitive_relation::TransitiveRelation};
@@ -110,7 +110,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
110110
continue;
111111
}
112112

113-
let (fn_def_id, arg, arg_ty, _) = unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind));
113+
let (fn_def_id, arg, arg_ty, clone_ret) =
114+
unwrap_or_continue!(is_call_with_ref_arg(cx, mir, &terminator.kind));
114115

115116
let from_borrow = match_def_path(cx, fn_def_id, &paths::CLONE_TRAIT_METHOD)
116117
|| match_def_path(cx, fn_def_id, &paths::TO_OWNED_METHOD)
@@ -132,16 +133,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
132133
statement_index: bbdata.statements.len(),
133134
};
134135

135-
// Cloned local
136-
let local = if from_borrow {
136+
// `Local` to be cloned, and a local of `clone` call's destination
137+
let (local, ret_local) = if from_borrow {
137138
// `res = clone(arg)` can be turned into `res = move arg;`
138139
// if `arg` is the only borrow of `cloned` at this point.
139140

140141
if cannot_move_out || !possible_borrower.only_borrowers(&[arg], cloned, loc) {
141142
continue;
142143
}
143144

144-
cloned
145+
(cloned, clone_ret)
145146
} else {
146147
// `arg` is a reference as it is `.deref()`ed in the previous block.
147148
// Look into the predecessor block and find out the source of deref.
@@ -153,15 +154,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
153154
let pred_terminator = mir[ps[0]].terminator();
154155

155156
// receiver of the `deref()` call
156-
let pred_arg = if_chain! {
157-
if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, Some(res))) =
157+
let (pred_arg, deref_clone_ret) = if_chain! {
158+
if let Some((pred_fn_def_id, pred_arg, pred_arg_ty, res)) =
158159
is_call_with_ref_arg(cx, mir, &pred_terminator.kind);
159-
if res.local == cloned;
160+
if res == cloned;
160161
if match_def_path(cx, pred_fn_def_id, &paths::DEREF_TRAIT_METHOD);
161162
if match_type(cx, pred_arg_ty, &paths::PATH_BUF)
162163
|| match_type(cx, pred_arg_ty, &paths::OS_STRING);
163164
then {
164-
pred_arg
165+
(pred_arg, res)
165166
} else {
166167
continue;
167168
}
@@ -188,25 +189,32 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
188189
continue;
189190
}
190191

191-
local
192+
(local, deref_clone_ret)
192193
};
193194

194-
// `local` cannot be moved out if it is used later
195-
let used_later = traversal::ReversePostorder::new(&mir, bb).skip(1).any(|(tbb, tdata)| {
196-
// Give up on loops
197-
if tdata.terminator().successors().any(|s| *s == bb) {
198-
return true;
199-
}
195+
// 1. `local` cannot be moved out if it is used later.
196+
// 2. If `ret_local` is not consumed, we can remove this `clone` call anyway.
197+
let (used, consumed) = traversal::ReversePostorder::new(&mir, bb).skip(1).fold(
198+
(false, false),
199+
|(used, consumed), (tbb, tdata)| {
200+
// Short-circuit
201+
if (used && consumed) ||
202+
// Give up on loops
203+
tdata.terminator().successors().any(|s| *s == bb)
204+
{
205+
return (true, true);
206+
}
200207

201-
let mut vis = LocalUseVisitor {
202-
local,
203-
used_other_than_drop: false,
204-
};
205-
vis.visit_basic_block_data(tbb, tdata);
206-
vis.used_other_than_drop
207-
});
208+
let mut vis = LocalUseVisitor {
209+
used: (local, false),
210+
consumed: (ret_local, false),
211+
};
212+
vis.visit_basic_block_data(tbb, tdata);
213+
(used || vis.used.1, consumed || vis.consumed.1)
214+
},
215+
);
208216

209-
if !used_later {
217+
if !used || !consumed {
210218
let span = terminator.source_info.span;
211219
let scope = terminator.source_info.scope;
212220
let node = mir.source_scopes[scope]
@@ -240,10 +248,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
240248
String::new(),
241249
app,
242250
);
243-
db.span_note(
244-
span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())),
245-
"this value is dropped without further use",
246-
);
251+
if used {
252+
db.span_note(
253+
span,
254+
"cloned value is not consumed",
255+
);
256+
} else {
257+
db.span_note(
258+
span.with_hi(span.lo() + BytePos(u32::try_from(dot).unwrap())),
259+
"this value is dropped without further use",
260+
);
261+
}
247262
});
248263
} else {
249264
span_lint_hir(cx, REDUNDANT_CLONE, node, span, "redundant clone");
@@ -259,7 +274,7 @@ fn is_call_with_ref_arg<'tcx>(
259274
cx: &LateContext<'_, 'tcx>,
260275
mir: &'tcx mir::Body<'tcx>,
261276
kind: &'tcx mir::TerminatorKind<'tcx>,
262-
) -> Option<(def_id::DefId, mir::Local, Ty<'tcx>, Option<&'tcx mir::Place<'tcx>>)> {
277+
) -> Option<(def_id::DefId, mir::Local, Ty<'tcx>, mir::Local)> {
263278
if_chain! {
264279
if let mir::TerminatorKind::Call { func, args, destination, .. } = kind;
265280
if args.len() == 1;
@@ -268,7 +283,7 @@ fn is_call_with_ref_arg<'tcx>(
268283
if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(&*mir, cx.tcx));
269284
if !is_copy(cx, inner_ty);
270285
then {
271-
Some((def_id, *local, inner_ty, destination.as_ref().map(|(dest, _)| dest)))
286+
Some((def_id, *local, inner_ty, destination.as_ref().map(|(dest, _)| dest)?.as_local()?))
272287
} else {
273288
None
274289
}
@@ -337,20 +352,15 @@ fn base_local_and_movability<'tcx>(
337352
}
338353

339354
struct LocalUseVisitor {
340-
local: mir::Local,
341-
used_other_than_drop: bool,
355+
used: (mir::Local, bool),
356+
consumed: (mir::Local, bool),
342357
}
343358

344359
impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
345360
fn visit_basic_block_data(&mut self, block: mir::BasicBlock, data: &mir::BasicBlockData<'tcx>) {
346361
let statements = &data.statements;
347362
for (statement_index, statement) in statements.iter().enumerate() {
348363
self.visit_statement(statement, mir::Location { block, statement_index });
349-
350-
// Once flagged, skip remaining statements
351-
if self.used_other_than_drop {
352-
return;
353-
}
354364
}
355365

356366
self.visit_terminator(
@@ -363,13 +373,14 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
363373
}
364374

365375
fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext, _: mir::Location) {
366-
match ctx {
367-
PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return,
368-
_ => {},
376+
if *local == self.used.0
377+
&& !matches!(ctx, PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_))
378+
{
379+
self.used.1 = true;
369380
}
370381

371-
if *local == self.local {
372-
self.used_other_than_drop = true;
382+
if *local == self.consumed.0 && matches!(ctx, PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)) {
383+
self.consumed.1 = true;
373384
}
374385
}
375386
}

‎tests/ui/format.fixed

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// run-rustfix
22

3-
#![allow(clippy::print_literal)]
3+
#![allow(clippy::print_literal, clippy::redundant_clone)]
44
#![warn(clippy::useless_format)]
55

66
struct Foo(pub String);

‎tests/ui/format.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// run-rustfix
22

3-
#![allow(clippy::print_literal)]
3+
#![allow(clippy::print_literal, clippy::redundant_clone)]
44
#![warn(clippy::useless_format)]
55

66
struct Foo(pub String);

‎tests/ui/redundant_clone.fixed

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ fn main() {
5050
cannot_double_move(Alpha);
5151
cannot_move_from_type_with_drop();
5252
borrower_propagation();
53+
not_consumed();
5354
}
5455

5556
#[derive(Clone)]
@@ -136,3 +137,12 @@ fn borrower_propagation() {
136137
let _f = f.clone(); // ok
137138
}
138139
}
140+
141+
fn not_consumed() {
142+
let x = std::path::PathBuf::from("home");
143+
let y = x.join("matthias");
144+
// join() creates a new owned PathBuf, does not take a &mut to x variable, thus the .clone() is
145+
// redundant. (It also does not consume the PathBuf)
146+
147+
println!("x: {:?}, y: {:?}", x, y);
148+
}

‎tests/ui/redundant_clone.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ fn main() {
5050
cannot_double_move(Alpha);
5151
cannot_move_from_type_with_drop();
5252
borrower_propagation();
53+
not_consumed();
5354
}
5455

5556
#[derive(Clone)]
@@ -136,3 +137,12 @@ fn borrower_propagation() {
136137
let _f = f.clone(); // ok
137138
}
138139
}
140+
141+
fn not_consumed() {
142+
let x = std::path::PathBuf::from("home");
143+
let y = x.clone().join("matthias");
144+
// join() creates a new owned PathBuf, does not take a &mut to x variable, thus the .clone() is
145+
// redundant. (It also does not consume the PathBuf)
146+
147+
println!("x: {:?}, y: {:?}", x, y);
148+
}

‎tests/ui/redundant_clone.stderr

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -108,52 +108,64 @@ LL | let _t = tup.0.clone();
108108
| ^^^^^
109109

110110
error: redundant clone
111-
--> $DIR/redundant_clone.rs:59:22
111+
--> $DIR/redundant_clone.rs:60:22
112112
|
113113
LL | (a.clone(), a.clone())
114114
| ^^^^^^^^ help: remove this
115115
|
116116
note: this value is dropped without further use
117-
--> $DIR/redundant_clone.rs:59:21
117+
--> $DIR/redundant_clone.rs:60:21
118118
|
119119
LL | (a.clone(), a.clone())
120120
| ^
121121

122122
error: redundant clone
123-
--> $DIR/redundant_clone.rs:119:15
123+
--> $DIR/redundant_clone.rs:120:15
124124
|
125125
LL | let _s = s.clone();
126126
| ^^^^^^^^ help: remove this
127127
|
128128
note: this value is dropped without further use
129-
--> $DIR/redundant_clone.rs:119:14
129+
--> $DIR/redundant_clone.rs:120:14
130130
|
131131
LL | let _s = s.clone();
132132
| ^
133133

134134
error: redundant clone
135-
--> $DIR/redundant_clone.rs:120:15
135+
--> $DIR/redundant_clone.rs:121:15
136136
|
137137
LL | let _t = t.clone();
138138
| ^^^^^^^^ help: remove this
139139
|
140140
note: this value is dropped without further use
141-
--> $DIR/redundant_clone.rs:120:14
141+
--> $DIR/redundant_clone.rs:121:14
142142
|
143143
LL | let _t = t.clone();
144144
| ^
145145

146146
error: redundant clone
147-
--> $DIR/redundant_clone.rs:130:19
147+
--> $DIR/redundant_clone.rs:131:19
148148
|
149149
LL | let _f = f.clone();
150150
| ^^^^^^^^ help: remove this
151151
|
152152
note: this value is dropped without further use
153-
--> $DIR/redundant_clone.rs:130:18
153+
--> $DIR/redundant_clone.rs:131:18
154154
|
155155
LL | let _f = f.clone();
156156
| ^
157157

158-
error: aborting due to 13 previous errors
158+
error: redundant clone
159+
--> $DIR/redundant_clone.rs:143:14
160+
|
161+
LL | let y = x.clone().join("matthias");
162+
| ^^^^^^^^ help: remove this
163+
|
164+
note: cloned value is not consumed
165+
--> $DIR/redundant_clone.rs:143:13
166+
|
167+
LL | let y = x.clone().join("matthias");
168+
| ^^^^^^^^^
169+
170+
error: aborting due to 14 previous errors
159171

0 commit comments

Comments
 (0)