Check arms for significant drops

Author: PrestonFrom

clippy_lints/src/matches/mod.rs

@@ -963,7 +963,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
                 return;
             }
             if matches!(source, MatchSource::Normal | MatchSource::ForLoopDesugar) {
-                significant_drop_in_scrutinee::check(cx, expr, ex, source);
+                significant_drop_in_scrutinee::check(cx, expr, ex, arms, source);
             }
 
             collapsible_match::check_match(cx, arms);

clippy_lints/src/matches/significant_drop_in_scrutinee.rs

@@ -4,18 +4,19 @@ use clippy_utils::get_attr;
 use clippy_utils::source::{indent_of, snippet};
 use rustc_errors::{Applicability, Diagnostic};
 use rustc_hir::intravisit::{walk_expr, Visitor};
-use rustc_hir::{Expr, ExprKind, MatchSource};
+use rustc_hir::{Arm, Expr, ExprKind, MatchSource};
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::ty::subst::GenericArgKind;
 use rustc_middle::ty::{Ty, TypeAndMut};
-use rustc_span::Span;
+use rustc_span::{Span};
 
 use super::SIGNIFICANT_DROP_IN_SCRUTINEE;
 
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx Expr<'tcx>,
     scrutinee: &'tcx Expr<'_>,
+    arms: &'tcx [Arm<'_>],
     source: MatchSource,
 ) {
     if let Some((suggestions, message)) = has_significant_drop_in_scrutinee(cx, scrutinee, source) {
@@ -25,8 +26,14 @@ pub(super) fn check<'tcx>(
                 let s = Span::new(expr.span.hi(), expr.span.hi(), expr.span.ctxt(), None);
                 diag.span_label(
                     s,
-                    "temporary lives until here. If mutex is locked within block, deadlocks can occur.",
+                    "original temporary lives until here",
                 );
+                if let Some(spans) = has_significant_drop_in_arms(cx, arms) {
+                    for span in spans {
+                        diag.span_label(span, "significant drop in arm here");
+                    }
+                    diag.note("this might lead to deadlocks or other unexpected behavior");
+                }
             });
         }
     }
@@ -93,14 +100,69 @@ fn has_significant_drop_in_scrutinee<'tcx, 'a>(
     })
 }
 
+struct SigDropChecker<'a, 'tcx> {
+    seen_types: FxHashSet<Ty<'tcx>>,
+    cx: &'a LateContext<'tcx>,
+}
+
+impl<'a, 'tcx> SigDropChecker<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'tcx>) -> SigDropChecker<'a, 'tcx> {
+        SigDropChecker {
+            seen_types: Default::default(),
+            cx
+        }
+    }
+
+    fn get_type(&self, ex: &'tcx Expr<'_>) -> Ty<'tcx> {
+        self.cx.typeck_results().expr_ty(ex)
+    }
+
+    fn has_seen_type(&mut self, ty: Ty<'tcx>) -> bool {
+        !self.seen_types.insert(ty)
+    }
+
+    fn has_sig_drop_attr(&mut self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+        if let Some(adt) = ty.ty_adt_def() {
+            if get_attr(cx.sess(), cx.tcx.get_attrs_unchecked(adt.did()), "has_significant_drop").count() > 0 {
+                return true;
+            }
+        }
+
+        match ty.kind() {
+            rustc_middle::ty::Adt(a, b) => {
+                for f in a.all_fields() {
+                    let ty = f.ty(cx.tcx, b);
+                    if !self.has_seen_type(ty) && self.has_sig_drop_attr(cx, ty) {
+                        return true;
+                    }
+                }
+
+                for generic_arg in b.iter() {
+                    if let GenericArgKind::Type(ty) = generic_arg.unpack() {
+                        if self.has_sig_drop_attr(cx, ty) {
+                            return true;
+                        }
+                    }
+                }
+                false
+            },
+            rustc_middle::ty::Array(ty, _)
+            | rustc_middle::ty::RawPtr(TypeAndMut { ty, .. })
+            | rustc_middle::ty::Ref(_, ty, _)
+            | rustc_middle::ty::Slice(ty) => self.has_sig_drop_attr(cx, *ty),
+            _ => false,
+        }
+    }
+}
+
 struct SigDropHelper<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
     is_chain_end: bool,
-    seen_types: FxHashSet<Ty<'tcx>>,
     has_significant_drop: bool,
     current_sig_drop: Option<FoundSigDrop>,
     sig_drop_spans: Option<Vec<FoundSigDrop>>,
     special_handling_for_binary_op: bool,
+    sig_drop_checker: SigDropChecker<'a, 'tcx>
 }
 
 #[expect(clippy::enum_variant_names)]
@@ -123,11 +185,11 @@ impl<'a, 'tcx> SigDropHelper<'a, 'tcx> {
         SigDropHelper {
             cx,
             is_chain_end: true,
-            seen_types: FxHashSet::default(),
             has_significant_drop: false,
             current_sig_drop: None,
             sig_drop_spans: None,
             special_handling_for_binary_op: false,
+            sig_drop_checker: SigDropChecker::new(cx)
         }
     }
 
@@ -168,7 +230,7 @@ impl<'a, 'tcx> SigDropHelper<'a, 'tcx> {
         if self.current_sig_drop.is_some() {
             return;
         }
-        let ty = self.get_type(expr);
+        let ty = self.sig_drop_checker.get_type(expr);
         if ty.is_ref() {
             // We checked that the type was ref, so builtin_deref will return Some TypeAndMut,
             // but let's avoid any chance of an ICE
@@ -192,14 +254,6 @@ impl<'a, 'tcx> SigDropHelper<'a, 'tcx> {
         }
     }
 
-    fn get_type(&self, ex: &'tcx Expr<'_>) -> Ty<'tcx> {
-        self.cx.typeck_results().expr_ty(ex)
-    }
-
-    fn has_seen_type(&mut self, ty: Ty<'tcx>) -> bool {
-        !self.seen_types.insert(ty)
-    }
-
     fn visit_exprs_for_binary_ops(
         &mut self,
         left: &'tcx Expr<'_>,
@@ -220,43 +274,12 @@ impl<'a, 'tcx> SigDropHelper<'a, 'tcx> {
         self.special_handling_for_binary_op = false;
     }
 
-    fn has_sig_drop_attr(&mut self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
-        if let Some(adt) = ty.ty_adt_def() {
-            if get_attr(cx.sess(), cx.tcx.get_attrs_unchecked(adt.did()), "has_significant_drop").count() > 0 {
-                return true;
-            }
-        }
 
-        match ty.kind() {
-            rustc_middle::ty::Adt(a, b) => {
-                for f in a.all_fields() {
-                    let ty = f.ty(cx.tcx, b);
-                    if !self.has_seen_type(ty) && self.has_sig_drop_attr(cx, ty) {
-                        return true;
-                    }
-                }
-
-                for generic_arg in b.iter() {
-                    if let GenericArgKind::Type(ty) = generic_arg.unpack() {
-                        if self.has_sig_drop_attr(cx, ty) {
-                            return true;
-                        }
-                    }
-                }
-                false
-            },
-            rustc_middle::ty::Array(ty, _)
-            | rustc_middle::ty::RawPtr(TypeAndMut { ty, .. })
-            | rustc_middle::ty::Ref(_, ty, _)
-            | rustc_middle::ty::Slice(ty) => self.has_sig_drop_attr(cx, *ty),
-            _ => false,
-        }
-    }
 }
 
 impl<'a, 'tcx> Visitor<'tcx> for SigDropHelper<'a, 'tcx> {
     fn visit_expr(&mut self, ex: &'tcx Expr<'_>) {
-        if !self.is_chain_end && self.has_sig_drop_attr(self.cx, self.get_type(ex)) {
+        if !self.is_chain_end && self.sig_drop_checker.has_sig_drop_attr(self.cx, self.sig_drop_checker.get_type(ex)) {
             self.has_significant_drop = true;
             return;
         }
@@ -334,4 +357,40 @@ impl<'a, 'tcx> Visitor<'tcx> for SigDropHelper<'a, 'tcx> {
             self.try_setting_current_suggestion(ex, false);
         }
     }
+
+}
+
+struct ArmSigDropHelper<'a, 'tcx> {
+    sig_drop_checker: SigDropChecker<'a, 'tcx>,
+    found_sig_drop_spans: Option<FxHashSet<Span>>
+}
+
+impl<'a, 'tcx> ArmSigDropHelper<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'tcx>) -> ArmSigDropHelper<'a, 'tcx> {
+        ArmSigDropHelper {
+            sig_drop_checker: SigDropChecker::new(cx),
+            found_sig_drop_spans: None
+        }
+    }
+}
+
+fn has_significant_drop_in_arms<'tcx, 'a>(
+    cx: &'a LateContext<'tcx>,
+    arms: &'tcx [Arm<'_>],
+) -> Option<FxHashSet<Span>> {
+    let mut helper = ArmSigDropHelper::new(cx);
+    for arm in arms {
+        helper.visit_expr(arm.body);
+    }
+    helper.found_sig_drop_spans
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for ArmSigDropHelper<'a, 'tcx> {
+    fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
+        if self.sig_drop_checker.has_sig_drop_attr(self.sig_drop_checker.cx, self.sig_drop_checker.get_type(ex)) {
+            self.found_sig_drop_spans.get_or_insert_with(FxHashSet::default).insert(ex.span);
+            return;
+        }
+        walk_expr(self, ex);
+    }
 }

tests/ui/significant_drop_in_scrutinee.rs

@@ -591,4 +591,20 @@ fn should_trigger_lint_for_read_write_lock_for_loop() {
     }
 }
 
+fn do_bar(mutex: &Mutex<State>) {
+    mutex.lock().unwrap().bar();
+}
+
+fn should_trigger_lint_without_significant_drop_in_arm() {
+    let mutex = Mutex::new(State {});
+
+    // Should trigger lint because the lifetime of the temporary MutexGuard is surprising because it
+    // is preserved until the end of the match, but there is no clear indication that this is the
+    // case.
+    match mutex.lock().unwrap().foo() {
+        true => do_bar(&mutex),
+        false => {},
+    };
+}
+
 fn main() {}