From df50398fa2e8e7161dc277dcb54f1d714d72138a Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Wed, 12 Oct 2022 04:14:42 -0400 Subject: [PATCH 1/6] Add tests --- tests/ui/needless_borrow.fixed | 11 +++++++++++ tests/ui/needless_borrow.rs | 11 +++++++++++ tests/ui/redundant_clone.fixed | 6 ++++++ tests/ui/redundant_clone.rs | 6 ++++++ 4 files changed, 34 insertions(+) diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index 80cdb4e472d4..4cc9dcc35d31 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -492,3 +492,14 @@ 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 99f735127eb8..13b2fd473579 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -492,3 +492,14 @@ 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/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index 5037c08ebd5f..a0566ee2b430 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -245,3 +245,9 @@ 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.rs b/tests/ui/redundant_clone.rs index 501898bf113c..234d560b7827 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -245,3 +245,9 @@ 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(); +} From 688afff5c39e6f2f9eaff2ea9118585c57342a48 Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Wed, 12 Oct 2022 04:34:25 -0400 Subject: [PATCH 2/6] Improve `possible_borrower` --- clippy_lints/src/dereference.rs | 8 +- clippy_lints/src/redundant_clone.rs | 4 +- clippy_utils/src/mir/possible_borrower.rs | 265 ++++++++++++++-------- 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, 195 insertions(+), 108 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 7787fc348daa..7175a4587430 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1311,10 +1311,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 685d738cbb7c..42585ecb1361 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -130,7 +130,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; } @@ -177,7 +177,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 920ce8e655be..e845a56405cc 100644 --- a/clippy_utils/src/mir/possible_borrower.rs +++ b/clippy_utils/src/mir/possible_borrower.rs @@ -1,90 +1,138 @@ -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::mir::{ + self, visit::Visitor as _, BasicBlock, Local, Location, Mutability, Statement, StatementKind, Terminator, +}; use rustc_middle::ty::{self, visit::TypeVisitor, TyCtxt}; -use rustc_mir_dataflow::{impls::MaybeStorageLive, Analysis, ResultsCursor}; +use rustc_mir_dataflow::{ + fmt::DebugWithContext, impls::MaybeStorageLive, lattice::JoinSemiLattice, Analysis, AnalysisDomain, + CallReturnPlaces, ResultsCursor, +}; use std::borrow::Cow; +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>, } -impl<'a, 'b, 'tcx> PossibleBorrowerVisitor<'a, 'b, 'tcx> { - fn new( - cx: &'a LateContext<'tcx>, - body: &'b mir::Body<'tcx>, - possible_origin: FxHashMap>, - ) -> Self { +#[derive(Clone, Debug, Eq, PartialEq)] +struct PossibleBorrowerState { + map: FxIndexMap>, + domain_size: usize, +} + +impl PossibleBorrowerState { + fn new(domain_size: usize) -> Self { Self { - possible_borrower: TransitiveRelation::default(), - cx, - body, - possible_origin, + map: FxIndexMap::default(), + domain_size, } } - fn into_map( - self, - cx: &'a LateContext<'tcx>, - maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive<'tcx>>, - ) -> 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; - } + #[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 DebugWithContext 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!() + } +} - let mut borrowers = self.possible_borrower.reachable_from(row, self.body.local_decls.len()); - borrowers.remove(mir::Local::from_usize(0)); +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() { - map.insert(row, borrowers); + changed |= self + .map + .entry(borrowed) + .or_insert(BitSet::new_empty(self.domain_size)) + .union(borrowers); } } + changed + } +} - let bs = BitSet::new_empty(self.body.local_decls.len()); - PossibleBorrowerMap { - map, - maybe_live, - bitset: (bs.clone(), bs), +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>, + ) -> Self { + Self { + tcx, + body, + possible_origin, } } } -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, .. }, @@ -124,10 +172,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); } } } @@ -163,73 +211,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>, - maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive<'tcx>>, - // Caches to avoid allocation of `BitSet` on every query - pub bitset: (BitSet, BitSet), + body: &'b mir::Body<'tcx>, + possible_borrower: ResultsCursor<'b, 'tcx, PossibleBorrowerAnalysis<'b, 'tcx>>, + maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive<'b>>, } -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(Cow::Owned(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) - } - - /// 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) + 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, + } } - /// 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.possible_borrower.seek_before_primary_effect(at); + self.maybe_live.seek_before_primary_effect(at); - 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); + 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); - } - - if !self.bitset.0.superset(&self.bitset.1) { - return false; - } + while let Some(borrower) = deque.pop_front() { + if maybe_live.contains(borrower) && !borrowers.contains(&borrower) { + return false; + } - for b in above { - self.bitset.0.remove(*b); + if let Some(borrowers) = possible_borrower.get(&borrower) { + for b in borrowers.iter() { + if queued.insert(b) { + deque.push_back(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 4cc9dcc35d31..4112dccff816 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -500,6 +500,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 f85b4fb46a65..b6ad4ce9b1a5 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 a0566ee2b430..5adabb94bee2 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -249,5 +249,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 8660c0e1f6a0..38367aa6663c 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 From 85e0f69221ba91331cda6f41dcd668573c2bb85a Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Tue, 13 Dec 2022 12:50:03 -0500 Subject: [PATCH 3/6] Address review comments --- clippy_utils/src/mir/possible_borrower.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/clippy_utils/src/mir/possible_borrower.rs b/clippy_utils/src/mir/possible_borrower.rs index e845a56405cc..0d42166c188c 100644 --- a/clippy_utils/src/mir/possible_borrower.rs +++ b/clippy_utils/src/mir/possible_borrower.rs @@ -12,7 +12,6 @@ use rustc_mir_dataflow::{ CallReturnPlaces, ResultsCursor, }; use std::borrow::Cow; -use std::collections::VecDeque; use std::ops::ControlFlow; /// Collects the possible borrowers of each local. @@ -217,6 +216,8 @@ 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, + stack: Vec, } impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { @@ -240,6 +241,8 @@ impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { body: mir, possible_borrower, maybe_live, + pushed: BitSet::new_empty(mir.local_decls.len()), + stack: Vec::with_capacity(mir.local_decls.len()), } } @@ -270,13 +273,13 @@ impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { 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()); + self.pushed.clear(); + self.stack.clear(); if let Some(borrowers) = possible_borrower.get(&borrowed) { for b in borrowers.iter() { - if queued.insert(b) { - deque.push_back(b); + if self.pushed.insert(b) { + self.stack.push(b); } } } else { @@ -284,15 +287,15 @@ impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { return true; } - while let Some(borrower) = deque.pop_front() { + while let Some(borrower) = self.stack.pop() { 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.pushed.insert(b) { + self.stack.push(b); } } } From c3dcb14faafa4166ed71d4dd092b540d0d0d6d8f Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Tue, 20 Dec 2022 05:30:12 -0500 Subject: [PATCH 4/6] Add tests from #10134 --- tests/ui/crashes/possible_borrower.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 tests/ui/crashes/possible_borrower.rs diff --git a/tests/ui/crashes/possible_borrower.rs b/tests/ui/crashes/possible_borrower.rs new file mode 100644 index 000000000000..d7d54a4bd14a --- /dev/null +++ b/tests/ui/crashes/possible_borrower.rs @@ -0,0 +1,24 @@ +// https://github.com/rust-lang/rust-clippy/issues/10134 + +fn meow(_s: impl AsRef) {} + +macro_rules! quad { + ($x:stmt) => { + $x + $x + $x + $x + }; +} + +fn main() { + let i = 0; + quad!(quad!(quad!(quad!(quad!(meow(format!("abc{i}"))))))); +} + +// https://github.com/rust-lang/rust-clippy/issues/10134#issuecomment-1374480660 +fn second_testcase() { + quad!(quad!(quad!(for i in 0..4 { + quad!(quad!(meow(format!("abc{i}")))); + }))); +} From 7f8e2c822d430a033ffa127feeac5f9fcf0876cc Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Sun, 5 Feb 2023 07:49:18 -0500 Subject: [PATCH 5/6] Implement `PossibleBorrowerMap` using the `borrowck` crate --- clippy_lints/src/dereference.rs | 2 +- clippy_lints/src/redundant_clone.rs | 2 +- clippy_utils/src/lib.rs | 4 + clippy_utils/src/mir/mod.rs | 200 +++++++++- clippy_utils/src/mir/possible_borrower.rs | 386 +++++++------------- clippy_utils/src/mir/possible_origin.rs | 59 --- clippy_utils/src/mir/transitive_relation.rs | 29 -- tests/ui/redundant_clone.fixed | 13 +- tests/ui/redundant_clone.rs | 13 +- tests/ui/redundant_clone.stderr | 4 +- 10 files changed, 358 insertions(+), 354 deletions(-) delete mode 100644 clippy_utils/src/mir/possible_origin.rs delete mode 100644 clippy_utils/src/mir/transitive_relation.rs diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 7175a4587430..5489bc515e5a 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1308,7 +1308,7 @@ fn referent_used_exactly_once<'tcx>( .last() .map_or(true, |&(local_def_id, _)| local_def_id != body_owner_local_def_id) { - possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, mir))); + possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, body_owner_local_def_id))); } 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 diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 42585ecb1361..b8bc1ee0ff6f 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -81,7 +81,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { let mir = cx.tcx.optimized_mir(def_id.to_def_id()); - let mut possible_borrower = PossibleBorrowerMap::new(cx, mir); + let mut possible_borrower = PossibleBorrowerMap::new(cx, def_id); for (bb, bbdata) in mir.basic_blocks.iter_enumerated() { let terminator = bbdata.terminator(); diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 311f6dbc696d..e133315a7d1a 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -20,6 +20,7 @@ extern crate rustc_ast; extern crate rustc_ast_pretty; extern crate rustc_attr; +extern crate rustc_borrowck; extern crate rustc_const_eval; extern crate rustc_data_structures; // The `rustc_driver` crate seems to be required in order to use the `rust_ast` crate. @@ -33,7 +34,10 @@ extern crate rustc_infer; extern crate rustc_lexer; extern crate rustc_lint; extern crate rustc_middle; +extern crate rustc_mir_build; extern crate rustc_mir_dataflow; +extern crate rustc_mir_transform; +extern crate rustc_parse_format; extern crate rustc_session; extern crate rustc_span; extern crate rustc_target; diff --git a/clippy_utils/src/mir/mod.rs b/clippy_utils/src/mir/mod.rs index 26c0015e87e0..0c54f9957616 100644 --- a/clippy_utils/src/mir/mod.rs +++ b/clippy_utils/src/mir/mod.rs @@ -1,17 +1,16 @@ use rustc_hir::{Expr, HirId}; -use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor}; +use rustc_middle::mir::visit::{ + MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, TyContext, Visitor, +}; use rustc_middle::mir::{ - traversal, Body, InlineAsmOperand, Local, Location, Place, StatementKind, TerminatorKind, START_BLOCK, + traversal, Body, InlineAsmOperand, Local, LocalDecl, Location, Place, Statement, StatementKind, Terminator, + TerminatorKind, START_BLOCK, }; -use rustc_middle::ty::TyCtxt; +use rustc_middle::ty::{Region, Ty, TyCtxt}; mod possible_borrower; pub use possible_borrower::PossibleBorrowerMap; -mod possible_origin; - -mod transitive_relation; - #[derive(Clone, Debug, Default)] pub struct LocalUsage { /// The locations where the local is used, if any. @@ -119,6 +118,193 @@ pub fn expr_local(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> Option { }) } +/// Tries to find the local in `to_mir` corresponding to `local` in `from_mir`. +pub fn translate_local<'tcx>( + tcx: TyCtxt<'tcx>, + from_mir: &Body<'tcx>, + to_mir: &Body<'tcx>, + local: Local, +) -> Option { + let equiv_decl = |lhs: &LocalDecl<'tcx>, rhs: &LocalDecl<'tcx>| { + lhs.mutability == rhs.mutability + && tcx.erase_regions_ty(lhs.ty) == tcx.erase_regions_ty(rhs.ty) + && lhs.source_info.span == rhs.source_info.span + }; + + let from_decl = &from_mir.local_decls[local]; + + // Fast path + if to_mir + .local_decls + .get(local) + .map_or(false, |to_decl| equiv_decl(from_decl, to_decl)) + { + return Some(local); + } + + // Slow path + to_mir + .local_decls + .iter() + .position(|to_decl| equiv_decl(from_decl, to_decl)) + .map(Into::into) +} + +/// Tries to find the location in `to_mir` corresponding to `location` in `from_mir`. +pub fn translate_location<'tcx>( + tcx: TyCtxt<'tcx>, + from_mir: &Body<'tcx>, + to_mir: &Body<'tcx>, + location: Location, +) -> Option { + let normalized_lhs = from_mir + .stmt_at(location) + .map_left(|statement| normalize_statement(tcx, statement)) + .map_right(|terminator| normalize_terminator(tcx, terminator)); + + for (block, block_data) in to_mir.basic_blocks.iter_enumerated() { + if let Some(location) = normalized_lhs.as_ref().either( + |normalized_lhs| { + (0..block_data.statements.len()).find_map(|statement_index| { + let rhs = &block_data.statements[statement_index]; + if normalized_lhs.source_info.span == rhs.source_info.span + && normalized_lhs.kind == normalize_statement(tcx, rhs).kind + { + Some(Location { block, statement_index }) + } else { + None + } + }) + }, + |normalized_lhs| { + if block_data.terminator.as_ref().map_or(false, |rhs| { + normalized_lhs.source_info.span == rhs.source_info.span + && normalized_lhs.kind == normalize_terminator(tcx, rhs).kind + }) { + Some(Location { + block, + statement_index: block_data.statements.len(), + }) + } else { + None + } + }, + ) { + return Some(location); + } + } + + None +} + +fn normalize_statement<'tcx>(tcx: TyCtxt<'tcx>, statement: &Statement<'tcx>) -> Statement<'tcx> { + let mut statement = statement.clone(); + Normalizer { tcx }.visit_statement(&mut statement, Location::START); + statement +} + +fn normalize_terminator<'tcx>(tcx: TyCtxt<'tcx>, terminator: &Terminator<'tcx>) -> Terminator<'tcx> { + let mut terminator = terminator.clone(); + Normalizer { tcx }.visit_terminator(&mut terminator, Location::START); + match terminator.kind { + // No basic blocks + TerminatorKind::Abort + | TerminatorKind::GeneratorDrop + | TerminatorKind::Resume + | TerminatorKind::Return + | TerminatorKind::Unreachable => {}, + + // One basic block + TerminatorKind::Goto { ref mut target } => { + *target = Location::START.block; + }, + + // Two basic blocks + TerminatorKind::FalseEdge { + ref mut real_target, + ref mut imaginary_target, + } => { + *real_target = Location::START.block; + *imaginary_target = Location::START.block; + }, + + // One optional and one non-optional basic block + TerminatorKind::Assert { + ref mut target, + cleanup: ref mut unwind, + .. + } + | TerminatorKind::Drop { + ref mut target, + ref mut unwind, + .. + } + | TerminatorKind::DropAndReplace { + ref mut target, + ref mut unwind, + .. + } + | TerminatorKind::FalseUnwind { + real_target: ref mut target, + ref mut unwind, + .. + } + | TerminatorKind::Yield { + resume: ref mut target, + drop: ref mut unwind, + .. + } => { + *target = Location::START.block; + *unwind = None; + }, + + // Two optional basic blocks + TerminatorKind::Call { + ref mut target, + ref mut cleanup, + .. + } + | TerminatorKind::InlineAsm { + destination: ref mut target, + ref mut cleanup, + .. + } => { + *target = None; + *cleanup = None; + }, + + // Arbitrarily many basic blocks + TerminatorKind::SwitchInt { ref mut targets, .. } => { + for (_, ref mut target) in targets.iter() { + *target = Location::START.block; + } + }, + } + terminator +} + +struct Normalizer<'tcx> { + tcx: TyCtxt<'tcx>, +} + +impl<'tcx> MutVisitor<'tcx> for Normalizer<'tcx> { + fn tcx<'a>(&'a self) -> TyCtxt<'tcx> { + self.tcx + } + + fn visit_local(&mut self, local: &mut Local, _context: PlaceContext, _location: Location) { + *local = Local::from_u32(0); + } + + fn visit_region(&mut self, region: &mut Region<'tcx>, _: Location) { + *region = self.tcx.lifetimes.re_erased; + } + + fn visit_ty(&mut self, ty: &mut Ty<'tcx>, _: TyContext) { + *ty = self.tcx.erase_regions_ty(*ty); + } +} + /// Returns a vector of `mir::Location` where `local` is assigned. pub fn local_assignments(mir: &Body<'_>, local: Local) -> Vec { let mut locations = Vec::new(); diff --git a/clippy_utils/src/mir/possible_borrower.rs b/clippy_utils/src/mir/possible_borrower.rs index 0d42166c188c..6c0b8cf6d7d8 100644 --- a/clippy_utils/src/mir/possible_borrower.rs +++ b/clippy_utils/src/mir/possible_borrower.rs @@ -1,249 +1,85 @@ -use super::possible_origin::PossibleOriginVisitor; +use super::{translate_local, translate_location}; use crate::ty::is_copy; -use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; -use rustc_index::bit_set::{BitSet, HybridBitSet}; +use rustc_borrowck::{borrow_set::BorrowSet, nll, region_infer::RegionInferenceContext, NllCtxt}; +use rustc_hir::def_id::LocalDefId; +use rustc_index::{bit_set::BitSet, vec::IndexVec}; +use rustc_infer::infer::{DefiningAnchor, TyCtxtInferExt}; 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 rustc_middle::mir; +use rustc_middle::ty::{self, visit::TypeVisitor, RegionKind, RegionVid, Ty, TyCtxt, WithOptConstParam}; +use rustc_mir_build::{build::mir_build_fn_or_const, thir::cx::thir_build}; +use rustc_mir_dataflow::{impls::MaybeStorageLive, Analysis, ResultsCursor}; +use rustc_mir_transform::{mir_compute_const_qualifs, mir_promote, mir_ready_for_const_eval}; use std::borrow::Cow; use std::ops::ControlFlow; +use std::rc::Rc; -/// Collects the possible borrowers of each local. -/// For example, `b = &a; c = &a;` will make `b` and (transitively) `c` -/// possible borrowers of `a`. +/// Result of `PossibleBorrowerAnalysis`. #[allow(clippy::module_name_repetitions)] -struct PossibleBorrowerAnalysis<'b, 'tcx> { +pub struct PossibleBorrowerMap<'b, 'tcx> { tcx: TyCtxt<'tcx>, - body: &'b mir::Body<'tcx>, - possible_origin: FxHashMap>, -} - -#[derive(Clone, Debug, Eq, PartialEq)] -struct PossibleBorrowerState { - map: FxIndexMap>, - domain_size: usize, + local_def_id: LocalDefId, + regioncx: Rc>, + body: Rc>, + borrow_set: Rc>, + maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive<'b>>, } -impl PossibleBorrowerState { - fn new(domain_size: usize) -> Self { - Self { - map: FxIndexMap::default(), - domain_size, - } - } +impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { + pub fn new(cx: &LateContext<'tcx>, local_def_id: LocalDefId) -> Self { + let tcx = cx.tcx; - #[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); - } -} + let infcx = tcx + .infer_ctxt() + .with_opaque_type_inference(DefiningAnchor::Bind(local_def_id)) + .build(); -impl DebugWithContext 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!() - } -} + let (input_body, input_promoted) = Self::mir_build(tcx, local_def_id); -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 - } -} + let mut nll_ctxt = NllCtxt::new(&infcx, &input_body, &input_promoted); -impl<'b, 'tcx> AnalysisDomain<'tcx> for PossibleBorrowerAnalysis<'b, 'tcx> { - type Domain = PossibleBorrowerState; + let nll::NllOutput { regioncx, .. } = nll_ctxt.compute_regions(false); - const NAME: &'static str = "possible_borrower"; + let NllCtxt { body, borrow_set, .. } = nll_ctxt; - fn bottom_value(&self, body: &mir::Body<'tcx>) -> Self::Domain { - PossibleBorrowerState::new(body.local_decls.len()) - } + let regioncx = Rc::new(regioncx); + let body = Rc::new(body); + let borrow_set = Rc::new(borrow_set); - fn initialize_start_block(&self, _body: &mir::Body<'tcx>, _entry_set: &mut Self::Domain) {} -} + let maybe_live = MaybeStorageLive::new(Cow::Owned(BitSet::new_empty(body.local_decls.len()))) + .into_engine(tcx, &body) + .pass_name("possible_borrower") + .iterate_to_fixpoint() + .into_results_cursor(body.clone()); -impl<'b, 'tcx> PossibleBorrowerAnalysis<'b, 'tcx> { - fn new( - tcx: TyCtxt<'tcx>, - body: &'b mir::Body<'tcx>, - possible_origin: FxHashMap>, - ) -> Self { - Self { + PossibleBorrowerMap { tcx, + local_def_id, + regioncx, body, - possible_origin, - } - } -} - -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 apply_terminator_effect(&self, state: &mut Self::Domain, terminator: &Terminator<'tcx>, _location: Location) { - if let mir::TerminatorKind::Call { - args, - destination: mir::Place { local: dest, .. }, - .. - } = &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`. - - let mut immutable_borrowers = vec![]; - let mut mutable_borrowers = vec![]; - - for op in args { - match op { - mir::Operand::Copy(p) | mir::Operand::Move(p) => { - 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_origin.get(r)) - .flat_map(HybridBitSet::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 { - state.add(*x, y); - } - for x in &mutable_borrowers { - state.add(*x, y); - } - } + borrow_set, + maybe_live, } } -} -struct ContainsRegion; + fn mir_build( + tcx: TyCtxt<'tcx>, + local_def_id: LocalDefId, + ) -> (mir::Body<'tcx>, IndexVec>) { + let def = WithOptConstParam { + did: local_def_id, + const_param_did: None, + }; -impl TypeVisitor> for ContainsRegion { - type BreakTy = (); + let (thir, expr) = thir_build(tcx, def).expect("`thir_build` should succeed"); - fn visit_region(&mut self, _: ty::Region<'_>) -> ControlFlow { - ControlFlow::Break(()) - } -} + let body_init = mir_build_fn_or_const(tcx, def, thir, expr); -fn rvalue_locals(rvalue: &mir::Rvalue<'_>, mut visit: impl FnMut(mir::Local)) { - use rustc_middle::mir::Rvalue::{Aggregate, BinaryOp, Cast, CheckedBinaryOp, Repeat, UnaryOp, Use}; - - let mut visit_op = |op: &mir::Operand<'_>| match op { - mir::Operand::Copy(p) | mir::Operand::Move(p) => visit(p.local), - mir::Operand::Constant(..) => (), - }; - - match rvalue { - Use(op) | Repeat(op, _) | Cast(_, op, _) | UnaryOp(_, op) => visit_op(op), - Aggregate(_, ops) => ops.iter().for_each(visit_op), - BinaryOp(_, box (lhs, rhs)) | CheckedBinaryOp(_, box (lhs, rhs)) => { - visit_op(lhs); - visit_op(rhs); - }, - _ => (), - } -} + let body_const = mir_ready_for_const_eval(tcx, body_init); -/// Result of `PossibleBorrowerAnalysis`. -#[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, - stack: Vec, -} + let const_qualifs = mir_compute_const_qualifs(tcx, def, || &body_const); -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 possible_borrower = PossibleBorrowerAnalysis::new(cx.tcx, mir, possible_origin) - .into_engine(cx.tcx, mir) - .pass_name("possible_borrower") - .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()), - } + mir_promote(tcx, const_qualifs, body_const) } /// Returns true if the set of borrowers of `borrowed` living at `at` includes no more than @@ -263,49 +99,101 @@ impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { borrowed: mir::Local, at: mir::Location, ) -> bool { - if is_copy(cx, self.body.local_decls[borrowed].ty) { + let mir = cx.tcx.optimized_mir(self.local_def_id.to_def_id()); + if is_copy(cx, mir.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; + let Some(borrowers) = borrowers + .iter() + .map(|&borrower| translate_local(cx.tcx, mir, &self.body, borrower)) + .collect::>>() + else { + debug_assert!(false, "can't find {borrowers:?}"); + return false; + }; - self.pushed.clear(); - self.stack.clear(); + let Some(borrowed) = translate_local(cx.tcx, mir, &self.body, borrowed) else { + debug_assert!(false, "can't find {borrowed:?}"); + return false; + }; - if let Some(borrowers) = possible_borrower.get(&borrowed) { - for b in borrowers.iter() { - if self.pushed.insert(b) { - self.stack.push(b); - } - } - } else { - // Nothing borrows `borrowed` at `at`. - return true; - } + let Some(at) = translate_location(cx.tcx, mir, &self.body, at) else { + debug_assert!(false, "can't find {at:?}: {:?}", mir.stmt_at(at)); + return false; + }; - while let Some(borrower) = self.stack.pop() { - if maybe_live.contains(borrower) && !borrowers.contains(&borrower) { - return false; - } + self.maybe_live.seek_before_primary_effect(at); - if let Some(borrowers) = possible_borrower.get(&borrower) { - for b in borrowers.iter() { - if self.pushed.insert(b) { - self.stack.push(b); + let maybe_live = self.maybe_live.get(); + + // For each borrow of `borrowed`, ask the following question: + // + // - Is there any local, live at location `at`, and with an associated region that is outlived by + // the borrow region, but that is not also outlived by any of the regions of `borrowers`? + // + // If the answer to any of these question is "yes," then there are potential additional borrowers of + // `borrowed`. + // + // Note that the `any` closure has no side effects. So the result is the same regardless of the + // order in which `index`es are visited. + #[allow(rustc::potential_query_instability)] + !self.borrow_set.local_map[&borrowed].iter().any(|index| { + let root_vid = self.borrow_set.location_map[index.as_usize()].region; + + maybe_live.iter().any(|local| { + let local_regions = collect_regions(self.body.local_decls[local].ty); + + local_regions.iter().any(|&local_vid| { + if !self.regioncx.eval_outlives(root_vid, local_vid) { + return false; } - } - } - } - true + !borrowers + .iter() + .filter_map(|&borrower| self.borrower_vid(borrower)) + .any(|borrower_vid| self.regioncx.eval_outlives(borrower_vid, local_vid)) + }) + }) + }) + } + + fn borrower_vid(&self, borrower: mir::Local) -> Option { + if let ty::Ref(region, _, _) = self.body.local_decls[borrower].ty.kind() + && let ty::RegionKind::ReVar(borrower_vid) = region.kind() + { + Some(borrower_vid) + } else { + None + } } pub fn local_is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool { + let mir = self.tcx.optimized_mir(self.local_def_id.to_def_id()); + + let Some(at) = translate_location(self.tcx, mir, &self.body, at) else { + debug_assert!(false, "can't find {at:?}: {:?}", mir.stmt_at(at)); + return false; + }; + self.maybe_live.seek_after_primary_effect(at); self.maybe_live.contains(local) } } + +fn collect_regions(ty: Ty<'_>) -> Vec { + let mut rc = RegionCollector(Vec::new()); + rc.visit_ty(ty); + rc.0 +} + +struct RegionCollector(Vec); + +impl<'tcx> TypeVisitor<'tcx> for RegionCollector { + fn visit_region(&mut self, region: ty::Region<'tcx>) -> ControlFlow { + if let RegionKind::ReVar(vid) = region.kind() { + self.0.push(vid); + } + ControlFlow::Continue(()) + } +} diff --git a/clippy_utils/src/mir/possible_origin.rs b/clippy_utils/src/mir/possible_origin.rs deleted file mode 100644 index 8e7513d740ab..000000000000 --- a/clippy_utils/src/mir/possible_origin.rs +++ /dev/null @@ -1,59 +0,0 @@ -use super::transitive_relation::TransitiveRelation; -use crate::ty::is_copy; -use rustc_data_structures::fx::FxHashMap; -use rustc_index::bit_set::HybridBitSet; -use rustc_lint::LateContext; -use rustc_middle::mir; - -/// Collect possible borrowed for every `&mut` local. -/// For example, `_1 = &mut _2` generate _1: {_2,...} -/// Known Problems: not sure all borrowed are tracked -#[allow(clippy::module_name_repetitions)] -pub(super) struct PossibleOriginVisitor<'a, 'tcx> { - possible_origin: TransitiveRelation, - body: &'a mir::Body<'tcx>, -} - -impl<'a, 'tcx> PossibleOriginVisitor<'a, 'tcx> { - pub fn new(body: &'a mir::Body<'tcx>) -> Self { - Self { - possible_origin: TransitiveRelation::default(), - body, - } - } - - pub 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 mut borrowers = self.possible_origin.reachable_from(row, self.body.local_decls.len()); - borrowers.remove(mir::Local::from_usize(0)); - if !borrowers.is_empty() { - map.insert(row, borrowers); - } - } - map - } -} - -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) | - // _2: &mut _; - // _3 = move _2 - mir::Rvalue::Use(mir::Operand::Move(borrowed)) | - // _3 = move _2 as &mut _; - mir::Rvalue::Cast(_, mir::Operand::Move(borrowed), _) - => { - self.possible_origin.add(lhs, borrowed.local); - }, - _ => {}, - } - } -} diff --git a/clippy_utils/src/mir/transitive_relation.rs b/clippy_utils/src/mir/transitive_relation.rs deleted file mode 100644 index 7fe2960739fa..000000000000 --- a/clippy_utils/src/mir/transitive_relation.rs +++ /dev/null @@ -1,29 +0,0 @@ -use rustc_data_structures::fx::FxHashMap; -use rustc_index::bit_set::HybridBitSet; -use rustc_middle::mir; - -#[derive(Default)] -pub(super) struct TransitiveRelation { - relations: FxHashMap>, -} - -impl TransitiveRelation { - pub fn add(&mut self, a: mir::Local, b: mir::Local) { - self.relations.entry(a).or_default().push(b); - } - - pub fn reachable_from(&self, a: mir::Local, domain_size: usize) -> HybridBitSet { - let mut seen = HybridBitSet::new_empty(domain_size); - let mut stack = vec![a]; - while let Some(u) = stack.pop() { - if let Some(edges) = self.relations.get(&u) { - for &v in edges { - if seen.insert(v) { - stack.push(v); - } - } - } - } - seen - } -} diff --git a/tests/ui/redundant_clone.fixed b/tests/ui/redundant_clone.fixed index 5adabb94bee2..1a3c5a577bb7 100644 --- a/tests/ui/redundant_clone.fixed +++ b/tests/ui/redundant_clone.fixed @@ -237,10 +237,17 @@ fn hashmap_neg() { } fn false_negative_5707() { - fn foo(_x: &Alpha, _y: &mut Alpha) {} + use std::marker::PhantomData; - let x = Alpha; - let mut y = Alpha; + #[derive(Clone, Debug)] + struct Alpha<'a> { + _phantom: PhantomData<&'a ()>, + } + + fn foo<'a>(_x: &'a Alpha<'_>, _y: &mut Alpha<'a>) {} + + let x = Alpha { _phantom: PhantomData }; + let mut y = Alpha { _phantom: PhantomData }; 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 234d560b7827..59c0fa9d18b8 100644 --- a/tests/ui/redundant_clone.rs +++ b/tests/ui/redundant_clone.rs @@ -237,10 +237,17 @@ fn hashmap_neg() { } fn false_negative_5707() { - fn foo(_x: &Alpha, _y: &mut Alpha) {} + use std::marker::PhantomData; - let x = Alpha; - let mut y = Alpha; + #[derive(Clone, Debug)] + struct Alpha<'a> { + _phantom: PhantomData<&'a ()>, + } + + fn foo<'a>(_x: &'a Alpha<'_>, _y: &mut Alpha<'a>) {} + + let x = Alpha { _phantom: PhantomData }; + let mut y = Alpha { _phantom: PhantomData }; 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 38367aa6663c..a2e6482a98f0 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -180,13 +180,13 @@ LL | foo(&x.clone(), move || { | ^ error: redundant clone - --> $DIR/redundant_clone.rs:246:40 + --> $DIR/redundant_clone.rs:253: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 + --> $DIR/redundant_clone.rs:253:9 | LL | s = s.chars().filter(|&c| c != 'o').to_owned().collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 031f7df384115faa3d4d39531e0e1d6a13dfc1c4 Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Fri, 30 Jun 2023 19:19:19 -0400 Subject: [PATCH 6/6] Trying to implement a "borrowck" pass By overriding the `mir_borrowck` query --- Cargo.toml | 1 + clippy_lints/src/dereference.rs | 158 ++++++++++------- clippy_lints/src/lib.rs | 89 +++++++++- clippy_lints/src/redundant_clone.rs | 42 ++--- clippy_lints/src/utils/conf.rs | 2 +- clippy_utils/src/lib.rs | 3 - clippy_utils/src/mir/mod.rs | 207 +--------------------- clippy_utils/src/mir/possible_borrower.rs | 144 ++++----------- src/driver.rs | 69 +++++++- tests/ui/redundant_clone.stderr | 8 +- 10 files changed, 313 insertions(+), 410 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ca8bf9fac91a..137e41f52db1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ path = "src/driver.rs" [dependencies] clippy_lints = { path = "clippy_lints" } +clippy_utils = { path = "clippy_utils" } rustc_tools_util = "0.3.0" tempfile = { version = "3.2", optional = true } termize = "0.1" diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 5489bc515e5a..9caed4a8b1db 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,5 +1,6 @@ +use crate::{BorrowckContext, BorrowckLintPass}; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; -use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap}; +use clippy_utils::mir::{expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap}; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; use clippy_utils::sugg::has_enclosing_paren; @@ -8,22 +9,23 @@ use clippy_utils::{ fn_def_id, get_parent_expr, get_parent_expr_for_hir, is_lint_allowed, path_to_local, walk_to_expr_usage, }; +use rustc_ast as ast; use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX}; +use rustc_borrowck::consumers::BodyWithBorrowckFacts; use rustc_data_structures::fx::FxIndexMap; use rustc_data_structures::graph::iterate::{CycleDetector, TriColorDepthFirstSearch}; use rustc_errors::Applicability; +use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_hir::intravisit::{walk_ty, Visitor}; use rustc_hir::{ - self as hir, - def_id::{DefId, LocalDefId}, - BindingAnnotation, Body, BodyId, BorrowKind, Closure, Expr, ExprKind, FnRetTy, GenericArg, HirId, ImplItem, + self as hir, BindingAnnotation, Body, BorrowKind, Closure, Expr, ExprKind, FnRetTy, GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, TraitItemKind, TyKind, UnOp, }; use rustc_index::bit_set::BitSet; use rustc_infer::infer::TyCtxtInferExt; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::mir::{Rvalue, StatementKind}; +use rustc_lint::{late_lint_methods, LateContext, LateLintPass, LintPass}; +use rustc_middle::mir::{PlaceElem, Rvalue, StatementKind}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc_middle::ty::{ self, Binder, BoundVariableKind, Clause, EarlyBinder, FnSig, GenericArgKind, List, ParamEnv, ParamTy, @@ -34,6 +36,7 @@ use rustc_span::{symbol::sym, Span, Symbol}; use rustc_trait_selection::infer::InferCtxtExt as _; use rustc_trait_selection::traits::{query::evaluate_obligation::InferCtxtExt as _, Obligation, ObligationCause}; use std::collections::VecDeque; +use std::rc::Rc; declare_clippy_lint! { /// ### What it does @@ -148,7 +151,7 @@ declare_clippy_lint! { "dereferencing when the compiler would automatically dereference" } -impl_lint_pass!(Dereferencing<'_> => [ +impl_lint_pass!(Dereferencing<'_, '_> => [ EXPLICIT_DEREF_METHODS, NEEDLESS_BORROW, REF_BINDING_TO_REFERENCE, @@ -156,7 +159,7 @@ impl_lint_pass!(Dereferencing<'_> => [ ]); #[derive(Default)] -pub struct Dereferencing<'tcx> { +pub struct Dereferencing<'b, 'tcx> { state: Option<(State, StateData)>, // While parsing a `deref` method call in ufcs form, the path to the function is itself an @@ -167,7 +170,7 @@ pub struct Dereferencing<'tcx> { /// The body the first local was found in. Used to emit lints when the traversal of the body has /// been finished. Note we can't lint at the end of every body as they can be nested within each /// other. - current_body: Option, + current_body: Option<&'b Rc>>, /// The list of locals currently being checked by the lint. /// If the value is `None`, then the binding has been seen as a ref pattern, but is not linted. @@ -177,16 +180,15 @@ pub struct Dereferencing<'tcx> { /// e.g. `m!(x) | Foo::Bar(ref x)` ref_locals: FxIndexMap>, - /// Stack of (body owner, `PossibleBorrowerMap`) pairs. Used by - /// `needless_borrow_impl_arg_position` to determine when a borrowed expression can instead - /// be moved. - possible_borrowers: Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>, + /// Used by `needless_borrow_impl_arg_position` to determine when a borrowed expression can + /// instead be moved. + possible_borrowers: Option>, // `IntoIterator` for arrays requires Rust 1.53. msrv: Msrv, } -impl<'tcx> Dereferencing<'tcx> { +impl<'b, 'tcx> Dereferencing<'b, 'tcx> { #[must_use] pub fn new(msrv: Msrv) -> Self { Self { @@ -256,7 +258,7 @@ struct RefPat { hir_id: HirId, } -impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { +impl<'b, 'tcx> LateLintPass<'tcx> for Dereferencing<'b, 'tcx> { #[expect(clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // Skip path expressions from deref calls. e.g. `Deref::deref(e)` @@ -288,7 +290,13 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { match (self.state.take(), kind) { (None, kind) => { let expr_ty = typeck.expr_ty(expr); - let (position, adjustments) = walk_parents(cx, &mut self.possible_borrowers, expr, &self.msrv); + let (position, adjustments) = walk_parents( + cx, + self.current_body.as_ref().unwrap(), + &mut self.possible_borrowers, + expr, + &self.msrv, + ); match kind { RefOp::Deref => { let sub_ty = typeck.expr_ty(sub_expr); @@ -548,7 +556,6 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { then { let mut app = Applicability::MachineApplicable; let snip = snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut app).0; - self.current_body = self.current_body.or(cx.enclosing_body); self.ref_locals.insert( id, Some(RefPat { @@ -564,40 +571,66 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { } } - fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) { - if self.possible_borrowers.last().map_or(false, |&(local_def_id, _)| { - local_def_id == cx.tcx.hir().body_owner_def_id(body.id()) - }) { - self.possible_borrowers.pop(); - } + fn check_body_post(&mut self, cx: &LateContext<'tcx>, _body: &'tcx Body<'_>) { + self.possible_borrowers = None; - if Some(body.id()) == self.current_body { - for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) { - let replacements = pat.replacements; - let app = pat.app; - let lint = if pat.always_deref { - NEEDLESS_BORROW - } else { - REF_BINDING_TO_REFERENCE - }; - span_lint_hir_and_then( - cx, - lint, - pat.hir_id, - pat.spans, - "this pattern creates a reference to a reference", - |diag| { - diag.multipart_suggestion("try this", replacements, app); - }, - ); - } - self.current_body = None; + for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) { + let replacements = pat.replacements; + let app = pat.app; + let lint = if pat.always_deref { + NEEDLESS_BORROW + } else { + REF_BINDING_TO_REFERENCE + }; + span_lint_hir_and_then( + cx, + lint, + pat.hir_id, + pat.spans, + "this pattern creates a reference to a reference", + |diag| { + diag.multipart_suggestion("try this", replacements, app); + }, + ); } } extract_msrv_attr!(LateContext); } +#[allow(rustc::lint_pass_impl_without_macro)] +impl<'b, 'tcx> LintPass for &mut Dereferencing<'b, 'tcx> { + fn name(&self) -> &'static str { + panic!() + } +} + +macro_rules! impl_late_lint_pass { + ([], [$($(#[$attr:meta])* fn $f:ident($($param:ident: $arg:ty),*);)*]) => { + impl<'b, 'tcx> LateLintPass<'tcx> for &mut Dereferencing<'b, 'tcx> { + $(fn $f(&mut self, context: &LateContext<'tcx>, $($param: $arg),*) { + (*self).$f(context, $($param),*); + })* + } + }; +} + +late_lint_methods!(impl_late_lint_pass, []); + +impl<'b, 'tcx> BorrowckLintPass<'b, 'tcx> for Dereferencing<'b, 'tcx> { + fn check_body_with_facts( + &mut self, + cx: &BorrowckContext<'tcx>, + body_with_facts: &'b Rc>, + ) { + self.current_body = Some(body_with_facts); + + let this = cx.visit_hir_body(self); + + this.current_body = None; + } +} + fn try_parse_ref_op<'tcx>( tcx: TyCtxt<'tcx>, typeck: &'tcx TypeckResults<'_>, @@ -701,9 +734,10 @@ impl Position { /// is, and which adjustments will be applied to it. Note this will not consider auto-borrow /// locations as those follow different rules. #[expect(clippy::too_many_lines)] -fn walk_parents<'tcx>( +fn walk_parents<'b, 'tcx>( cx: &LateContext<'tcx>, - possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>, + body_with_facts: &'b Rc>, + possible_borrowers: &mut Option>, e: &'tcx Expr<'_>, msrv: &Msrv, ) -> (Position, &'tcx [Adjustment<'tcx>]) { @@ -841,6 +875,7 @@ fn walk_parents<'tcx>( { needless_borrow_impl_arg_position( cx, + body_with_facts, possible_borrowers, parent, i, @@ -912,6 +947,7 @@ fn walk_parents<'tcx>( { needless_borrow_impl_arg_position( cx, + body_with_facts, possible_borrowers, parent, i + 1, @@ -1108,9 +1144,10 @@ fn call_is_qualified(expr: &Expr<'_>) -> bool { // The "is copyable" condition is to avoid the case where removing the `&` means `e` would have to // be moved, but it cannot be. #[expect(clippy::too_many_arguments, clippy::too_many_lines)] -fn needless_borrow_impl_arg_position<'tcx>( +fn needless_borrow_impl_arg_position<'b, 'tcx>( cx: &LateContext<'tcx>, - possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>, + body_with_facts: &'b Rc>, + possible_borrowers: &mut Option>, parent: &Expr<'tcx>, arg_index: usize, param_ty: ParamTy, @@ -1188,7 +1225,7 @@ fn needless_borrow_impl_arg_position<'tcx>( if !is_copy(cx, referent_ty) && (referent_ty.has_significant_drop(cx.tcx, cx.param_env) - || !referent_used_exactly_once(cx, possible_borrowers, reference)) + || !referent_used_exactly_once(cx, body_with_facts, possible_borrowers, reference)) { return false; } @@ -1289,28 +1326,25 @@ fn is_mixed_projection_predicate<'tcx>( } } -fn referent_used_exactly_once<'tcx>( +fn referent_used_exactly_once<'b, 'tcx>( cx: &LateContext<'tcx>, - possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>, + body_with_facts: &'b Rc>, + possible_borrowers: &mut Option>, reference: &Expr<'tcx>, ) -> bool { - let mir = enclosing_mir(cx.tcx, reference.hir_id); - if let Some(local) = expr_local(cx.tcx, reference) + let mir = &body_with_facts.body; + if let Some(local) = expr_local(mir, reference) && let [location] = *local_assignments(mir, local).as_slice() && let Some(statement) = mir.basic_blocks[location.block].statements.get(location.statement_index) && let StatementKind::Assign(box (_, Rvalue::Ref(_, _, place))) = statement.kind - && !place.has_deref() + && !place.projection.contains(&PlaceElem::Deref) // Ensure not in a loop (https://github.com/rust-lang/rust-clippy/issues/9710) && TriColorDepthFirstSearch::new(&mir.basic_blocks).run_from(location.block, &mut CycleDetector).is_none() { - let body_owner_local_def_id = cx.tcx.hir().enclosing_body_owner(reference.hir_id); - if possible_borrowers - .last() - .map_or(true, |&(local_def_id, _)| local_def_id != body_owner_local_def_id) - { - possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, body_owner_local_def_id))); + if possible_borrowers.is_none() { + *possible_borrowers = Some(PossibleBorrowerMap::new(cx.tcx, body_with_facts)); } - let possible_borrower = &mut possible_borrowers.last_mut().unwrap().1; + let possible_borrower = possible_borrowers.as_mut().unwrap(); // 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. @@ -1625,7 +1659,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data } } -impl<'tcx> Dereferencing<'tcx> { +impl<'b, 'tcx> Dereferencing<'b, 'tcx> { fn check_local_usage(&mut self, cx: &LateContext<'tcx>, e: &Expr<'tcx>, local: HirId) { if let Some(outer_pat) = self.ref_locals.get_mut(&local) { if let Some(pat) = outer_pat { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 243d1fd8606a..6670e0fd285e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -24,6 +24,7 @@ extern crate rustc_arena; extern crate rustc_ast; extern crate rustc_ast_pretty; +extern crate rustc_borrowck; extern crate rustc_data_structures; extern crate rustc_driver; extern crate rustc_errors; @@ -48,12 +49,17 @@ extern crate clippy_utils; #[macro_use] extern crate declare_clippy_lint; -use std::io; -use std::path::PathBuf; +use std::cell::RefCell; +use std::rc::Rc; +use std::sync::OnceLock; use clippy_utils::msrvs::Msrv; +use rustc_borrowck::consumers::BodyWithBorrowckFacts; use rustc_data_structures::fx::FxHashSet; -use rustc_lint::{Lint, LintId}; +use rustc_hir::intravisit::Visitor; +use rustc_hir::BodyId; +use rustc_lint::{LateContext, LateContextAndPass, LateLintPass, Lint, LintId, LintPass}; +use rustc_middle::ty::TyCtxt; use rustc_session::Session; #[cfg(feature = "internal")] @@ -340,9 +346,9 @@ mod zero_div_zero; mod zero_sized_map_values; // end lints modules, do not remove this comment, it’s used in `update_lints` -pub use crate::utils::conf::{lookup_conf_file, Conf}; +pub use crate::utils::conf::Conf; use crate::utils::{ - conf::{metadata::get_configuration_metadata, TryConf}, + conf::{metadata::get_configuration_metadata, lookup_conf_file, TryConf}, FindAll, }; @@ -363,7 +369,16 @@ pub fn register_pre_expansion_lints(store: &mut rustc_lint::LintStore, sess: &Se } #[doc(hidden)] -pub fn read_conf(sess: &Session, path: &io::Result<(Option, Vec)>) -> Conf { +pub fn read_conf(sess: &Session) -> &'static Conf { + static CONF: OnceLock = OnceLock::new(); + + CONF.get_or_init(|| read_conf_inner(sess)) +} + +fn read_conf_inner(sess: &Session) -> Conf { + let conf_path = lookup_conf_file(); + let path = &conf_path; + if let Ok((_, warnings)) = path { for warning in warnings { sess.warn(warning.clone()); @@ -781,7 +796,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(|_| Box::new(non_copy_const::NonCopyConst)); store.register_late_pass(|_| Box::new(ptr_offset_with_cast::PtrOffsetWithCast)); - store.register_late_pass(|_| Box::new(redundant_clone::RedundantClone)); store.register_late_pass(|_| Box::new(slow_vector_initialization::SlowVectorInit)); store.register_late_pass(move |_| Box::new(unnecessary_wraps::UnnecessaryWraps::new(avoid_breaking_exported_api))); store.register_late_pass(|_| Box::new(assertions_on_constants::AssertionsOnConstants)); @@ -866,7 +880,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move |_| Box::new(wildcard_imports::WildcardImports::new(warn_on_all_wildcard_imports))); store.register_late_pass(|_| Box::::default()); store.register_late_pass(|_| Box::new(unnamed_address::UnnamedAddress)); - store.register_late_pass(move |_| Box::new(dereference::Dereferencing::new(msrv()))); store.register_late_pass(|_| Box::new(option_if_let_else::OptionIfLetElse)); store.register_late_pass(|_| Box::new(future_not_send::FutureNotSend)); let future_size_threshold = conf.future_size_threshold; @@ -1048,6 +1061,66 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: // add lints here, do not remove this comment, it's used in `new_lint` } +pub struct BorrowckContext<'tcx> { + cell: RefCell>>>, +} + +impl<'tcx> BorrowckContext<'tcx> { + pub fn new(tcx: TyCtxt<'tcx>, body_id: BodyId) -> Self { + let cx = LateContext::new(tcx, Some(body_id)); + + Self { + cell: RefCell::new(Some(Rc::new(cx))), + } + } + + /// Returns a reference to the internal `LateContext` as an `Rc`. The `Rc` must be dropped + /// before `visit_hir_body` is called (if ever). + fn as_late_context(&self) -> Rc> { + self.cell.borrow().as_ref().unwrap().clone() + } + + /// Visits the current HIR body with `pass`. Panics if references to the internal `LateContext` + /// returned by `as_late_context` have not yet been dropped. + fn visit_hir_body>(&self, pass: T) -> T { + let rc = self.cell.take().unwrap(); + + let context = Rc::into_inner(rc).unwrap(); + + let body = context.tcx.hir().body(context.enclosing_body.unwrap()); + + let mut context_and_pass = LateContextAndPass { context, pass }; + + context_and_pass.visit_body(body); + + let LateContextAndPass { context, pass } = context_and_pass; + + self.cell.replace(Some(Rc::new(context))); + + pass + } +} + +pub trait BorrowckLintPass<'b, 'tcx>: LintPass { + fn check_body_with_facts( + &mut self, + cx: &BorrowckContext<'tcx>, + body_with_facts: &'b Rc>, + ); +} + +pub fn borrowck_lint_passes<'b, 'tcx>(msrv: &Msrv) -> Vec + 'b>> +where + 'tcx: 'b, +{ + let msrv = msrv.clone(); + + vec![ + Box::new(redundant_clone::RedundantClone), + Box::new(dereference::Dereferencing::new(msrv.clone())), + ] +} + #[rustfmt::skip] fn register_removed_non_tool_lints(store: &mut rustc_lint::LintStore) { store.register_removed( diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index b8bc1ee0ff6f..df394c32b6de 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -1,19 +1,20 @@ +use crate::{BorrowckContext, BorrowckLintPass}; use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then}; use clippy_utils::mir::{visit_local_usage, LocalUsage, PossibleBorrowerMap}; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{has_drop, is_copy, is_type_diagnostic_item, is_type_lang_item, walk_ptrs_ty_depth}; use clippy_utils::{fn_has_unsatisfiable_preds, match_def_path, paths}; use if_chain::if_chain; +use rustc_borrowck::consumers::BodyWithBorrowckFacts; use rustc_errors::Applicability; -use rustc_hir::intravisit::FnKind; -use rustc_hir::{def_id, Body, FnDecl, LangItem}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_hir::{def_id, LangItem}; +use rustc_lint::LateContext; use rustc_middle::mir; use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::def_id::LocalDefId; -use rustc_span::source_map::{BytePos, Span}; +use rustc_span::source_map::BytePos; use rustc_span::sym; +use std::rc::Rc; macro_rules! unwrap_or_continue { ($x:expr) => { @@ -63,25 +64,26 @@ declare_clippy_lint! { declare_lint_pass!(RedundantClone => [REDUNDANT_CLONE]); -impl<'tcx> LateLintPass<'tcx> for RedundantClone { +impl<'b, 'tcx> BorrowckLintPass<'b, 'tcx> for RedundantClone { #[expect(clippy::too_many_lines)] - fn check_fn( + fn check_body_with_facts( &mut self, - cx: &LateContext<'tcx>, - _: FnKind<'tcx>, - _: &'tcx FnDecl<'_>, - _: &'tcx Body<'_>, - _: Span, - def_id: LocalDefId, + cx: &BorrowckContext<'tcx>, + body_with_facts: &'b Rc>, ) { + let rc = cx.as_late_context(); + let cx: &LateContext<'tcx> = &rc; + + let def_id = body_with_facts.body.source.def_id(); + // Building MIR for `fn`s with unsatisfiable preds results in ICE. - if fn_has_unsatisfiable_preds(cx, def_id.to_def_id()) { + if fn_has_unsatisfiable_preds(cx, def_id) { return; } - let mir = cx.tcx.optimized_mir(def_id.to_def_id()); + let mir = &body_with_facts.body; - let mut possible_borrower = PossibleBorrowerMap::new(cx, def_id); + let mut possible_borrower = PossibleBorrowerMap::new(cx.tcx, body_with_facts); for (bb, bbdata) in mir.basic_blocks.iter_enumerated() { let terminator = bbdata.terminator(); @@ -257,10 +259,10 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { } /// If `kind` is `y = func(x: &T)` where `T: !Copy`, returns `(DefId of func, x, T, y)`. -fn is_call_with_ref_arg<'tcx>( +fn is_call_with_ref_arg<'b, 'tcx>( cx: &LateContext<'tcx>, - mir: &'tcx mir::Body<'tcx>, - kind: &'tcx mir::TerminatorKind<'tcx>, + mir: &'b mir::Body<'tcx>, + kind: &'b mir::TerminatorKind<'tcx>, ) -> Option<(def_id::DefId, mir::Local, Ty<'tcx>, mir::Local)> { if_chain! { if let mir::TerminatorKind::Call { func, args, destination, .. } = kind; @@ -268,7 +270,7 @@ fn is_call_with_ref_arg<'tcx>( if let mir::Operand::Move(mir::Place { local, .. }) = &args[0]; if let ty::FnDef(def_id, _) = *func.ty(mir, cx.tcx).kind(); if let (inner_ty, 1) = walk_ptrs_ty_depth(args[0].ty(mir, cx.tcx)); - if !is_copy(cx, inner_ty); + if !is_copy(cx, cx.tcx.erase_regions(inner_ty)); then { Some((def_id, *local, inner_ty, destination.as_local()?)) } else { diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index f6c7c1fa065a..4b8d1bdd055f 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -545,7 +545,7 @@ define_Conf! { /// # Errors /// /// Returns any unexpected filesystem error encountered when searching for the config file -pub fn lookup_conf_file() -> io::Result<(Option, Vec)> { +pub(crate) fn lookup_conf_file() -> io::Result<(Option, Vec)> { /// Possible filename to search for. const CONFIG_FILE_NAMES: [&str; 2] = [".clippy.toml", "clippy.toml"]; diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index e133315a7d1a..593e4e45432e 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -34,10 +34,7 @@ extern crate rustc_infer; extern crate rustc_lexer; extern crate rustc_lint; extern crate rustc_middle; -extern crate rustc_mir_build; extern crate rustc_mir_dataflow; -extern crate rustc_mir_transform; -extern crate rustc_parse_format; extern crate rustc_session; extern crate rustc_span; extern crate rustc_target; diff --git a/clippy_utils/src/mir/mod.rs b/clippy_utils/src/mir/mod.rs index 0c54f9957616..bf504e02148d 100644 --- a/clippy_utils/src/mir/mod.rs +++ b/clippy_utils/src/mir/mod.rs @@ -1,12 +1,8 @@ -use rustc_hir::{Expr, HirId}; -use rustc_middle::mir::visit::{ - MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, TyContext, Visitor, -}; +use rustc_hir::Expr; +use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor}; use rustc_middle::mir::{ - traversal, Body, InlineAsmOperand, Local, LocalDecl, Location, Place, Statement, StatementKind, Terminator, - TerminatorKind, START_BLOCK, + traversal, Body, InlineAsmOperand, Local, Location, Place, StatementKind, TerminatorKind, START_BLOCK, }; -use rustc_middle::ty::{Region, Ty, TyCtxt}; mod possible_borrower; pub use possible_borrower::PossibleBorrowerMap; @@ -98,17 +94,9 @@ pub fn used_exactly_once(mir: &rustc_middle::mir::Body<'_>, local: rustc_middle: }) } -/// Returns the `mir::Body` containing the node associated with `hir_id`. -#[allow(clippy::module_name_repetitions)] -pub fn enclosing_mir(tcx: TyCtxt<'_>, hir_id: HirId) -> &Body<'_> { - let body_owner_local_def_id = tcx.hir().enclosing_body_owner(hir_id); - tcx.optimized_mir(body_owner_local_def_id.to_def_id()) -} - /// Tries to determine the `Local` corresponding to `expr`, if any. /// This function is expensive and should be used sparingly. -pub fn expr_local(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> Option { - let mir = enclosing_mir(tcx, expr.hir_id); +pub fn expr_local(mir: &Body<'_>, expr: &Expr<'_>) -> Option { mir.local_decls.iter_enumerated().find_map(|(local, local_decl)| { if local_decl.source_info.span == expr.span { Some(local) @@ -118,193 +106,6 @@ pub fn expr_local(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> Option { }) } -/// Tries to find the local in `to_mir` corresponding to `local` in `from_mir`. -pub fn translate_local<'tcx>( - tcx: TyCtxt<'tcx>, - from_mir: &Body<'tcx>, - to_mir: &Body<'tcx>, - local: Local, -) -> Option { - let equiv_decl = |lhs: &LocalDecl<'tcx>, rhs: &LocalDecl<'tcx>| { - lhs.mutability == rhs.mutability - && tcx.erase_regions_ty(lhs.ty) == tcx.erase_regions_ty(rhs.ty) - && lhs.source_info.span == rhs.source_info.span - }; - - let from_decl = &from_mir.local_decls[local]; - - // Fast path - if to_mir - .local_decls - .get(local) - .map_or(false, |to_decl| equiv_decl(from_decl, to_decl)) - { - return Some(local); - } - - // Slow path - to_mir - .local_decls - .iter() - .position(|to_decl| equiv_decl(from_decl, to_decl)) - .map(Into::into) -} - -/// Tries to find the location in `to_mir` corresponding to `location` in `from_mir`. -pub fn translate_location<'tcx>( - tcx: TyCtxt<'tcx>, - from_mir: &Body<'tcx>, - to_mir: &Body<'tcx>, - location: Location, -) -> Option { - let normalized_lhs = from_mir - .stmt_at(location) - .map_left(|statement| normalize_statement(tcx, statement)) - .map_right(|terminator| normalize_terminator(tcx, terminator)); - - for (block, block_data) in to_mir.basic_blocks.iter_enumerated() { - if let Some(location) = normalized_lhs.as_ref().either( - |normalized_lhs| { - (0..block_data.statements.len()).find_map(|statement_index| { - let rhs = &block_data.statements[statement_index]; - if normalized_lhs.source_info.span == rhs.source_info.span - && normalized_lhs.kind == normalize_statement(tcx, rhs).kind - { - Some(Location { block, statement_index }) - } else { - None - } - }) - }, - |normalized_lhs| { - if block_data.terminator.as_ref().map_or(false, |rhs| { - normalized_lhs.source_info.span == rhs.source_info.span - && normalized_lhs.kind == normalize_terminator(tcx, rhs).kind - }) { - Some(Location { - block, - statement_index: block_data.statements.len(), - }) - } else { - None - } - }, - ) { - return Some(location); - } - } - - None -} - -fn normalize_statement<'tcx>(tcx: TyCtxt<'tcx>, statement: &Statement<'tcx>) -> Statement<'tcx> { - let mut statement = statement.clone(); - Normalizer { tcx }.visit_statement(&mut statement, Location::START); - statement -} - -fn normalize_terminator<'tcx>(tcx: TyCtxt<'tcx>, terminator: &Terminator<'tcx>) -> Terminator<'tcx> { - let mut terminator = terminator.clone(); - Normalizer { tcx }.visit_terminator(&mut terminator, Location::START); - match terminator.kind { - // No basic blocks - TerminatorKind::Abort - | TerminatorKind::GeneratorDrop - | TerminatorKind::Resume - | TerminatorKind::Return - | TerminatorKind::Unreachable => {}, - - // One basic block - TerminatorKind::Goto { ref mut target } => { - *target = Location::START.block; - }, - - // Two basic blocks - TerminatorKind::FalseEdge { - ref mut real_target, - ref mut imaginary_target, - } => { - *real_target = Location::START.block; - *imaginary_target = Location::START.block; - }, - - // One optional and one non-optional basic block - TerminatorKind::Assert { - ref mut target, - cleanup: ref mut unwind, - .. - } - | TerminatorKind::Drop { - ref mut target, - ref mut unwind, - .. - } - | TerminatorKind::DropAndReplace { - ref mut target, - ref mut unwind, - .. - } - | TerminatorKind::FalseUnwind { - real_target: ref mut target, - ref mut unwind, - .. - } - | TerminatorKind::Yield { - resume: ref mut target, - drop: ref mut unwind, - .. - } => { - *target = Location::START.block; - *unwind = None; - }, - - // Two optional basic blocks - TerminatorKind::Call { - ref mut target, - ref mut cleanup, - .. - } - | TerminatorKind::InlineAsm { - destination: ref mut target, - ref mut cleanup, - .. - } => { - *target = None; - *cleanup = None; - }, - - // Arbitrarily many basic blocks - TerminatorKind::SwitchInt { ref mut targets, .. } => { - for (_, ref mut target) in targets.iter() { - *target = Location::START.block; - } - }, - } - terminator -} - -struct Normalizer<'tcx> { - tcx: TyCtxt<'tcx>, -} - -impl<'tcx> MutVisitor<'tcx> for Normalizer<'tcx> { - fn tcx<'a>(&'a self) -> TyCtxt<'tcx> { - self.tcx - } - - fn visit_local(&mut self, local: &mut Local, _context: PlaceContext, _location: Location) { - *local = Local::from_u32(0); - } - - fn visit_region(&mut self, region: &mut Region<'tcx>, _: Location) { - *region = self.tcx.lifetimes.re_erased; - } - - fn visit_ty(&mut self, ty: &mut Ty<'tcx>, _: TyContext) { - *ty = self.tcx.erase_regions_ty(*ty); - } -} - /// Returns a vector of `mir::Location` where `local` is assigned. pub fn local_assignments(mir: &Body<'_>, local: Local) -> Vec { let mut locations = Vec::new(); diff --git a/clippy_utils/src/mir/possible_borrower.rs b/clippy_utils/src/mir/possible_borrower.rs index 6c0b8cf6d7d8..fc40e8cdf9b4 100644 --- a/clippy_utils/src/mir/possible_borrower.rs +++ b/clippy_utils/src/mir/possible_borrower.rs @@ -1,15 +1,10 @@ -use super::{translate_local, translate_location}; use crate::ty::is_copy; -use rustc_borrowck::{borrow_set::BorrowSet, nll, region_infer::RegionInferenceContext, NllCtxt}; -use rustc_hir::def_id::LocalDefId; -use rustc_index::{bit_set::BitSet, vec::IndexVec}; -use rustc_infer::infer::{DefiningAnchor, TyCtxtInferExt}; +use rustc_borrowck::consumers::BodyWithBorrowckFacts; +use rustc_index::bit_set::BitSet; use rustc_lint::LateContext; use rustc_middle::mir; -use rustc_middle::ty::{self, visit::TypeVisitor, RegionKind, RegionVid, Ty, TyCtxt, WithOptConstParam}; -use rustc_mir_build::{build::mir_build_fn_or_const, thir::cx::thir_build}; +use rustc_middle::ty::{self, visit::TypeVisitor, RegionKind, RegionVid, Ty, TyCtxt}; use rustc_mir_dataflow::{impls::MaybeStorageLive, Analysis, ResultsCursor}; -use rustc_mir_transform::{mir_compute_const_qualifs, mir_promote, mir_ready_for_const_eval}; use std::borrow::Cow; use std::ops::ControlFlow; use std::rc::Rc; @@ -17,71 +12,26 @@ use std::rc::Rc; /// Result of `PossibleBorrowerAnalysis`. #[allow(clippy::module_name_repetitions)] pub struct PossibleBorrowerMap<'b, 'tcx> { - tcx: TyCtxt<'tcx>, - local_def_id: LocalDefId, - regioncx: Rc>, - body: Rc>, - borrow_set: Rc>, + body_with_facts: Rc>, maybe_live: ResultsCursor<'b, 'tcx, MaybeStorageLive<'b>>, } impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { - pub fn new(cx: &LateContext<'tcx>, local_def_id: LocalDefId) -> Self { - let tcx = cx.tcx; - - let infcx = tcx - .infer_ctxt() - .with_opaque_type_inference(DefiningAnchor::Bind(local_def_id)) - .build(); - - let (input_body, input_promoted) = Self::mir_build(tcx, local_def_id); - - let mut nll_ctxt = NllCtxt::new(&infcx, &input_body, &input_promoted); - - let nll::NllOutput { regioncx, .. } = nll_ctxt.compute_regions(false); - - let NllCtxt { body, borrow_set, .. } = nll_ctxt; - - let regioncx = Rc::new(regioncx); - let body = Rc::new(body); - let borrow_set = Rc::new(borrow_set); + pub fn new(tcx: TyCtxt<'tcx>, body_with_facts: &'b Rc>) -> Self { + let body = &body_with_facts.body; let maybe_live = MaybeStorageLive::new(Cow::Owned(BitSet::new_empty(body.local_decls.len()))) - .into_engine(tcx, &body) + .into_engine(tcx, body) .pass_name("possible_borrower") .iterate_to_fixpoint() - .into_results_cursor(body.clone()); + .into_results_cursor(body); PossibleBorrowerMap { - tcx, - local_def_id, - regioncx, - body, - borrow_set, + body_with_facts: body_with_facts.clone(), maybe_live, } } - fn mir_build( - tcx: TyCtxt<'tcx>, - local_def_id: LocalDefId, - ) -> (mir::Body<'tcx>, IndexVec>) { - let def = WithOptConstParam { - did: local_def_id, - const_param_did: None, - }; - - let (thir, expr) = thir_build(tcx, def).expect("`thir_build` should succeed"); - - let body_init = mir_build_fn_or_const(tcx, def, thir, expr); - - let body_const = mir_ready_for_const_eval(tcx, body_init); - - let const_qualifs = mir_compute_const_qualifs(tcx, def, || &body_const); - - mir_promote(tcx, const_qualifs, body_const) - } - /// Returns true if the set of borrowers of `borrowed` living at `at` includes no more than /// `borrowers`. /// Notes: @@ -99,30 +49,11 @@ impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { borrowed: mir::Local, at: mir::Location, ) -> bool { - let mir = cx.tcx.optimized_mir(self.local_def_id.to_def_id()); - if is_copy(cx, mir.local_decls[borrowed].ty) { + let mir = &self.body_with_facts.body; + if is_copy(cx, cx.tcx.erase_regions(mir.local_decls[borrowed].ty)) { return true; } - let Some(borrowers) = borrowers - .iter() - .map(|&borrower| translate_local(cx.tcx, mir, &self.body, borrower)) - .collect::>>() - else { - debug_assert!(false, "can't find {borrowers:?}"); - return false; - }; - - let Some(borrowed) = translate_local(cx.tcx, mir, &self.body, borrowed) else { - debug_assert!(false, "can't find {borrowed:?}"); - return false; - }; - - let Some(at) = translate_location(cx.tcx, mir, &self.body, at) else { - debug_assert!(false, "can't find {at:?}: {:?}", mir.stmt_at(at)); - return false; - }; - self.maybe_live.seek_before_primary_effect(at); let maybe_live = self.maybe_live.get(); @@ -138,28 +69,38 @@ impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { // Note that the `any` closure has no side effects. So the result is the same regardless of the // order in which `index`es are visited. #[allow(rustc::potential_query_instability)] - !self.borrow_set.local_map[&borrowed].iter().any(|index| { - let root_vid = self.borrow_set.location_map[index.as_usize()].region; - - maybe_live.iter().any(|local| { - let local_regions = collect_regions(self.body.local_decls[local].ty); - - local_regions.iter().any(|&local_vid| { - if !self.regioncx.eval_outlives(root_vid, local_vid) { - return false; - } - - !borrowers - .iter() - .filter_map(|&borrower| self.borrower_vid(borrower)) - .any(|borrower_vid| self.regioncx.eval_outlives(borrower_vid, local_vid)) + !self.body_with_facts.borrow_set.local_map[&borrowed] + .iter() + .any(|index| { + let root_vid = self.body_with_facts.borrow_set.location_map[index.as_usize()].region; + + maybe_live.iter().any(|local| { + let local_regions = collect_regions(self.body_with_facts.body.local_decls[local].ty); + + local_regions.iter().any(|&local_vid| { + if !self + .body_with_facts + .region_inference_context + .eval_outlives(root_vid, local_vid) + { + return false; + } + + !borrowers + .iter() + .filter_map(|&borrower| self.borrower_vid(borrower)) + .any(|borrower_vid| { + self.body_with_facts + .region_inference_context + .eval_outlives(borrower_vid, local_vid) + }) + }) }) }) - }) } fn borrower_vid(&self, borrower: mir::Local) -> Option { - if let ty::Ref(region, _, _) = self.body.local_decls[borrower].ty.kind() + if let ty::Ref(region, _, _) = self.body_with_facts.body.local_decls[borrower].ty.kind() && let ty::RegionKind::ReVar(borrower_vid) = region.kind() { Some(borrower_vid) @@ -169,13 +110,6 @@ impl<'b, 'tcx> PossibleBorrowerMap<'b, 'tcx> { } pub fn local_is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool { - let mir = self.tcx.optimized_mir(self.local_def_id.to_def_id()); - - let Some(at) = translate_location(self.tcx, mir, &self.body, at) else { - debug_assert!(false, "can't find {at:?}: {:?}", mir.stmt_at(at)); - return false; - }; - self.maybe_live.seek_after_primary_effect(at); self.maybe_live.contains(local) } @@ -189,7 +123,7 @@ fn collect_regions(ty: Ty<'_>) -> Vec { struct RegionCollector(Vec); -impl<'tcx> TypeVisitor<'tcx> for RegionCollector { +impl<'tcx> TypeVisitor> for RegionCollector { fn visit_region(&mut self, region: ty::Region<'tcx>) -> ControlFlow { if let RegionKind::ReVar(vid) = region.kind() { self.0.push(vid); diff --git a/src/driver.rs b/src/driver.rs index 3c5b6e12b968..b5a588e11635 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -10,12 +10,17 @@ // FIXME: switch to something more ergonomic here, once available. // (Currently there is no way to opt into sysroot crates without `extern crate`.) +extern crate rustc_borrowck; extern crate rustc_driver; extern crate rustc_interface; +extern crate rustc_lint; extern crate rustc_session; extern crate rustc_span; +use clippy_lints::{BorrowckContext, BorrowckLintPass}; +use rustc_borrowck::consumers::{do_mir_borrowck, BodyWithBorrowckFacts, ConsumerOptions}; use rustc_interface::interface; +use rustc_lint::LintPass; use rustc_session::parse::ParseSess; use rustc_span::symbol::Symbol; @@ -23,6 +28,7 @@ use std::env; use std::ops::Deref; use std::path::Path; use std::process::exit; +use std::rc::Rc; /// If a command-line option matches `find_arg`, then apply the predicate `pred` on its value. If /// true, then return it. The parameter is assumed to be either `--arg=value` or `--arg value`. @@ -118,17 +124,44 @@ struct ClippyCallbacks { clippy_args_var: Option, } +type BorrowckLintPassObject<'b, 'tcx> = Box + 'b>; + +/// Like [`rustc_lint::late::RuntimeCombinedLateLintPass`], but with [`BorrowckPass`]es instead of +/// [`rustc_lint::LateLintPass`]es, and the passes are owned instead of borrowed. +pub struct RuntimeCombinedBorrowckLintPass<'b, 'tcx> { + passes: Vec>, +} + +#[allow(rustc::lint_pass_impl_without_macro)] +impl LintPass for RuntimeCombinedBorrowckLintPass<'_, '_> { + fn name(&self) -> &'static str { + panic!() + } +} + +impl<'b, 'tcx> BorrowckLintPass<'b, 'tcx> for RuntimeCombinedBorrowckLintPass<'b, 'tcx> { + fn check_body_with_facts( + &mut self, + cx: &BorrowckContext<'tcx>, + body_with_facts: &'b Rc>, + ) { + for pass in &mut self.passes { + pass.check_body_with_facts(cx, body_with_facts); + } + } +} + impl rustc_driver::Callbacks for ClippyCallbacks { // JUSTIFICATION: necessary in clippy driver to set `mir_opt_level` #[allow(rustc::bad_opt_access)] fn config(&mut self, config: &mut interface::Config) { - let conf_path = clippy_lints::lookup_conf_file(); let previous = config.register_lints.take(); let clippy_args_var = self.clippy_args_var.take(); config.parse_sess_created = Some(Box::new(move |parse_sess| { track_clippy_args(parse_sess, &clippy_args_var); track_files(parse_sess); })); + config.register_lints = Some(Box::new(move |sess, lint_store| { // technically we're ~guaranteed that this is none but might as well call anything that // is there already. Certainly it can't hurt. @@ -136,12 +169,40 @@ impl rustc_driver::Callbacks for ClippyCallbacks { (previous)(sess, lint_store); } - let conf = clippy_lints::read_conf(sess, &conf_path); - clippy_lints::register_plugins(lint_store, sess, &conf); - clippy_lints::register_pre_expansion_lints(lint_store, sess, &conf); + let conf = clippy_lints::read_conf(sess); + clippy_lints::register_plugins(lint_store, sess, conf); + clippy_lints::register_pre_expansion_lints(lint_store, sess, conf); clippy_lints::register_renamed(lint_store); })); + config.override_queries = Some(|_sess, providers, _external_providers| { + providers.mir_borrowck = |tcx, local_def_id| { + let (result, body_with_facts) = + do_mir_borrowck(tcx, local_def_id, ConsumerOptions::RegionInferenceContext); + + let body_with_facts = Rc::new(*body_with_facts.unwrap()); + + let def_id = body_with_facts.body.source.def_id(); + let body_id = tcx.hir().body_owned_by(def_id.as_local().unwrap()); + + let cx = BorrowckContext::new(tcx, body_id); + + // FIXME: How to compute this just once? + let mut pass = { + let conf = clippy_lints::read_conf(tcx.sess); + let msrv = clippy_utils::msrvs::Msrv::read(&conf.msrv, tcx.sess); + + let passes = clippy_lints::borrowck_lint_passes(msrv); + + RuntimeCombinedBorrowckLintPass { passes } + }; + + pass.check_body_with_facts(&cx, &body_with_facts); + + tcx.arena.alloc(result) + }; + }); + // FIXME: #4825; This is required, because Clippy lints that are based on MIR have to be // run on the unoptimized MIR. On the other hand this results in some false negatives. If // MIR passes can be enabled / disabled separately, we should figure out, what passes to diff --git a/tests/ui/redundant_clone.stderr b/tests/ui/redundant_clone.stderr index a2e6482a98f0..fd4cb5ccc410 100644 --- a/tests/ui/redundant_clone.stderr +++ b/tests/ui/redundant_clone.stderr @@ -173,20 +173,20 @@ error: redundant clone LL | foo(&x.clone(), move || { | ^^^^^^^^ help: remove this | -note: this value is dropped without further use +note: cloned value is neither consumed nor mutated --> $DIR/redundant_clone.rs:210:10 | LL | foo(&x.clone(), move || { - | ^ + | ^^^^^^^^^ error: redundant clone - --> $DIR/redundant_clone.rs:253:40 + --> $DIR/redundant_clone.rs:259: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:253:9 + --> $DIR/redundant_clone.rs:259:9 | LL | s = s.chars().filter(|&c| c != 'o').to_owned().collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^