Auto merge of rust-lang#12870 - lrh2000:sig-drop-while, r=blyxyas

`significant_drop_in_scrutinee`: Trigger lint also for scrutinees in `while let` and `if let`

This lint should also work for `if let` and `while let`, so this PR makes it actually work.

For `while let`, I can't think of any reason why this lint shouldn't be enabled. The only problem is that the lint suggests moving the significant drop above the `while let`, which is clearly invalid in the case of `while let`. I don't know if this is fixable, but this PR simply disables the wrong suggestions.

For `if let`, it seems that another lint called `if_let_mutex` has some overlapping functionality. But `significant_drop_in_scrutinee` is a bit stricter, as it will trigger even if the `else` branch does not try to lock the same mutex.

changelog: [`significant_drop_in_scrutinee`]: Trigger lint also for scrutinees in `while let` and `if let`.

r? `@blyxyas` (the third PR as promised in rust-lang/rust-clippy#12740 (comment), thanks for your review!)

Author: bors

clippy_lints/src/matches/mod.rs

@@ -1019,6 +1019,7 @@ impl_lint_pass!(Matches => [
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Matches {
+    #[expect(clippy::too_many_lines)]
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if is_direct_expn_of(expr.span, "matches").is_none() && in_external_macro(cx.sess(), expr.span) {
             return;
@@ -1037,7 +1038,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
                 return;
             }
             if matches!(source, MatchSource::Normal | MatchSource::ForLoopDesugar) {
-                significant_drop_in_scrutinee::check(cx, expr, ex, arms, source);
+                significant_drop_in_scrutinee::check_match(cx, expr, ex, arms, source);
             }
 
             collapsible_match::check_match(cx, arms, &self.msrv);
@@ -1084,6 +1085,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
             }
         } else if let Some(if_let) = higher::IfLet::hir(cx, expr) {
             collapsible_match::check_if_let(cx, if_let.let_pat, if_let.if_then, if_let.if_else, &self.msrv);
+            significant_drop_in_scrutinee::check_if_let(cx, expr, if_let.let_expr, if_let.if_then, if_let.if_else);
             if !from_expansion {
                 if let Some(else_expr) = if_let.if_else {
                     if self.msrv.meets(msrvs::MATCHES_MACRO) {
@@ -1126,8 +1128,13 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
                 );
                 needless_match::check_if_let(cx, expr, &if_let);
             }
-        } else if !from_expansion {
-            redundant_pattern_match::check(cx, expr);
+        } else {
+            if let Some(while_let) = higher::WhileLet::hir(expr) {
+                significant_drop_in_scrutinee::check_while_let(cx, expr, while_let.let_expr, while_let.if_then);
+            }
+            if !from_expansion {
+                redundant_pattern_match::check(cx, expr);
+            }
         }
     }
 

clippy_lints/src/matches/significant_drop_in_scrutinee.rs

@@ -16,7 +16,7 @@ use rustc_span::Span;
 
 use super::SIGNIFICANT_DROP_IN_SCRUTINEE;
 
-pub(super) fn check<'tcx>(
+pub(super) fn check_match<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx Expr<'tcx>,
     scrutinee: &'tcx Expr<'_>,
@@ -27,10 +27,89 @@ pub(super) fn check<'tcx>(
         return;
     }
 
-    let (suggestions, message) = has_significant_drop_in_scrutinee(cx, scrutinee, source);
+    let scrutinee = match (source, &scrutinee.kind) {
+        (MatchSource::ForLoopDesugar, ExprKind::Call(_, [e])) => e,
+        _ => scrutinee,
+    };
+
+    let message = if source == MatchSource::Normal {
+        "temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression"
+    } else {
+        "temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression"
+    };
+
+    let arms = arms.iter().map(|arm| arm.body).collect::<Vec<_>>();
+
+    check(cx, expr, scrutinee, &arms, message, Suggestion::Emit);
+}
+
+pub(super) fn check_if_let<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    scrutinee: &'tcx Expr<'_>,
+    if_then: &'tcx Expr<'_>,
+    if_else: Option<&'tcx Expr<'_>>,
+) {
+    if is_lint_allowed(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, expr.hir_id) {
+        return;
+    }
+
+    let message =
+        "temporary with significant `Drop` in `if let` scrutinee will live until the end of the `if let` expression";
+
+    if let Some(if_else) = if_else {
+        check(cx, expr, scrutinee, &[if_then, if_else], message, Suggestion::Emit);
+    } else {
+        check(cx, expr, scrutinee, &[if_then], message, Suggestion::Emit);
+    }
+}
+
+pub(super) fn check_while_let<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    scrutinee: &'tcx Expr<'_>,
+    body: &'tcx Expr<'_>,
+) {
+    if is_lint_allowed(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, expr.hir_id) {
+        return;
+    }
+
+    check(
+        cx,
+        expr,
+        scrutinee,
+        &[body],
+        "temporary with significant `Drop` in `while let` scrutinee will live until the end of the `while let` expression",
+        // Don't emit wrong suggestions: We cannot fix the significant drop in the `while let` scrutinee by simply
+        // moving it out. We need to change the `while` to a `loop` instead.
+        Suggestion::DontEmit,
+    );
+}
+
+#[derive(Copy, Clone, Debug)]
+enum Suggestion {
+    Emit,
+    DontEmit,
+}
+
+fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    scrutinee: &'tcx Expr<'_>,
+    arms: &[&'tcx Expr<'_>],
+    message: &'static str,
+    sugg: Suggestion,
+) {
+    let mut helper = SigDropHelper::new(cx);
+    let suggestions = helper.find_sig_drop(scrutinee);
+
     for found in suggestions {
         span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| {
-            set_diagnostic(diag, cx, expr, found);
+            match sugg {
+                Suggestion::Emit => set_suggestion(diag, cx, expr, found),
+                Suggestion::DontEmit => (),
+            }
+
             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) {
@@ -41,7 +120,7 @@ pub(super) fn check<'tcx>(
     }
 }
 
-fn set_diagnostic<'tcx>(diag: &mut Diag<'_, ()>, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, found: FoundSigDrop) {
+fn set_suggestion<'tcx>(diag: &mut Diag<'_, ()>, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, found: FoundSigDrop) {
     let original = snippet(cx, found.found_span, "..");
     let trailing_indent = " ".repeat(indent_of(cx, found.found_span).unwrap_or(0));
 
@@ -79,26 +158,6 @@ fn set_diagnostic<'tcx>(diag: &mut Diag<'_, ()>, cx: &LateContext<'tcx>, expr: &
     );
 }
 
-/// If the expression is an `ExprKind::Match`, check if the scrutinee has a significant drop that
-/// may have a surprising lifetime.
-fn has_significant_drop_in_scrutinee<'tcx>(
-    cx: &LateContext<'tcx>,
-    scrutinee: &'tcx Expr<'tcx>,
-    source: MatchSource,
-) -> (Vec<FoundSigDrop>, &'static str) {
-    let mut helper = SigDropHelper::new(cx);
-    let scrutinee = match (source, &scrutinee.kind) {
-        (MatchSource::ForLoopDesugar, ExprKind::Call(_, [e])) => e,
-        _ => scrutinee,
-    };
-    let message = if source == MatchSource::Normal {
-        "temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression"
-    } else {
-        "temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression"
-    };
-    (helper.find_sig_drop(scrutinee), message)
-}
-
 struct SigDropChecker<'a, 'tcx> {
     seen_types: FxHashSet<Ty<'tcx>>,
     cx: &'a LateContext<'tcx>,
@@ -428,10 +487,10 @@ impl<'a, 'tcx> ArmSigDropHelper<'a, 'tcx> {
     }
 }
 
-fn has_significant_drop_in_arms<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) -> FxHashSet<Span> {
+fn has_significant_drop_in_arms<'tcx>(cx: &LateContext<'tcx>, arms: &[&'tcx Expr<'_>]) -> FxHashSet<Span> {
     let mut helper = ArmSigDropHelper::new(cx);
     for arm in arms {
-        helper.visit_expr(arm.body);
+        helper.visit_expr(arm);
     }
     helper.found_sig_drop_spans
 }

tests/ui/significant_drop_in_scrutinee.rs

@@ -801,4 +801,30 @@ fn should_not_trigger_lint_with_explicit_drop() {
     }
 }
 
+fn should_trigger_lint_in_if_let() {
+    let mutex = Mutex::new(vec![1]);
+
+    if let Some(val) = mutex.lock().unwrap().first().copied() {
+        //~^ ERROR: temporary with significant `Drop` in `if let` scrutinee will live until the
+        //~| NOTE: this might lead to deadlocks or other unexpected behavior
+        println!("{}", val);
+    }
+
+    // Should not trigger lint without the final `copied()`, because we actually hold a reference
+    // (i.e., the `val`) to the locked data.
+    if let Some(val) = mutex.lock().unwrap().first() {
+        println!("{}", val);
+    };
+}
+
+fn should_trigger_lint_in_while_let() {
+    let mutex = Mutex::new(vec![1]);
+
+    while let Some(val) = mutex.lock().unwrap().pop() {
+        //~^ ERROR: temporary with significant `Drop` in `while let` scrutinee will live until the
+        //~| NOTE: this might lead to deadlocks or other unexpected behavior
+        println!("{}", val);
+    }
+}
+
 fn main() {}

tests/ui/significant_drop_in_scrutinee.stderr

@@ -541,5 +541,32 @@ LL ~     let value = mutex.lock().unwrap()[0];
 LL ~     for val in [value, 2] {
    |
 
-error: aborting due to 27 previous errors
+error: temporary with significant `Drop` in `if let` scrutinee will live until the end of the `if let` expression
+  --> tests/ui/significant_drop_in_scrutinee.rs:807:24
+   |
+LL |     if let Some(val) = mutex.lock().unwrap().first().copied() {
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     }
+   |      - temporary lives until here
+   |
+   = note: this might lead to deadlocks or other unexpected behavior
+help: try moving the temporary above the match
+   |
+LL ~     let value = mutex.lock().unwrap().first().copied();
+LL ~     if let Some(val) = value {
+   |
+
+error: temporary with significant `Drop` in `while let` scrutinee will live until the end of the `while let` expression
+  --> tests/ui/significant_drop_in_scrutinee.rs:823:27
+   |
+LL |     while let Some(val) = mutex.lock().unwrap().pop() {
+   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     }
+   |      - temporary lives until here
+   |
+   = note: this might lead to deadlocks or other unexpected behavior
+
+error: aborting due to 29 previous errors