[if_let_mutex]: make the mutex check part of the expr visitor

This commit is contained in:
y21 2024-07-28 18:27:53 +02:00
parent 668b659b47
commit 1a1c978f8e
3 changed files with 42 additions and 21 deletions

View File

@ -1,7 +1,7 @@
use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::visitors::for_each_expr_without_closures; use clippy_utils::visitors::for_each_expr_without_closures;
use clippy_utils::{higher, SpanlessEq}; use clippy_utils::{eq_expr_value, higher};
use core::ops::ControlFlow; use core::ops::ControlFlow;
use rustc_errors::Diag; use rustc_errors::Diag;
use rustc_hir::{Expr, ExprKind}; use rustc_hir::{Expr, ExprKind};
@ -52,20 +52,11 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
.. ..
}) = higher::IfLet::hir(cx, expr) }) = higher::IfLet::hir(cx, expr)
{ {
let is_mutex_lock = |e: &'tcx Expr<'tcx>| { let op_mutex = for_each_expr_without_closures(let_expr, |e| mutex_lock_call(cx, e, None));
if let Some(mutex) = is_mutex_lock_call(cx, e) {
ControlFlow::Break(mutex)
} else {
ControlFlow::Continue(())
}
};
let op_mutex = for_each_expr_without_closures(let_expr, is_mutex_lock);
if let Some(op_mutex) = op_mutex { if let Some(op_mutex) = op_mutex {
let arm_mutex = for_each_expr_without_closures((if_then, if_else), is_mutex_lock); let arm_mutex =
if let Some(arm_mutex) = arm_mutex for_each_expr_without_closures((if_then, if_else), |e| mutex_lock_call(cx, e, Some(op_mutex)));
&& SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex) if let Some(arm_mutex) = arm_mutex {
{
let diag = |diag: &mut Diag<'_, ()>| { let diag = |diag: &mut Diag<'_, ()>| {
diag.span_label( diag.span_label(
op_mutex.span, op_mutex.span,
@ -90,14 +81,19 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
} }
} }
fn is_mutex_lock_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { fn mutex_lock_call<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
op_mutex: Option<&'tcx Expr<'_>>,
) -> ControlFlow<&'tcx Expr<'tcx>> {
if let ExprKind::MethodCall(path, self_arg, ..) = &expr.kind if let ExprKind::MethodCall(path, self_arg, ..) = &expr.kind
&& path.ident.as_str() == "lock" && path.ident.as_str() == "lock"
&& let ty = cx.typeck_results().expr_ty(self_arg).peel_refs() && let ty = cx.typeck_results().expr_ty(self_arg).peel_refs()
&& is_type_diagnostic_item(cx, ty, sym::Mutex) && is_type_diagnostic_item(cx, ty, sym::Mutex)
&& op_mutex.map_or(true, |op| eq_expr_value(cx, self_arg, op))
{ {
Some(self_arg) ControlFlow::Break(self_arg)
} else { } else {
None ControlFlow::Continue(())
} }
} }

View File

@ -1,4 +1,5 @@
#![warn(clippy::if_let_mutex)] #![warn(clippy::if_let_mutex)]
#![allow(clippy::redundant_pattern_matching)]
use std::ops::Deref; use std::ops::Deref;
use std::sync::Mutex; use std::sync::Mutex;
@ -50,4 +51,12 @@ fn mutex_ref(mutex: &Mutex<i32>) {
}; };
} }
fn multiple_mutexes(m1: &Mutex<()>, m2: &Mutex<()>) {
if let Ok(_) = m1.lock() {
m2.lock();
} else {
m1.lock();
}
}
fn main() {} fn main() {}

View File

@ -1,5 +1,5 @@
error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock
--> tests/ui/if_let_mutex.rs:10:5 --> tests/ui/if_let_mutex.rs:11:5
| |
LL | if let Err(locked) = m.lock() { LL | if let Err(locked) = m.lock() {
| ^ - this Mutex will remain locked for the entire `if let`-block... | ^ - this Mutex will remain locked for the entire `if let`-block...
@ -19,7 +19,7 @@ LL | | };
= help: to override `-D warnings` add `#[allow(clippy::if_let_mutex)]` = help: to override `-D warnings` add `#[allow(clippy::if_let_mutex)]`
error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock
--> tests/ui/if_let_mutex.rs:23:5 --> tests/ui/if_let_mutex.rs:24:5
| |
LL | if let Some(locked) = m.lock().unwrap().deref() { LL | if let Some(locked) = m.lock().unwrap().deref() {
| ^ - this Mutex will remain locked for the entire `if let`-block... | ^ - this Mutex will remain locked for the entire `if let`-block...
@ -37,7 +37,7 @@ LL | | };
= help: move the lock call outside of the `if let ...` expression = help: move the lock call outside of the `if let ...` expression
error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock
--> tests/ui/if_let_mutex.rs:45:5 --> tests/ui/if_let_mutex.rs:46:5
| |
LL | if let Ok(i) = mutex.lock() { LL | if let Ok(i) = mutex.lock() {
| ^ ----- this Mutex will remain locked for the entire `if let`-block... | ^ ----- this Mutex will remain locked for the entire `if let`-block...
@ -53,5 +53,21 @@ LL | | };
| |
= help: move the lock call outside of the `if let ...` expression = help: move the lock call outside of the `if let ...` expression
error: aborting due to 3 previous errors error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock
--> tests/ui/if_let_mutex.rs:55:5
|
LL | if let Ok(_) = m1.lock() {
| ^ -- this Mutex will remain locked for the entire `if let`-block...
| _____|
| |
LL | | m2.lock();
LL | | } else {
LL | | m1.lock();
| | -- ... and is tried to lock again here, which will always deadlock.
LL | | }
| |_____^
|
= help: move the lock call outside of the `if let ...` expression
error: aborting due to 4 previous errors