diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/if_let_mutex.rs | 135 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 5 | ||||
| -rw-r--r-- | src/lintlist/mod.rs | 7 | ||||
| -rw-r--r-- | tests/ui/if_let_mutex.rs | 16 |
5 files changed, 164 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index e513787a53a..2668c89bdc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1286,6 +1286,7 @@ Released 2018-09-13 [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap [`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion [`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op +[`if_let_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_mutex [`if_let_redundant_pattern_matching`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_redundant_pattern_matching [`if_let_some_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_let_some_result [`if_not_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs new file mode 100644 index 00000000000..34f0b22f65e --- /dev/null +++ b/clippy_lints/src/if_let_mutex.rs @@ -0,0 +1,135 @@ +use crate::utils::{ + match_type, method_calls, method_chain_args, paths, snippet, snippet_with_applicability, span_lint_and_sugg, +}; +use if_chain::if_chain; +use rustc::ty; +use rustc_errors::Applicability; +use rustc_hir::{print, Expr, ExprKind, MatchSource, PatKind, QPath, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +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 + /// # use std::sync::Mutex; + /// let mutex = Mutex::new(10); + /// if let Ok(thing) = mutex.lock() { + /// do_thing(); + /// } else { + /// mutex.lock(); + /// } + /// ``` + pub IF_LET_MUTEX, + correctness, + "locking a `Mutex` in an `if let` block can cause deadlock" +} + +declare_lint_pass!(IfLetMutex => [IF_LET_MUTEX]); + +impl LateLintPass<'_, '_> for IfLetMutex { + fn check_expr(&mut self, cx: &LateContext<'_, '_>, ex: &'_ Expr<'_>) { + if_chain! { + if let ExprKind::Match(ref op, ref arms, MatchSource::IfLetDesugar { + contains_else_clause: true, + }) = 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 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) + } 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 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) + } 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 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) + } else { + suggestion.push_str(&format!(" {}\n", snippet(cx, e.span, "_"))); + false + } + }, + _ => { suggestion.push_str(&format!(" {}\n", snippet(cx, stmt.span, "_"))); false }, + }); + then { + true + } else { + suggestion.push_str(&format!("else {}\n", snippet(cx, arm.span, "_"))); + false + } + }); + then { + println!("{}", suggestion); + span_lint_and_sugg( + cx, + IF_LET_MUTEX, + ex.span, + "using a `Mutex` in inner scope of `.lock` call", + "try", + format!("{:?}", "hello"), + Applicability::MachineApplicable, + ); + } + } + } +} + +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 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 +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 19c46476263..819230b6a61 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -222,6 +222,7 @@ mod future_not_send; mod get_last_with_len; mod identity_conversion; mod identity_op; +mod if_let_mutex; mod if_let_some_result; mod if_not_else; mod implicit_return; @@ -572,6 +573,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &get_last_with_len::GET_LAST_WITH_LEN, &identity_conversion::IDENTITY_CONVERSION, &identity_op::IDENTITY_OP, + &if_let_mutex::IF_LET_MUTEX, &if_let_some_result::IF_LET_SOME_RESULT, &if_not_else::IF_NOT_ELSE, &implicit_return::IMPLICIT_RETURN, @@ -1053,6 +1055,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box dereference::Dereferencing); store.register_late_pass(|| box future_not_send::FutureNotSend); store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls); + store.register_late_pass(|| box if_let_mutex::IfLetMutex); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1231,6 +1234,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&get_last_with_len::GET_LAST_WITH_LEN), LintId::of(&identity_conversion::IDENTITY_CONVERSION), LintId::of(&identity_op::IDENTITY_OP), + LintId::of(&if_let_mutex::IF_LET_MUTEX), LintId::of(&if_let_some_result::IF_LET_SOME_RESULT), LintId::of(&indexing_slicing::OUT_OF_BOUNDS_INDEXING), LintId::of(&infinite_iter::INFINITE_ITER), @@ -1618,6 +1622,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&erasing_op::ERASING_OP), LintId::of(&formatting::POSSIBLE_MISSING_COMMA), LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF), + LintId::of(&if_let_mutex::IF_LET_MUTEX), LintId::of(&indexing_slicing::OUT_OF_BOUNDS_INDEXING), LintId::of(&infinite_iter::INFINITE_ITER), LintId::of(&inherent_to_string::INHERENT_TO_STRING_SHADOW_DISPLAY), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 213d054e403..229fd3ac824 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -732,6 +732,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![ module: "identity_op", }, Lint { + name: "if_let_mutex", + group: "correctness", + desc: "default lint description", + deprecation: None, + module: "if_let_mutex", + }, + Lint { name: "if_let_some_result", group: "style", desc: "usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead", diff --git a/tests/ui/if_let_mutex.rs b/tests/ui/if_let_mutex.rs new file mode 100644 index 00000000000..13fe44eed2c --- /dev/null +++ b/tests/ui/if_let_mutex.rs @@ -0,0 +1,16 @@ +#![warn(clippy::if_let_mutex)] + +use std::sync::Mutex; + +fn do_stuff() {} +fn foo() { + let m = Mutex::new(1u8); + + if let Ok(locked) = m.lock() { + do_stuff(); + } else { + m.lock().unwrap(); + }; +} + +fn main() {} |