diff options
| -rw-r--r-- | clippy_lints/src/dereference.rs | 8 | ||||
| -rw-r--r-- | clippy_lints/src/redundant_clone.rs | 4 | ||||
| -rw-r--r-- | clippy_utils/src/mir/possible_borrower.rs | 271 | ||||
| -rw-r--r-- | tests/ui/needless_borrow.fixed | 13 | ||||
| -rw-r--r-- | tests/ui/needless_borrow.rs | 13 | ||||
| -rw-r--r-- | tests/ui/needless_borrow.stderr | 8 | ||||
| -rw-r--r-- | tests/ui/redundant_clone.fixed | 6 | ||||
| -rw-r--r-- | tests/ui/redundant_clone.rs | 6 | ||||
| -rw-r--r-- | tests/ui/redundant_clone.stderr | 14 |
9 files changed, 109 insertions, 234 deletions
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 728941b8b3d..7b43d8ccc67 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1282,10 +1282,10 @@ fn referent_used_exactly_once<'tcx>( possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, mir))); } let possible_borrower = &mut possible_borrowers.last_mut().unwrap().1; - // If `place.local` were not included here, the `copyable_iterator::warn` test would fail. The - // reason is that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible - // borrower of itself. See the comment in that method for an explanation as to why. - possible_borrower.at_most_borrowers(cx, &[local, place.local], place.local, location) + // If `only_borrowers` were used here, the `copyable_iterator::warn` test would fail. The reason is + // that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible borrower of + // itself. See the comment in that method for an explanation as to why. + possible_borrower.bounded_borrowers(&[local], &[local, place.local], place.local, location) && used_exactly_once(mir, place.local).unwrap_or(false) } else { false diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 0e7c5cca724..c1677fb3da1 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -131,7 +131,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { // `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.at_most_borrowers(cx, &[arg], cloned, loc) { + if cannot_move_out || !possible_borrower.only_borrowers(&[arg], cloned, loc) { continue; } @@ -178,7 +178,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { // StorageDead(pred_arg); // res = to_path_buf(cloned); // ``` - if cannot_move_out || !possible_borrower.at_most_borrowers(cx, &[arg, cloned], local, loc) { + if cannot_move_out || !possible_borrower.only_borrowers(&[arg, cloned], local, loc) { continue; } diff --git a/clippy_utils/src/mir/possible_borrower.rs b/clippy_utils/src/mir/possible_borrower.rs index 395d46e7a2f..8c695801c73 100644 --- a/clippy_utils/src/mir/possible_borrower.rs +++ b/clippy_utils/src/mir/possible_borrower.rs @@ -1,137 +1,89 @@ -use super::possible_origin::PossibleOriginVisitor; +use super::{possible_origin::PossibleOriginVisitor, transitive_relation::TransitiveRelation}; use crate::ty::is_copy; -use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; +use rustc_data_structures::fx::FxHashMap; use rustc_index::bit_set::{BitSet, HybridBitSet}; use rustc_lint::LateContext; -use rustc_middle::mir::{ - self, visit::Visitor as _, BasicBlock, Local, Location, Mutability, Statement, StatementKind, Terminator, -}; -use rustc_middle::ty::{self, visit::TypeVisitor, TyCtxt}; -use rustc_mir_dataflow::{ - fmt::DebugWithContext, impls::MaybeStorageLive, lattice::JoinSemiLattice, Analysis, AnalysisDomain, - CallReturnPlaces, ResultsCursor, -}; -use std::borrow::Cow; +use rustc_middle::mir::{self, visit::Visitor as _, Mutability}; +use rustc_middle::ty::{self, visit::TypeVisitor}; +use rustc_mir_dataflow::{impls::MaybeStorageLive, Analysis, ResultsCursor}; use std::ops::ControlFlow; /// Collects the possible borrowers of each local. /// For example, `b = &a; c = &a;` will make `b` and (transitively) `c` /// possible borrowers of `a`. #[allow(clippy::module_name_repetitions)] -struct PossibleBorrowerAnalysis<'b, 'tcx> { - tcx: TyCtxt<'tcx>, +struct PossibleBorrowerVisitor<'a, 'b, 'tcx> { + possible_borrower: TransitiveRelation, body: &'b mir::Body<'tcx>, + cx: &'a LateContext<'tcx>, possible_origin: FxHashMap<mir::Local, HybridBitSet<mir::Local>>, } -#[derive(Clone, Debug, Eq, PartialEq)] -struct PossibleBorrowerState { - map: FxIndexMap<Local, BitSet<Local>>, - domain_size: usize, -} - -impl PossibleBorrowerState { - fn new(domain_size: usize) -> Self { +impl<'a, 'b, 'tcx> PossibleBorrowerVisitor<'a, 'b, 'tcx> { + fn new( + cx: &'a LateContext<'tcx>, + body: &'b mir::Body<'tcx>, + possible_origin: FxHashMap<mir::Local, HybridBitSet<mir::Local>>, + ) -> Self { Self { - map: FxIndexMap::default(), - domain_size, + possible_borrower: TransitiveRelation::default(), + cx, + body, + possible_origin, } } - #[allow(clippy::similar_names)] - fn add(&mut self, borrowed: Local, borrower: Local) { - self.map - .entry(borrowed) - .or_insert(BitSet::new_empty(self.domain_size)) - .insert(borrower); - } -} - -impl<C> DebugWithContext<C> for PossibleBorrowerState { - fn fmt_with(&self, _ctxt: &C, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - <_ as std::fmt::Debug>::fmt(self, f) - } - fn fmt_diff_with(&self, _old: &Self, _ctxt: &C, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - unimplemented!() - } -} + fn into_map( + self, + cx: &'a LateContext<'tcx>, + maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive>, + ) -> PossibleBorrowerMap<'b, 'tcx> { + 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; + } -impl JoinSemiLattice for PossibleBorrowerState { - fn join(&mut self, other: &Self) -> bool { - let mut changed = false; - for (&borrowed, borrowers) in other.map.iter() { + let mut borrowers = self.possible_borrower.reachable_from(row, self.body.local_decls.len()); + borrowers.remove(mir::Local::from_usize(0)); if !borrowers.is_empty() { - changed |= self - .map - .entry(borrowed) - .or_insert(BitSet::new_empty(self.domain_size)) - .union(borrowers); + map.insert(row, borrowers); } } - changed - } -} - -impl<'b, 'tcx> AnalysisDomain<'tcx> for PossibleBorrowerAnalysis<'b, 'tcx> { - type Domain = PossibleBorrowerState; - - const NAME: &'static str = "possible_borrower"; - - fn bottom_value(&self, body: &mir::Body<'tcx>) -> Self::Domain { - PossibleBorrowerState::new(body.local_decls.len()) - } - - fn initialize_start_block(&self, _body: &mir::Body<'tcx>, _entry_set: &mut Self::Domain) {} -} -impl<'b, 'tcx> PossibleBorrowerAnalysis<'b, 'tcx> { - fn new( - tcx: TyCtxt<'tcx>, - body: &'b mir::Body<'tcx>, - possible_origin: FxHashMap<mir::Local, HybridBitSet<mir::Local>>, - ) -> Self { - Self { - tcx, - body, - possible_origin, + let bs = BitSet::new_empty(self.body.local_decls.len()); + PossibleBorrowerMap { + map, + maybe_live, + bitset: (bs.clone(), bs), } } } -impl<'b, 'tcx> Analysis<'tcx> for PossibleBorrowerAnalysis<'b, 'tcx> { - fn apply_call_return_effect( - &self, - _state: &mut Self::Domain, - _block: BasicBlock, - _return_places: CallReturnPlaces<'_, 'tcx>, - ) { - } - - fn apply_statement_effect(&self, state: &mut Self::Domain, statement: &Statement<'tcx>, _location: Location) { - if let StatementKind::Assign(box (place, rvalue)) = &statement.kind { - let lhs = place.local; - match rvalue { - mir::Rvalue::Ref(_, _, borrowed) => { - state.add(borrowed.local, lhs); - }, - other => { - if ContainsRegion - .visit_ty(place.ty(&self.body.local_decls, self.tcx).ty) - .is_continue() - { - return; +impl<'a, 'b, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'b, 'tcx> { + fn visit_assign(&mut self, place: &mir::Place<'tcx>, rvalue: &mir::Rvalue<'_>, _location: mir::Location) { + let lhs = place.local; + match rvalue { + mir::Rvalue::Ref(_, _, borrowed) => { + self.possible_borrower.add(borrowed.local, lhs); + }, + other => { + if ContainsRegion + .visit_ty(place.ty(&self.body.local_decls, self.cx.tcx).ty) + .is_continue() + { + return; + } + rvalue_locals(other, |rhs| { + if lhs != rhs { + self.possible_borrower.add(rhs, lhs); } - rvalue_locals(other, |rhs| { - if lhs != rhs { - state.add(rhs, lhs); - } - }); - }, - } + }); + }, } } - fn apply_terminator_effect(&self, state: &mut Self::Domain, terminator: &Terminator<'tcx>, _location: Location) { + fn visit_terminator(&mut self, terminator: &mir::Terminator<'_>, _loc: mir::Location) { if let mir::TerminatorKind::Call { args, destination: mir::Place { local: dest, .. }, @@ -171,10 +123,10 @@ impl<'b, 'tcx> Analysis<'tcx> for PossibleBorrowerAnalysis<'b, 'tcx> { for y in mutable_variables { for x in &immutable_borrowers { - state.add(*x, y); + self.possible_borrower.add(*x, y); } for x in &mutable_borrowers { - state.add(*x, y); + self.possible_borrower.add(*x, y); } } } @@ -210,98 +162,73 @@ fn rvalue_locals(rvalue: &mir::Rvalue<'_>, mut visit: impl FnMut(mir::Local)) { } } -/// Result of `PossibleBorrowerAnalysis`. +/// Result of `PossibleBorrowerVisitor`. #[allow(clippy::module_name_repetitions)] pub struct PossibleBorrowerMap<'b, 'tcx> { - body: &'b mir::Body<'tcx>, - possible_borrower: ResultsCursor<'b, 'tcx, PossibleBorrowerAnalysis<'b, 'tcx>>, - maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive<'b>>, - pushed: BitSet<Local>, - stack: Vec<Local>, + /// Mapping `Local -> its possible borrowers` + pub map: FxHashMap<mir::Local, HybridBitSet<mir::Local>>, + maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive>, + // Caches to avoid allocation of `BitSet` on every query + pub bitset: (BitSet<mir::Local>, BitSet<mir::Local>), } -impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { - pub fn new(cx: &LateContext<'tcx>, mir: &'b mir::Body<'tcx>) -> Self { +impl<'a, 'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { + pub fn new(cx: &'a LateContext<'tcx>, mir: &'b mir::Body<'tcx>) -> Self { let possible_origin = { let mut vis = PossibleOriginVisitor::new(mir); vis.visit_body(mir); vis.into_map(cx) }; - let possible_borrower = PossibleBorrowerAnalysis::new(cx.tcx, mir, possible_origin) + let maybe_storage_live_result = MaybeStorageLive::new(BitSet::new_empty(mir.local_decls.len())) .into_engine(cx.tcx, mir) - .pass_name("possible_borrower") + .pass_name("redundant_clone") .iterate_to_fixpoint() .into_results_cursor(mir); - let maybe_live = MaybeStorageLive::new(Cow::Owned(BitSet::new_empty(mir.local_decls.len()))) - .into_engine(cx.tcx, mir) - .pass_name("possible_borrower") - .iterate_to_fixpoint() - .into_results_cursor(mir); - PossibleBorrowerMap { - body: mir, - possible_borrower, - maybe_live, - pushed: BitSet::new_empty(mir.local_decls.len()), - stack: Vec::with_capacity(mir.local_decls.len()), - } + let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_origin); + vis.visit_body(mir); + vis.into_map(cx, maybe_storage_live_result) } - /// Returns true if the set of borrowers of `borrowed` living at `at` includes no more than - /// `borrowers`. - /// Notes: - /// 1. It would be nice if `PossibleBorrowerMap` could store `cx` so that `at_most_borrowers` - /// would not require it to be passed in. But a `PossibleBorrowerMap` is stored in `LintPass` - /// `Dereferencing`, which outlives any `LateContext`. - /// 2. In all current uses of `at_most_borrowers`, `borrowers` is a slice of at most two - /// elements. Thus, `borrowers.contains(...)` is effectively a constant-time operation. If - /// `at_most_borrowers`'s uses were to expand beyond this, its implementation might have to be - /// adjusted. - pub fn at_most_borrowers( + /// Returns true if the set of borrowers of `borrowed` living at `at` matches with `borrowers`. + pub fn only_borrowers(&mut self, borrowers: &[mir::Local], borrowed: mir::Local, at: mir::Location) -> bool { + self.bounded_borrowers(borrowers, borrowers, borrowed, at) + } + + /// Returns true if the set of borrowers of `borrowed` living at `at` includes at least `below` + /// but no more than `above`. + pub fn bounded_borrowers( &mut self, - cx: &LateContext<'tcx>, - borrowers: &[mir::Local], + below: &[mir::Local], + above: &[mir::Local], borrowed: mir::Local, at: mir::Location, ) -> bool { - if is_copy(cx, self.body.local_decls[borrowed].ty) { - return true; - } - - self.possible_borrower.seek_before_primary_effect(at); - self.maybe_live.seek_before_primary_effect(at); - - let possible_borrower = &self.possible_borrower.get().map; - let maybe_live = &self.maybe_live; - - self.pushed.clear(); - self.stack.clear(); + self.maybe_live.seek_after_primary_effect(at); - if let Some(borrowers) = possible_borrower.get(&borrowed) { - for b in borrowers.iter() { - if self.pushed.insert(b) { - self.stack.push(b); - } + self.bitset.0.clear(); + let maybe_live = &mut self.maybe_live; + if let Some(bitset) = self.map.get(&borrowed) { + for b in bitset.iter().filter(move |b| maybe_live.contains(*b)) { + self.bitset.0.insert(b); } } else { - // Nothing borrows `borrowed` at `at`. - return true; + return false; } - while let Some(borrower) = self.stack.pop() { - if maybe_live.contains(borrower) && !borrowers.contains(&borrower) { - return false; - } + self.bitset.1.clear(); + for b in below { + self.bitset.1.insert(*b); + } - if let Some(borrowers) = possible_borrower.get(&borrower) { - for b in borrowers.iter() { - if self.pushed.insert(b) { - self.stack.push(b); - } - } - } + if !self.bitset.0.superset(&self.bitset.1) { + return false; + } + + for b in above { + self.bitset.0.remove(*b); } - true + self.bitset.0.is_empty() } pub fn local_is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool { diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index 31e1cb6c3d7..4cb7f6b687f 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -1,5 +1,5 @@ // run-rustfix -#![feature(custom_inner_attributes, lint_reasons, rustc_private)] +#![feature(lint_reasons)] #![allow( unused, clippy::uninlined_format_args, @@ -491,14 +491,3 @@ mod issue_9782_method_variant { S.foo::<&[u8; 100]>(&a); } } - -extern crate rustc_lint; -extern crate rustc_span; - -#[allow(dead_code)] -mod span_lint { - use rustc_lint::{LateContext, Lint, LintContext}; - fn foo(cx: &LateContext<'_>, lint: &'static Lint) { - cx.struct_span_lint(lint, rustc_span::Span::default(), "", |diag| diag.note(String::new())); - } -} diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs index 55c2738fcf2..9a01190ed8d 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -1,5 +1,5 @@ // run-rustfix -#![feature(custom_inner_attributes, lint_reasons, rustc_private)] +#![feature(lint_reasons)] #![allow( unused, clippy::uninlined_format_args, @@ -491,14 +491,3 @@ mod issue_9782_method_variant { S.foo::<&[u8; 100]>(&a); } } - -extern crate rustc_lint; -extern crate rustc_span; - -#[allow(dead_code)] -mod span_lint { - use rustc_lint::{LateContext, Lint, LintContext}; - fn foo(cx: &LateContext<'_>, lint: &'static Lint) { - cx.struct_span_lint(lint, rustc_span::Span::default(), "", |diag| diag.note(&String::new())); - } -} diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr index 98a48d68317..d26c317124b 100644 --- a/tests/ui/needless_borrow.stderr +++ b/tests/ui/needless_borrow.stderr @@ -216,11 +216,5 @@ error: the borrowed expression implements the required traits LL | foo(&a); | ^^ help: change this to: `a` -error: the borrowed expression implements the required traits - --> $DIR/needless_borrow.rs:502:85 - | -LL | cx.struct_span_lint(lint, rustc_span::Span::default(), "", |diag| diag.note(&String::new())); - | ^^^^^^^^^^^^^^ help: change this to: `String::new()` - -error: aborting due to 37 previous errors +error: aborting due to 36 previous errors diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index a157b6a6f9a..00b42745093 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -239,9 +239,3 @@ fn false_negative_5707() { let _z = x.clone(); // pr 7346 can't lint on `x` drop(y); } - -#[allow(unused, clippy::manual_retain)] -fn possible_borrower_improvements() { - let mut s = String::from("foobar"); - s = s.chars().filter(|&c| c != 'o').collect(); -} diff --git a/tests/ui/redundant_clone.rs b/tests/ui/redundant_clone.rs index 430672e8b8d..f899127db8d 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -239,9 +239,3 @@ fn false_negative_5707() { let _z = x.clone(); // pr 7346 can't lint on `x` drop(y); } - -#[allow(unused, clippy::manual_retain)] -fn possible_borrower_improvements() { - let mut s = String::from("foobar"); - s = s.chars().filter(|&c| c != 'o').to_owned().collect(); -} diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index 1bacc2c76af..782590034d0 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -179,17 +179,5 @@ note: this value is dropped without further use LL | foo(&x.clone(), move || { | ^ -error: redundant clone - --> $DIR/redundant_clone.rs:246:40 - | -LL | s = s.chars().filter(|&c| c != 'o').to_owned().collect(); - | ^^^^^^^^^^^ help: remove this - | -note: this value is dropped without further use - --> $DIR/redundant_clone.rs:246:9 - | -LL | s = s.chars().filter(|&c| c != 'o').to_owned().collect(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: aborting due to 16 previous errors +error: aborting due to 15 previous errors |