diff --git a/clippy_lints/src/mutex_atomic.rs b/clippy_lints/src/mutex_atomic.rs index a93665ef3e9d..2fef8404f824 100644 --- a/clippy_lints/src/mutex_atomic.rs +++ b/clippy_lints/src/mutex_atomic.rs @@ -1,7 +1,12 @@ -use clippy_utils::diagnostics::span_lint; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::res::MaybeDef; -use rustc_hir::Expr; -use rustc_lint::{LateContext, LateLintPass}; +use clippy_utils::source::{IntoSpan, SpanRangeExt}; +use clippy_utils::sugg::Sugg; +use clippy_utils::ty::ty_from_hir_ty; +use rustc_errors::{Applicability, Diag}; +use rustc_hir::{self as hir, Expr, ExprKind, Item, ItemKind, LetStmt, QPath}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::mir::Mutability; use rustc_middle::ty::{self, IntTy, Ty, UintTy}; use rustc_session::declare_lint_pass; use rustc_span::sym; @@ -88,24 +93,83 @@ declare_clippy_lint! { declare_lint_pass!(Mutex => [MUTEX_ATOMIC, MUTEX_INTEGER]); +// NOTE: we don't use `check_expr` because that would make us lint every _use_ of such mutexes, not +// just their definitions impl<'tcx> LateLintPass<'tcx> for Mutex { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - let ty = cx.typeck_results().expr_ty(expr); - if let ty::Adt(_, subst) = ty.kind() - && ty.is_diag_item(cx, sym::Mutex) + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { + if !item.span.from_expansion() + && let ItemKind::Static(_, _, ty, body_id) = item.kind { - let mutex_param = subst.type_at(0); - if let Some(atomic_name) = get_atomic_name(mutex_param) { - let msg = format!( - "consider using an `{atomic_name}` instead of a `Mutex` here; if you just want the locking \ - behavior and not the internal type, consider using `Mutex<()>`" - ); - match *mutex_param.kind() { - ty::Uint(t) if t != UintTy::Usize => span_lint(cx, MUTEX_INTEGER, expr.span, msg), - ty::Int(t) if t != IntTy::Isize => span_lint(cx, MUTEX_INTEGER, expr.span, msg), - _ => span_lint(cx, MUTEX_ATOMIC, expr.span, msg), + let body = cx.tcx.hir_body(body_id); + let mid_ty = ty_from_hir_ty(cx, ty); + check_expr(cx, body.value.peel_blocks(), &TypeAscriptionKind::Required(ty), mid_ty); + } + } + fn check_local(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx LetStmt<'_>) { + if !stmt.span.from_expansion() + && let Some(init) = stmt.init + { + let mid_ty = cx.typeck_results().expr_ty(init); + check_expr(cx, init.peel_blocks(), &TypeAscriptionKind::Optional(stmt.ty), mid_ty); + } + } +} + +/// Whether the type ascription `: Mutex` (which we'll suggest replacing with `AtomicX`) is +/// required +enum TypeAscriptionKind<'tcx> { + /// Yes; for us, this is the case for statics + Required(&'tcx hir::Ty<'tcx>), + /// No; the ascription might've been necessary in an expression like: + /// ```ignore + /// let mutex: Mutex = Mutex::new(0); + /// ``` + /// to specify the type of `0`, but since `AtomicX` already refers to a concrete type, we won't + /// need this ascription anymore. + Optional(Option<&'tcx hir::Ty<'tcx>>), +} + +fn check_expr<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, ty_ascription: &TypeAscriptionKind<'tcx>, ty: Ty<'tcx>) { + if let ty::Adt(_, subst) = ty.kind() + && ty.is_diag_item(cx, sym::Mutex) + && let mutex_param = subst.type_at(0) + && let Some(atomic_name) = get_atomic_name(mutex_param) + { + let msg = "using a `Mutex` where an atomic would do"; + let diag = |diag: &mut Diag<'_, _>| { + // if `expr = Mutex::new(arg)`, we can try emitting a suggestion + if let ExprKind::Call(qpath, [arg]) = expr.kind + && let ExprKind::Path(QPath::TypeRelative(_mutex, new)) = qpath.kind + && new.ident.name == sym::new + { + let mut applicability = Applicability::MaybeIncorrect; + let arg = Sugg::hir_with_applicability(cx, arg, "_", &mut applicability); + let mut suggs = vec![(expr.span, format!("std::sync::atomic::{atomic_name}::new({arg})"))]; + match ty_ascription { + TypeAscriptionKind::Required(ty_ascription) => { + suggs.push((ty_ascription.span, format!("std::sync::atomic::{atomic_name}"))); + }, + TypeAscriptionKind::Optional(Some(ty_ascription)) => { + // See https://github.com/rust-lang/rust-clippy/pull/15386 for why this is + // required + let colon_ascription = (cx.sess().source_map()) + .span_extend_to_prev_char_before(ty_ascription.span, ':', true) + .with_leading_whitespace(cx) + .into_span(); + suggs.push((colon_ascription, String::new())); + }, + TypeAscriptionKind::Optional(None) => {}, // nothing to remove/replace } + diag.multipart_suggestion("try", suggs, applicability); + } else { + diag.help(format!("consider using an `{atomic_name}` instead")); } + diag.help("if you just want the locking behavior and not the internal type, consider using `Mutex<()>`"); + }; + match *mutex_param.kind() { + ty::Uint(t) if t != UintTy::Usize => span_lint_and_then(cx, MUTEX_INTEGER, expr.span, msg, diag), + ty::Int(t) if t != IntTy::Isize => span_lint_and_then(cx, MUTEX_INTEGER, expr.span, msg, diag), + _ => span_lint_and_then(cx, MUTEX_ATOMIC, expr.span, msg, diag), } } } @@ -135,7 +199,8 @@ fn get_atomic_name(ty: Ty<'_>) -> Option<&'static str> { IntTy::I128 => None, } }, - ty::RawPtr(_, _) => Some("AtomicPtr"), + // `AtomicPtr` only accepts `*mut T` + ty::RawPtr(_, Mutability::Mut) => Some("AtomicPtr"), _ => None, } } diff --git a/tests/ui/mutex_atomic.fixed b/tests/ui/mutex_atomic.fixed new file mode 100644 index 000000000000..e4218726019f --- /dev/null +++ b/tests/ui/mutex_atomic.fixed @@ -0,0 +1,67 @@ +#![warn(clippy::mutex_integer)] +#![warn(clippy::mutex_atomic)] +#![allow(clippy::borrow_as_ptr)] + +use std::sync::Mutex; + +fn main() { + let _ = std::sync::atomic::AtomicBool::new(true); + //~^ mutex_atomic + + let _ = std::sync::atomic::AtomicUsize::new(5usize); + //~^ mutex_atomic + + let _ = std::sync::atomic::AtomicIsize::new(9isize); + //~^ mutex_atomic + + let mut x = 4u32; + // `AtomicPtr` only accepts `*mut T`, so this should not lint + let _ = Mutex::new(&x as *const u32); + + let _ = std::sync::atomic::AtomicPtr::new(&mut x as *mut u32); + //~^ mutex_atomic + + let _ = std::sync::atomic::AtomicU32::new(0u32); + //~^ mutex_integer + + let _ = std::sync::atomic::AtomicI32::new(0i32); + //~^ mutex_integer + + let _ = Mutex::new(0f32); // there are no float atomics, so this should not lint + let _ = std::sync::atomic::AtomicU8::new(0u8); + //~^ mutex_integer + + let _ = std::sync::atomic::AtomicI16::new(0i16); + //~^ mutex_integer + + let _x = std::sync::atomic::AtomicI8::new(0); + //~^ mutex_integer + + const X: i64 = 0; + let _ = std::sync::atomic::AtomicI64::new(X); + //~^ mutex_integer + + // there are no 128 atomics, so these two should not lint + { + let _ = Mutex::new(0u128); + let _x: Mutex = Mutex::new(0); + } +} + +// don't lint on _use_, only declaration +fn issue13378() { + static MTX: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); + //~^ mutex_integer + + let mtx = std::sync::atomic::AtomicI32::new(0); + //~^ mutex_integer + // This will still lint, since we're reassigning the mutex to a variable -- oh well. + // But realistically something like this won't really come up. + let reassigned = mtx; + //~^ mutex_integer + + // don't eat the `)` when removing the type ascription -- see + // https://github.com/rust-lang/rust-clippy/issues/15377 + let (funky_mtx) = std::sync::atomic::AtomicU64::new(0); + //~^ mutex_integer +} diff --git a/tests/ui/mutex_atomic.rs b/tests/ui/mutex_atomic.rs index 7db5c9f274f6..95f2b135903f 100644 --- a/tests/ui/mutex_atomic.rs +++ b/tests/ui/mutex_atomic.rs @@ -2,47 +2,66 @@ #![warn(clippy::mutex_atomic)] #![allow(clippy::borrow_as_ptr)] +use std::sync::Mutex; + fn main() { - use std::sync::Mutex; - Mutex::new(true); + let _ = Mutex::new(true); //~^ mutex_atomic - Mutex::new(5usize); + let _ = Mutex::new(5usize); //~^ mutex_atomic - Mutex::new(9isize); + let _ = Mutex::new(9isize); //~^ mutex_atomic let mut x = 4u32; - Mutex::new(&x as *const u32); - //~^ mutex_atomic + // `AtomicPtr` only accepts `*mut T`, so this should not lint + let _ = Mutex::new(&x as *const u32); - Mutex::new(&mut x as *mut u32); + let _ = Mutex::new(&mut x as *mut u32); //~^ mutex_atomic - Mutex::new(0u32); + let _ = Mutex::new(0u32); //~^ mutex_integer - Mutex::new(0i32); + let _ = Mutex::new(0i32); //~^ mutex_integer - Mutex::new(0f32); // there are no float atomics, so this should not lint - Mutex::new(0u8); + let _ = Mutex::new(0f32); // there are no float atomics, so this should not lint + let _ = Mutex::new(0u8); //~^ mutex_integer - Mutex::new(0i16); + let _ = Mutex::new(0i16); //~^ mutex_integer let _x: Mutex = Mutex::new(0); //~^ mutex_integer const X: i64 = 0; - Mutex::new(X); + let _ = Mutex::new(X); //~^ mutex_integer // there are no 128 atomics, so these two should not lint { - Mutex::new(0u128); + let _ = Mutex::new(0u128); let _x: Mutex = Mutex::new(0); } } + +// don't lint on _use_, only declaration +fn issue13378() { + static MTX: Mutex = Mutex::new(0); + //~^ mutex_integer + + let mtx = Mutex::new(0); + //~^ mutex_integer + // This will still lint, since we're reassigning the mutex to a variable -- oh well. + // But realistically something like this won't really come up. + let reassigned = mtx; + //~^ mutex_integer + + // don't eat the `)` when removing the type ascription -- see + // https://github.com/rust-lang/rust-clippy/issues/15377 + let (funky_mtx): Mutex = Mutex::new(0); + //~^ mutex_integer +} diff --git a/tests/ui/mutex_atomic.stderr b/tests/ui/mutex_atomic.stderr index a6d5d60fbf05..0afc6d541dea 100644 --- a/tests/ui/mutex_atomic.stderr +++ b/tests/ui/mutex_atomic.stderr @@ -1,74 +1,134 @@ -error: consider using an `AtomicBool` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:7:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:8:13 | -LL | Mutex::new(true); - | ^^^^^^^^^^^^^^^^ +LL | let _ = Mutex::new(true); + | ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicBool::new(true)` | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` = note: `-D clippy::mutex-atomic` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::mutex_atomic)]` -error: consider using an `AtomicUsize` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:10:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:11:13 | -LL | Mutex::new(5usize); - | ^^^^^^^^^^^^^^^^^^ - -error: consider using an `AtomicIsize` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:13:5 +LL | let _ = Mutex::new(5usize); + | ^^^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicUsize::new(5usize)` | -LL | Mutex::new(9isize); - | ^^^^^^^^^^^^^^^^^^ + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` -error: consider using an `AtomicPtr` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:17:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:14:13 + | +LL | let _ = Mutex::new(9isize); + | ^^^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicIsize::new(9isize)` | -LL | Mutex::new(&x as *const u32); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` -error: consider using an `AtomicPtr` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:20:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:21:13 | -LL | Mutex::new(&mut x as *mut u32); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | let _ = Mutex::new(&mut x as *mut u32); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicPtr::new(&mut x as *mut u32)` + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` -error: consider using an `AtomicU32` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:23:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:24:13 | -LL | Mutex::new(0u32); - | ^^^^^^^^^^^^^^^^ +LL | let _ = Mutex::new(0u32); + | ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicU32::new(0u32)` | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` = note: `-D clippy::mutex-integer` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::mutex_integer)]` -error: consider using an `AtomicI32` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:26:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:27:13 + | +LL | let _ = Mutex::new(0i32); + | ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicI32::new(0i32)` | -LL | Mutex::new(0i32); - | ^^^^^^^^^^^^^^^^ + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` -error: consider using an `AtomicU8` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:30:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:31:13 | -LL | Mutex::new(0u8); - | ^^^^^^^^^^^^^^^ +LL | let _ = Mutex::new(0u8); + | ^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicU8::new(0u8)` + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` -error: consider using an `AtomicI16` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:33:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:34:13 + | +LL | let _ = Mutex::new(0i16); + | ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicI16::new(0i16)` | -LL | Mutex::new(0i16); - | ^^^^^^^^^^^^^^^^ + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` -error: consider using an `AtomicI8` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:36:25 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:37:25 | LL | let _x: Mutex = Mutex::new(0); | ^^^^^^^^^^^^^ + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` +help: try + | +LL - let _x: Mutex = Mutex::new(0); +LL + let _x = std::sync::atomic::AtomicI8::new(0); + | + +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:41:13 + | +LL | let _ = Mutex::new(X); + | ^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicI64::new(X)` + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` + +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:53:30 + | +LL | static MTX: Mutex = Mutex::new(0); + | ^^^^^^^^^^^^^ + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` +help: try + | +LL - static MTX: Mutex = Mutex::new(0); +LL + static MTX: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); + | + +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:56:15 + | +LL | let mtx = Mutex::new(0); + | ^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicI32::new(0)` + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` -error: consider using an `AtomicI64` instead of a `Mutex` here; if you just want the locking behavior and not the internal type, consider using `Mutex<()>` - --> tests/ui/mutex_atomic.rs:40:5 +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:60:22 + | +LL | let reassigned = mtx; + | ^^^ + | + = help: consider using an `AtomicI32` instead + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` + +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic.rs:65:35 + | +LL | let (funky_mtx): Mutex = Mutex::new(0); + | ^^^^^^^^^^^^^ + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` +help: try + | +LL - let (funky_mtx): Mutex = Mutex::new(0); +LL + let (funky_mtx) = std::sync::atomic::AtomicU64::new(0); | -LL | Mutex::new(X); - | ^^^^^^^^^^^^^ -error: aborting due to 11 previous errors +error: aborting due to 14 previous errors diff --git a/tests/ui/mutex_atomic_unfixable.rs b/tests/ui/mutex_atomic_unfixable.rs new file mode 100644 index 000000000000..0c04f48cf8a9 --- /dev/null +++ b/tests/ui/mutex_atomic_unfixable.rs @@ -0,0 +1,13 @@ +//@no-rustfix +#![warn(clippy::mutex_atomic, clippy::mutex_integer)] + +use std::sync::Mutex; + +fn issue13378() { + static MTX: Mutex = Mutex::new(0); + //~^ mutex_integer + + // unfixable because we don't fix this `lock` + let mut guard = MTX.lock().unwrap(); + *guard += 1; +} diff --git a/tests/ui/mutex_atomic_unfixable.stderr b/tests/ui/mutex_atomic_unfixable.stderr new file mode 100644 index 000000000000..27ffb1304c69 --- /dev/null +++ b/tests/ui/mutex_atomic_unfixable.stderr @@ -0,0 +1,17 @@ +error: using a `Mutex` where an atomic would do + --> tests/ui/mutex_atomic_unfixable.rs:7:30 + | +LL | static MTX: Mutex = Mutex::new(0); + | ^^^^^^^^^^^^^ + | + = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>` + = note: `-D clippy::mutex-integer` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::mutex_integer)]` +help: try + | +LL - static MTX: Mutex = Mutex::new(0); +LL + static MTX: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); + | + +error: aborting due to 1 previous error +