progress work on suggestion for auto fix

DevinR528 · DevinR528 · commit 8af9b4921d52 · 2020-04-10T06:20:46.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1284,6 +1284,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
@@ -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) = &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
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -220,6 +220,7 @@ mod functions;
 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;
@@ -566,6 +567,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,
@@ -1039,6 +1041,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box verbose_file_reads::VerboseFileReads);
     store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
     store.register_late_pass(|| box unnamed_address::UnnamedAddress);
+    store.register_late_pass(|| box if_let_mutex::IfLetMutex);
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
         LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1214,6 +1217,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),
@@ -1600,6 +1604,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
@@ -717,6 +717,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         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",
diff --git a/tests/ui/if_let_mutex.rs 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() {}
