diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index bff1952cce77..577e0e206e0a 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/mutex_mutable_self.rs b/clippy_lints/src/mutex_mutable_self.rs new file mode 100644 index 000000000000..9bea0ddeba33 --- /dev/null +++ b/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 + }, + _ => {}, + } + } + } + } +} + diff --git a/src/lib.rs b/src/lib.rs index 76e1c711656b..d1b08151c42c 100644 --- a/src/lib.rs +++ b/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() { diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index f44ef226847f..2233ce2e621a 100644 --- a/src/lintlist/mod.rs +++ b/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", diff --git a/tests/ui/mutex_mutable_self.rs b/tests/ui/mutex_mutable_self.rs new file mode 100644 index 000000000000..35efac006682 --- /dev/null +++ b/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 { + *m.lock().unwrap() +} + +fn get_value(m: &Mutex) -> u32 { + *m.lock().unwrap() +} + +fn mk_mutex() -> Mutex { + 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(); +} diff --git a/tests/ui/mutex_mutable_self.stderr b/tests/ui/mutex_mutable_self.stderr new file mode 100644 index 000000000000..010bf70c29eb --- /dev/null +++ b/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 +