diff --git a/clippy_lints/src/borrow_pats/mod.rs b/clippy_lints/src/borrow_pats/mod.rs index 151b94845612..66497d458cf4 100644 --- a/clippy_lints/src/borrow_pats/mod.rs +++ b/clippy_lints/src/borrow_pats/mod.rs @@ -44,6 +44,7 @@ mod owned; mod ret; mod info; +mod prelude; mod rustc_extention; mod util; pub use info::*; diff --git a/clippy_lints/src/borrow_pats/owned.rs b/clippy_lints/src/borrow_pats/owned.rs index 63744f9fb7b5..f320c7d3ac71 100644 --- a/clippy_lints/src/borrow_pats/owned.rs +++ b/clippy_lints/src/borrow_pats/owned.rs @@ -1,21 +1,12 @@ -use std::collections::{BTreeMap, BTreeSet}; - -use crate::borrow_pats::PlaceMagic; +use super::prelude::*; use super::ret::ReturnPat; -use super::{visit_body_in_order, AnalysisInfo, LocalKind, PatternEnum, PatternStorage, Validity}; - -use hir::Mutability; -use itertools::Itertools; -use mid::mir::visit::{MutatingUseContext, Visitor}; -use mid::mir::{BorrowKind, Operand, StatementKind, VarDebugInfo, VarDebugInfoContents, START_BLOCK}; -use mid::ty::TypeVisitableExt; -use rustc_data_structures::fx::{FxHashMap, FxHashSet}; -use rustc_index::IndexVec; +use super::visit_body_in_order; + use rustc_middle::mir; -use rustc_middle::mir::{BasicBlock, FakeReadCause, Local, Place, Rvalue}; -use rustc_span::{sym, Symbol}; -use {rustc_borrowck as borrowck, rustc_hir as hir, rustc_middle as mid}; + +mod state; +use state::*; #[derive(Debug)] pub struct OwnedAnalysis<'a, 'tcx> { @@ -26,6 +17,7 @@ pub struct OwnedAnalysis<'a, 'tcx> { /// The kind can diviate from the kind in info, in cases where we determine /// that this is most likely a deconstructed argument. local_kind: &'a LocalKind, + /// This should be a `BTreeSet` to have it ordered and consistent. pats: BTreeSet, /// Counts how many times a value was used. This starts at 1 for arguments otherwise 0. use_count: u32, @@ -88,155 +80,7 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { } fn add_borrow(&mut self, bb: BasicBlock, borrow: Place<'tcx>, broker: Place<'tcx>, kind: BorrowKind) { - self.update_bros(bb, broker, kind.mutability()); - - if matches!(kind, BorrowKind::Shared) - && let Some((_loc, phase_place)) = self.states[bb].phase_borrow - && self.info.places_conflict(phase_place, broker) - { - self.pats.insert(OwnedPat::TwoPhasedBorrow); - } - - // So: It turns out that MIR is an inconsisten hot mess. Two-Phase-Borrows are apparently - // allowed to violate rust borrow semantics... - // - // Simple example: `x.push(x.len())` - if matches!(self.info.locals[&borrow.local].kind, LocalKind::AnonVar) { - assert!(borrow.just_local()); - if kind.allows_two_phase_borrow() { - let old = self.states[bb].phase_borrow.replace((borrow.local, broker)); - assert_eq!(old, None); - } else { - self.states[bb] - .temp_bros - .insert(borrow.local, (broker, kind.mutability())); - } - } else { - todo!("Named Local: {borrow:#?} = {broker:#?} (at {bb:#?})\n{self:#?}"); - } - } - - fn update_bros(&mut self, bb: BasicBlock, broker: Place<'tcx>, muta: Mutability) { - let state = &mut self.states[bb]; - - if broker.just_local() && matches!(muta, Mutability::Mut) { - // If the entire thing is borrowed mut, every reference get's cleared - state.temp_bros.clear(); - } else { - // I switch on muta before the `retain`, to make the `retain`` specialized and therefore faster. - match muta { - // Not mutable aka aliasable - Mutability::Not => state.temp_bros.retain(|_key, (broed_place, muta)| { - !(matches!(muta, Mutability::Mut) && self.info.places_conflict(*broed_place, broker)) - }), - Mutability::Mut => state - .temp_bros - .retain(|_key, (broed_place, _muta)| !self.info.places_conflict(*broed_place, broker)), - } - } - } -} - -#[derive(Clone, Debug, Default)] -struct StateInfo<'tcx> { - state: State, - /// This is a set of values that *might* contain the owned value. - /// MIR has this *beautiful* habit of moving stuff into anonymous - /// locals first before using it further. - anons: BTreeSet, - /// This set contains anonymous borrows, these are AFAIK always used - /// for temporary borrows. - /// - /// Note: If I can confirm that these borrows are always used for - /// temporary borrows, it might be possible to prevent tracking them - /// to safe some performance. - temp_bros: BTreeMap, Mutability)>, - /// This tracks mut borrows, which might be used for two phased borrows. - /// Based on the docs, it sounds like there can always only be one. Let's - /// use an option and cry when it fails. - /// - /// See: https://rustc-dev-guide.rust-lang.org/borrow_check/two_phase_borrows.html - phase_borrow: Option<(Local, Place<'tcx>)>, -} - -#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd, Default)] -enum State { - #[default] - None, - Empty, - Filled, - Moved, - Dropped, - MaybeFilled, -} - -impl<'tcx> StateInfo<'tcx> { - /// Retruns true if this state contains valid data, which can be dropped or moved. - fn validity(&self) -> Validity { - match self.state { - State::None => unreachable!(), - State::Filled => Validity::Valid, - State::MaybeFilled => Validity::Maybe, - State::Empty | State::Moved | State::Dropped => Validity::Not, - } - } - - fn join(mut self, other: &StateInfo<'tcx>) -> StateInfo<'tcx> { - assert_ne!(other.state, State::None); - if self.state == State::None { - return other.clone(); - } - - self.state = match (self.validity(), other.validity()) { - (Validity::Valid, Validity::Valid) => State::Filled, - (Validity::Not, Validity::Not) => State::Empty, - (_, _) => State::MaybeFilled, - }; - - // FIXME: Here we can have collisions where two anons reference different places... oh no... - self.anons.extend(other.anons.iter()); - self.temp_bros.extend(other.temp_bros.iter()); - - assert!(!(self.phase_borrow.is_some() && other.phase_borrow.is_some())); - self.phase_borrow = self.phase_borrow.or(other.phase_borrow); - - self - } - - fn take_phase_borrow(&mut self, anon: &Place<'_>) -> Option> { - assert!(anon.just_local()); - - self.phase_borrow - .take_if(|(local, _)| *local == anon.local) - .map(|(_, place)| place) - } - - /// This tries to remove the given place from the known anons that hold this value. - /// It will retrun `true`, if the removal was successfull. - /// Places with projections will be ignored. - fn remove_anon(&mut self, anon: &Place<'_>) -> bool { - assert!(anon.just_local()); - self.anons.remove(&anon.local) - } - - fn remove_anon_bro(&mut self, anon: &Place<'_>) -> Option<(Place<'tcx>, Mutability)> { - assert!(anon.just_local()); - - if let Some((_loc, place)) = self.phase_borrow.take_if(|(local, _place)| *local == anon.local) { - // TwoPhaseBorrows are always mutable - Some((place, Mutability::Mut)) - } else { - self.temp_bros.remove(&anon.local) - } - } - - /// This clears this state. The `state` field has to be set afterwards - fn clear(&mut self) { - self.anons.clear(); - self.temp_bros.clear(); - self.phase_borrow = None; - - self.state = State::None; + self.states[bb].add_borrow(borrow, broker, kind, self.info, &mut self.pats); } } @@ -319,7 +163,7 @@ pub enum OwnedPat { } impl<'a, 'tcx> Visitor<'tcx> for OwnedAnalysis<'a, 'tcx> { - fn visit_basic_block_data(&mut self, bb: BasicBlock, bbd: &mir::BasicBlockData<'tcx>) { + fn visit_basic_block_data(&mut self, bb: BasicBlock, bbd: &BasicBlockData<'tcx>) { self.init_state(bb); self.super_basic_block_data(bb, bbd); } @@ -329,17 +173,17 @@ impl<'a, 'tcx> Visitor<'tcx> for OwnedAnalysis<'a, 'tcx> { // is nice but context is better. Imagine `_0 = move X`. So at last, I need // to write these things with other visitors. - fn visit_statement(&mut self, stmt: &mir::Statement<'tcx>, loc: mir::Location) { + fn visit_statement(&mut self, stmt: &Statement<'tcx>, loc: Location) { if let StatementKind::StorageDead(local) = &stmt.kind { - self.states[loc.block].temp_bros.remove(local); + self.states[loc.block].remove_anon_bro(&local.as_place()); } self.super_statement(stmt, loc); } - fn visit_assign(&mut self, target: &Place<'tcx>, rvalue: &Rvalue<'tcx>, loc: mir::Location) { + fn visit_assign(&mut self, target: &Place<'tcx>, rvalue: &Rvalue<'tcx>, loc: Location) { // TODO Ensure that moves always invalidate all borrows. IE. that the current // borrow check is really CFG insensitive. - if let Rvalue::Ref(_region, mir::BorrowKind::Fake, _place) = &rvalue { + if let Rvalue::Ref(_region, BorrowKind::Fake, _place) = &rvalue { return; } @@ -353,13 +197,13 @@ impl<'a, 'tcx> Visitor<'tcx> for OwnedAnalysis<'a, 'tcx> { self.super_assign(target, rvalue, loc); } - fn visit_terminator(&mut self, term: &mir::Terminator<'tcx>, loc: mir::Location) { + fn visit_terminator(&mut self, term: &Terminator<'tcx>, loc: Location) { self.visit_terminator_for_args(term, loc.block); self.visit_terminator_for_anons(term, loc.block); self.super_terminator(term, loc); } - fn visit_local(&mut self, local: Local, context: mir::visit::PlaceContext, loc: mir::Location) { + fn visit_local(&mut self, local: Local, _context: mir::visit::PlaceContext, loc: Location) { // TODO: Check that this isn't called for StorageLife and StorageDead if local == self.local { self.use_count += 1; @@ -465,23 +309,21 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { } } - if let mir::Rvalue::Ref(_reg, _kind, src) = &rval - && let Some(muta) = self.states[bb].temp_bros.get(&src.local).copied() - { + if let Rvalue::Ref(_reg, _kind, src) = &rval { match src.projection.as_slice() { - [mir::PlaceElem::Deref] => { + [PlaceElem::Deref] => { // &(*_1) = Copy assert!(target.just_local()); - self.states[bb].temp_bros.insert(target.local, muta); + self.states[bb].add_ref_copy(target.local, src) }, _ => todo!("Handle ref of anon ref {target:#?} = {rval:#?} (at {bb:#?})\n{self:#?}"), } } } - fn visit_terminator_for_args(&mut self, term: &mir::Terminator<'tcx>, bb: BasicBlock) { + fn visit_terminator_for_args(&mut self, term: &Terminator<'tcx>, bb: BasicBlock) { match &term.kind { - mir::TerminatorKind::Drop { place, .. } => { + TerminatorKind::Drop { place, .. } => { if place.local == self.local { match self.states[bb].validity() { Validity::Valid => { @@ -503,7 +345,7 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { } } }, - mir::TerminatorKind::Call { + TerminatorKind::Call { func, args, destination: dest, @@ -530,7 +372,7 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { // Both of these operate on copy types. They are uninteresting for now. // They can still be important since these a reads which cancel mutable borrows and fields can be read - mir::TerminatorKind::SwitchInt { discr: op, .. } | mir::TerminatorKind::Assert { cond: op, .. } => { + TerminatorKind::SwitchInt { discr: op, .. } | TerminatorKind::Assert { cond: op, .. } => { if let Some(place) = op.place() && place.local == self.local { @@ -538,27 +380,27 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { } }, // Controll flow or unstable features. Uninteresting for values - mir::TerminatorKind::Goto { .. } - | mir::TerminatorKind::UnwindResume - | mir::TerminatorKind::UnwindTerminate(_) - | mir::TerminatorKind::Return - | mir::TerminatorKind::Unreachable - | mir::TerminatorKind::Yield { .. } - | mir::TerminatorKind::CoroutineDrop - | mir::TerminatorKind::FalseEdge { .. } - | mir::TerminatorKind::FalseUnwind { .. } - | mir::TerminatorKind::InlineAsm { .. } => {}, + TerminatorKind::Goto { .. } + | TerminatorKind::UnwindResume + | TerminatorKind::UnwindTerminate(_) + | TerminatorKind::Return + | TerminatorKind::Unreachable + | TerminatorKind::Yield { .. } + | TerminatorKind::CoroutineDrop + | TerminatorKind::FalseEdge { .. } + | TerminatorKind::FalseUnwind { .. } + | TerminatorKind::InlineAsm { .. } => {}, } } - fn visit_terminator_for_anons(&mut self, term: &mir::Terminator<'tcx>, bb: BasicBlock) { + fn visit_terminator_for_anons(&mut self, term: &Terminator<'tcx>, bb: BasicBlock) { match &term.kind { - mir::TerminatorKind::Drop { place, .. } => { + TerminatorKind::Drop { place, .. } => { // TODO: Is this even interesting or can we just ignore this? // if let Some((index, _)) = find_anon(place.local) { // self.states[bb].anons.swap_remove(index); // } }, - mir::TerminatorKind::Call { + TerminatorKind::Call { func, args, destination: dest, @@ -593,9 +435,6 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { { self.pats.insert(OwnedPat::ManualDrop); } - } else if let Some(place) = self.states[bb].take_phase_borrow(&arg) { - temp_bros_mut_ctn += 1; - self.pats.insert(OwnedPat::TempBorrowMut); } else if let Some((place, muta)) = self.states[bb].remove_anon_bro(&arg) { let pat = match muta { Mutability::Not => { @@ -634,7 +473,7 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { // Both of these operate on copy types. They are uninteresting for now. // They can still be important since these a reads which cancel mutable borrows and fields can be read - mir::TerminatorKind::SwitchInt { discr: op, .. } | mir::TerminatorKind::Assert { cond: op, .. } => { + TerminatorKind::SwitchInt { discr: op, .. } | TerminatorKind::Assert { cond: op, .. } => { if let Some(place) = op.place() && place.local == self.local { @@ -642,16 +481,16 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> { } }, // Controll flow or unstable features. Uninteresting for values - mir::TerminatorKind::Goto { .. } - | mir::TerminatorKind::UnwindResume - | mir::TerminatorKind::UnwindTerminate(_) - | mir::TerminatorKind::Return - | mir::TerminatorKind::Unreachable - | mir::TerminatorKind::Yield { .. } - | mir::TerminatorKind::CoroutineDrop - | mir::TerminatorKind::FalseEdge { .. } - | mir::TerminatorKind::FalseUnwind { .. } - | mir::TerminatorKind::InlineAsm { .. } => {}, + TerminatorKind::Goto { .. } + | TerminatorKind::UnwindResume + | TerminatorKind::UnwindTerminate(_) + | TerminatorKind::Return + | TerminatorKind::Unreachable + | TerminatorKind::Yield { .. } + | TerminatorKind::CoroutineDrop + | TerminatorKind::FalseEdge { .. } + | TerminatorKind::FalseUnwind { .. } + | TerminatorKind::InlineAsm { .. } => {}, } } } diff --git a/clippy_lints/src/borrow_pats/owned/state.rs b/clippy_lints/src/borrow_pats/owned/state.rs new file mode 100644 index 000000000000..bd80c93b7bfc --- /dev/null +++ b/clippy_lints/src/borrow_pats/owned/state.rs @@ -0,0 +1,160 @@ +#![warn(unused)] + +use super::super::prelude::*; +use super::OwnedPat; + +#[derive(Clone, Debug, Default)] +pub struct StateInfo<'tcx> { + pub state: State, + /// This is a set of values that *might* contain the owned value. + /// MIR has this *beautiful* habit of moving stuff into anonymous + /// locals first before using it further. + pub anons: FxHashSet, + /// This set contains anonymous borrows, these are AFAIK always used + /// for temporary borrows. + /// + /// Note: If I can confirm that these borrows are always used for + /// temporary borrows, it might be possible to prevent tracking them + /// to safe some performance. + anon_bros: FxHashMap, Mutability)>, + /// This tracks mut borrows, which might be used for two phased borrows. + /// Based on the docs, it sounds like there can always only be one. Let's + /// use an option and cry when it fails. + /// + /// See: https://rustc-dev-guide.rust-lang.org/borrow_check/two_phase_borrows.html + phase_borrow: Option<(Local, Place<'tcx>)>, +} + +#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd, Default)] +pub enum State { + #[default] + None, + Empty, + Filled, + Moved, + Dropped, + MaybeFilled, +} + +impl<'tcx> StateInfo<'tcx> { + /// Retruns true if this state contains valid data, which can be dropped or moved. + pub fn validity(&self) -> Validity { + match self.state { + State::None => unreachable!(), + State::Filled => Validity::Valid, + State::MaybeFilled => Validity::Maybe, + State::Empty | State::Moved | State::Dropped => Validity::Not, + } + } + + pub fn join(mut self, other: &StateInfo<'tcx>) -> StateInfo<'tcx> { + assert_ne!(other.state, State::None); + if self.state == State::None { + return other.clone(); + } + + self.state = match (self.validity(), other.validity()) { + (Validity::Valid, Validity::Valid) => State::Filled, + (Validity::Not, Validity::Not) => State::Empty, + (_, _) => State::MaybeFilled, + }; + + // FIXME: Here we can have collisions where two anons reference different places... oh no... + self.anons.extend(other.anons.iter()); + self.anon_bros.extend(other.anon_bros.iter()); + + assert!(!(self.phase_borrow.is_some() && other.phase_borrow.is_some())); + self.phase_borrow = self.phase_borrow.or(other.phase_borrow); + + self + } + + /// This tries to remove the given place from the known anons that hold this value. + /// It will retrun `true`, if the removal was successfull. + /// Places with projections will be ignored. + pub fn remove_anon(&mut self, anon: &Place<'_>) -> bool { + assert!(anon.just_local()); + self.anons.remove(&anon.local) + } + + /// This clears this state. The `state` field has to be set afterwards + pub fn clear(&mut self) { + self.anons.clear(); + self.anon_bros.clear(); + self.phase_borrow = None; + + self.state = State::None; + } + + pub fn add_borrow( + &mut self, + borrow: Place<'tcx>, + broker: Place<'tcx>, + kind: BorrowKind, + info: &AnalysisInfo<'tcx>, + pats: &mut BTreeSet, + ) { + self.update_bros(broker, kind.mutability(), info); + + if matches!(kind, BorrowKind::Shared) + && let Some((_loc, phase_place)) = self.phase_borrow + && info.places_conflict(phase_place, broker) + { + pats.insert(OwnedPat::TwoPhasedBorrow); + } + + // So: It turns out that MIR is an inconsisten hot mess. Two-Phase-Borrows are apparently + // allowed to violate rust borrow semantics... + // + // Simple example: `x.push(x.len())` + if matches!(info.locals[&borrow.local].kind, LocalKind::AnonVar) { + assert!(borrow.just_local()); + if kind.allows_two_phase_borrow() { + let old = self.phase_borrow.replace((borrow.local, broker)); + assert_eq!(old, None); + } else { + self.anon_bros.insert(borrow.local, (broker, kind.mutability())); + } + } else { + todo!("Named Local: {borrow:#?} = {broker:#?}\n{self:#?}"); + } + } + + /// This function informs the state, that a local loan was just copied. + pub fn add_ref_copy(&mut self, dst: Local, src: &Place<'_>) { + if src.just_local() + && let Some(data) = self.anon_bros.get(&src.local).copied() + { + self.anon_bros.insert(dst, data); + } + } + + fn update_bros(&mut self, broker: Place<'tcx>, muta: Mutability, info: &AnalysisInfo<'tcx>) { + if broker.just_local() && matches!(muta, Mutability::Mut) { + // If the entire thing is borrowed mut, every reference get's cleared + self.anon_bros.clear(); + } else { + // I switch on muta before the `retain`, to make the `retain`` specialized and therefore faster. + match muta { + // Not mutable aka aliasable + Mutability::Not => self.anon_bros.retain(|_key, (broed_place, muta)| { + !(matches!(muta, Mutability::Mut) && info.places_conflict(*broed_place, broker)) + }), + Mutability::Mut => self + .anon_bros + .retain(|_key, (broed_place, _muta)| !info.places_conflict(*broed_place, broker)), + } + } + } + + pub fn remove_anon_bro(&mut self, anon: &Place<'_>) -> Option<(Place<'tcx>, Mutability)> { + assert!(anon.just_local()); + + if let Some((_loc, place)) = self.phase_borrow.take_if(|(local, _place)| *local == anon.local) { + // TwoPhaseBorrows are always mutable + Some((place, Mutability::Mut)) + } else { + self.anon_bros.remove(&anon.local) + } + } +} diff --git a/clippy_lints/src/borrow_pats/prelude.rs b/clippy_lints/src/borrow_pats/prelude.rs new file mode 100644 index 000000000000..23f7df576eb5 --- /dev/null +++ b/clippy_lints/src/borrow_pats/prelude.rs @@ -0,0 +1,22 @@ +// Aliases +pub use rustc_middle::mir; + +// Traits: +pub use super::rustc_extention::{BodyMagic, LocalMagic, PlaceMagic}; +pub use itertools::Itertools; +pub use rustc_lint::LateLintPass; +pub use rustc_middle::mir::visit::Visitor; + +// Data Structures +pub use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +pub use rustc_index::IndexVec; +pub use std::collections::{BTreeMap, BTreeSet}; + +// Common Types +pub use super::{AnalysisInfo, LocalKind, Validity}; +pub use rustc_ast::Mutability; +pub use rustc_middle::mir::{ + BasicBlock, BasicBlockData, BorrowKind, Local, Location, Operand, Place, PlaceElem, Rvalue, Statement, + StatementKind, Terminator, TerminatorKind, VarDebugInfo, VarDebugInfoContents, START_BLOCK, +}; +pub use rustc_span::{sym, Symbol}; diff --git a/clippy_lints/src/borrow_pats/rustc_extention.rs b/clippy_lints/src/borrow_pats/rustc_extention.rs index 47c4b20fc65e..0bc5e1ae95ad 100644 --- a/clippy_lints/src/borrow_pats/rustc_extention.rs +++ b/clippy_lints/src/borrow_pats/rustc_extention.rs @@ -1,3 +1,4 @@ +use mid::mir::{Local, Place}; use rustc_hir as hir; use rustc_middle::{self as mid, mir}; use std::collections::VecDeque; @@ -71,6 +72,19 @@ impl PlaceMagic for mir::Place<'_> { } } +pub trait LocalMagic { + fn as_place(&self) -> Place<'static>; +} + +impl LocalMagic for Local { + fn as_place(&self) -> Place<'static> { + Place { + local: *self, + projection: rustc_middle::ty::List::empty(), + } + } +} + pub fn print_body(body: &mir::Body<'_>) { for (idx, data) in body.basic_blocks.iter_enumerated() { println!("bb{}:", idx.index());