Lint significant drop on while let and if let

This commit is contained in:
Ruihan Li 2024-04-29 12:39:31 +08:00
parent a4bdab38f0
commit 378962f861
4 changed files with 149 additions and 30 deletions

View file

@ -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);
}
}
}

View file

@ -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
}

View file

@ -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() {}

View file

@ -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