Skip to content

Commit 109d4b1

Browse files
sinkuusinkuu
committed
Lint redundant clone of projection
1 parent a4fe567 commit 109d4b1

File tree

5 files changed

+106
-23
lines changed

5 files changed

+106
-23
lines changed

‎clippy_lints/src/no_effect.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
5757
}
5858
match expr.node {
5959
ExprKind::Lit(..) | ExprKind::Closure(.., _) => true,
60-
ExprKind::Path(..) => !has_drop(cx, expr),
60+
ExprKind::Path(..) => !has_drop(cx, cx.tables.expr_ty(expr)),
6161
ExprKind::Index(ref a, ref b) | ExprKind::Binary(_, ref a, ref b) => {
6262
has_no_effect(cx, a) && has_no_effect(cx, b)
6363
},
@@ -70,7 +70,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
7070
| ExprKind::AddrOf(_, ref inner)
7171
| ExprKind::Box(ref inner) => has_no_effect(cx, inner),
7272
ExprKind::Struct(_, ref fields, ref base) => {
73-
!has_drop(cx, expr)
73+
!has_drop(cx, cx.tables.expr_ty(expr))
7474
&& fields.iter().all(|field| has_no_effect(cx, &field.expr))
7575
&& match *base {
7676
Some(ref base) => has_no_effect(cx, base),
@@ -82,7 +82,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
8282
let def = cx.tables.qpath_def(qpath, callee.hir_id);
8383
match def {
8484
Def::Struct(..) | Def::Variant(..) | Def::StructCtor(..) | Def::VariantCtor(..) => {
85-
!has_drop(cx, expr) && args.iter().all(|arg| has_no_effect(cx, arg))
85+
!has_drop(cx, cx.tables.expr_ty(expr)) && args.iter().all(|arg| has_no_effect(cx, arg))
8686
},
8787
_ => false,
8888
}
@@ -161,7 +161,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr) -> Option<Vec
161161
| ExprKind::AddrOf(_, ref inner)
162162
| ExprKind::Box(ref inner) => reduce_expression(cx, inner).or_else(|| Some(vec![inner])),
163163
ExprKind::Struct(_, ref fields, ref base) => {
164-
if has_drop(cx, expr) {
164+
if has_drop(cx, cx.tables.expr_ty(expr)) {
165165
None
166166
} else {
167167
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<Vec
172172
let def = cx.tables.qpath_def(qpath, callee.hir_id);
173173
match def {
174174
Def::Struct(..) | Def::Variant(..) | Def::StructCtor(..) | Def::VariantCtor(..)
175-
if !has_drop(cx, expr) =>
175+
if !has_drop(cx, cx.tables.expr_ty(expr)) =>
176176
{
177177
Some(args.iter().collect())
178178
},

‎clippy_lints/src/redundant_clone.rs

Lines changed: 61 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,11 @@ use crate::syntax::{
2323
source_map::{BytePos, Span},
2424
};
2525
use crate::utils::{
26-
in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node, span_lint_node_and_then,
27-
walk_ptrs_ty_depth,
26+
has_drop, in_macro, is_copy, match_def_path, match_type, paths, snippet_opt, span_lint_node,
27+
span_lint_node_and_then, walk_ptrs_ty_depth,
2828
};
2929
use if_chain::if_chain;
30+
use matches::matches;
3031
use std::convert::TryFrom;
3132

3233
macro_rules! unwrap_or_continue {
@@ -126,7 +127,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
126127
// _1 in MIR `{ _2 = &_1; clone(move _2); }` or `{ _2 = _1; to_path_buf(_2); } (from_deref)
127128
// In case of `from_deref`, `arg` is already a reference since it is `deref`ed in the previous
128129
// block.
129-
let cloned = unwrap_or_continue!(find_stmt_assigns_to(arg, from_borrow, bbdata.statements.iter().rev()));
130+
let (cloned, cannot_move_out) = unwrap_or_continue!(find_stmt_assigns_to(
131+
cx,
132+
mir,
133+
arg,
134+
from_borrow,
135+
bbdata.statements.iter().rev()
136+
));
137+
138+
if from_borrow && cannot_move_out {
139+
continue;
140+
}
130141

131142
// _1 in MIR `{ _2 = &_1; _3 = deref(move _2); } -> { _4 = _3; to_path_buf(move _4); }`
132143
let referent = if from_deref {
@@ -150,7 +161,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for RedundantClone {
150161
}
151162
};
152163

153-
unwrap_or_continue!(find_stmt_assigns_to(pred_arg, true, mir[ps[0]].statements.iter().rev()))
164+
let (local, cannot_move_out) = unwrap_or_continue!(find_stmt_assigns_to(
165+
cx,
166+
mir,
167+
pred_arg,
168+
true,
169+
mir[ps[0]].statements.iter().rev()
170+
));
171+
if cannot_move_out {
172+
continue;
173+
}
174+
local
154175
} else {
155176
cloned
156177
};
@@ -227,21 +248,25 @@ fn is_call_with_ref_arg<'tcx>(
227248
}
228249
}
229250

251+
type CannotMoveOut = bool;
252+
230253
/// Finds the first `to = (&)from`, and returns `Some(from)`.
231254
fn find_stmt_assigns_to<'a, 'tcx: 'a>(
255+
cx: &LateContext<'_, 'tcx>,
256+
mir: &mir::Mir<'tcx>,
232257
to: mir::Local,
233258
by_ref: bool,
234259
mut stmts: impl Iterator<Item = &'a mir::Statement<'tcx>>,
235-
) -> Option<mir::Local> {
260+
) -> Option<(mir::Local, CannotMoveOut)> {
236261
stmts.find_map(|stmt| {
237262
if let mir::StatementKind::Assign(mir::Place::Local(local), v) = &stmt.kind {
238263
if *local == to {
239264
if by_ref {
240-
if let mir::Rvalue::Ref(_, _, mir::Place::Local(r)) = **v {
241-
return Some(r);
265+
if let mir::Rvalue::Ref(_, _, ref place) = **v {
266+
return base_local(cx, mir, place);
242267
}
243-
} else if let mir::Rvalue::Use(mir::Operand::Copy(mir::Place::Local(r))) = **v {
244-
return Some(r);
268+
} else if let mir::Rvalue::Use(mir::Operand::Copy(ref place)) = **v {
269+
return base_local(cx, mir, place);
245270
}
246271
}
247272
}
@@ -250,6 +275,32 @@ fn find_stmt_assigns_to<'a, 'tcx: 'a>(
250275
})
251276
}
252277

278+
fn base_local<'tcx>(
279+
cx: &LateContext<'_, 'tcx>,
280+
mir: &mir::Mir<'tcx>,
281+
mut place: &mir::Place<'tcx>,
282+
) -> Option<(mir::Local, CannotMoveOut)> {
283+
use rustc::mir::Place::*;
284+
285+
let mut deref = false;
286+
// Accessing a field of an ADT that has `Drop`
287+
let mut field = false;
288+
289+
loop {
290+
match place {
291+
Local(local) => return Some((*local, deref || field)),
292+
Projection(proj) => {
293+
place = &proj.base;
294+
deref = deref || matches!(proj.elem, mir::ProjectionElem::Deref);
295+
if !field && matches!(proj.elem, mir::ProjectionElem::Field(..)) {
296+
field = has_drop(cx, place.ty(&mir.local_decls, cx.tcx).to_ty(cx.tcx));
297+
}
298+
},
299+
_ => return None,
300+
}
301+
}
302+
}
303+
253304
struct LocalUseVisitor {
254305
local: mir::Local,
255306
used_other_than_drop: bool,
@@ -280,7 +331,7 @@ impl<'tcx> mir::visit::Visitor<'tcx> for LocalUseVisitor {
280331
fn visit_local(&mut self, local: &mir::Local, ctx: PlaceContext<'tcx>, _: mir::Location) {
281332
match ctx {
282333
PlaceContext::MutatingUse(MutatingUseContext::Drop) | PlaceContext::NonUse(_) => return,
283-
_ => {}
334+
_ => {},
284335
}
285336

286337
if *local == self.local {

‎clippy_lints/src/utils/mod.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -266,9 +266,8 @@ pub fn implements_trait<'a, 'tcx>(
266266
}
267267

268268
/// Check whether this type implements Drop.
269-
pub fn has_drop(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
270-
let struct_ty = cx.tables.expr_ty(expr);
271-
match struct_ty.ty_adt_def() {
269+
pub fn has_drop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
270+
match ty.ty_adt_def() {
272271
Some(def) => def.has_dtor(cx.tcx),
273272
_ => false,
274273
}

‎tests/ui/redundant_clone.rs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ fn main() {
3434

3535
// Check that lint level works
3636
#[allow(clippy::redundant_clone)] let _ = String::new().to_string();
37+
38+
let tup = (String::from("foo"),);
39+
let _ = tup.0.clone();
40+
41+
let tup_ref = &(String::from("foo"),);
42+
let _s = tup_ref.0.clone(); // this `.clone()` cannot be removed
3743
}
3844

3945
#[derive(Clone)]
@@ -45,3 +51,18 @@ fn with_branch(a: Alpha, b: bool) -> (Alpha, Alpha) {
4551
(Alpha, a)
4652
}
4753
}
54+
55+
struct TypeWithDrop {
56+
x: String,
57+
}
58+
59+
impl Drop for TypeWithDrop {
60+
fn drop(&mut self) {}
61+
}
62+
63+
fn cannot_move_from_type_with_drop() -> String {
64+
let s = TypeWithDrop {
65+
x: String::new()
66+
};
67+
s.x.clone() // removing this `clone()` summons E0509
68+
}

‎tests/ui/redundant_clone.stderr

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,16 +96,28 @@ note: this value is dropped without further use
9696
| ^^^^^^^^^^^^^^^
9797

9898
error: redundant clone
99-
--> $DIR/redundant_clone.rs:43:22
99+
--> $DIR/redundant_clone.rs:39:18
100100
|
101-
43 | (a.clone(), a.clone())
101+
39 | let _ = tup.0.clone();
102+
| ^^^^^^^^ help: remove this
103+
|
104+
note: this value is dropped without further use
105+
--> $DIR/redundant_clone.rs:39:13
106+
|
107+
39 | let _ = tup.0.clone();
108+
| ^^^^^
109+
110+
error: redundant clone
111+
--> $DIR/redundant_clone.rs:49:22
112+
|
113+
49 | (a.clone(), a.clone())
102114
| ^^^^^^^^ help: remove this
103115
|
104116
note: this value is dropped without further use
105-
--> $DIR/redundant_clone.rs:43:21
117+
--> $DIR/redundant_clone.rs:49:21
106118
|
107-
43 | (a.clone(), a.clone())
119+
49 | (a.clone(), a.clone())
108120
| ^
109121

110-
error: aborting due to 9 previous errors
122+
error: aborting due to 10 previous errors
111123

0 commit comments

Comments
 (0)