Address #9386 (comment)

Author: smoelius

clippy_lints/src/dereference.rs

@@ -1,5 +1,7 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
-use clippy_utils::mir::{dropped_without_further_use, enclosing_mir, expr_local, local_assignments};
+use clippy_utils::mir::{
+    dropped_without_further_use, enclosing_mir, expr_local, local_assignments, PossibleBorrowerMap,
+};
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::has_enclosing_paren;
 use clippy_utils::ty::{expr_sig, is_copy, peel_mid_ty_refs, ty_sig, variant_of_res};
@@ -1080,14 +1082,19 @@ fn needless_borrow_impl_arg_position<'tcx>(
         return Position::Other(precedence);
     }
 
+    let mut possible_borrower = None;
+
     // `substs_with_referent_ty` can be constructed outside of `check_referent` because the same
     // elements are modified each time `check_referent` is called.
     let mut substs_with_referent_ty = substs_with_expr_ty.to_vec();
 
     let mut check_reference_and_referent = |reference, referent| {
         let referent_ty = cx.typeck_results().expr_ty(referent);
 
-        if !(is_copy(cx, referent_ty) || referent_dropped_without_further_use(cx.tcx, reference)) {
+        if !is_copy(cx, referent_ty)
+            && (referent_ty.has_significant_drop(cx.tcx, cx.param_env)
+                || !referent_dropped_without_further_use(cx, &mut possible_borrower, reference))
+        {
             return false;
         }
 
@@ -1157,15 +1164,24 @@ fn has_ref_mut_self_method(cx: &LateContext<'_>, trait_def_id: DefId) -> bool {
         })
 }
 
-fn referent_dropped_without_further_use(tcx: TyCtxt<'_>, reference: &Expr<'_>) -> bool {
-    let mir = enclosing_mir(tcx, reference.hir_id);
-    if let Some(local) = expr_local(tcx, reference)
+fn referent_dropped_without_further_use<'a, 'tcx>(
+    cx: &'a LateContext<'tcx>,
+    possible_borrower: &mut Option<PossibleBorrowerMap<'a, 'tcx>>,
+    reference: &Expr<'tcx>,
+) -> bool {
+    let mir = enclosing_mir(cx.tcx, reference.hir_id);
+    if let Some(local) = expr_local(cx.tcx, reference)
         && let [location] = *local_assignments(mir, local).as_slice()
         && let StatementKind::Assign(box (_, Rvalue::Ref(_, _, place))) =
             mir.basic_blocks[location.block].statements[location.statement_index].kind
         && !place.has_deref()
     {
-        dropped_without_further_use(mir, place.local, location).unwrap_or(false)
+        let possible_borrower = possible_borrower.get_or_insert_with(|| PossibleBorrowerMap::new(cx, mir));
+        // If `only_borrowers` were used here, the `copyable_iterator::warn` test would fail. The reason is
+        // that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible borrower of
+        // itself. See the comment in that method for an explanation as to why.
+        possible_borrower.bounded_borrowers(&[local], &[local, place.local], place.local, location)
+            && dropped_without_further_use(mir, place.local, location).unwrap_or(false)
     } else {
         false
     }

clippy_lints/src/lib.rs

@@ -38,7 +38,6 @@ extern crate rustc_infer;
 extern crate rustc_lexer;
 extern crate rustc_lint;
 extern crate rustc_middle;
-extern crate rustc_mir_dataflow;
 extern crate rustc_parse;
 extern crate rustc_session;
 extern crate rustc_span;

clippy_lints/src/redundant_clone/mod.rs renamed to clippy_lints/src/redundant_clone.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::{span_lint_hir, span_lint_hir_and_then};
-use clippy_utils::mir::{visit_local_usage, LocalUsage};
+use clippy_utils::mir::{visit_local_usage, LocalUsage, PossibleBorrowerMap};
 use clippy_utils::source::snippet_opt;
 use clippy_utils::ty::{has_drop, is_copy, is_type_diagnostic_item, walk_ptrs_ty_depth};
 use clippy_utils::{fn_has_unsatisfiable_preds, match_def_path, paths};
@@ -8,24 +8,12 @@ use rustc_errors::Applicability;
 use rustc_hir::intravisit::FnKind;
 use rustc_hir::{def_id, Body, FnDecl, HirId};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::mir::{self, visit::Visitor as _};
+use rustc_middle::mir;
 use rustc_middle::ty::{self, Ty};
-use rustc_mir_dataflow::Analysis;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::{BytePos, Span};
 use rustc_span::sym;
 
-mod maybe_storage_live;
-use maybe_storage_live::MaybeStorageLive;
-
-mod possible_borrower;
-use possible_borrower::PossibleBorrowerVisitor;
-
-mod possible_origin;
-use possible_origin::PossibleOriginVisitor;
-
-mod transitive_relation;
-
 macro_rules! unwrap_or_continue {
     ($x:expr) => {
         match $x {
@@ -94,21 +82,7 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone {
 
         let mir = cx.tcx.optimized_mir(def_id.to_def_id());
 
-        let possible_origin = {
-            let mut vis = PossibleOriginVisitor::new(mir);
-            vis.visit_body(mir);
-            vis.into_map(cx)
-        };
-        let maybe_storage_live_result = MaybeStorageLive
-            .into_engine(cx.tcx, mir)
-            .pass_name("redundant_clone")
-            .iterate_to_fixpoint()
-            .into_results_cursor(mir);
-        let mut possible_borrower = {
-            let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_origin);
-            vis.visit_body(mir);
-            vis.into_map(cx, maybe_storage_live_result)
-        };
+        let mut possible_borrower = PossibleBorrowerMap::new(cx, mir);
 
         for (bb, bbdata) in mir.basic_blocks.iter_enumerated() {
             let terminator = bbdata.terminator();

clippy_utils/src/lib.rs

@@ -24,10 +24,12 @@ extern crate rustc_data_structures;
 extern crate rustc_errors;
 extern crate rustc_hir;
 extern crate rustc_hir_analysis;
+extern crate rustc_index;
 extern crate rustc_infer;
 extern crate rustc_lexer;
 extern crate rustc_lint;
 extern crate rustc_middle;
+extern crate rustc_mir_dataflow;
 extern crate rustc_parse_format;
 extern crate rustc_session;
 extern crate rustc_span;

clippy_lints/src/redundant_clone/maybe_storage_live.rs renamed to clippy_utils/src/mir/maybe_storage_live.rs

@@ -4,7 +4,7 @@ use rustc_mir_dataflow::{AnalysisDomain, CallReturnPlaces, GenKill, GenKillAnaly
 
 /// Determines liveness of each local purely based on `StorageLive`/`Dead`.
 #[derive(Copy, Clone)]
-pub struct MaybeStorageLive;
+pub(super) struct MaybeStorageLive;
 
 impl<'tcx> AnalysisDomain<'tcx> for MaybeStorageLive {
     type Domain = BitSet<mir::Local>;

clippy_utils/src/mir.rs renamed to clippy_utils/src/mir/mod.rs

@@ -3,6 +3,15 @@ use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceC
 use rustc_middle::mir::{traversal, Body, Local, Location, Place, StatementKind};
 use rustc_middle::ty::TyCtxt;
 
+mod maybe_storage_live;
+
+mod possible_borrower;
+pub use possible_borrower::PossibleBorrowerMap;
+
+mod possible_origin;
+
+mod transitive_relation;
+
 #[derive(Clone, Default)]
 pub struct LocalUsage {
     /// The first location where the local is used, if any.

clippy_lints/src/redundant_clone/possible_borrower.rs renamed to clippy_utils/src/mir/possible_borrower.rs

@@ -1,26 +1,29 @@
-use super::{maybe_storage_live::MaybeStorageLive, transitive_relation::TransitiveRelation};
-use clippy_utils::ty::is_copy;
+use super::{
+    maybe_storage_live::MaybeStorageLive, possible_origin::PossibleOriginVisitor,
+    transitive_relation::TransitiveRelation,
+};
+use crate::ty::is_copy;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::bit_set::{BitSet, HybridBitSet};
 use rustc_lint::LateContext;
-use rustc_middle::mir::{self, Mutability};
+use rustc_middle::mir::{self, visit::Visitor as _, Mutability};
 use rustc_middle::ty::{self, visit::TypeVisitor};
-use rustc_mir_dataflow::ResultsCursor;
+use rustc_mir_dataflow::{Analysis, ResultsCursor};
 use std::ops::ControlFlow;
 
 /// Collects the possible borrowers of each local.
 /// For example, `b = &a; c = &a;` will make `b` and (transitively) `c`
 /// possible borrowers of `a`.
 #[allow(clippy::module_name_repetitions)]
-pub struct PossibleBorrowerVisitor<'a, 'tcx> {
+struct PossibleBorrowerVisitor<'a, 'tcx> {
     possible_borrower: TransitiveRelation,
     body: &'a mir::Body<'tcx>,
     cx: &'a LateContext<'tcx>,
     possible_origin: FxHashMap<mir::Local, HybridBitSet<mir::Local>>,
 }
 
 impl<'a, 'tcx> PossibleBorrowerVisitor<'a, 'tcx> {
-    pub fn new(
+    fn new(
         cx: &'a LateContext<'tcx>,
         body: &'a mir::Body<'tcx>,
         possible_origin: FxHashMap<mir::Local, HybridBitSet<mir::Local>>,
@@ -33,10 +36,10 @@ impl<'a, 'tcx> PossibleBorrowerVisitor<'a, 'tcx> {
         }
     }
 
-    pub fn into_map(
+    fn into_map(
         self,
         cx: &LateContext<'tcx>,
-        maybe_live: ResultsCursor<'tcx, 'tcx, MaybeStorageLive>,
+        maybe_live: ResultsCursor<'a, 'tcx, MaybeStorageLive>,
     ) -> PossibleBorrowerMap<'a, 'tcx> {
         let mut map = FxHashMap::default();
         for row in (1..self.body.local_decls.len()).map(mir::Local::from_usize) {
@@ -172,9 +175,37 @@ pub struct PossibleBorrowerMap<'a, 'tcx> {
     bitset: (BitSet<mir::Local>, BitSet<mir::Local>),
 }
 
-impl PossibleBorrowerMap<'_, '_> {
+impl<'a, 'tcx> PossibleBorrowerMap<'a, 'tcx> {
+    pub fn new(cx: &'a LateContext<'tcx>, mir: &'a mir::Body<'tcx>) -> Self {
+        let possible_origin = {
+            let mut vis = PossibleOriginVisitor::new(mir);
+            vis.visit_body(mir);
+            vis.into_map(cx)
+        };
+        let maybe_storage_live_result = MaybeStorageLive
+            .into_engine(cx.tcx, mir)
+            .pass_name("redundant_clone")
+            .iterate_to_fixpoint()
+            .into_results_cursor(mir);
+        let mut vis = PossibleBorrowerVisitor::new(cx, mir, possible_origin);
+        vis.visit_body(mir);
+        vis.into_map(cx, maybe_storage_live_result)
+    }
+
     /// Returns true if the set of borrowers of `borrowed` living at `at` matches with `borrowers`.
     pub fn only_borrowers(&mut self, borrowers: &[mir::Local], borrowed: mir::Local, at: mir::Location) -> bool {
+        self.bounded_borrowers(borrowers, borrowers, borrowed, at)
+    }
+
+    /// Returns true if the set of borrowers of `borrowed` living at `at` includes at least `below`
+    /// but no more than `above`.
+    pub fn bounded_borrowers(
+        &mut self,
+        below: &[mir::Local],
+        above: &[mir::Local],
+        borrowed: mir::Local,
+        at: mir::Location,
+    ) -> bool {
         self.maybe_live.seek_after_primary_effect(at);
 
         self.bitset.0.clear();
@@ -188,11 +219,19 @@ impl PossibleBorrowerMap<'_, '_> {
         }
 
         self.bitset.1.clear();
-        for b in borrowers {
+        for b in below {
             self.bitset.1.insert(*b);
         }
 
-        self.bitset.0 == self.bitset.1
+        if !self.bitset.0.superset(&self.bitset.1) {
+            return false;
+        }
+
+        for b in above {
+            self.bitset.0.remove(*b);
+        }
+
+        self.bitset.0.is_empty()
     }
 
     pub fn local_is_alive_at(&mut self, local: mir::Local, at: mir::Location) -> bool {

clippy_lints/src/redundant_clone/possible_origin.rs renamed to clippy_utils/src/mir/possible_origin.rs

@@ -1,5 +1,5 @@
 use super::transitive_relation::TransitiveRelation;
-use clippy_utils::ty::is_copy;
+use crate::ty::is_copy;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::bit_set::HybridBitSet;
 use rustc_lint::LateContext;
@@ -9,7 +9,7 @@ use rustc_middle::mir;
 /// For example, `_1 = &mut _2` generate _1: {_2,...}
 /// Known Problems: not sure all borrowed are tracked
 #[allow(clippy::module_name_repetitions)]
-pub struct PossibleOriginVisitor<'a, 'tcx> {
+pub(super) struct PossibleOriginVisitor<'a, 'tcx> {
     possible_origin: TransitiveRelation,
     body: &'a mir::Body<'tcx>,
 }

clippy_lints/src/redundant_clone/transitive_relation.rs renamed to clippy_utils/src/mir/transitive_relation.rs

@@ -3,7 +3,7 @@ use rustc_index::bit_set::HybridBitSet;
 use rustc_middle::mir;
 
 #[derive(Default)]
-pub struct TransitiveRelation {
+pub(super) struct TransitiveRelation {
     relations: FxHashMap<mir::Local, Vec<mir::Local>>,
 }
 

tests/ui/needless_borrow.fixed

@@ -340,5 +340,26 @@ fn closure_test() {
         let _ = std::fs::write("x", arg);
         let _ = std::fs::write("x", loc);
     };
+    let _ = std::fs::write("x", &env); // Don't lint. Borrowed by `f`
     f(arg);
 }
+
+#[allow(dead_code)]
+mod significant_drop {
+    #[derive(Debug)]
+    struct X;
+
+    #[derive(Debug)]
+    struct Y;
+
+    impl Drop for Y {
+        fn drop(&mut self) {}
+    }
+
+    fn foo(x: X, y: Y) {
+        debug(x);
+        debug(&y); // Don't lint. Has significant drop
+    }
+
+    fn debug(_: impl std::fmt::Debug) {}
+}