diff options
| -rw-r--r-- | clippy_lints/src/if_let_mutex.rs | 82 | ||||
| -rw-r--r-- | src/lintlist/mod.rs | 2 | ||||
| -rw-r--r-- | tests/ui/if_let_mutex.rs | 9 | ||||
| -rw-r--r-- | tests/ui/if_let_mutex.stderr | 16 |
4 files changed, 64 insertions, 45 deletions
diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs index 52e5b12c933..2238821ab70 100644 --- a/clippy_lints/src/if_let_mutex.rs +++ b/clippy_lints/src/if_let_mutex.rs @@ -1,10 +1,6 @@ -use crate::utils::{ - match_type, method_calls, method_chain_args, paths, snippet, snippet_with_applicability, span_lint_and_sugg, -}; +use crate::utils::{match_type, paths, span_lint_and_help}; use if_chain::if_chain; -use rustc::ty; -use rustc_errors::Applicability; -use rustc_hir::{print, Expr, ExprKind, MatchSource, PatKind, QPath, StmtKind}; +use rustc_hir::{Expr, ExprKind, MatchSource, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -15,19 +11,26 @@ declare_clippy_lint! { /// **Why is this bad?** The Mutex lock remains held for the whole /// `if let ... else` block and deadlocks. /// - /// **Known problems:** None. + /// **Known problems:** This lint does not generate an auto-applicable suggestion. /// /// **Example:** /// - /// ```rust - /// # use std::sync::Mutex; - /// let mutex = Mutex::new(10); + /// ```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" @@ -43,93 +46,92 @@ impl LateLintPass<'_, '_> for IfLetMutex { }) = ex.kind; // if let ... {} else {} if let ExprKind::MethodCall(_, _, ref args) = op.kind; let ty = cx.tables.expr_ty(&args[0]); - if let ty::Adt(_, subst) = ty.kind; if match_type(cx, ty, &paths::MUTEX); // make sure receiver is Mutex if method_chain_names(op, 10).iter().any(|s| s == "lock"); // and lock is called - let mut suggestion = String::from(&format!("if let _ = {} {{\n", snippet(cx, op.span, "_"))); - if arms.iter().any(|arm| if_chain! { if let ExprKind::Block(ref block, _l) = arm.body.kind; if block.stmts.iter().any(|stmt| match stmt.kind { StmtKind::Local(l) => if_chain! { if let Some(ex) = l.init; - if let ExprKind::MethodCall(_, _, ref args) = op.kind; + if let ExprKind::MethodCall(_, _, _) = op.kind; if method_chain_names(ex, 10).iter().any(|s| s == "lock"); // and lock is called then { - let ty = cx.tables.expr_ty(&args[0]); - // // make sure receiver is Result<MutexGuard<...>> - match_type(cx, ty, &paths::RESULT) + match_type_method_chain(cx, ex, 5) } else { - suggestion.push_str(&format!(" {}\n", snippet(cx, l.span, "_"))); false } }, StmtKind::Expr(e) => if_chain! { - if let ExprKind::MethodCall(_, _, ref args) = e.kind; + if let ExprKind::MethodCall(_, _, _) = e.kind; if method_chain_names(e, 10).iter().any(|s| s == "lock"); // and lock is called then { - let ty = cx.tables.expr_ty(&args[0]); - // // make sure receiver is Result<MutexGuard<...>> - match_type(cx, ty, &paths::RESULT) + match_type_method_chain(cx, ex, 5) } else { - suggestion.push_str(&format!(" {}\n", snippet(cx, e.span, "_"))); false } }, StmtKind::Semi(e) => if_chain! { - if let ExprKind::MethodCall(_, _, ref args) = e.kind; + if let ExprKind::MethodCall(_, _, _) = e.kind; if method_chain_names(e, 10).iter().any(|s| s == "lock"); // and lock is called then { - let ty = cx.tables.expr_ty(&args[0]); - // // make sure receiver is Result<MutexGuard<...>> - match_type(cx, ty, &paths::RESULT) + match_type_method_chain(cx, ex, 5) } else { - suggestion.push_str(&format!(" {}\n", snippet(cx, e.span, "_"))); false } }, - _ => { suggestion.push_str(&format!(" {}\n", snippet(cx, stmt.span, "_"))); false }, + _ => { + false + }, }); then { true } else { - suggestion.push_str(&format!("else {}\n", snippet(cx, arm.span, "_"))); false } }); then { - println!("{}", suggestion); - span_lint_and_sugg( + span_lint_and_help( cx, IF_LET_MUTEX, ex.span, - "using a `Mutex` in inner scope of `.lock` call", - "try", - format!("{:?}", "hello"), - Applicability::MachineApplicable, + "calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock", + "move the lock call outside of the `if let ...` expression", ); } } } } +/// Return the names of `max_depth` number of methods called in the chain. fn method_chain_names<'tcx>(expr: &'tcx Expr<'tcx>, max_depth: usize) -> Vec<String> { let mut method_names = Vec::with_capacity(max_depth); - let mut current = expr; for _ in 0..max_depth { - if let ExprKind::MethodCall(path, span, args) = ¤t.kind { + if let ExprKind::MethodCall(path, _, args) = ¤t.kind { if args.iter().any(|e| e.span.from_expansion()) { break; } method_names.push(path.ident.to_string()); - println!("{:?}", method_names); current = &args[0]; } else { break; } } - method_names } + +/// Check that lock is called on a `Mutex`. +fn match_type_method_chain<'tcx>(cx: &LateContext<'_, '_>, expr: &'tcx Expr<'tcx>, max_depth: usize) -> bool { + let mut current = expr; + for _ in 0..max_depth { + if let ExprKind::MethodCall(_, _, args) = ¤t.kind { + let ty = cx.tables.expr_ty(&args[0]); + if match_type(cx, ty, &paths::MUTEX) { + return true; + } + current = &args[0]; + } + } + false +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 229fd3ac824..6088f9b9ef8 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -734,7 +734,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![ Lint { name: "if_let_mutex", group: "correctness", - desc: "default lint description", + desc: "locking a `Mutex` in an `if let` block can cause deadlocks", deprecation: None, module: "if_let_mutex", }, diff --git a/tests/ui/if_let_mutex.rs b/tests/ui/if_let_mutex.rs index 13fe44eed2c..059764e9b21 100644 --- a/tests/ui/if_let_mutex.rs +++ b/tests/ui/if_let_mutex.rs @@ -2,14 +2,15 @@ use std::sync::Mutex; -fn do_stuff() {} +fn do_stuff<T>(_: T) {} fn foo() { let m = Mutex::new(1u8); - if let Ok(locked) = m.lock() { - do_stuff(); + if let Err(locked) = m.lock() { + do_stuff(locked); } else { - m.lock().unwrap(); + let lock = m.lock().unwrap(); + do_stuff(lock); }; } diff --git a/tests/ui/if_let_mutex.stderr b/tests/ui/if_let_mutex.stderr new file mode 100644 index 00000000000..84c19ed03d5 --- /dev/null +++ b/tests/ui/if_let_mutex.stderr @@ -0,0 +1,16 @@ +error: calling `Mutex::lock` inside the scope of another `Mutex::lock` causes a deadlock + --> $DIR/if_let_mutex.rs:9: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: aborting due to previous error + |