From ed519ad746e31f64c4e9255be561785612532d37 Mon Sep 17 00:00:00 2001
From: Samuel Moelius <sam@moeli.us>
Date: Wed, 12 Oct 2022 04:34:25 -0400
Subject: [PATCH] Improve `possible_borrower`
---
clippy_lints/src/dereference.rs | 8 +-
clippy_lints/src/redundant_clone.rs | 4 +-
clippy_utils/src/mir/possible_borrower.rs | 271 ++++++++++++++--------
tests/ui/needless_borrow.fixed | 2 +-
tests/ui/needless_borrow.stderr | 8 +-
tests/ui/redundant_clone.fixed | 2 +-
tests/ui/redundant_clone.stderr | 14 +-
7 files changed, 198 insertions(+), 111 deletions(-)
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
index 7b43d8ccc67..728941b8b3d 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 `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)
+ // 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)
&& 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 c1677fb3da1..0e7c5cca724 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.only_borrowers(&[arg], cloned, loc) {
+ if cannot_move_out || !possible_borrower.at_most_borrowers(cx, &[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.only_borrowers(&[arg, cloned], local, loc) {
+ if cannot_move_out || !possible_borrower.at_most_borrowers(cx, &[arg, cloned], local, loc) {
continue;
}
diff --git a/clippy_utils/src/mir/possible_borrower.rs b/clippy_utils/src/mir/possible_borrower.rs
index 8c695801c73..ebe7d339309 100644
--- a/clippy_utils/src/mir/possible_borrower.rs
+++ b/clippy_utils/src/mir/possible_borrower.rs
@@ -1,89 +1,137 @@
-use super::{possible_origin::PossibleOriginVisitor, transitive_relation::TransitiveRelation};
+use super::possible_origin::PossibleOriginVisitor;
use crate::ty::is_copy;
-use rustc_data_structures::fx::FxHashMap;
+use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
use rustc_index::bit_set::{BitSet, HybridBitSet};
use rustc_lint::LateContext;
-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 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::collections::VecDeque;
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 PossibleBorrowerVisitor<'a, 'b, 'tcx> {
- possible_borrower: TransitiveRelation,
+struct PossibleBorrowerAnalysis<'b, 'tcx> {
+ tcx: TyCtxt<'tcx>,
body: &'b mir::Body<'tcx>,
- cx: &'a LateContext<'tcx>,
possible_origin: FxHashMap<mir::Local, HybridBitSet<mir::Local>>,
}
-impl<'a, 'b, 'tcx> PossibleBorrowerVisitor<'a, 'b, 'tcx> {
+#[derive(Clone, Debug, Eq, PartialEq)]
+struct PossibleBorrowerState {
+ map: FxIndexMap<Local, BitSet<Local>>,
+ domain_size: usize,
+}
+
+impl PossibleBorrowerState {
+ fn new(domain_size: usize) -> Self {
+ Self {
+ map: FxIndexMap::default(),
+ domain_size,
+ }
+ }
+
+ #[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!()
+ }
+}
+
+impl JoinSemiLattice for PossibleBorrowerState {
+ fn join(&mut self, other: &Self) -> bool {
+ let mut changed = false;
+ for (&borrowed, borrowers) in other.map.iter() {
+ if !borrowers.is_empty() {
+ changed |= self
+ .map
+ .entry(borrowed)
+ .or_insert(BitSet::new_empty(self.domain_size))
+ .union(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(
- cx: &'a LateContext<'tcx>,
+ tcx: TyCtxt<'tcx>,
body: &'b mir::Body<'tcx>,
possible_origin: FxHashMap<mir::Local, HybridBitSet<mir::Local>>,
) -> Self {
Self {
- possible_borrower: TransitiveRelation::default(),
- cx,
+ tcx,
body,
possible_origin,
}
}
-
- 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;
- }
-
- 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() {
- map.insert(row, borrowers);
- }
- }
-
- let bs = BitSet::new_empty(self.body.local_decls.len());
- PossibleBorrowerMap {
- map,
- maybe_live,
- bitset: (bs.clone(), bs),
- }
- }
}
-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);
+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;
}
- });
- },
+ rvalue_locals(other, |rhs| {
+ if lhs != rhs {
+ state.add(rhs, lhs);
+ }
+ });
+ },
+ }
}
}
- fn visit_terminator(&mut self, terminator: &mir::Terminator<'_>, _loc: mir::Location) {
+ fn apply_terminator_effect(&self, state: &mut Self::Domain, terminator: &Terminator<'tcx>, _location: Location) {
if let mir::TerminatorKind::Call {
args,
destination: mir::Place { local: dest, .. },
@@ -123,10 +171,10 @@ impl<'a, 'b, 'tcx> mir::visit::Visitor<'tcx> for PossibleBorrowerVisitor<'a, 'b,
for y in mutable_variables {
for x in &immutable_borrowers {
- self.possible_borrower.add(*x, y);
+ state.add(*x, y);
}
for x in &mutable_borrowers {
- self.possible_borrower.add(*x, y);
+ state.add(*x, y);
}
}
}
@@ -162,73 +210,94 @@ fn rvalue_locals(rvalue: &mir::Rvalue<'_>, mut visit: impl FnMut(mir::Local)) {
}
}
-/// Result of `PossibleBorrowerVisitor`.
+/// Result of `PossibleBorrowerAnalysis`.
#[allow(clippy::module_name_repetitions)]
pub struct PossibleBorrowerMap<'b, 'tcx> {
- /// Mapping `Local -> its possible borrowers`
- pub map: FxHashMap<mir::Local, HybridBitSet<mir::Local>>,
+ body: &'b mir::Body<'tcx>,
+ possible_borrower: ResultsCursor<'b, 'tcx, PossibleBorrowerAnalysis<'b, 'tcx>>,
maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive>,
- // Caches to avoid allocation of `BitSet` on every query
- pub bitset: (BitSet<mir::Local>, BitSet<mir::Local>),
}
-impl<'a, 'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> {
- pub fn new(cx: &'a LateContext<'tcx>, mir: &'b mir::Body<'tcx>) -> Self {
+impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> {
+ pub fn new(cx: &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 maybe_storage_live_result = MaybeStorageLive::new(BitSet::new_empty(mir.local_decls.len()))
+ let possible_borrower = PossibleBorrowerAnalysis::new(cx.tcx, mir, possible_origin)
.into_engine(cx.tcx, mir)
- .pass_name("redundant_clone")
+ .pass_name("possible_borrower")
.iterate_to_fixpoint()
.into_results_cursor(mir);
- let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_origin);
- vis.visit_body(mir);
- vis.into_map(cx, maybe_storage_live_result)
+ let maybe_live = MaybeStorageLive::new(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,
+ }
}
- /// 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(
+ /// 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(
&mut self,
- below: &[mir::Local],
- above: &[mir::Local],
+ cx: &LateContext<'tcx>,
+ borrowers: &[mir::Local],
borrowed: mir::Local,
at: mir::Location,
) -> bool {
- self.maybe_live.seek_after_primary_effect(at);
+ if is_copy(cx, self.body.local_decls[borrowed].ty) {
+ return true;
+ }
- 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);
+ 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;
+
+ let mut queued = BitSet::new_empty(self.body.local_decls.len());
+ let mut deque = VecDeque::with_capacity(self.body.local_decls.len());
+
+ if let Some(borrowers) = possible_borrower.get(&borrowed) {
+ for b in borrowers.iter() {
+ if queued.insert(b) {
+ deque.push_back(b);
+ }
}
} else {
- return false;
+ // Nothing borrows `borrowed` at `at`.
+ return true;
}
- self.bitset.1.clear();
- for b in below {
- self.bitset.1.insert(*b);
+ while let Some(borrower) = deque.pop_front() {
+ if maybe_live.contains(borrower) && !borrowers.contains(&borrower) {
+ return false;
+ }
+
+ if let Some(borrowers) = possible_borrower.get(&borrower) {
+ for b in borrowers.iter() {
+ if queued.insert(b) {
+ deque.push_back(b);
+ }
+ }
+ }
}
- if !self.bitset.0.superset(&self.bitset.1) {
- return false;
- }
-
- for b in above {
- self.bitset.0.remove(*b);
- }
-
- self.bitset.0.is_empty()
+ true
}
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 e970f87e226..31e1cb6c3d7 100644
--- a/tests/ui/needless_borrow.fixed
+++ b/tests/ui/needless_borrow.fixed
@@ -499,6 +499,6 @@ extern crate rustc_span;
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()));
+ 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 d26c317124b..98a48d68317 100644
--- a/tests/ui/needless_borrow.stderr
+++ b/tests/ui/needless_borrow.stderr
@@ -216,5 +216,11 @@ error: the borrowed expression implements the required traits
LL | foo(&a);
| ^^ help: change this to: `a`
-error: aborting due to 36 previous errors
+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
diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed
index 369ae3f7147..a157b6a6f9a 100644
--- a/tests/ui/redundant_clone.fixed
+++ b/tests/ui/redundant_clone.fixed
@@ -243,5 +243,5 @@ fn false_negative_5707() {
#[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();
+ s = s.chars().filter(|&c| c != 'o').collect();
}
diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr
index 782590034d0..1bacc2c76af 100644
--- a/tests/ui/redundant_clone.stderr
+++ b/tests/ui/redundant_clone.stderr
@@ -179,5 +179,17 @@ note: this value is dropped without further use
LL | foo(&x.clone(), move || {
| ^
-error: aborting due to 15 previous errors
+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