Skip to content

Commit

Permalink
Tracking all assignments
Browse files Browse the repository at this point in the history
  • Loading branch information
xFrednet committed Apr 21, 2024
1 parent 2376401 commit c09e1c8
Show file tree
Hide file tree
Showing 10 changed files with 393 additions and 228 deletions.
1 change: 1 addition & 0 deletions clippy_lints/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ unicode-script = { version = "0.5", default-features = false }
semver = "1.0"
rustc-semver = "1.1"
url = "2.2"
smallvec = { version = "1.8.1", features = ["union", "may_dangle"] }

[dev-dependencies]
walkdir = "2.3"
Expand Down
210 changes: 210 additions & 0 deletions clippy_lints/src/borrow_pats/body.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
//! This module analyzes the relationship between the function signature and
//! the internal dataflow. Specifically, it checks for the following things:
//!
//! - Might an owned argument be returned
//! - Are arguments stored in `&mut` loans
//! - Are dependent loans returned
//! - Might a returned loan be `'static`
//! - Are all returned values const
//! - Is the unit type returned
//! - How often do `&mut self` refs need to be `&mut`
//! - Are all the dependencies from the function signature used.
#![warn(unused)]

use super::prelude::*;
use super::visit_body;

mod pattern;
pub use pattern::*;

use super::{PatternEnum, PatternStorage};

#[derive(Debug)]
pub struct BodyAnalysis<'a, 'tcx> {
info: &'a AnalysisInfo<'tcx>,
pats: BTreeSet<BodyPat>,
data_flow: IndexVec<Local, SmallVec<[AssignInfo<'tcx>; 2]>>,
}

/// This indicates an assignment to `to`. In most cases, there is also a `from`.
#[expect(unused)]
#[derive(Debug, Copy, Clone)]
enum AssignInfo<'tcx> {
Place {
from: Place<'tcx>,
to: Place<'tcx>,
},
Const {
to: Place<'tcx>,
},
Calc {
to: Place<'tcx>,
},
/// This is typical for loans and function calls. If a places might depend
/// multiple things this will be added mutiple times.
Dep {
from: Place<'tcx>,
to: Place<'tcx>,
},
/// A value was constructed with this data
Ctor {
/// The `to` indicates the part of the target, that hols the from value.
to: Place<'tcx>,
from: Place<'tcx>,
},
}

impl<'tcx> AssignInfo<'tcx> {
#[expect(unused)]
fn to(&self) -> Place<'tcx> {
match self {
Self::Place { to, .. }
| Self::Const { to }
| Self::Calc { to }
| Self::Dep { to, .. }
| Self::Ctor { to, .. } => *to,
}
}
}

impl<'a, 'tcx> BodyAnalysis<'a, 'tcx> {
fn new(info: &'a AnalysisInfo<'tcx>) -> Self {
let mut data_flow = IndexVec::default();
data_flow.resize(info.locals.len(), Default::default());
Self {
info,
pats: BTreeSet::default(),
data_flow,
}
}

pub fn run(
info: &'a AnalysisInfo<'tcx>,
_def_id: LocalDefId,
hir_sig: &rustc_hir::FnSig<'_>,
context: BodyContext,
) -> (BodyInfo, BTreeSet<BodyPat>) {
let mut anly = Self::new(info);

visit_body(&mut anly, info);

let body_info = BodyInfo::from_sig(hir_sig, context);
(body_info, anly.pats)
}
}

impl<'a, 'tcx> Visitor<'tcx> for BodyAnalysis<'a, 'tcx> {
fn visit_assign(&mut self, target: &Place<'tcx>, rval: &Rvalue<'tcx>, _loc: mir::Location) {
match rval {
Rvalue::Ref(_reg, _kind, src) => {
match src.projection.as_slice() {
[mir::PlaceElem::Deref] => {
// &(*_1) => Copy
self.data_flow[target.local].push(AssignInfo::Place {
from: *src,
to: *target,
});
},
_ => {
// _1 = &_2 => simple loan
self.data_flow[target.local].push(AssignInfo::Dep {
from: *src,
to: *target,
});
},
}
},
Rvalue::Cast(_, op, _) | Rvalue::Use(op) => {
let event = match &op {
Operand::Constant(_) => AssignInfo::Const { to: *target },
Operand::Copy(from) | Operand::Move(from) => AssignInfo::Place {
from: *from,
to: *target,
},
};
self.data_flow[target.local].push(event);
},
Rvalue::CopyForDeref(from) => {
self.data_flow[target.local].push(AssignInfo::Place {
from: *from,
to: *target,
});
},
Rvalue::Repeat(op, _) => {
let event = match &op {
Operand::Constant(_) => AssignInfo::Const { to: *target },
Operand::Copy(from) | Operand::Move(from) => AssignInfo::Ctor {
from: *from,
to: *target,
},
};
self.data_flow[target.local].push(event);
},
// Constructed Values
Rvalue::Aggregate(_, _fields) => {
todo!();
},
// Casts should depend on the input data
Rvalue::ThreadLocalRef(_)
| Rvalue::NullaryOp(_, _)
| Rvalue::AddressOf(_, _)
| Rvalue::Discriminant(_)
| Rvalue::ShallowInitBox(_, _)
| Rvalue::Len(_)
| Rvalue::BinaryOp(_, _)
| Rvalue::UnaryOp(_, _)
| Rvalue::CheckedBinaryOp(_, _) => {
self.data_flow[target.local].push(AssignInfo::Calc { to: *target });
},
}
}

fn visit_terminator(&mut self, term: &Terminator<'tcx>, loc: Location) {
let TerminatorKind::Call { destination: dest, .. } = &term.kind else {
return;
};

let rels = &self.info.terms[&loc.block];

assert!(dest.just_local());
if !rels.contains_key(&dest.local) {
self.data_flow[dest.local].push(AssignInfo::Calc { to: *dest });
}

for (target, sources) in rels {
for src in sources {
// At this point, I don't care anymore
let from = unsafe { std::mem::transmute::<Place<'static>, Place<'tcx>>((*src).as_place()) };
let to = unsafe { std::mem::transmute::<Place<'static>, Place<'tcx>>((*target).as_place()) };
self.data_flow[*target].push(AssignInfo::Dep { from, to });
}
}
}
}

#[expect(unused)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub enum ReturnPat {
/// A constant value is returned.
Const,
/// A argument is returned
Argument,
/// This is a part of an argument
ArgumentPart,
/// A computed value is returned
Computed,
/// A loan to a constant value. This only means that the lifetime can be
/// static. The lifetime for calling functions still depends on the
/// function signature.
StaticLoan,

/// Just the unit type is returned, nothing interesting
Unit,
/// The return depends on some kind of condition
Conditional,
/// All returned values are constant
AllConst,
}
impl PatternEnum for ReturnPat {}
pub type ReturnPats = PatternStorage<ReturnPat>;
78 changes: 78 additions & 0 deletions clippy_lints/src/borrow_pats/body/pattern.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
use rustc_hir::FnSig;

#[expect(unused)]
#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub enum BodyPat {
/// This function doesn't take any arguments
NoArguments,
/// Indicates that a body contained an anonymous loan. These are usually
/// only used for temp borrows.
HasAnonBorrow,
/// Indicates that a body contained a named loan. So cases like
/// `_2 = &_1`, where `_2` is a named variable.
HasNamedBorrow,
/// This function uses a two phased borrow. This is different from rustc's
/// value tracked in `BorrowKind` as this this pattern is only added if a two
/// phase borrow actually happened (i.e. the code would be rejected without)
HasTwoPhaseBorrow,
ReturnedStaticLoanForNonStatic,
}

#[derive(Clone)]
pub struct BodyInfo {
pub(super) unit_return: bool,
pub(super) is_const: bool,
pub(super) is_safe: bool,
pub(super) is_sync: bool,
pub(super) context: BodyContext,
}

impl BodyInfo {
pub fn from_sig(hir_sig: &FnSig<'_>, context: BodyContext) -> Self {
let unit_return = match hir_sig.decl.output {
rustc_hir::FnRetTy::DefaultReturn(_) => true,
rustc_hir::FnRetTy::Return(hir_ty) => {
matches!(hir_ty.kind, rustc_hir::TyKind::Tup(&[]))
},
};
Self {
unit_return,
is_const: hir_sig.header.is_const(),
is_safe: !hir_sig.header.is_unsafe(),
is_sync: !hir_sig.header.is_async(),
context,
}
}
}

impl std::fmt::Debug for BodyInfo {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "BodyInfo({self})")
}
}

impl std::fmt::Display for BodyInfo {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let unit_return = if self.unit_return { "UnitReturn" } else { "OtherReturn" };
let constness = if self.is_const { "Const" } else { "NotConst" };
let safety = if self.is_safe { "Safe" } else { "Unsafe" };
let sync = if self.is_sync { "Sync" } else { "Async" };
let context = format!("{:?}", self.context);
write!(
f,
"{unit_return:<11}, {constness:<9}, {safety:<6}, {sync:<5}, {context:<10}"
)
}
}

#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub enum BodyContext {
/// The function is simple and free.
Free,
/// The function is inside an `impl` block.
Impl,
/// The function is inside an `impl Trait` block.
TraitImpl,
/// The function is inside a trait definition.
TraitDef,
}
34 changes: 4 additions & 30 deletions clippy_lints/src/borrow_pats/info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use rustc_middle::mir::{BasicBlock, Local, Place};
use rustc_middle::ty::TyCtxt;
use rustc_span::Symbol;

use super::ret::ReturnPats;
use super::body::ReturnPats;
use super::{PatternStorage, PlaceMagic};

use {rustc_borrowck as borrowck, rustc_hir as hir};
Expand Down Expand Up @@ -109,26 +109,6 @@ impl<'tcx> AnalysisInfo<'tcx> {
pub fn find_loop(&self, bb: BasicBlock) -> Option<&(BitSet<BasicBlock>, BasicBlock)> {
super::find_loop(&self.loops, bb)
}

pub fn local_constness(&self, local: Local) -> LocalConstness {
match &self.locals[&local].data {
DataInfo::Unresolved => unreachable!("{self:#?}"),
DataInfo::Computed | DataInfo::Argument => LocalConstness::Nope,
DataInfo::Mixed => LocalConstness::Maybe,
DataInfo::Local(other) => self.local_constness(*other),
DataInfo::Loan(place) | DataInfo::Part(place) => {
LocalConstness::max(LocalConstness::Maybe, self.local_constness(place.local))
},
DataInfo::Const => LocalConstness::Const,
DataInfo::Cast(other) => self.local_constness(*other),
DataInfo::Ctor(others) => others
.iter()
.filter_map(LocalOrConst::local)
.map(|local| self.local_constness(local))
.max()
.unwrap_or(LocalConstness::Const),
}
}
}

#[derive(Debug)]
Expand Down Expand Up @@ -282,15 +262,6 @@ pub enum LocalOrConst {
Const,
}

impl LocalOrConst {
fn local(&self) -> Option<Local> {
match self {
LocalOrConst::Local(local) => Some(*local),
LocalOrConst::Const => None,
}
}
}

impl From<&mir::Operand<'_>> for LocalOrConst {
fn from(value: &mir::Operand<'_>) -> Self {
if let Some(place) = value.place() {
Expand All @@ -305,9 +276,12 @@ impl From<&mir::Operand<'_>> for LocalOrConst {
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
pub enum LocalConstness {
/// Is const
#[expect(unused)]
Const,
/// Maybe const
#[expect(unused)]
Maybe,
/// Def not const
#[expect(unused)]
Nope,
}
Loading

0 comments on commit c09e1c8

Please sign in to comment.