[WIP] initial work on #1765 (Mutex::get_mut instead of lock)

clippy_lints/src/lib.rs

@@ -231,6 +231,7 @@ pub mod multiple_crate_versions;
 pub mod mut_mut;
 pub mod mut_reference;
 pub mod mutex_atomic;
+pub mod mutex_mutable_self;
 pub mod needless_bool;
 pub mod needless_borrow;
 pub mod needless_borrowed_ref;
@@ -610,6 +611,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
     reg.register_late_lint_pass(box comparison_chain::ComparisonChain);
     reg.register_late_lint_pass(box mul_add::MulAddCheck);
     reg.register_late_lint_pass(box unused_self::UnusedSelf);
+    reg.register_late_lint_pass(box mutex_mutable_self::MutexMutableSelf);
 
     reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -1011,6 +1013,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
         misc_early::REDUNDANT_PATTERN,
         misc_early::UNNEEDED_FIELD_PATTERN,
         mut_reference::UNNECESSARY_MUT_PASSED,
+        mutex_mutable_self::MUTEX_MUTABLE_SELF,
         neg_multiply::NEG_MULTIPLY,
         new_without_default::NEW_WITHOUT_DEFAULT,
         non_expressive_names::JUST_UNDERSCORES_AND_DIGITS,

clippy_lints/src/mutex_mutable_self.rs

@@ -0,0 +1,69 @@
+#![allow(rustc::usage_of_ty_tykind)]
+
+//! Suggest `get_mut` instead of `lock` on `Mutex` when the mutex is in scope as a mutable variable
+//! (currently doesn't properly work when the Mutex is owned by the scope calling `lock`)
+
+use if_chain::if_chain;
+
+use rustc::{declare_lint_pass, declare_tool_lint};
+use rustc::hir::*;
+use rustc::lint::{LateContext, LateLintPass, LintPass, LintArray};
+use rustc::ty::TyKind;
+
+use crate::utils::{match_type, span_help_and_lint, paths};
+
+declare_clippy_lint! {
+    /// **What it does**: Checks for unnecessary calls to `Mutex::lock` when `Mutex::get_mut` would suffice.
+    ///
+    /// **Why is this bad?** Calling `Mutex::get_mut` only needs to access the inner value,
+    /// as it's statically guaranteed that no other thread is concurrently accessing it.
+    /// `Mutex::lock` is much more expensive.
+    ///
+    /// **Known problems**:
+    ///   * doesn't correctly detect the case where the Mutex is owned by this scope,
+    ///     so only warns for &mut self
+    ///
+    /// **Example**:
+    /// // TODO, see tests/ui/mutex_mutable_self.rs
+    ///
+    pub MUTEX_MUTABLE_SELF,
+    style,
+    "usage of `Mutex::lock` when `Mutex::get_mut` would suffice (i.e. self is a mutable ref)"
+}
+
+declare_lint_pass!(MutexMutableSelf => [MUTEX_MUTABLE_SELF]);
+
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MutexMutableSelf {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if_chain! {
+            if let ExprKind::MethodCall(ref method_name, _, ref args) = expr.kind;
+            if method_name.ident.name == sym!(lock);
+            if args.len() == 1; // mutex.lock() has no params, just the receiver
+            let type_of_receiver = cx.tables.expr_ty(&args[0]);
+
+            then {
+                match type_of_receiver.kind {
+                    TyKind::Ref(_region, ty, mutability) => {
+                        if match_type(cx, ty, &paths::MUTEX) && mutability == Mutability::MutMutable {
+                            span_help_and_lint(
+                                cx,
+                                MUTEX_MUTABLE_SELF,
+                                expr.span,
+                                "exclusive mutex variable",
+                                &format!("consider `Mutex::get_mut` instead"),
+                            );
+                        }
+                    },
+                    // i think that receiving self by value (at least sometimes) matches this one
+                    TyKind::Adt(adtdef, substs) => {
+                        // TODO: not sure how to get the type here; the Debug fmt shows
+                        // 'std::sync::Mutex' though, so there must be a way i presume
+                    },
+                    _ => {},
+                }
+            }
+        }
+    }
+}
+

src/lib.rs

@@ -1,5 +1,4 @@
 // error-pattern:cargo-clippy
-#![feature(plugin_registrar)]
 #![feature(rustc_private)]
 #![warn(rust_2018_idioms)]
 
@@ -9,7 +8,6 @@
 extern crate rustc_driver;
 use self::rustc_driver::plugin::Registry;
 
-#[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry<'_>) {
     reg.sess.lint_store.with_read_lock(|lint_store| {
         for (lint, _, _) in lint_store.get_lint_groups() {

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 331] = [
+pub const ALL_LINTS: [Lint; 332] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1211,6 +1211,13 @@ pub const ALL_LINTS: [Lint; 331] = [
         deprecation: None,
         module: "mutex_atomic",
     },
+    Lint {
+        name: "mutex_mutable_self",
+        group: "style",
+        desc: "using `Mutex::lock` when `Mutex::get_mut` would suffice",
+        deprecation: None,
+        module: "mutex_mutable_self",
+    },
     Lint {
         name: "mutex_integer",
         group: "nursery",

tests/ui/mutex_mutable_self.rs

@@ -0,0 +1,28 @@
+#![allow(clippy::all)]
+#![warn(clippy::mutex_mutable_self)]
+#![allow(unused)]
+
+use std::sync::Mutex;
+
+
+fn get_value_mut(m: &mut Mutex<u32>) -> u32 {
+    *m.lock().unwrap()
+}
+
+fn get_value(m: &Mutex<u32>) -> u32 {
+    *m.lock().unwrap()
+}
+
+fn mk_mutex() -> Mutex<u32> {
+    Mutex::new(10)
+}
+
+fn main() {
+    let mut m = Mutex::new(42);
+    *m.lock().unwrap() = 64;
+
+    let _ = get_value(&m);
+    let _ = get_value_mut(&mut m);
+
+    let _ = mk_mutex().lock().unwrap();
+}

tests/ui/mutex_mutable_self.stderr

@@ -0,0 +1,11 @@
+error: exclusive mutex variable
+  --> $DIR/mutex_mutable_self.rs:9:6
+   |
+LL |     *m.lock().unwrap()
+   |      ^^^^^^^^
+   |
+   = note: `-D clippy::mutex-mutable-self` implied by `-D warnings`
+   = help: consider `Mutex::get_mut` instead
+
+error: aborting due to previous error
+