Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions clippy_lints/src/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
}
Comment on lines +1131 to 1138
Copy link
Member

@J-ZhengLi J-ZhengLi Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (feel free to ignore as it might not be related), could this be written as:

} 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, while_let);
    }
}

and remove the other higher::WhileLet::hir(expr) in redundant_pattern_match::check, to improve readability for a little bit, plus it's a "redundant pattern matching" 😂. Unless that check function is called somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. Well, your suggestion makes sense to me.

However, after having a closer look at redundant_pattern_match::check, I find out that this method needs expr to be passed to find_method_sugg_for_if_let (which in turn needs at least expr.span and expr.hir_id). It seems that such information is not available in WhileLet. I'm afraid it might not be worth refactoring such methods?

find_method_sugg_for_if_let(cx, expr, let_pat, let_expr, "while", false);

fn find_method_sugg_for_if_let<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
let_pat: &Pat<'_>,
let_expr: &'tcx Expr<'_>,
keyword: &'static str,
has_else: bool,
) {

}

Expand Down
111 changes: 85 additions & 26 deletions clippy_lints/src/matches/significant_drop_in_scrutinee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_>,
Expand All @@ -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;
}
blyxyas marked this conversation as resolved.
Show resolved Hide resolved

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;
}
blyxyas marked this conversation as resolved.
Show resolved Hide resolved

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) {
Expand All @@ -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));

Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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
}
Expand Down
26 changes: 26 additions & 0 deletions tests/ui/significant_drop_in_scrutinee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
29 changes: 28 additions & 1 deletion tests/ui/significant_drop_in_scrutinee.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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