-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #5332 - DevinR528:if-let-else-mutex, r=flip1995
If let else mutex changelog: Adds lint to catch incorrect use of `Mutex::lock` in `if let` expressions with lock calls in any of the blocks. closes: #5219
- Loading branch information
Showing
8 changed files
with
248 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| use crate::utils::{match_type, paths, span_lint_and_help, SpanlessEq}; | ||
| use if_chain::if_chain; | ||
| use rustc_hir::intravisit::{self as visit, NestedVisitorMap, Visitor}; | ||
| use rustc_hir::{Expr, ExprKind, MatchSource}; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_middle::hir::map::Map; | ||
| use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// **What it does:** Checks for `Mutex::lock` calls in `if let` expression | ||
| /// with lock calls in any of the else blocks. | ||
| /// | ||
| /// **Why is this bad?** The Mutex lock remains held for the whole | ||
| /// `if let ... else` block and deadlocks. | ||
| /// | ||
| /// **Known problems:** None. | ||
| /// | ||
| /// **Example:** | ||
| /// | ||
| /// ```rust,ignore | ||
| /// if let Ok(thing) = mutex.lock() { | ||
| /// do_thing(); | ||
| /// } else { | ||
| /// mutex.lock(); | ||
| /// } | ||
| /// ``` | ||
| /// Should be written | ||
| /// ```rust,ignore | ||
| /// let locked = mutex.lock(); | ||
| /// if let Ok(thing) = locked { | ||
| /// do_thing(thing); | ||
| /// } else { | ||
| /// use_locked(locked); | ||
| /// } | ||
| /// ``` | ||
| pub IF_LET_MUTEX, | ||
| correctness, | ||
| "locking a `Mutex` in an `if let` block can cause deadlocks" | ||
| } | ||
|
|
||
| declare_lint_pass!(IfLetMutex => [IF_LET_MUTEX]); | ||
|
|
||
| impl LateLintPass<'_, '_> for IfLetMutex { | ||
| fn check_expr(&mut self, cx: &LateContext<'_, '_>, ex: &'_ Expr<'_>) { | ||
| let mut arm_visit = ArmVisitor { | ||
| mutex_lock_called: false, | ||
| found_mutex: None, | ||
| cx, | ||
| }; | ||
| let mut op_visit = OppVisitor { | ||
| mutex_lock_called: false, | ||
| found_mutex: None, | ||
| cx, | ||
| }; | ||
| if let ExprKind::Match( | ||
| ref op, | ||
| ref arms, | ||
| MatchSource::IfLetDesugar { | ||
| contains_else_clause: true, | ||
| }, | ||
| ) = ex.kind | ||
| { | ||
| op_visit.visit_expr(op); | ||
| if op_visit.mutex_lock_called { | ||
| for arm in *arms { | ||
| arm_visit.visit_arm(arm); | ||
| } | ||
|
|
||
| if arm_visit.mutex_lock_called && arm_visit.same_mutex(cx, op_visit.found_mutex.unwrap()) { | ||
| span_lint_and_help( | ||
| cx, | ||
| IF_LET_MUTEX, | ||
| ex.span, | ||
| "calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock", | ||
| None, | ||
| "move the lock call outside of the `if let ...` expression", | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Checks if `Mutex::lock` is called in the `if let _ = expr. | ||
| pub struct OppVisitor<'tcx, 'l> { | ||
| mutex_lock_called: bool, | ||
| found_mutex: Option<&'tcx Expr<'tcx>>, | ||
| cx: &'tcx LateContext<'tcx, 'l>, | ||
| } | ||
|
|
||
| impl<'tcx, 'l> Visitor<'tcx> for OppVisitor<'tcx, 'l> { | ||
| type Map = Map<'tcx>; | ||
|
|
||
| fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { | ||
| if_chain! { | ||
| if let Some(mutex) = is_mutex_lock_call(self.cx, expr); | ||
| then { | ||
| self.found_mutex = Some(mutex); | ||
| self.mutex_lock_called = true; | ||
| return; | ||
| } | ||
| } | ||
| visit::walk_expr(self, expr); | ||
| } | ||
|
|
||
| fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { | ||
| NestedVisitorMap::None | ||
| } | ||
| } | ||
|
|
||
| /// Checks if `Mutex::lock` is called in any of the branches. | ||
| pub struct ArmVisitor<'tcx, 'l> { | ||
| mutex_lock_called: bool, | ||
| found_mutex: Option<&'tcx Expr<'tcx>>, | ||
| cx: &'tcx LateContext<'tcx, 'l>, | ||
| } | ||
|
|
||
| impl<'tcx, 'l> Visitor<'tcx> for ArmVisitor<'tcx, 'l> { | ||
| type Map = Map<'tcx>; | ||
|
|
||
| fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { | ||
| if_chain! { | ||
| if let Some(mutex) = is_mutex_lock_call(self.cx, expr); | ||
| then { | ||
| self.found_mutex = Some(mutex); | ||
| self.mutex_lock_called = true; | ||
| return; | ||
| } | ||
| } | ||
| visit::walk_expr(self, expr); | ||
| } | ||
|
|
||
| fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { | ||
| NestedVisitorMap::None | ||
| } | ||
| } | ||
|
|
||
| impl<'tcx, 'l> ArmVisitor<'tcx, 'l> { | ||
| fn same_mutex(&self, cx: &LateContext<'_, '_>, op_mutex: &Expr<'_>) -> bool { | ||
| if let Some(arm_mutex) = self.found_mutex { | ||
| SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex) | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn is_mutex_lock_call<'a>(cx: &LateContext<'a, '_>, expr: &'a Expr<'_>) -> Option<&'a Expr<'a>> { | ||
| if_chain! { | ||
| if let ExprKind::MethodCall(path, _span, args) = &expr.kind; | ||
| if path.ident.to_string() == "lock"; | ||
| let ty = cx.tables.expr_ty(&args[0]); | ||
| if match_type(cx, ty, &paths::MUTEX); | ||
| then { | ||
| Some(&args[0]) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| #![warn(clippy::if_let_mutex)] | ||
|
|
||
| use std::ops::Deref; | ||
| use std::sync::Mutex; | ||
|
|
||
| fn do_stuff<T>(_: T) {} | ||
|
|
||
| fn if_let() { | ||
| let m = Mutex::new(1_u8); | ||
| if let Err(locked) = m.lock() { | ||
| do_stuff(locked); | ||
| } else { | ||
| let lock = m.lock().unwrap(); | ||
| do_stuff(lock); | ||
| }; | ||
| } | ||
|
|
||
| // This is the most common case as the above case is pretty | ||
| // contrived. | ||
| fn if_let_option() { | ||
| let m = Mutex::new(Some(0_u8)); | ||
| if let Some(locked) = m.lock().unwrap().deref() { | ||
| do_stuff(locked); | ||
| } else { | ||
| let lock = m.lock().unwrap(); | ||
| do_stuff(lock); | ||
| }; | ||
| } | ||
|
|
||
| // When mutexs are different don't warn | ||
| fn if_let_different_mutex() { | ||
| let m = Mutex::new(Some(0_u8)); | ||
| let other = Mutex::new(None::<u8>); | ||
| if let Some(locked) = m.lock().unwrap().deref() { | ||
| do_stuff(locked); | ||
| } else { | ||
| let lock = other.lock().unwrap(); | ||
| do_stuff(lock); | ||
| }; | ||
| } | ||
|
|
||
| fn main() {} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock | ||
| --> $DIR/if_let_mutex.rs:10:5 | ||
| | | ||
| LL | / if let Err(locked) = m.lock() { | ||
| LL | | do_stuff(locked); | ||
| LL | | } else { | ||
| LL | | let lock = m.lock().unwrap(); | ||
| LL | | do_stuff(lock); | ||
| LL | | }; | ||
| | |_____^ | ||
| | | ||
| = note: `-D clippy::if-let-mutex` implied by `-D warnings` | ||
| = 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 | ||
| --> $DIR/if_let_mutex.rs:20:5 | ||
| | | ||
| LL | / if let Some(locked) = m.lock().unwrap().deref() { | ||
| LL | | do_stuff(locked); | ||
| LL | | } else { | ||
| LL | | let lock = m.lock().unwrap(); | ||
| LL | | do_stuff(lock); | ||
| LL | | }; | ||
| | |_____^ | ||
| | | ||
| = help: move the lock call outside of the `if let ...` expression | ||
|
|
||
| error: aborting due to 2 previous errors | ||
|
|