From f82f9c2c55392ef9d7649bf2e485f7e509fd0038 Mon Sep 17 00:00:00 2001 From: Francis Murillo Date: Fri, 2 Oct 2020 09:05:30 +0800 Subject: [PATCH] Add lint for `&mut Mutex::lock` --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 7 +++ clippy_lints/src/mut_mutex_lock.rs | 75 ++++++++++++++++++++++++++++++ src/lintlist/mod.rs | 7 +++ tests/ui/mut_mutex_lock.rs | 19 ++++++++ tests/ui/mut_mutex_lock.stderr | 19 ++++++++ 6 files changed, 128 insertions(+) create mode 100644 clippy_lints/src/mut_mutex_lock.rs create mode 100644 tests/ui/mut_mutex_lock.rs create mode 100644 tests/ui/mut_mutex_lock.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d995cc9701..ba080835f5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1843,6 +1843,7 @@ Released 2018-09-13 [`must_use_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#must_use_unit [`mut_from_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref [`mut_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_mut +[`mut_mutex_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_mutex_lock [`mut_range_bound`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_range_bound [`mutable_key_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type [`mutex_atomic`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutex_atomic diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8d53b9799f0..3a108bcfe6a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -255,6 +255,7 @@ macro_rules! declare_clippy_lint { mod multiple_crate_versions; mod mut_key; mod mut_mut; +mod mut_mutex_lock; mod mut_reference; mod mutable_debug_assertion; mod mutex_atomic; @@ -744,6 +745,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &multiple_crate_versions::MULTIPLE_CRATE_VERSIONS, &mut_key::MUTABLE_KEY_TYPE, &mut_mut::MUT_MUT, + &mut_mutex_lock::MUT_MUTEX_LOCK, &mut_reference::UNNECESSARY_MUT_PASSED, &mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL, &mutex_atomic::MUTEX_ATOMIC, @@ -1112,6 +1114,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: 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_late_pass(|| box mut_mutex_lock::MutMutexLock); store.register_late_pass(|| box match_on_vec_items::MatchOnVecItems); store.register_early_pass(|| box manual_non_exhaustive::ManualNonExhaustive); store.register_late_pass(|| box manual_async_fn::ManualAsyncFn); @@ -1446,6 +1449,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&misc_early::UNNEEDED_WILDCARD_PATTERN), LintId::of(&misc_early::ZERO_PREFIXED_LITERAL), LintId::of(&mut_key::MUTABLE_KEY_TYPE), + LintId::of(&mut_mutex_lock::MUT_MUTEX_LOCK), LintId::of(&mut_reference::UNNECESSARY_MUT_PASSED), LintId::of(&mutex_atomic::MUTEX_ATOMIC), LintId::of(&needless_arbitrary_self_type::NEEDLESS_ARBITRARY_SELF_TYPE), @@ -1780,6 +1784,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&misc::FLOAT_CMP), LintId::of(&misc::MODULO_ONE), LintId::of(&mut_key::MUTABLE_KEY_TYPE), + LintId::of(&mut_mutex_lock::MUT_MUTEX_LOCK), + LintId::of(&non_copy_const::BORROW_INTERIOR_MUTABLE_CONST), + LintId::of(&non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST), LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP), LintId::of(&ptr::MUT_FROM_REF), diff --git a/clippy_lints/src/mut_mutex_lock.rs b/clippy_lints/src/mut_mutex_lock.rs new file mode 100644 index 00000000000..4f3108355ca --- /dev/null +++ b/clippy_lints/src/mut_mutex_lock.rs @@ -0,0 +1,75 @@ +use crate::utils::{is_type_diagnostic_item, span_lint_and_help}; +use if_chain::if_chain; +use rustc_hir::{Expr, ExprKind, Mutability}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks for `&mut Mutex::lock` calls + /// + /// **Why is this bad?** `Mutex::lock` is less efficient than + /// calling `Mutex::get_mut` + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// use std::sync::{Arc, Mutex}; + /// + /// let mut value_rc = Arc::new(Mutex::new(42_u8)); + /// let value_mutex = Arc::get_mut(&mut value_rc).unwrap(); + /// + /// let value = value_mutex.lock().unwrap(); + /// do_stuff(value); + /// ``` + /// Use instead: + /// ```rust + /// use std::sync::{Arc, Mutex}; + /// + /// let mut value_rc = Arc::new(Mutex::new(42_u8)); + /// let value_mutex = Arc::get_mut(&mut value_rc).unwrap(); + /// + /// let value = value_mutex.get_mut().unwrap(); + /// do_stuff(value); + /// ``` + pub MUT_MUTEX_LOCK, + correctness, + "`&mut Mutex::lock` does unnecessary locking" +} + +declare_lint_pass!(MutMutexLock => [MUT_MUTEX_LOCK]); + +impl<'tcx> LateLintPass<'tcx> for MutMutexLock { + fn check_expr(&mut self, cx: &LateContext<'tcx>, ex: &'tcx Expr<'tcx>) { + if_chain! { + if is_mut_mutex_lock_call(cx, ex).is_some(); + then { + span_lint_and_help( + cx, + MUT_MUTEX_LOCK, + ex.span, + "calling `&mut Mutex::lock` unnecessarily locks an exclusive (mutable) reference", + None, + "use `&mut Mutex::get_mut` instead", + ); + } + } + } +} + +fn is_mut_mutex_lock_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { + if_chain! { + if let ExprKind::MethodCall(path, _span, args, _) = &expr.kind; + if path.ident.name == sym!(lock); + let ty = cx.typeck_results().expr_ty(&args[0]); + if let ty::Ref(_, inner_ty, Mutability::Mut) = ty.kind(); + if is_type_diagnostic_item(cx, inner_ty, sym!(mutex_type)); + then { + Some(&args[0]) + } else { + None + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 98ad994ea7b..5e48757a4c5 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1502,6 +1502,13 @@ deprecation: None, module: "mut_mut", }, + Lint { + name: "mut_mutex_lock", + group: "correctness", + desc: "`&mut Mutex::lock` does unnecessary locking", + deprecation: None, + module: "mut_mutex_lock", + }, Lint { name: "mut_range_bound", group: "complexity", diff --git a/tests/ui/mut_mutex_lock.rs b/tests/ui/mut_mutex_lock.rs new file mode 100644 index 00000000000..516d44bb7a9 --- /dev/null +++ b/tests/ui/mut_mutex_lock.rs @@ -0,0 +1,19 @@ +#![warn(clippy::mut_mutex_lock)] + +use std::sync::{Arc, Mutex}; + +fn mut_mutex_lock() { + let mut value_rc = Arc::new(Mutex::new(42_u8)); + let value_mutex = Arc::get_mut(&mut value_rc).unwrap(); + + let value = value_mutex.lock().unwrap(); + *value += 1; +} + +fn no_owned_mutex_lock() { + let mut value_rc = Arc::new(Mutex::new(42_u8)); + let value = value_rc.lock().unwrap(); + *value += 1; +} + +fn main() {} diff --git a/tests/ui/mut_mutex_lock.stderr b/tests/ui/mut_mutex_lock.stderr new file mode 100644 index 00000000000..426e0240830 --- /dev/null +++ b/tests/ui/mut_mutex_lock.stderr @@ -0,0 +1,19 @@ +error[E0596]: cannot borrow `value` as mutable, as it is not declared as mutable + --> $DIR/mut_mutex_lock.rs:10:6 + | +LL | let value = value_mutex.lock().unwrap(); + | ----- help: consider changing this to be mutable: `mut value` +LL | *value += 1; + | ^^^^^ cannot borrow as mutable + +error[E0596]: cannot borrow `value` as mutable, as it is not declared as mutable + --> $DIR/mut_mutex_lock.rs:16:6 + | +LL | let value = value_rc.lock().unwrap(); + | ----- help: consider changing this to be mutable: `mut value` +LL | *value += 1; + | ^^^^^ cannot borrow as mutable + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0596`. -- 2.44.0