Skip to content

Commit

Permalink
Identify named borrows for owned values better
Browse files Browse the repository at this point in the history
  • Loading branch information
@xFrednet
xFrednet committed Apr 27, 2024
1 parent 839da51 commit 9541343
Show file tree
Hide file tree
Showing 14 changed files with 245 additions and 114 deletions.
10 changes: 0 additions & 10 deletions clippy_lints/src/borrow_pats/body.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,22 +191,12 @@ impl<'a, 'tcx> Visitor<'tcx> for BodyAnalysis<'a, 'tcx> {
match self.info.locals[&target.local].kind {
LocalKind::UserVar(_, _) => {
self.pats.insert(BodyPat::HasNamedBorrow);
if is_mut {
self.stats.named_borrow_count_mut += 1;
} else {
self.stats.named_borrow_count += 1;
}
},
// Returns are also counted as anon borrows. The
// relations are checked later. Otherwise, it's hard to
// correctly handle `_2 = &_1; _0 = _1`
LocalKind::Return | LocalKind::AnonVar => {
self.pats.insert(BodyPat::HasAnonBorrow);
if is_mut {
self.stats.anon_borrow_count_mut += 1;
} else {
self.stats.anon_borrow_count += 1;
}
},
LocalKind::Unused => unreachable!(),
}
Expand Down
32 changes: 32 additions & 0 deletions clippy_lints/src/borrow_pats/body/pattern.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,50 @@ pub enum BodyContext {
TraitDef,
}

/// Most of these statistics need to be filled by the individual analysis passed.
/// Every value should document which pass might modify/fill it.
///
/// Without more context and tracking the data flow, it's impossible to know what
/// certain instructions are.
///
/// For example, a named borrow can have different shapes. Assuming `_1` is the
/// owned value and `_2` is the named references, they could have the following
/// shapes:
///
/// ```
/// // Direct
/// _2 = &_1
///
/// // Indirect
/// _3 = &_1
/// _2 = &(*_3)
///
/// // Indirect + Copy
/// _3 = &_1
/// _4 = &(*_3)
/// _2 = move _4
/// ```
#[derive(Debug, Clone, Default)]
pub struct BodyStats {
/// Number of relations between the arguments and the return value accoring
/// to the function signature
///
/// Filled by `BodyAnalysis`
pub return_relations_signature: usize,
/// Number of relations between the arguments and the return value that have
/// been found inside the body
///
/// Filled by `BodyAnalysis`
pub return_relations_found: usize,
/// Number of relations between arguments according to the signature
///
/// Filled by `BodyAnalysis`
pub arg_relations_signature: usize,
/// Number of relations between arguments that have been found in the body
///
/// Filled by `BodyAnalysis`
pub arg_relations_found: usize,

/// The number of borrows into anonymous values.
///
/// These are collected by the BodyAnalysis
Expand Down
24 changes: 23 additions & 1 deletion clippy_lints/src/borrow_pats/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@
//!
//! # Report
//! - Mention Crater Blacklist: https://github.com/rust-lang/crater/blob/master/config.toml (170)
//! - Write about fricking named borrows...
//! - MIR can violate rust semantics:
//! - `(_1 = &(*_2))`
//! - Two Phased Borrows
//!
//! # Optional and good todos:
//! - Investigate the `explicit_outlives_requirements` lint
Expand Down Expand Up @@ -97,6 +101,8 @@ pub struct BorrowPats {
/// Indicates if the collected patterns should be printed for each pattern.
print_pats: bool,
print_call_relations: bool,
print_locals: bool,
print_stats: bool,
}

impl BorrowPats {
Expand All @@ -105,12 +111,16 @@ impl BorrowPats {
let enabled = true;
let print_pats = std::env::var("CLIPPY_PETS_PRINT").is_ok();
let print_call_relations = std::env::var("CLIPPY_PETS_TEST_RELATIONS").is_ok();
let print_locals = std::env::var("CLIPPY_LOCALS_PRINT").is_ok();
let print_stats = std::env::var("CLIPPY_STATS_PRINT").is_ok();

Self {
msrv,
enabled,
print_pats,
print_call_relations,
print_locals,
print_stats,
}
}

Expand Down Expand Up @@ -152,16 +162,22 @@ impl BorrowPats {
let mut info = AnalysisInfo::new(cx, def_id);

let (body_info, body_pats, body_stats) = body::BodyAnalysis::run(&info, def_id, hir_sig, context);
info.stats.replace(body_stats);

if self.print_call_relations {
println!("# Relations for {body_name:?}");
println!("- Self relations: {:#?}", body_stats);
println!("- Self relations: {:#?}", info.stats.borrow());
println!("- Called function relations: {:#?}", info.terms);
println!("- Body: {} {:?}", body_info, body_pats);
println!();
return;
}

if self.print_locals {
println!("# Locals");
println!("{:#?}", info.locals);
}

for (local, local_info) in info.locals.iter().skip(1) {
match &local_info.kind {
LocalKind::Return => unreachable!("Skipped before"),
Expand All @@ -183,6 +199,12 @@ impl BorrowPats {
// Return must be printed at the end, as it might be modified by
// the following analysis thingies
println!("- Body: {} {:?}", body_info, body_pats);
}
if self.print_stats {
println!("- Stats: {:#?}", info.stats.borrow());
}

if self.print_pats || self.print_stats {
println!();
}
}
Expand Down
70 changes: 40 additions & 30 deletions clippy_lints/src/borrow_pats/owned.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ pub enum OwnedPat {
/// For `Copy` types this is only tracked, if the values have the same name.
/// as the value is otherwise still accessible.
ModMutShadowUnmut,
/// A loan of this value was assigned to a named place
NamedLoan,
}

impl<'a, 'tcx> MyVisitor<'tcx> for OwnedAnalysis<'a, 'tcx> {
Expand Down Expand Up @@ -346,44 +348,52 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
fn visit_assign_for_anon(&mut self, target: &Place<'tcx>, rval: &Rvalue<'tcx>, bb: BasicBlock) {
if let Rvalue::Use(op) = &rval
&& let Operand::Move(place) = op
&& self.states[bb].remove_anon(place)
{
match self.info.locals[&target.local].kind {
LocalKind::Return => {
self.pats.insert(OwnedPat::Returned);
},
LocalKind::UserVar(_, _) => {
if self.info.locals[&target.local].kind.is_arg() {
// Check if this assignment can escape the function
todo!("{target:#?}\n{rval:#?}\n{bb:#?}\n{self:#?}")
}
if place.is_part() {
self.pats.insert(OwnedPat::MovedToVarPart);
} else if place.just_local() {
// TODO: Check for `let x = x`, where x was mut and x no longer is and assignment count = 0
self.pats.insert(OwnedPat::MovedToVar);
} else {
todo!("{target:#?}\n{rval:#?}\n{bb:#?}\n{self:#?}");
}
},
LocalKind::AnonVar => {
assert!(place.just_local());
self.states[bb].anons.insert(target.local);
},
LocalKind::Unused => unreachable!(),
if self.states[bb].remove_anon(place) {
match self.info.locals[&target.local].kind {
LocalKind::Return => {
self.pats.insert(OwnedPat::Returned);
},
LocalKind::UserVar(_, _) => {
if self.info.locals[&target.local].kind.is_arg() {
// Check if this assignment can escape the function
todo!("{target:#?}\n{rval:#?}\n{bb:#?}\n{self:#?}")
}
if place.is_part() {
self.pats.insert(OwnedPat::MovedToVarPart);
} else if place.just_local() {
// TODO: Check for `let x = x`, where x was mut and x no
// longer is and assignment count = 0
self.pats.insert(OwnedPat::MovedToVar);
} else {
todo!("{target:#?}\n{rval:#?}\n{bb:#?}\n{self:#?}");
}
},
LocalKind::AnonVar => {
assert!(place.just_local());
self.states[bb].anons.insert(target.local);
},
LocalKind::Unused => unreachable!(),
}
}

self.states[bb].add_ref_copy(*target, *place, None, self.info, &mut self.pats)
}

if let Rvalue::Ref(_reg, _kind, src) = &rval
&& self.states[bb].has_anon_ref(src.local)
{
if let Rvalue::Ref(_reg, new_borrow_kind, src) = &rval {
match src.projection.as_slice() {
// &(*_1) = Copy
[PlaceElem::Deref] => {
// &(*_1) = Copy
// This will surely fail at one point. It was correct while this was only
// for anon vars. But let's fail for now, to handle it later.
assert!(target.just_local());
self.states[bb].add_ref_copy(target.local, src.local)
self.states[bb].add_ref_copy(*target, *src, Some(*new_borrow_kind), self.info, &mut self.pats)
},
_ => {
if self.states[bb].has_bro(src).is_some() {
todo!("Handle ref to ref {target:#?} = {rval:#?} (at {bb:#?})\n{self:#?}");
}
},
_ => todo!("Handle ref of anon ref {target:#?} = {rval:#?} (at {bb:#?})\n{self:#?}"),
}
}
}
Expand Down
104 changes: 89 additions & 15 deletions clippy_lints/src/borrow_pats/owned/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,39 @@ pub struct StateInfo<'tcx> {
/// MIR has this *beautiful* habit of moving stuff into anonymous
/// locals first before using it further.
pub anons: FxHashSet<Local>,
/// This set contains anonymous borrows, these are AFAIK always used
/// for temporary borrows.
/// This set contains borrows, these are often used for temporary
/// borrows
///
/// Note: If I can confirm that these borrows are always used for
/// **Note 1**: Named borrows can be created in two ways (Because of course
/// they can...)
/// ```
/// // From: `mut_named_ref_non_kill`
/// // let mut x = 1;
/// // let mut p: &u32 = &x;
/// _4 = &_1
/// _3 = &(*_4)
///
/// // From: `call_extend_named`
/// // let data = String::new();
/// // let loan = &data;
/// _2 = &_3
/// ```
///
/// **Note 2**: Correction there are three ways to created named borrows...
/// Not sure why but let's take `mut_named_ref_non_kill` as and example for `y`
///
/// ```
/// // y => _2
/// // named => _3
/// _8 = &_2
/// _7 = &(*_8)
/// _3 = move _7
/// ```
///
/// **Note 3**: 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.
/// to safe some performance. (Confirmed, that they are not just
/// used for temp borrows :D)
borrows: FxHashMap<Local, (Place<'tcx>, Mutability, BroKind)>,
/// 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
Expand Down Expand Up @@ -117,6 +144,10 @@ impl<'tcx> StateInfo<'tcx> {
}

let is_named = matches!(info.locals[&borrow.local].kind, LocalKind::UserVar(..));
if is_named {
pats.insert(OwnedPat::NamedLoan);
}

let bro_kind = if let Some(bro_kind) = bro_kind {
bro_kind
} else if is_named {
Expand All @@ -137,23 +168,68 @@ impl<'tcx> StateInfo<'tcx> {
self.borrows.insert(borrow.local, (broker, kind.mutability(), bro_kind));
}
} else {
todo!("Named Local: {borrow:#?} = {broker:#?}\n{self:#?}");
// Mut loans can also be used for two-phased-borrows, but only with themselfs.
// Taking the mut loan and the owned value failes.
//
// ```
// fn test(mut vec: Vec<usize>) {
// let loan = &mut vec;
// loan.push(vec.len());
// }
// ```
//
// The two-phased-borrow will be detected by the owned reference. So we can
// ignore it here :D
self.borrows.insert(borrow.local, (broker, kind.mutability(), bro_kind));
// todo!("Named Local: {borrow:#?} = {broker:#?}\n{self:#?}");
}
}

pub fn has_anon_ref(&self, src: Local) -> bool {
self.borrows.contains_key(&src)
}

/// This function informs the state, that a local loan was just copied.
pub fn add_ref_copy(&mut self, dst: Local, src: Local) {
if let Some(data) = self.borrows.get(&src).copied() {
self.borrows.insert(dst, data);
pub fn add_ref_copy(
&mut self,
dst: Place<'tcx>,
src: Place<'tcx>,
_copy_kind: Option<BorrowKind>,
info: &AnalysisInfo<'tcx>,
pats: &mut BTreeSet<OwnedPat>,
) {
// This function has to share quite some magic with `add_borrow` but
// again is different enough that they can't be merged directly AFAIK

let Some((broker, muta, bro_kind)) = self.borrows.get(&src.local).copied() else {
return;
};

// It looks like loans preserve the mutability of th copy. This is perfectly
// inconsitent. Maybe the previous `&mut (*_2)` came from a different
// MIR version. At this point there is no value in even checking.
//
// Looking at `temp_borrow_mixed_2` it seems like the copy mutability depends
// on the use case. I'm not even disappointed anymore
let new_muta = muta;
match bro_kind {
BroKind::Dep | BroKind::Named => {
// FIXME: Maybe this doesn't even needs to be tracked?
self.borrows.insert(dst.local, (broker, new_muta, BroKind::Dep));
},
// Only anons should be able to add new information
BroKind::Anon => {
let is_named = matches!(info.locals[&dst.local].kind, LocalKind::UserVar(..));
if is_named {
pats.insert(OwnedPat::NamedLoan);
};

let new_bro_kind = if is_named { BroKind::Named } else { BroKind::Anon };

// `copy_kind` can only be mutable if `src` is also mutable
self.borrows.insert(dst.local, (broker, new_muta, new_bro_kind));
},
}
}

fn update_bros(&mut self, broker: Place<'tcx>, muta: Mutability, info: &AnalysisInfo<'tcx>) {
// TODO: Check if this function is even needed with the kill command.
// TODO: Check if this function is even needed with the kill command!!!!

if broker.just_local() && matches!(muta, Mutability::Mut) {
// If the entire thing is borrowed mut, every reference get's cleared
Expand All @@ -173,8 +249,6 @@ impl<'tcx> StateInfo<'tcx> {
}

pub fn has_bro(&mut self, anon: &Place<'_>) -> Option<(Place<'tcx>, Mutability, BroKind)> {
assert!(anon.just_local());

if let Some((_loc, place)) = self.phase_borrow.iter().find(|(local, _place)| *local == anon.local) {
// TwoPhaseBorrows are always mutable
Some((*place, Mutability::Mut, BroKind::Anon))
Expand Down
Loading

0 comments on commit 9541343

Please sign in to comment.