use span_lint_and_help, cargo dev fmt

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) = &current.kind {
+        if let ExprKind::MethodCall(path, _, args) = &current.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) = &current.kind {
+            let ty = cx.tables.expr_ty(&args[0]);
+            if match_type(cx, ty, &paths::MUTEX) {
+                return true;
+            }
+            current = &args[0];
+        }
+    }
+    false
+}

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",
     },

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);
     };
 }
 

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
+