diff --git a/CHANGELOG.md b/CHANGELOG.md index e513787a53a6..2668c89bdc8f 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/README.md b/README.md index 222b81023a77..9d5939db8d5d 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are over 350 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 362 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs new file mode 100644 index 000000000000..b2ece37fdb0a --- /dev/null +++ b/clippy_lints/src/if_let_mutex.rs @@ -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 { + 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 { + 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 + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 19c46476263b..819230b6a61c 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/doc/adding_lints.md b/doc/adding_lints.md index 94d6ccb316ea..9ad1315c1752 100644 --- a/doc/adding_lints.md +++ b/doc/adding_lints.md @@ -101,7 +101,9 @@ Once we are satisfied with the output, we need to run Please note that, we should run `TESTNAME=foo_functions cargo uitest` every time before running `tests/ui/update-all-references.sh`. Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit -our lint, we need to commit the generated `.stderr` files, too. +our lint, we need to commit the generated `.stderr` files, too. In general, you +should only commit files changed by `tests/ui/update-all-references.sh` for the +specific lint you are creating/editing. ## Rustfix tests diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 213d054e403d..6088f9b9ef8c 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -731,6 +731,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "identity_op", }, + Lint { + name: "if_let_mutex", + group: "correctness", + desc: "locking a `Mutex` in an `if let` block can cause deadlocks", + deprecation: None, + module: "if_let_mutex", + }, Lint { name: "if_let_some_result", group: "style", diff --git a/tests/ui/if_let_mutex.rs b/tests/ui/if_let_mutex.rs new file mode 100644 index 000000000000..58feae422a3c --- /dev/null +++ b/tests/ui/if_let_mutex.rs @@ -0,0 +1,42 @@ +#![warn(clippy::if_let_mutex)] + +use std::ops::Deref; +use std::sync::Mutex; + +fn do_stuff(_: 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::); + if let Some(locked) = m.lock().unwrap().deref() { + do_stuff(locked); + } else { + let lock = other.lock().unwrap(); + do_stuff(lock); + }; +} + +fn main() {} diff --git a/tests/ui/if_let_mutex.stderr b/tests/ui/if_let_mutex.stderr new file mode 100644 index 000000000000..e9c4d9163328 --- /dev/null +++ b/tests/ui/if_let_mutex.stderr @@ -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:22: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 +