Refactorings for borrow tracking

clippy_lints/src/borrow_pats/owned.rs

@@ -60,8 +60,15 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
         anly.pats
     }
 
-    fn add_borrow(&mut self, bb: BasicBlock, borrow: Place<'tcx>, broker: Place<'tcx>, kind: BorrowKind) {
-        self.states[bb].add_borrow(borrow, broker, kind, self.info, &mut self.pats);
+    fn add_borrow(
+        &mut self,
+        bb: BasicBlock,
+        borrow: Place<'tcx>,
+        broker: Place<'tcx>,
+        kind: BorrowKind,
+        bro: Option<BroKind>,
+    ) {
+        self.states[bb].add_borrow(borrow, broker, kind, bro, self.info, &mut self.pats);
     }
 }
 
@@ -89,7 +96,9 @@ pub enum OwnedPat {
     /// This value was manually dropped by calling `std::mem::drop()`
     ManualDrop,
     TempBorrow,
+    TempBorrowExtended,
     TempBorrowMut,
+    TempBorrowMutExtended,
     /// Two temp borrows might alias each other, for example like this:
     /// ```
     /// take_2(&self.field, &self.field);
@@ -98,7 +107,7 @@ pub enum OwnedPat {
     /// ```
     /// take_2(&self.field, &self.field.sub_field);
     /// ```
-    TempBorrowAliased,
+    AliasedBorrow,
     /// A function takes mutliple `&mut` references to different parts of the object
     /// ```
     /// take_2(&mut self.field_a, &mut self.field_b);
@@ -201,7 +210,7 @@ impl<'a, 'tcx> Visitor<'tcx> for OwnedAnalysis<'a, 'tcx> {
 
     fn visit_statement(&mut self, stmt: &Statement<'tcx>, loc: Location) {
         if let StatementKind::StorageDead(local) = &stmt.kind {
-            self.states[loc.block].remove_anon_bro(&local.as_place());
+            self.states[loc.block].kill_local(*local);
         }
         self.super_statement(stmt, loc);
     }
@@ -217,7 +226,7 @@ impl<'a, 'tcx> Visitor<'tcx> for OwnedAnalysis<'a, 'tcx> {
             self.visit_assign_to_self(target, rvalue, loc.block);
         }
 
-        self.visit_assign_for_args(target, rvalue, loc.block);
+        self.visit_assign_for_self_in_args(target, rvalue, loc.block);
         self.visit_assign_for_anon(target, rvalue, loc.block);
 
         self.super_assign(target, rvalue, loc);
@@ -238,7 +247,7 @@ impl<'a, 'tcx> Visitor<'tcx> for OwnedAnalysis<'a, 'tcx> {
 }
 
 impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
-    fn visit_assign_for_args(&mut self, target: &Place<'tcx>, rval: &Rvalue<'tcx>, bb: BasicBlock) {
+    fn visit_assign_for_self_in_args(&mut self, target: &Place<'tcx>, rval: &Rvalue<'tcx>, bb: BasicBlock) {
         if let Rvalue::Use(op) = &rval
             && let Some(place) = op.place()
             && place.local == self.local
@@ -299,7 +308,7 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
             && place.local == self.local
         {
             if target.just_local() {
-                self.add_borrow(bb, *target, *place, *kind)
+                self.add_borrow(bb, *target, *place, *kind, None)
             } else {
                 todo!("{target:#?} = {rval:#?} (at {bb:#?})\n{self:#?}");
             }
@@ -456,12 +465,7 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
     }
     fn visit_terminator_for_anons(&mut self, term: &Terminator<'tcx>, bb: BasicBlock) {
         match &term.kind {
-            TerminatorKind::Call {
-                func,
-                args,
-                destination: dest,
-                ..
-            } => {
+            TerminatorKind::Call { func, args, .. } => {
                 if let Some(place) = func.place()
                     && self.states[bb].remove_anon(&place)
                 {
@@ -478,10 +482,11 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
                     }
                 });
 
-                let mut temp_bros = vec![];
+                let mut immut_bros = vec![];
                 // Mutable borrows can't be aliased, therefore it's suffcient
                 // to just count them
-                let mut temp_bros_mut_ctn = 0;
+                let mut mut_bro_ctn = 0;
+                let mut dep_loans: Vec<(Local, Place<'tcx>, Mutability)> = vec![];
                 for arg in args {
                     if self.states[bb].remove_anon(&arg) {
                         self.pats.insert(OwnedPat::MovedToFn);
@@ -491,39 +496,66 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
                         {
                             self.pats.insert(OwnedPat::ManualDrop);
                         }
-                    } else if let Some((place, muta)) = self.states[bb].remove_anon_bro(&arg) {
-                        let pat = match muta {
+                    } else if let Some((place, muta, bro_kind)) = self.states[bb].has_bro(&arg) {
+                        // Regardless of bro, we're interested in extentions
+                        let loan_extended = {
+                            let dep_loans_len = dep_loans.len();
+                            dep_loans.extend(self.info.terms[&bb].iter().filter_map(|(local, deps)| {
+                                deps.contains(&arg.local).then_some((*local, place, muta))
+                            }));
+                            dep_loans_len != dep_loans.len()
+                        };
+
+                        match muta {
                             Mutability::Not => {
-                                temp_bros.push(place);
-                                OwnedPat::TempBorrow
+                                immut_bros.push(place);
+
+                                if matches!(bro_kind, BroKind::Anon) {
+                                    self.pats.insert(OwnedPat::TempBorrow);
+                                    if loan_extended {
+                                        self.pats.insert(OwnedPat::TempBorrowExtended);
+                                    }
+                                }
                             },
                             Mutability::Mut => {
-                                temp_bros_mut_ctn += 1;
-                                OwnedPat::TempBorrowMut
+                                mut_bro_ctn += 1;
+                                if matches!(bro_kind, BroKind::Anon) {
+                                    self.pats.insert(OwnedPat::TempBorrowMut);
+                                    if loan_extended {
+                                        self.pats.insert(OwnedPat::TempBorrowMutExtended);
+                                    }
+                                }
                             },
                         };
-
-                        self.pats.insert(pat);
                     }
                 }
 
-                if temp_bros.len() > 1
-                    && temp_bros
+                if immut_bros.len() > 1
+                    && immut_bros
                         .iter()
                         .tuple_combinations()
                         .any(|(a, b)| self.info.places_conflict(*a, *b))
                 {
-                    self.pats.insert(OwnedPat::TempBorrowAliased);
+                    self.pats.insert(OwnedPat::AliasedBorrow);
                 }
-                if temp_bros_mut_ctn > 1 {
+
+                if mut_bro_ctn > 1 {
                     self.pats.insert(OwnedPat::MultipleMutBorrowsInArgs);
                 }
-                if !temp_bros.is_empty() && temp_bros_mut_ctn >= 1 {
+
+                if !immut_bros.is_empty() && mut_bro_ctn >= 1 {
                     self.pats.insert(OwnedPat::MixedBorrowsInArgs);
                 }
 
-                if self.states[bb].remove_anon(&dest) {
-                    unreachable!()
+                for (borrower, broker, muta) in dep_loans {
+                    let kind = match muta {
+                        Mutability::Not => BorrowKind::Shared,
+                        Mutability::Mut => BorrowKind::Mut {
+                            kind: mir::MutBorrowKind::Default,
+                        },
+                    };
+                    let borrow = unsafe { std::mem::transmute::<Place<'static>, Place<'tcx>>(borrower.as_place()) };
+                    self.add_borrow(bb, borrow, broker, kind, Some(BroKind::Dep));
                 }
             },
 

clippy_lints/src/borrow_pats/owned/state.rs

@@ -18,17 +18,13 @@ pub struct StateInfo<'tcx> {
     /// 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<Local, (Place<'tcx>, Mutability)>,
+    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
     /// 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>)>,
-    /// This tracks assignment to the local itself. Assignments to parts should
-    /// not be tracked here.
-    assignments: SmallVec<[BasicBlock; 2]>,
-    // TODO: Include this in merging. Detect patterns from it, be happy
 }
 
 #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd, Default)]
@@ -42,6 +38,23 @@ pub enum State {
     MaybeFilled,
 }
 
+#[expect(unused)]
+enum Event<'tcx> {
+    Init,
+    Loan,
+    Mutated,
+    // Moved or Dropped
+    Moved(Place<'tcx>),
+    Drop,
+}
+
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)]
+pub enum BroKind {
+    Anon,
+    Named,
+    Dep,
+}
+
 impl<'tcx> StateInfo<'tcx> {
     /// Retruns true if this state contains valid data, which can be dropped or moved.
     pub fn validity(&self) -> Validity {
@@ -53,6 +66,13 @@ impl<'tcx> StateInfo<'tcx> {
         }
     }
 
+    /// Notifies the state that a local has been killed
+    pub fn kill_local(&mut self, local: Local) {
+        self.anons.remove(&local);
+        self.borrows.remove(&local);
+        self.phase_borrow.take_if(|(phase_local, _place)| *phase_local == local);
+    }
+
     /// 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.
@@ -69,7 +89,7 @@ impl<'tcx> StateInfo<'tcx> {
     /// 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.borrows.clear();
         self.phase_borrow = None;
 
         self.state = State::None;
@@ -80,6 +100,7 @@ impl<'tcx> StateInfo<'tcx> {
         borrow: Place<'tcx>,
         broker: Place<'tcx>,
         kind: BorrowKind,
+        bro_kind: Option<BroKind>,
         info: &AnalysisInfo<'tcx>,
         pats: &mut BTreeSet<OwnedPat>,
     ) {
@@ -93,66 +114,76 @@ impl<'tcx> StateInfo<'tcx> {
             info.stats.borrow_mut().two_phase_borrows += 1;
         }
 
+        let is_named = matches!(info.locals[&borrow.local].kind, LocalKind::UserVar(..));
+        let bro_kind = if let Some(bro_kind) = bro_kind {
+            bro_kind
+        } else if is_named {
+            BroKind::Named
+        } else {
+            BroKind::Anon
+        };
+
         // 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) {
+        if !is_named {
             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()));
+                self.borrows.insert(borrow.local, (broker, kind.mutability(), bro_kind));
             }
         } else {
             todo!("Named Local: {borrow:#?} = {broker:#?}\n{self:#?}");
         }
     }
 
     pub fn has_anon_ref(&self, src: Local) -> bool {
-        self.anon_bros.contains_key(&src)
+        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.anon_bros.get(&src).copied() {
-            self.anon_bros.insert(dst, data);
+        if let Some(data) = self.borrows.get(&src).copied() {
+            self.borrows.insert(dst, data);
         }
     }
 
     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.
+
         if broker.just_local() && matches!(muta, Mutability::Mut) {
             // If the entire thing is borrowed mut, every reference get's cleared
-            self.anon_bros.clear();
+            self.borrows.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)| {
+                Mutability::Not => self.borrows.retain(|_key, (broed_place, muta, _bro)| {
                     !(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)),
+                    .borrows
+                    .retain(|_key, (broed_place, _muta, _kind)| !info.places_conflict(*broed_place, broker)),
             }
         }
     }
 
-    pub fn remove_anon_bro(&mut self, anon: &Place<'_>) -> Option<(Place<'tcx>, Mutability)> {
+    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.take_if(|(local, _place)| *local == anon.local) {
+        if let Some((_loc, place)) = self.phase_borrow.as_ref().filter(|(local, _place)| *local == anon.local) {
             // TwoPhaseBorrows are always mutable
-            Some((place, Mutability::Mut))
+            Some((*place, Mutability::Mut, BroKind::Anon))
         } else {
-            self.anon_bros.remove(&anon.local)
+            self.borrows.get(&anon.local).copied()
         }
     }
 
-    pub fn add_assign(&mut self, bb: BasicBlock) {
+    pub fn add_assign(&mut self, _bb: BasicBlock) {
         self.state = State::Filled;
-        self.assignments.push(bb);
     }
 }
 
@@ -177,10 +208,10 @@ impl<'a, 'tcx> MyStateInfo<super::OwnedAnalysis<'a, 'tcx>> for StateInfo<'tcx> {
 
         // FIXME: Here we can have collisions where two anons reference different places... oh no...
         let anons_previous_len = self.anons.len();
-        let anon_bros_previous_len = self.anon_bros.len();
+        let anon_bros_previous_len = self.borrows.len();
         self.anons.extend(other.anons.iter());
-        self.anon_bros.extend(other.anon_bros.iter());
-        changed |= (self.anons.len() != anons_previous_len) || (self.anon_bros.len() != anon_bros_previous_len);
+        self.borrows.extend(other.borrows.iter());
+        changed |= (self.anons.len() != anons_previous_len) || (self.borrows.len() != anon_bros_previous_len);
 
         assert!(!(self.phase_borrow.is_some() && other.phase_borrow.is_some()));
         if let Some(data) = other.phase_borrow {

tests/ui/thesis/owned.rs

@@ -38,7 +38,7 @@ fn pat_maybe_return_owned_arg_1_test(a: u32) -> u32 {
 }
 
 #[forbid(clippy::borrow_pats)]
-/// FIXME: The argument return is not yet detected both in `a` and `Return`
+/// FIXME: The argument return is not yet detected both in `a`
 fn pat_maybe_return_owned_arg_2(a: String) -> String {
     let ret;
     if !a.is_empty() {