From 251c3b64dab8982be5d0d002fe695f693ec4a5ab Mon Sep 17 00:00:00 2001 From: lyj Date: Fri, 11 Jun 2021 20:57:11 +0800 Subject: [PATCH 1/2] fix 5707 --- clippy_lints/src/redundant_clone.rs | 111 ++++++++++++++++++++++++++-- tests/ui/redundant_clone.fixed | 28 +++++++ tests/ui/redundant_clone.rs | 28 +++++++ tests/ui/redundant_clone.stderr | 20 ++--- 4 files changed, 171 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 380557c81a19a..8253397160a91 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -12,6 +12,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::mir::{ self, traversal, visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor as _}, + Mutability, }; use rustc_middle::ty::{self, fold::TypeVisitor, Ty}; use rustc_mir::dataflow::{Analysis, AnalysisDomain, GenKill, GenKillAnalysis, ResultsCursor}; @@ -87,13 +88,18 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { let mir = cx.tcx.optimized_mir(def_id.to_def_id()); + let possible_borrowed = { + let mut vis = PossibleBorrowedVisitor::new(mir); + vis.visit_body(mir); + vis.into_map(cx) + }; let maybe_storage_live_result = MaybeStorageLive .into_engine(cx.tcx, mir) .pass_name("redundant_clone") .iterate_to_fixpoint() .into_results_cursor(mir); let mut possible_borrower = { - let mut vis = PossibleBorrowerVisitor::new(cx, mir); + let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_borrowed); vis.visit_body(mir); vis.into_map(cx, maybe_storage_live_result) }; @@ -509,14 +515,20 @@ struct PossibleBorrowerVisitor<'a, 'tcx> { possible_borrower: TransitiveRelation, body: &'a mir::Body<'tcx>, cx: &'a LateContext<'tcx>, + possible_borrowed: FxHashMap>, } impl<'a, 'tcx> PossibleBorrowerVisitor<'a, 'tcx> { - fn new(cx: &'a LateContext<'tcx>, body: &'a mir::Body<'tcx>) -> Self { + fn new( + cx: &'a LateContext<'tcx>, + body: &'a mir::Body<'tcx>, + possible_borrowed: FxHashMap>, + ) -> Self { Self { possible_borrower: TransitiveRelation::default(), cx, body, + possible_borrowed, } } @@ -585,21 +597,108 @@ impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'tcx> { .. } = &terminator.kind { + // TODO add doc // If the call returns something with lifetimes, // let's conservatively assume the returned value contains lifetime of all the arguments. // For example, given `let y: Foo<'a> = foo(x)`, `y` is considered to be a possible borrower of `x`. - if ContainsRegion.visit_ty(self.body.local_decls[*dest].ty).is_continue() { - return; - } + + let mut immutable_borrowers = vec![]; + let mut mutable_borrowers = vec![]; for op in args { match op { mir::Operand::Copy(p) | mir::Operand::Move(p) => { - self.possible_borrower.add(p.local, *dest); + if let ty::Ref(_, _, Mutability::Mut) = self.body.local_decls[p.local].ty.kind() { + mutable_borrowers.push(p.local); + } else { + immutable_borrowers.push(p.local); + } }, mir::Operand::Constant(..) => (), } } + + let mut mutable_variables: Vec = mutable_borrowers + .iter() + .filter_map(|r| self.possible_borrowed.get(r)) + .flat_map(|r| r.iter()) + .collect(); + + if ContainsRegion.visit_ty(self.body.local_decls[*dest].ty).is_break() { + mutable_variables.push(*dest); + } + + for y in mutable_variables { + for x in &immutable_borrowers { + self.possible_borrower.add(*x, y); + } + for x in &mutable_borrowers { + self.possible_borrower.add(*x, y); + } + } + } + } +} + +/// Collect possible borrowed for every `&mut` local. +/// For exampel, `_1 = &mut _2` generate _1: {_2,...} +/// Known Problems: not sure all borrowed are tracked +struct PossibleBorrowedVisitor<'a, 'tcx> { + possible_borrowed: TransitiveRelation, + body: &'a mir::Body<'tcx>, +} + +impl<'a, 'tcx> PossibleBorrowedVisitor<'a, 'tcx> { + fn new(body: &'a mir::Body<'tcx>) -> Self { + Self { + possible_borrowed: TransitiveRelation::default(), + body, + } + } + + fn into_map(self, cx: &LateContext<'tcx>) -> FxHashMap> { + let mut map = FxHashMap::default(); + for row in (1..self.body.local_decls.len()).map(mir::Local::from_usize) { + if is_copy(cx, self.body.local_decls[row].ty) { + continue; + } + + let borrowers = self.possible_borrowed.reachable_from(&row); + if !borrowers.is_empty() { + let mut bs = HybridBitSet::new_empty(self.body.local_decls.len()); + for &c in borrowers { + if c != mir::Local::from_usize(0) { + bs.insert(c); + } + } + + if !bs.is_empty() { + map.insert(row, bs); + } + } + } + map + } +} + +impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowedVisitor<'a, 'tcx> { + fn visit_assign(&mut self, place: &mir::Place<'tcx>, rvalue: &mir::Rvalue<'_>, _location: mir::Location) { + let lhs = place.local; + match rvalue { + // Only consider `&mut`, which can modify origin place + mir::Rvalue::Ref(_, rustc_middle::mir::BorrowKind::Mut { .. }, borrowed) => { + self.possible_borrowed.add(lhs, borrowed.local); + }, + // _2: &mut _; + // _3 = move _2 + mir::Rvalue::Use(mir::Operand::Move(borrowed)) => { + self.possible_borrowed.add(lhs, borrowed.local); + }, + // _3 = move _2 as &mut _; + mir::Rvalue::Cast(_, mir::Operand::Move(borrowed), _) => { + self.possible_borrowed.add(lhs, borrowed.local); + }, + _ => {}, } } } diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index f5da703cd1dea..2d711082746e7 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -55,6 +55,8 @@ fn main() { issue_5405(); manually_drop(); clone_then_move_cloned(); + hashmap_neg(); + false_negative_5707(); } #[derive(Clone)] @@ -206,3 +208,29 @@ fn clone_then_move_cloned() { let mut x = S(String::new()); x.0.clone().chars().for_each(|_| x.m()); } + +fn hashmap_neg() { + // issue 5707 + use std::collections::HashMap; + use std::path::PathBuf; + + let p = PathBuf::from("/"); + + let mut h: HashMap<&str, &str> = HashMap::new(); + h.insert("orig-p", p.to_str().unwrap()); + + let mut q = p.clone(); + q.push("foo"); + + println!("{:?} {}", h, q.display()); +} + +fn false_negative_5707() { + fn foo(_x: &Alpha, _y: &mut Alpha) {} + + let x = Alpha; + let mut y = Alpha; + foo(&x, &mut y); + let _z = x.clone(); // pr 7346 can't lint on `x` + drop(y); +} diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index fd7f31a1cc5b6..bd3d7365229fb 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -55,6 +55,8 @@ fn main() { issue_5405(); manually_drop(); clone_then_move_cloned(); + hashmap_neg(); + false_negative_5707(); } #[derive(Clone)] @@ -206,3 +208,29 @@ fn clone_then_move_cloned() { let mut x = S(String::new()); x.0.clone().chars().for_each(|_| x.m()); } + +fn hashmap_neg() { + // issue 5707 + use std::collections::HashMap; + use std::path::PathBuf; + + let p = PathBuf::from("/"); + + let mut h: HashMap<&str, &str> = HashMap::new(); + h.insert("orig-p", p.to_str().unwrap()); + + let mut q = p.clone(); + q.push("foo"); + + println!("{:?} {}", h, q.display()); +} + +fn false_negative_5707() { + fn foo(_x: &Alpha, _y: &mut Alpha) {} + + let x = Alpha; + let mut y = Alpha; + foo(&x, &mut y); + let _z = x.clone(); // pr 7346 can't lint on `x` + drop(y); +} diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index 529a6de91e266..fbc90493ae94b 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -108,61 +108,61 @@ LL | let _t = tup.0.clone(); | ^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:63:25 + --> $DIR/redundant_clone.rs:65:25 | LL | if b { (a.clone(), a.clone()) } else { (Alpha, a) } | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:63:24 + --> $DIR/redundant_clone.rs:65:24 | LL | if b { (a.clone(), a.clone()) } else { (Alpha, a) } | ^ error: redundant clone - --> $DIR/redundant_clone.rs:120:15 + --> $DIR/redundant_clone.rs:122:15 | LL | let _s = s.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:120:14 + --> $DIR/redundant_clone.rs:122:14 | LL | let _s = s.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:121:15 + --> $DIR/redundant_clone.rs:123:15 | LL | let _t = t.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:121:14 + --> $DIR/redundant_clone.rs:123:14 | LL | let _t = t.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:131:19 + --> $DIR/redundant_clone.rs:133:19 | LL | let _f = f.clone(); | ^^^^^^^^ help: remove this | note: this value is dropped without further use - --> $DIR/redundant_clone.rs:131:18 + --> $DIR/redundant_clone.rs:133:18 | LL | let _f = f.clone(); | ^ error: redundant clone - --> $DIR/redundant_clone.rs:143:14 + --> $DIR/redundant_clone.rs:145: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 + --> $DIR/redundant_clone.rs:145:13 | LL | let y = x.clone().join("matthias"); | ^^^^^^^^^ From 10910020ecc56373336f294c827e6f1f62616d03 Mon Sep 17 00:00:00 2001 From: lyj Date: Wed, 14 Jul 2021 11:29:39 +0800 Subject: [PATCH 2/2] rename possible_borrowed to possible_origin; pass dogfood --- clippy_lints/src/redundant_clone.rs | 41 +++++++++++++---------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 8253397160a91..56ef95a88c880 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -88,8 +88,8 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { let mir = cx.tcx.optimized_mir(def_id.to_def_id()); - let possible_borrowed = { - let mut vis = PossibleBorrowedVisitor::new(mir); + let possible_origin = { + let mut vis = PossibleOriginVisitor::new(mir); vis.visit_body(mir); vis.into_map(cx) }; @@ -99,7 +99,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { .iterate_to_fixpoint() .into_results_cursor(mir); let mut possible_borrower = { - let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_borrowed); + let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_origin); vis.visit_body(mir); vis.into_map(cx, maybe_storage_live_result) }; @@ -515,20 +515,20 @@ struct PossibleBorrowerVisitor<'a, 'tcx> { possible_borrower: TransitiveRelation, body: &'a mir::Body<'tcx>, cx: &'a LateContext<'tcx>, - possible_borrowed: FxHashMap>, + possible_origin: FxHashMap>, } impl<'a, 'tcx> PossibleBorrowerVisitor<'a, 'tcx> { fn new( cx: &'a LateContext<'tcx>, body: &'a mir::Body<'tcx>, - possible_borrowed: FxHashMap>, + possible_origin: FxHashMap>, ) -> Self { Self { possible_borrower: TransitiveRelation::default(), cx, body, - possible_borrowed, + possible_origin, } } @@ -620,8 +620,8 @@ impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'tcx> { let mut mutable_variables: Vec = mutable_borrowers .iter() - .filter_map(|r| self.possible_borrowed.get(r)) - .flat_map(|r| r.iter()) + .filter_map(|r| self.possible_origin.get(r)) + .flat_map(HybridBitSet::iter) .collect(); if ContainsRegion.visit_ty(self.body.local_decls[*dest].ty).is_break() { @@ -643,15 +643,15 @@ impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'tcx> { /// Collect possible borrowed for every `&mut` local. /// For exampel, `_1 = &mut _2` generate _1: {_2,...} /// Known Problems: not sure all borrowed are tracked -struct PossibleBorrowedVisitor<'a, 'tcx> { - possible_borrowed: TransitiveRelation, +struct PossibleOriginVisitor<'a, 'tcx> { + possible_origin: TransitiveRelation, body: &'a mir::Body<'tcx>, } -impl<'a, 'tcx> PossibleBorrowedVisitor<'a, 'tcx> { +impl<'a, 'tcx> PossibleOriginVisitor<'a, 'tcx> { fn new(body: &'a mir::Body<'tcx>) -> Self { Self { - possible_borrowed: TransitiveRelation::default(), + possible_origin: TransitiveRelation::default(), body, } } @@ -663,7 +663,7 @@ impl<'a, 'tcx> PossibleBorrowedVisitor<'a, 'tcx> { continue; } - let borrowers = self.possible_borrowed.reachable_from(&row); + let borrowers = self.possible_origin.reachable_from(&row); if !borrowers.is_empty() { let mut bs = HybridBitSet::new_empty(self.body.local_decls.len()); for &c in borrowers { @@ -681,22 +681,19 @@ impl<'a, 'tcx> PossibleBorrowedVisitor<'a, 'tcx> { } } -impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowedVisitor<'a, 'tcx> { +impl<'a, 'tcx> mir::visit::Visitor<'tcx> for PossibleOriginVisitor<'a, 'tcx> { fn visit_assign(&mut self, place: &mir::Place<'tcx>, rvalue: &mir::Rvalue<'_>, _location: mir::Location) { let lhs = place.local; match rvalue { // Only consider `&mut`, which can modify origin place - mir::Rvalue::Ref(_, rustc_middle::mir::BorrowKind::Mut { .. }, borrowed) => { - self.possible_borrowed.add(lhs, borrowed.local); - }, + mir::Rvalue::Ref(_, rustc_middle::mir::BorrowKind::Mut { .. }, borrowed) | // _2: &mut _; // _3 = move _2 - mir::Rvalue::Use(mir::Operand::Move(borrowed)) => { - self.possible_borrowed.add(lhs, borrowed.local); - }, + mir::Rvalue::Use(mir::Operand::Move(borrowed)) | // _3 = move _2 as &mut _; - mir::Rvalue::Cast(_, mir::Operand::Move(borrowed), _) => { - self.possible_borrowed.add(lhs, borrowed.local); + mir::Rvalue::Cast(_, mir::Operand::Move(borrowed), _) + => { + self.possible_origin.add(lhs, borrowed.local); }, _ => {}, }