mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #13174 - y21:if_let_mutex_multi_mutex, r=llogiq

Browse Source 
Emit `if_let_mutex` in presence of other mutexes

Currently (master, not nightly nor stable) `if_let_mutex` does not emit a warning here:
```rs
let m1 = Mutex::new(10);
let m2 = Mutex::new(());

if let 100..=200 = *m1.lock().unwrap() {
  m2.lock();
} else {
  m1.lock();
}
```
It currently looks for the first call to `.lock()` on *any* mutex receiver inside of the if/else body, and only later (outside of the visitor) checks that the receiver matches the mutex in the scrutinee. That means that in cases like the above, it finds the `m2.lock()` expression, stops the visitor, fails the check that it's the same mutex (`m2` != `m1`) and then does not look for any other `.lock()` calls.

So, just make the receiver check also part of the visitor so that we only stop the visitor when we also find the right receiver.

The first commit has the actual changes described here. The sceond one just unnests all the `if let`s

----

changelog: none
This commit is contained in:
bors 2024-07-29 17:41:34 +00:00
commit 834b691a9f
3 changed files with 58 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

66
clippy_lints/src/if_let_mutex.rs
View File

@ -1,7 +1,7 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::ty::is_type_diagnostic_item;
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 rustc_errors::Diag;
use rustc_hir::{Expr, ExprKind};
@ -51,53 +51,45 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if_else: Some(if_else),
..
}) = higher::IfLet::hir(cx, expr)
&& let Some(op_mutex) = for_each_expr_without_closures(let_expr, |e| mutex_lock_call(cx, e, None))
&& let Some(arm_mutex) =
for_each_expr_without_closures((if_then, if_else), |e| mutex_lock_call(cx, e, Some(op_mutex)))
{
let is_mutex_lock = |e: &'tcx Expr<'tcx>| {
if let Some(mutex) = is_mutex_lock_call(cx, e) {
ControlFlow::Break(mutex)
} else {
ControlFlow::Continue(())
}
let diag = |diag: &mut Diag<'_, ()>| {
diag.span_label(
op_mutex.span,
"this Mutex will remain locked for the entire `if let`-block...",
);
diag.span_label(
arm_mutex.span,
"... and is tried to lock again here, which will always deadlock.",
);
diag.help("move the lock call outside of the `if let ...` expression");
};
let op_mutex = for_each_expr_without_closures(let_expr, is_mutex_lock);
if let Some(op_mutex) = op_mutex {
let arm_mutex = for_each_expr_without_closures((if_then, if_else), is_mutex_lock);
if let Some(arm_mutex) = arm_mutex
&& SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex)
{
let diag = |diag: &mut Diag<'_, ()>| {
diag.span_label(
op_mutex.span,
"this Mutex will remain locked for the entire `if let`-block...",
);
diag.span_label(
arm_mutex.span,
"... and is tried to lock again here, which will always deadlock.",
);
diag.help("move the lock call outside of the `if let ...` expression");
};
span_lint_and_then(
cx,
IF_LET_MUTEX,
expr.span,
"calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock",
diag,
);
}
}
span_lint_and_then(
cx,
IF_LET_MUTEX,
expr.span,
"calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock",
diag,
);
}
}
}
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
&& path.ident.as_str() == "lock"
&& let ty = cx.typeck_results().expr_ty(self_arg).peel_refs()
&& 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 {
None
ControlFlow::Continue(())
}
}

9
tests/ui/if_let_mutex.rs
View File

@ -1,4 +1,5 @@
#![warn(clippy::if_let_mutex)]
#![allow(clippy::redundant_pattern_matching)]
use std::ops::Deref;
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() {}

24
tests/ui/if_let_mutex.stderr
View File

@ -1,5 +1,5 @@
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() {
| ^ - 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)]`
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() {
| ^ - 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
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() {
| ^ ----- 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
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