Skip to content

Commit 251c3b6

Browse files
lengyijunlengyijun
committed
fix 5707
1 parent 8131445 commit 251c3b6

File tree

4 files changed

+171
-16
lines changed

4 files changed

+171
-16
lines changed

‎clippy_lints/src/redundant_clone.rs‎

Lines changed: 105 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use rustc_lint::{LateContext, LateLintPass};
1212
use rustc_middle::mir::{
1313
self, traversal,
1414
visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor as _},
15+
Mutability,
1516
};
1617
use rustc_middle::ty::{self, fold::TypeVisitor, Ty};
1718
use rustc_mir::dataflow::{Analysis, AnalysisDomain, GenKill, GenKillAnalysis, ResultsCursor};
@@ -87,13 +88,18 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone {
8788

8889
let mir = cx.tcx.optimized_mir(def_id.to_def_id());
8990

91+
let possible_borrowed = {
92+
let mut vis = PossibleBorrowedVisitor::new(mir);
93+
vis.visit_body(mir);
94+
vis.into_map(cx)
95+
};
9096
let maybe_storage_live_result = MaybeStorageLive
9197
.into_engine(cx.tcx, mir)
9298
.pass_name("redundant_clone")
9399
.iterate_to_fixpoint()
94100
.into_results_cursor(mir);
95101
let mut possible_borrower = {
96-
let mut vis = PossibleBorrowerVisitor::new(cx, mir);
102+
let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_borrowed);
97103
vis.visit_body(mir);
98104
vis.into_map(cx, maybe_storage_live_result)
99105
};
@@ -509,14 +515,20 @@ struct PossibleBorrowerVisitor<'a, 'tcx> {
509515
possible_borrower: TransitiveRelation<mir::Local>,
510516
body: &'a mir::Body<'tcx>,
511517
cx: &'a LateContext<'tcx>,
518+
possible_borrowed: FxHashMap<mir::Local, HybridBitSet<mir::Local>>,
512519
}
513520

514521
impl<'a, 'tcx> PossibleBorrowerVisitor<'a, 'tcx> {
515-
fn new(cx: &'a LateContext<'tcx>, body: &'a mir::Body<'tcx>) -> Self {
522+
fn new(
523+
cx: &'a LateContext<'tcx>,
524+
body: &'a mir::Body<'tcx>,
525+
possible_borrowed: FxHashMap<mir::Local, HybridBitSet<mir::Local>>,
526+
) -> Self {
516527
Self {
517528
possible_borrower: TransitiveRelation::default(),
518529
cx,
519530
body,
531+
possible_borrowed,
520532
}
521533
}
522534

@@ -585,21 +597,108 @@ impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'tcx> {
585597
..
586598
} = &terminator.kind
587599
{
600+
// TODO add doc
588601
// If the call returns something with lifetimes,
589602
// let's conservatively assume the returned value contains lifetime of all the arguments.
590603
// For example, given `let y: Foo<'a> = foo(x)`, `y` is considered to be a possible borrower of `x`.
591-
if ContainsRegion.visit_ty(self.body.local_decls[*dest].ty).is_continue() {
592-
return;
593-
}
604+
605+
let mut immutable_borrowers = vec![];
606+
let mut mutable_borrowers = vec![];
594607

595608
for op in args {
596609
match op {
597610
mir::Operand::Copy(p) | mir::Operand::Move(p) => {
598-
self.possible_borrower.add(p.local, *dest);
611+
if let ty::Ref(_, _, Mutability::Mut) = self.body.local_decls[p.local].ty.kind() {
612+
mutable_borrowers.push(p.local);
613+
} else {
614+
immutable_borrowers.push(p.local);
615+
}
599616
},
600617
mir::Operand::Constant(..) => (),
601618
}
602619
}
620+
621+
let mut mutable_variables: Vec<mir::Local> = mutable_borrowers
622+
.iter()
623+
.filter_map(|r| self.possible_borrowed.get(r))
624+
.flat_map(|r| r.iter())
625+
.collect();
626+
627+
if ContainsRegion.visit_ty(self.body.local_decls[*dest].ty).is_break() {
628+
mutable_variables.push(*dest);
629+
}
630+
631+
for y in mutable_variables {
632+
for x in &immutable_borrowers {
633+
self.possible_borrower.add(*x, y);
634+
}
635+
for x in &mutable_borrowers {
636+
self.possible_borrower.add(*x, y);
637+
}
638+
}
639+
}
640+
}
641+
}
642+
643+
/// Collect possible borrowed for every `&mut` local.
644+
/// For exampel, `_1 = &mut _2` generate _1: {_2,...}
645+
/// Known Problems: not sure all borrowed are tracked
646+
struct PossibleBorrowedVisitor<'a, 'tcx> {
647+
possible_borrowed: TransitiveRelation<mir::Local>,
648+
body: &'a mir::Body<'tcx>,
649+
}
650+
651+
impl<'a, 'tcx> PossibleBorrowedVisitor<'a, 'tcx> {
652+
fn new(body: &'a mir::Body<'tcx>) -> Self {
653+
Self {
654+
possible_borrowed: TransitiveRelation::default(),
655+
body,
656+
}
657+
}
658+
659+
fn into_map(self, cx: &LateContext<'tcx>) -> FxHashMap<mir::Local, HybridBitSet<mir::Local>> {
660+
let mut map = FxHashMap::default();
661+
for row in (1..self.body.local_decls.len()).map(mir::Local::from_usize) {
662+
if is_copy(cx, self.body.local_decls[row].ty) {
663+
continue;
664+
}
665+
666+
let borrowers = self.possible_borrowed.reachable_from(&row);
667+
if !borrowers.is_empty() {
668+
let mut bs = HybridBitSet::new_empty(self.body.local_decls.len());
669+
for &c in borrowers {
670+
if c != mir::Local::from_usize(0) {
671+
bs.insert(c);
672+
}
673+
}
674+
675+
if !bs.is_empty() {
676+
map.insert(row, bs);
677+
}
678+
}
679+
}
680+
map
681+
}
682+
}
683+
684+
impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowedVisitor<'a, 'tcx> {
685+
fn visit_assign(&mut self, place: &mir::Place<'tcx>, rvalue: &mir::Rvalue<'_>, _location: mir::Location) {
686+
let lhs = place.local;
687+
match rvalue {
688+
// Only consider `&mut`, which can modify origin place
689+
mir::Rvalue::Ref(_, rustc_middle::mir::BorrowKind::Mut { .. }, borrowed) => {
690+
self.possible_borrowed.add(lhs, borrowed.local);
691+
},
692+
// _2: &mut _;
693+
// _3 = move _2
694+
mir::Rvalue::Use(mir::Operand::Move(borrowed)) => {
695+
self.possible_borrowed.add(lhs, borrowed.local);
696+
},
697+
// _3 = move _2 as &mut _;
698+
mir::Rvalue::Cast(_, mir::Operand::Move(borrowed), _) => {
699+
self.possible_borrowed.add(lhs, borrowed.local);
700+
},
701+
_ => {},
603702
}
604703
}
605704
}

‎tests/ui/redundant_clone.fixed‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ fn main() {
5555
issue_5405();
5656
manually_drop();
5757
clone_then_move_cloned();
58+
hashmap_neg();
59+
false_negative_5707();
5860
}
5961

6062
#[derive(Clone)]
@@ -206,3 +208,29 @@ fn clone_then_move_cloned() {
206208
let mut x = S(String::new());
207209
x.0.clone().chars().for_each(|_| x.m());
208210
}
211+
212+
fn hashmap_neg() {
213+
// issue 5707
214+
use std::collections::HashMap;
215+
use std::path::PathBuf;
216+
217+
let p = PathBuf::from("/");
218+
219+
let mut h: HashMap<&str, &str> = HashMap::new();
220+
h.insert("orig-p", p.to_str().unwrap());
221+
222+
let mut q = p.clone();
223+
q.push("foo");
224+
225+
println!("{:?} {}", h, q.display());
226+
}
227+
228+
fn false_negative_5707() {
229+
fn foo(_x: &Alpha, _y: &mut Alpha) {}
230+
231+
let x = Alpha;
232+
let mut y = Alpha;
233+
foo(&x, &mut y);
234+
let _z = x.clone(); // pr 7346 can't lint on `x`
235+
drop(y);
236+
}

‎tests/ui/redundant_clone.rs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ fn main() {
5555
issue_5405();
5656
manually_drop();
5757
clone_then_move_cloned();
58+
hashmap_neg();
59+
false_negative_5707();
5860
}
5961

6062
#[derive(Clone)]
@@ -206,3 +208,29 @@ fn clone_then_move_cloned() {
206208
let mut x = S(String::new());
207209
x.0.clone().chars().for_each(|_| x.m());
208210
}
211+
212+
fn hashmap_neg() {
213+
// issue 5707
214+
use std::collections::HashMap;
215+
use std::path::PathBuf;
216+
217+
let p = PathBuf::from("/");
218+
219+
let mut h: HashMap<&str, &str> = HashMap::new();
220+
h.insert("orig-p", p.to_str().unwrap());
221+
222+
let mut q = p.clone();
223+
q.push("foo");
224+
225+
println!("{:?} {}", h, q.display());
226+
}
227+
228+
fn false_negative_5707() {
229+
fn foo(_x: &Alpha, _y: &mut Alpha) {}
230+
231+
let x = Alpha;
232+
let mut y = Alpha;
233+
foo(&x, &mut y);
234+
let _z = x.clone(); // pr 7346 can't lint on `x`
235+
drop(y);
236+
}

‎tests/ui/redundant_clone.stderr‎

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

110110
error: redundant clone
111-
--> $DIR/redundant_clone.rs:63:25
111+
--> $DIR/redundant_clone.rs:65:25
112112
|
113113
LL | if b { (a.clone(), a.clone()) } else { (Alpha, a) }
114114
| ^^^^^^^^ help: remove this
115115
|
116116
note: this value is dropped without further use
117-
--> $DIR/redundant_clone.rs:63:24
117+
--> $DIR/redundant_clone.rs:65:24
118118
|
119119
LL | if b { (a.clone(), a.clone()) } else { (Alpha, a) }
120120
| ^
121121

122122
error: redundant clone
123-
--> $DIR/redundant_clone.rs:120:15
123+
--> $DIR/redundant_clone.rs:122: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:120:14
129+
--> $DIR/redundant_clone.rs:122:14
130130
|
131131
LL | let _s = s.clone();
132132
| ^
133133

134134
error: redundant clone
135-
--> $DIR/redundant_clone.rs:121:15
135+
--> $DIR/redundant_clone.rs:123: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:121:14
141+
--> $DIR/redundant_clone.rs:123:14
142142
|
143143
LL | let _t = t.clone();
144144
| ^
145145

146146
error: redundant clone
147-
--> $DIR/redundant_clone.rs:131:19
147+
--> $DIR/redundant_clone.rs:133: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:131:18
153+
--> $DIR/redundant_clone.rs:133:18
154154
|
155155
LL | let _f = f.clone();
156156
| ^
157157

158158
error: redundant clone
159-
--> $DIR/redundant_clone.rs:143:14
159+
--> $DIR/redundant_clone.rs:145:14
160160
|
161161
LL | let y = x.clone().join("matthias");
162162
| ^^^^^^^^ help: remove this
163163
|
164164
note: cloned value is neither consumed nor mutated
165-
--> $DIR/redundant_clone.rs:143:13
165+
--> $DIR/redundant_clone.rs:145:13
166166
|
167167
LL | let y = x.clone().join("matthias");
168168
| ^^^^^^^^^

0 commit comments

Comments
 (0)