From f8aa0431bdea0b5adb8a53a7a8eb32b091759561 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Wed, 7 Oct 2015 01:17:57 +0200 Subject: [PATCH 1/3] Suggest using an atomic value instead of a Mutex where possible --- README.md | 3 +- src/lib.rs | 3 ++ src/mutex_atomic.rs | 51 ++++++++++++++++++++++++++++++ src/utils.rs | 1 + tests/compile-fail/mutex_atomic.rs | 15 +++++++++ 5 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 src/mutex_atomic.rs create mode 100644 tests/compile-fail/mutex_atomic.rs diff --git a/README.md b/README.md index a5ab856fc21..a09a9fefc35 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 59 lints included in this crate: +There are 60 lints included in this crate: name | default | meaning -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -37,6 +37,7 @@ name [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max) | warn | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut) | allow | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) +[mutex_atomic](https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic) | warn | using a Mutex where an atomic value could be used instead [needless_bool](https://github.com/Manishearth/rust-clippy/wiki#needless_bool) | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` [needless_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes) | warn | using explicit lifetimes for references in function arguments when elision rules would allow omitting them [needless_range_loop](https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop) | warn | for-looping over a range of indices where an iterator over items would do diff --git a/src/lib.rs b/src/lib.rs index 2f71d8cc9df..0e560978979 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -47,6 +47,7 @@ pub mod loops; pub mod ranges; pub mod matches; pub mod precedence; +pub mod mutex_atomic; mod reexport { pub use syntax::ast::{Name, Ident, NodeId}; @@ -88,6 +89,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box matches::MatchPass); reg.register_late_lint_pass(box misc::PatternPass); reg.register_late_lint_pass(box minmax::MinMaxPass); + reg.register_late_lint_pass(box mutex_atomic::MutexAtomic); reg.register_lint_group("clippy_pedantic", vec![ methods::OPTION_UNWRAP_USED, @@ -141,6 +143,7 @@ pub fn plugin_registrar(reg: &mut Registry) { misc::REDUNDANT_PATTERN, misc::TOPLEVEL_REF_ARG, mut_reference::UNNECESSARY_MUT_PASSED, + mutex_atomic::MUTEX_ATOMIC, needless_bool::NEEDLESS_BOOL, precedence::PRECEDENCE, ranges::RANGE_STEP_BY_ZERO, diff --git a/src/mutex_atomic.rs b/src/mutex_atomic.rs new file mode 100644 index 00000000000..6cf71fe23bc --- /dev/null +++ b/src/mutex_atomic.rs @@ -0,0 +1,51 @@ +//! Checks for uses of Mutex where an atomic value could be used +//! +//! This lint is **warn** by default + +use rustc::lint::{LintPass, LintArray, LateLintPass, LateContext}; +use rustc_front::hir::Expr; + +use syntax::ast; +use rustc::middle::ty; +use rustc::middle::subst::ParamSpace; + +use utils::{span_lint, MUTEX_PATH, match_type}; + +declare_lint! { + pub MUTEX_ATOMIC, + Warn, + "using a Mutex where an atomic value could be used instead" +} + +impl LintPass for MutexAtomic { + fn get_lints(&self) -> LintArray { + lint_array!(MUTEX_ATOMIC) + } +} +pub struct MutexAtomic; + +impl LateLintPass for MutexAtomic { + fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { + let ty = cx.tcx.expr_ty(expr); + if let &ty::TyStruct(_, subst) = &ty.sty { + if match_type(cx, ty, &MUTEX_PATH) { + let mutex_param = &subst.types.get(ParamSpace::TypeSpace, 0).sty; + if let Some(atomic_name) = get_atomic_name(mutex_param) { + let msg = format!("Consider using an {} instead of a \ + Mutex here.", atomic_name); + span_lint(cx, MUTEX_ATOMIC, expr.span, &msg); + } + } + } + } +} + +fn get_atomic_name(ty: &ty::TypeVariants) -> Option<(&'static str)> { + match *ty { + ty::TyBool => Some("AtomicBool"), + ty::TyUint(ast::TyUs) => Some("AtomicUsize"), + ty::TyInt(ast::TyIs) => Some("AtomicIsize"), + ty::TyRawPtr(_) => Some("AtomicPtr"), + _ => None + } +} diff --git a/src/utils.rs b/src/utils.rs index ac155617011..4c9f1fb4b3e 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -14,6 +14,7 @@ pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"]; pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"]; pub const VEC_PATH: [&'static str; 3] = ["collections", "vec", "Vec"]; pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"]; +pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"]; /// returns true this expn_info was expanded by any macro pub fn in_macro(cx: &LateContext, span: Span) -> bool { diff --git a/tests/compile-fail/mutex_atomic.rs b/tests/compile-fail/mutex_atomic.rs new file mode 100644 index 00000000000..97e08d7ba36 --- /dev/null +++ b/tests/compile-fail/mutex_atomic.rs @@ -0,0 +1,15 @@ +#![feature(plugin)] + +#![plugin(clippy)] +#![deny(clippy)] + +fn main() { + use std::sync::Mutex; + Mutex::new(true); //~ERROR Consider using an AtomicBool instead of a Mutex here. + Mutex::new(5usize); //~ERROR Consider using an AtomicUsize instead of a Mutex here. + Mutex::new(9isize); //~ERROR Consider using an AtomicIsize instead of a Mutex here. + let mut x = 4u32; + Mutex::new(&x as *const u32); //~ERROR Consider using an AtomicPtr instead of a Mutex here. + Mutex::new(&mut x as *mut u32); //~ERROR Consider using an AtomicPtr instead of a Mutex here. + Mutex::new(0f32); // there are no float atomics, so this should not lint +} From 26b2733b15a0011ff1569e059ae7520d1a3a519d Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Wed, 7 Oct 2015 22:58:34 +0200 Subject: [PATCH 2/3] Add a lint for sized integer types in a mutex --- README.md | 3 ++- src/lib.rs | 1 + src/mutex_atomic.rs | 21 +++++++++++++++++---- tests/compile-fail/mutex_atomic.rs | 3 +++ 4 files changed, 23 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index a09a9fefc35..50789cc3db2 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 60 lints included in this crate: +There are 61 lints included in this crate: name | default | meaning -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -38,6 +38,7 @@ name [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut) | allow | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) [mutex_atomic](https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic) | warn | using a Mutex where an atomic value could be used instead +[mutex_integer](https://github.com/Manishearth/rust-clippy/wiki#mutex_integer) | allow | using a Mutex for an integer type [needless_bool](https://github.com/Manishearth/rust-clippy/wiki#needless_bool) | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` [needless_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes) | warn | using explicit lifetimes for references in function arguments when elision rules would allow omitting them [needless_range_loop](https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop) | warn | for-looping over a range of indices where an iterator over items would do diff --git a/src/lib.rs b/src/lib.rs index 0e560978979..3483efea868 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,6 +96,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::RESULT_UNWRAP_USED, methods::WRONG_PUB_SELF_CONVENTION, mut_mut::MUT_MUT, + mutex_atomic::MUTEX_INTEGER, ptr_arg::PTR_ARG, shadow::SHADOW_REUSE, shadow::SHADOW_SAME, diff --git a/src/mutex_atomic.rs b/src/mutex_atomic.rs index 6cf71fe23bc..994e7937984 100644 --- a/src/mutex_atomic.rs +++ b/src/mutex_atomic.rs @@ -17,11 +17,18 @@ declare_lint! { "using a Mutex where an atomic value could be used instead" } +declare_lint! { + pub MUTEX_INTEGER, + Allow, + "using a Mutex for an integer type" +} + impl LintPass for MutexAtomic { fn get_lints(&self) -> LintArray { - lint_array!(MUTEX_ATOMIC) + lint_array!(MUTEX_ATOMIC, MUTEX_INTEGER) } } + pub struct MutexAtomic; impl LateLintPass for MutexAtomic { @@ -33,7 +40,13 @@ impl LateLintPass for MutexAtomic { if let Some(atomic_name) = get_atomic_name(mutex_param) { let msg = format!("Consider using an {} instead of a \ Mutex here.", atomic_name); - span_lint(cx, MUTEX_ATOMIC, expr.span, &msg); + match *mutex_param { + ty::TyUint(t) if t != ast::TyUs => + span_lint(cx, MUTEX_INTEGER, expr.span, &msg), + ty::TyInt(t) if t != ast::TyIs => + span_lint(cx, MUTEX_INTEGER, expr.span, &msg), + _ => span_lint(cx, MUTEX_ATOMIC, expr.span, &msg) + } } } } @@ -43,8 +56,8 @@ impl LateLintPass for MutexAtomic { fn get_atomic_name(ty: &ty::TypeVariants) -> Option<(&'static str)> { match *ty { ty::TyBool => Some("AtomicBool"), - ty::TyUint(ast::TyUs) => Some("AtomicUsize"), - ty::TyInt(ast::TyIs) => Some("AtomicIsize"), + ty::TyUint(_) => Some("AtomicUsize"), + ty::TyInt(_) => Some("AtomicIsize"), ty::TyRawPtr(_) => Some("AtomicPtr"), _ => None } diff --git a/tests/compile-fail/mutex_atomic.rs b/tests/compile-fail/mutex_atomic.rs index 97e08d7ba36..20a34ba5547 100644 --- a/tests/compile-fail/mutex_atomic.rs +++ b/tests/compile-fail/mutex_atomic.rs @@ -2,6 +2,7 @@ #![plugin(clippy)] #![deny(clippy)] +#![deny(mutex_integer)] fn main() { use std::sync::Mutex; @@ -11,5 +12,7 @@ fn main() { let mut x = 4u32; Mutex::new(&x as *const u32); //~ERROR Consider using an AtomicPtr instead of a Mutex here. Mutex::new(&mut x as *mut u32); //~ERROR Consider using an AtomicPtr instead of a Mutex here. + Mutex::new(0u32); //~ERROR Consider using an AtomicUsize instead of a Mutex here. + Mutex::new(0i32); //~ERROR Consider using an AtomicIsize instead of a Mutex here. Mutex::new(0f32); // there are no float atomics, so this should not lint } From b48db27152caf31c892312bc8a8ea8db7418e157 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Sun, 11 Oct 2015 16:07:00 +0200 Subject: [PATCH 3/3] Recommend using Mutex<()> for locking --- src/mutex_atomic.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/mutex_atomic.rs b/src/mutex_atomic.rs index 994e7937984..9c10a062419 100644 --- a/src/mutex_atomic.rs +++ b/src/mutex_atomic.rs @@ -39,7 +39,10 @@ impl LateLintPass for MutexAtomic { let mutex_param = &subst.types.get(ParamSpace::TypeSpace, 0).sty; if let Some(atomic_name) = get_atomic_name(mutex_param) { let msg = format!("Consider using an {} instead of a \ - Mutex here.", atomic_name); + Mutex here. If you just want the \ + locking behaviour and not the internal \ + type, consider using Mutex<()>.", + atomic_name); match *mutex_param { ty::TyUint(t) if t != ast::TyUs => span_lint(cx, MUTEX_INTEGER, expr.span, &msg),