Skip to content

Commit c00cd51

Browse files
committed
Lint significant drop on while let and if let
1 parent db6d63b commit c00cd51

File tree

4 files changed

+137
-28
lines changed

4 files changed

+137
-28
lines changed

clippy_lints/src/matches/mod.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -1037,7 +1037,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
10371037
return;
10381038
}
10391039
if matches!(source, MatchSource::Normal | MatchSource::ForLoopDesugar) {
1040-
significant_drop_in_scrutinee::check(cx, expr, ex, arms, source);
1040+
significant_drop_in_scrutinee::check_match(cx, expr, ex, arms, source);
10411041
}
10421042

10431043
collapsible_match::check_match(cx, arms);
@@ -1084,6 +1084,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
10841084
}
10851085
} else if let Some(if_let) = higher::IfLet::hir(cx, expr) {
10861086
collapsible_match::check_if_let(cx, if_let.let_pat, if_let.if_then, if_let.if_else);
1087+
significant_drop_in_scrutinee::check_if_let(cx, expr, if_let.let_expr, if_let.if_then, if_let.if_else);
10871088
if !from_expansion {
10881089
if let Some(else_expr) = if_let.if_else {
10891090
if self.msrv.meets(msrvs::MATCHES_MACRO) {
@@ -1118,8 +1119,13 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
11181119
);
11191120
needless_match::check_if_let(cx, expr, &if_let);
11201121
}
1121-
} else if !from_expansion {
1122-
redundant_pattern_match::check(cx, expr);
1122+
} else {
1123+
if let Some(while_let) = higher::WhileLet::hir(expr) {
1124+
significant_drop_in_scrutinee::check_while_let(cx, expr, while_let.let_expr, while_let.if_then);
1125+
}
1126+
if !from_expansion {
1127+
redundant_pattern_match::check(cx, expr);
1128+
}
11231129
}
11241130
}
11251131

clippy_lints/src/matches/significant_drop_in_scrutinee.rs

+69-24
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use rustc_span::Span;
1616

1717
use super::SIGNIFICANT_DROP_IN_SCRUTINEE;
1818

19-
pub(super) fn check<'tcx>(
19+
pub(super) fn check_match<'tcx>(
2020
cx: &LateContext<'tcx>,
2121
expr: &'tcx Expr<'tcx>,
2222
scrutinee: &'tcx Expr<'_>,
@@ -27,7 +27,72 @@ pub(super) fn check<'tcx>(
2727
return;
2828
}
2929

30-
let (suggestions, message) = has_significant_drop_in_scrutinee(cx, scrutinee, source);
30+
let scrutinee = match (source, &scrutinee.kind) {
31+
(MatchSource::ForLoopDesugar, ExprKind::Call(_, [e])) => e,
32+
_ => scrutinee,
33+
};
34+
35+
let message = if source == MatchSource::Normal {
36+
"temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression"
37+
} else {
38+
"temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression"
39+
};
40+
41+
let arms = arms.iter().map(|arm| arm.body).collect::<Vec<_>>();
42+
43+
check(cx, expr, scrutinee, &arms, message);
44+
}
45+
46+
pub(super) fn check_if_let<'tcx>(
47+
cx: &LateContext<'tcx>,
48+
expr: &'tcx Expr<'tcx>,
49+
scrutinee: &'tcx Expr<'_>,
50+
if_then: &'tcx Expr<'_>,
51+
if_else: Option<&'tcx Expr<'_>>,
52+
) {
53+
if is_lint_allowed(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, expr.hir_id) {
54+
return;
55+
}
56+
57+
let message =
58+
"temporary with significant `Drop` in `if let` scrutinee will live until the end of the `if let` expression";
59+
60+
if let Some(if_else) = if_else {
61+
check(cx, expr, scrutinee, &[if_then, if_else], message);
62+
} else {
63+
check(cx, expr, scrutinee, &[if_then], message);
64+
}
65+
}
66+
67+
pub(super) fn check_while_let<'tcx>(
68+
cx: &LateContext<'tcx>,
69+
expr: &'tcx Expr<'tcx>,
70+
scrutinee: &'tcx Expr<'_>,
71+
body: &'tcx Expr<'_>,
72+
) {
73+
if is_lint_allowed(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, expr.hir_id) {
74+
return;
75+
}
76+
77+
check(
78+
cx,
79+
expr,
80+
scrutinee,
81+
&[body],
82+
"temporary with significant `Drop` in `while let` scrutinee will live until the end of the `while let` expression",
83+
);
84+
}
85+
86+
fn check<'tcx>(
87+
cx: &LateContext<'tcx>,
88+
expr: &'tcx Expr<'tcx>,
89+
scrutinee: &'tcx Expr<'_>,
90+
arms: &[&'tcx Expr<'_>],
91+
message: &'static str,
92+
) {
93+
let mut helper = SigDropHelper::new(cx);
94+
let suggestions = helper.find_sig_drop(scrutinee);
95+
3196
for found in suggestions {
3297
span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| {
3398
set_diagnostic(diag, cx, expr, found);
@@ -81,26 +146,6 @@ fn set_diagnostic<'tcx>(diag: &mut Diag<'_, ()>, cx: &LateContext<'tcx>, expr: &
81146
);
82147
}
83148

84-
/// If the expression is an `ExprKind::Match`, check if the scrutinee has a significant drop that
85-
/// may have a surprising lifetime.
86-
fn has_significant_drop_in_scrutinee<'tcx>(
87-
cx: &LateContext<'tcx>,
88-
scrutinee: &'tcx Expr<'tcx>,
89-
source: MatchSource,
90-
) -> (Vec<FoundSigDrop>, &'static str) {
91-
let mut helper = SigDropHelper::new(cx);
92-
let scrutinee = match (source, &scrutinee.kind) {
93-
(MatchSource::ForLoopDesugar, ExprKind::Call(_, [e])) => e,
94-
_ => scrutinee,
95-
};
96-
let message = if source == MatchSource::Normal {
97-
"temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression"
98-
} else {
99-
"temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression"
100-
};
101-
(helper.find_sig_drop(scrutinee), message)
102-
}
103-
104149
struct SigDropChecker<'a, 'tcx> {
105150
seen_types: FxHashSet<Ty<'tcx>>,
106151
cx: &'a LateContext<'tcx>,
@@ -430,10 +475,10 @@ impl<'a, 'tcx> ArmSigDropHelper<'a, 'tcx> {
430475
}
431476
}
432477

433-
fn has_significant_drop_in_arms<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) -> FxHashSet<Span> {
478+
fn has_significant_drop_in_arms<'tcx>(cx: &LateContext<'tcx>, arms: &[&'tcx Expr<'_>]) -> FxHashSet<Span> {
434479
let mut helper = ArmSigDropHelper::new(cx);
435480
for arm in arms {
436-
helper.visit_expr(arm.body);
481+
helper.visit_expr(arm);
437482
}
438483
helper.found_sig_drop_spans
439484
}

tests/ui/significant_drop_in_scrutinee.rs

+26
Original file line numberDiff line numberDiff line change
@@ -801,4 +801,30 @@ fn should_not_trigger_lint_with_explicit_drop() {
801801
}
802802
}
803803

804+
fn should_trigger_lint_in_if_let() {
805+
let mutex = Mutex::new(vec![1]);
806+
807+
if let Some(val) = mutex.lock().unwrap().first().copied() {
808+
//~^ ERROR: temporary with significant `Drop` in `if let` scrutinee will live until the
809+
//~| NOTE: this might lead to deadlocks or other unexpected behavior
810+
println!("{}", val);
811+
}
812+
813+
// Should not trigger lint without the final `copied()`, because we actually hold a reference
814+
// (i.e., the `val`) to the locked data.
815+
if let Some(val) = mutex.lock().unwrap().first() {
816+
println!("{}", val);
817+
};
818+
}
819+
820+
fn should_trigger_lint_in_while_let() {
821+
let mutex = Mutex::new(vec![1]);
822+
823+
while let Some(val) = mutex.lock().unwrap().pop() {
824+
//~^ ERROR: temporary with significant `Drop` in `while let` scrutinee will live until the
825+
//~| NOTE: this might lead to deadlocks or other unexpected behavior
826+
println!("{}", val);
827+
}
828+
}
829+
804830
fn main() {}

tests/ui/significant_drop_in_scrutinee.stderr

+33-1
Original file line numberDiff line numberDiff line change
@@ -541,5 +541,37 @@ LL ~ let value = mutex.lock().unwrap()[0];
541541
LL ~ for val in [value, 2] {
542542
|
543543

544-
error: aborting due to 27 previous errors
544+
error: temporary with significant `Drop` in `if let` scrutinee will live until the end of the `if let` expression
545+
--> tests/ui/significant_drop_in_scrutinee.rs:807:24
546+
|
547+
LL | if let Some(val) = mutex.lock().unwrap().first().copied() {
548+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
549+
...
550+
LL | }
551+
| - temporary lives until here
552+
|
553+
= note: this might lead to deadlocks or other unexpected behavior
554+
help: try moving the temporary above the match and create a copy
555+
|
556+
LL ~ let value = mutex.lock().unwrap().first().copied();
557+
LL ~ if let Some(val) = value {
558+
|
559+
560+
error: temporary with significant `Drop` in `while let` scrutinee will live until the end of the `while let` expression
561+
--> tests/ui/significant_drop_in_scrutinee.rs:823:27
562+
|
563+
LL | while let Some(val) = mutex.lock().unwrap().pop() {
564+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
565+
...
566+
LL | }
567+
| - temporary lives until here
568+
|
569+
= note: this might lead to deadlocks or other unexpected behavior
570+
help: try moving the temporary above the match and create a copy
571+
|
572+
LL ~ let value = mutex.lock().unwrap().pop();
573+
LL ~ while let Some(val) = value {
574+
|
575+
576+
error: aborting due to 29 previous errors
545577

0 commit comments

Comments
 (0)