Handle double refs correctly

xFrednet · Apr 29, 2024 · dcafa28 · dcafa28
1 parent 931312f
commit dcafa28
Show file tree

Hide file tree

Showing 4 changed files with 269 additions and 83 deletions.
diff --git a/clippy_lints/src/borrow_pats/owned.rs b/clippy_lints/src/borrow_pats/owned.rs
@@ -533,25 +533,18 @@ impl<'a, 'tcx> OwnedAnalysis<'a, 'tcx> {
                             self.pats.insert(OwnedPat::ManualDrop);
                         }
                     } else if let Some(bro_info) = self.states[bb].has_bro(&arg) {
-                        // let mut inner_mut = None;
-                        // for_each_ref_region(info.body.local_decls[dst.local].ty, &mut |_reg, _ty, mutability| {
-                        //     inner_mut = Some(mutability);
-                        // });
-                        // let inner_mut = inner_mut.unwrap();
-                        // TODO
-                        let inner_mut = bro_info.muta;
 
                         // 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, bro_info.broker, inner_mut))
+                                    .then_some((*local, bro_info.broker, bro_info.muta))
                             }));
                             dep_loans_len != dep_loans.len()
                         };
 
-                        match inner_mut {
+                        match bro_info.muta {
                             Mutability::Not => {
                                 immut_bros.push(bro_info.broker);
 

diff --git a/clippy_lints/src/borrow_pats/owned/state.rs b/clippy_lints/src/borrow_pats/owned/state.rs
@@ -1,7 +1,5 @@
 #![warn(unused)]
 
-use clippy_utils::ty::for_each_ref_region;
-
 use crate::borrow_pats::MyStateInfo;
 
 use super::super::prelude::*;
@@ -237,7 +235,6 @@ impl<'tcx> StateInfo<'tcx> {
             // ignore it here :D
             self.borrows
                 .insert(borrow.local, BorrowInfo::new(broker, kind.mutability(), bro_kind));
-            // todo!("Named Local: {borrow:#?} = {broker:#?}\n{self:#?}");
         }
     }
 
@@ -249,7 +246,7 @@ impl<'tcx> StateInfo<'tcx> {
         info: &AnalysisInfo<'tcx>,
         pats: &mut BTreeSet<OwnedPat>,
     ) {
-        self.add_ref_dep(dst, src, true, info, pats)
+        self.add_ref_dep(dst, src, info, pats)
     }
     /// This function informs the state that a ref to a ref was created
     pub fn add_ref_ref(
@@ -259,14 +256,13 @@ impl<'tcx> StateInfo<'tcx> {
         info: &AnalysisInfo<'tcx>,
         pats: &mut BTreeSet<OwnedPat>,
     ) {
-        self.add_ref_dep(dst, src, false, info, pats)
+        self.add_ref_dep(dst, src, info, pats)
     }
     /// If `kind` is empty it indicates that the mutability of `src`` should be taken
     fn add_ref_dep(
         &mut self,
         dst: Place<'tcx>,
         src: Place<'tcx>,
-        is_copy: bool,
         info: &AnalysisInfo<'tcx>,
         pats: &mut BTreeSet<OwnedPat>,
     ) {
@@ -283,7 +279,6 @@ impl<'tcx> StateInfo<'tcx> {
         //
         // Looking at `temp_borrow_mixed_2` it seems like the copy mutability depends
         // on the use case. I'm not even disappointed anymore
-        let prev_muta = bro_info.muta;
         match bro_info.kind {
             BroKind::Dep | BroKind::Named => {
                 // FIXME: Maybe this doesn't even needs to be tracked?
@@ -294,13 +289,7 @@ impl<'tcx> StateInfo<'tcx> {
                 let is_named = matches!(info.locals[&dst.local].kind, LocalKind::UserVar(..));
                 if is_named {
                     // FIXME: THis is broken:
-                    let mut inner_mut = None;
-                    for_each_ref_region(info.body.local_decls[dst.local].ty, &mut |_reg, _ty, mutability| {
-                        inner_mut = Some(mutability);
-                    });
-                    let inner_mut = inner_mut.unwrap();
-
-                    if matches!(inner_mut, Mutability::Mut) {
+                    if matches!(bro_info.muta, Mutability::Mut) {
                         info.stats.borrow_mut().owned.named_borrow_mut_count += 1;
                         pats.insert(OwnedPat::NamedBorrowMut);
                     } else {
@@ -311,10 +300,7 @@ impl<'tcx> StateInfo<'tcx> {
 
                 let new_bro_kind = if is_named { BroKind::Named } else { BroKind::Anon };
 
-                // `copy_kind` can only be mutable if `src` is also mutable
-                let new_muta = if is_copy { prev_muta } else { Mutability::Not };
-                self.borrows
-                    .insert(dst.local, BorrowInfo::new(bro_info.broker, new_muta, new_bro_kind));
+                self.borrows.insert(dst.local, bro_info.copy_with(new_bro_kind));
             },
         }
     }

diff --git a/tests/ui/thesis/pass/owned_borrows.rs b/tests/ui/thesis/pass/owned_borrows.rs
@@ -6,82 +6,82 @@ struct Animal {
     simple_name: String,
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_1(a: String) -> bool {
     a.is_empty()
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_2(a: String) {
     take_1_loan(&a);
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_3(a: String) {
     take_2_loan(&a, &a);
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_mut_1(mut a: String) {
     a.clear();
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_mut_2(mut a: String) {
     take_1_mut_loan(&mut a);
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_mut_3(mut a: Animal) {
     take_2_mut_loan(&mut a.science_name, &mut a.simple_name);
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_mixed(mut a: String) {
     take_1_mut_loan(&mut a);
     take_1_loan(&a);
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn temp_borrow_mixed_2(mut a: Animal) {
     take_2_mixed_loan(&a.science_name, &mut a.simple_name);
 }
 
 /// https://github.com/nikomatsakis/nll-rfc/issues/37
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn two_phase_borrow_1(mut vec: Vec<usize>) {
     vec.push(vec.len());
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn two_phase_borrow_2(mut num: usize, mut vec: Vec<usize>) {
     vec.push({
         num = vec.len();
         num
     })
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn nested_two_phase_borrow(mut vecs: NestedVecs) {
     vecs.a.push({
         vecs.b.push(vecs.a.len());
         vecs.b.len()
     });
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn test_double_loan() {
     let data = "Side effects".to_string();
     take_double_loan(&&data);
 }
 
-#[forbid(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn test_double_mut_loan() {
     let mut data = "Can Side effects".to_string();
     take_double_mut_loan(&&mut data);
 }
 
-// #[warn(clippy::borrow_pats)]
+#[warn(clippy::borrow_pats)]
 fn test_mut_double_loan() {
     let data = "Can't really have Side effects".to_string();
     take_mut_double_loan(&mut &data);