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

Author: bors

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

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:
 

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<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
+        }
+    }
+}

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;
@@ -573,6 +574,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,
@@ -1054,6 +1056,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),
@@ -1233,6 +1236,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),
@@ -1620,6 +1624,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),

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
 

src/lintlist/mod.rs

@@ -731,6 +731,13 @@ pub static ref ALL_LINTS: Vec<Lint> = 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",

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>(_: 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() {}

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
+