Auto merge of #8981 - PrestonFrom:more_details_for_significant_drop_lint, r=flip1995

Add details about how significant drop in match scrutinees can cause deadlocks

Adds more details about how a significant drop in a match scrutinee can cause a deadlock and include link to documentation.

changelog: Add more details to significant drop lint to explicitly show how temporaries in match scrutinees can cause deadlocks.

Author: bors

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

@@ -1,10 +1,10 @@
 use crate::FxHashSet;
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::get_attr;
 use clippy_utils::source::{indent_of, snippet};
+use clippy_utils::{get_attr, is_lint_allowed};
 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};
@@ -16,12 +16,23 @@ pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx Expr<'tcx>,
     scrutinee: &'tcx Expr<'_>,
+    arms: &'tcx [Arm<'_>],
     source: MatchSource,
 ) {
+    if is_lint_allowed(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, expr.hir_id) {
+        return;
+    }
+
     if let Some((suggestions, message)) = has_significant_drop_in_scrutinee(cx, scrutinee, source) {
         for found in suggestions {
             span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| {
                 set_diagnostic(diag, cx, expr, found);
+                let s = Span::new(expr.span.hi(), expr.span.hi(), expr.span.ctxt(), None);
+                diag.span_label(s, "temporary lives until here");
+                for span in has_significant_drop_in_arms(cx, arms) {
+                    diag.span_label(span, "another value with significant `Drop` created here");
+                }
+                diag.note("this might lead to deadlocks or other unexpected behavior");
             });
         }
     }
@@ -80,22 +91,77 @@ fn has_significant_drop_in_scrutinee<'tcx, 'a>(
     let mut helper = SigDropHelper::new(cx);
     helper.find_sig_drop(scrutinee).map(|drops| {
         let message = if source == MatchSource::Normal {
-            "temporary with significant drop in match scrutinee"
+            "temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression"
         } else {
-            "temporary with significant drop in for loop"
+            "temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression"
         };
         (drops, message)
     })
 }
 
+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: FxHashSet::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)]
@@ -118,11 +184,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),
         }
     }
 
@@ -163,7 +229,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
@@ -187,14 +253,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<'_>,
@@ -214,44 +272,15 @@ 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;
         }
@@ -330,3 +359,38 @@ impl<'a, 'tcx> Visitor<'tcx> for SigDropHelper<'a, 'tcx> {
         }
     }
 }
+
+struct ArmSigDropHelper<'a, 'tcx> {
+    sig_drop_checker: SigDropChecker<'a, 'tcx>,
+    found_sig_drop_spans: 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: FxHashSet::<Span>::default(),
+        }
+    }
+}
+
+fn has_significant_drop_in_arms<'tcx, 'a>(cx: &'a LateContext<'tcx>, arms: &'tcx [Arm<'_>]) -> 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.insert(ex.span);
+            return;
+        }
+        walk_expr(self, ex);
+    }
+}

tests/ui/significant_drop_in_scrutinee.rs

@@ -64,6 +64,19 @@ fn should_trigger_lint_with_mutex_guard_in_match_scrutinee() {
     };
 }
 
+fn should_not_trigger_lint_with_mutex_guard_in_match_scrutinee_when_lint_allowed() {
+    let mutex = Mutex::new(State {});
+
+    // Lint should not be triggered because it is "allowed" below.
+    #[allow(clippy::significant_drop_in_scrutinee)]
+    match mutex.lock().unwrap().foo() {
+        true => {
+            mutex.lock().unwrap().bar();
+        },
+        false => {},
+    };
+}
+
 fn should_not_trigger_lint_for_insignificant_drop() {
     // Should not trigger lint because there are no temporaries whose drops have a significant
     // side effect.
@@ -591,4 +604,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() {}