suggest adjusting the type ascription

Author: ada4a

clippy_lints/src/mutex_atomic.rs

@@ -1,10 +1,11 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::res::MaybeDef;
+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::{Expr, ExprKind, Item, ItemKind, LetStmt, QPath};
-use rustc_lint::{LateContext, LateLintPass};
+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;
@@ -101,20 +102,34 @@ impl<'tcx> LateLintPass<'tcx> for Mutex {
         {
             let body = cx.tcx.hir_body(body_id);
             let mid_ty = ty_from_hir_ty(cx, ty);
-            check_expr(cx, body.value.peel_blocks(), mid_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(), mid_ty);
+            check_expr(cx, init.peel_blocks(), &TypeAscriptionKind::Optional(stmt.ty), mid_ty);
         }
     }
 }
 
-fn check_expr<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, ty: Ty<'tcx>) {
+/// Whether the type ascription `: Mutex<X>` (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<u64> = 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)
@@ -129,8 +144,22 @@ fn check_expr<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, ty: Ty<'tcx>) {
             {
                 let mut applicability = Applicability::MaybeIncorrect;
                 let arg = Sugg::hir_with_applicability(cx, arg, "_", &mut applicability);
-
-                let suggs = vec![(expr.span, format!("std::sync::atomic::{atomic_name}::new({arg})"))];
+                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"));

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<i128> = 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
+}

tests/ui/mutex_atomic.rs

@@ -1,4 +1,3 @@
-//@no-rustfix
 #![warn(clippy::mutex_integer)]
 #![warn(clippy::mutex_atomic)]
 #![allow(clippy::borrow_as_ptr)]
@@ -54,13 +53,15 @@ fn issue13378() {
     static MTX: Mutex<u32> = Mutex::new(0);
     //~^ mutex_integer
 
-    let mut guard = MTX.lock().unwrap();
-    *guard += 1;
-
     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<u64> = Mutex::new(0);
+    //~^ mutex_integer
 }

tests/ui/mutex_atomic.stderr

@@ -1,5 +1,5 @@
 error: using a `Mutex` where an atomic would do
-  --> tests/ui/mutex_atomic.rs:9:13
+  --> tests/ui/mutex_atomic.rs:8:13
    |
 LL |     let _ = Mutex::new(true);
    |             ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicBool::new(true)`
@@ -9,31 +9,31 @@ LL |     let _ = Mutex::new(true);
    = help: to override `-D warnings` add `#[allow(clippy::mutex_atomic)]`
 
 error: using a `Mutex` where an atomic would do
-  --> tests/ui/mutex_atomic.rs:12:13
+  --> tests/ui/mutex_atomic.rs:11:13
    |
 LL |     let _ = Mutex::new(5usize);
    |             ^^^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicUsize::new(5usize)`
    |
    = 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:15:13
+  --> tests/ui/mutex_atomic.rs:14:13
    |
 LL |     let _ = Mutex::new(9isize);
    |             ^^^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicIsize::new(9isize)`
    |
    = 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:22:13
+  --> tests/ui/mutex_atomic.rs:21:13
    |
 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: using a `Mutex` where an atomic would do
-  --> tests/ui/mutex_atomic.rs:25:13
+  --> tests/ui/mutex_atomic.rs:24:13
    |
 LL |     let _ = Mutex::new(0u32);
    |             ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicU32::new(0u32)`
@@ -43,69 +43,92 @@ LL |     let _ = Mutex::new(0u32);
    = help: to override `-D warnings` add `#[allow(clippy::mutex_integer)]`
 
 error: using a `Mutex` where an atomic would do
-  --> tests/ui/mutex_atomic.rs:28:13
+  --> tests/ui/mutex_atomic.rs:27:13
    |
 LL |     let _ = Mutex::new(0i32);
    |             ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicI32::new(0i32)`
    |
    = 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:32:13
+  --> tests/ui/mutex_atomic.rs:31:13
    |
 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: using a `Mutex` where an atomic would do
-  --> tests/ui/mutex_atomic.rs:35:13
+  --> tests/ui/mutex_atomic.rs:34:13
    |
 LL |     let _ = Mutex::new(0i16);
    |             ^^^^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicI16::new(0i16)`
    |
    = 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:38:25
+  --> tests/ui/mutex_atomic.rs:37:25
    |
 LL |     let _x: Mutex<i8> = Mutex::new(0);
-   |                         ^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicI8::new(0)`
+   |                         ^^^^^^^^^^^^^
    |
    = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>`
+help: try
+   |
+LL -     let _x: Mutex<i8> = 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:42:13
+  --> 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:54:30
+  --> tests/ui/mutex_atomic.rs:53:30
    |
 LL |     static MTX: Mutex<u32> = Mutex::new(0);
-   |                              ^^^^^^^^^^^^^ help: try: `std::sync::atomic::AtomicU32::new(0)`
+   |                              ^^^^^^^^^^^^^
    |
    = help: if you just want the locking behavior and not the internal type, consider using `Mutex<()>`
+help: try
+   |
+LL -     static MTX: Mutex<u32> = 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:60:15
+  --> 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: using a `Mutex` where an atomic would do
-  --> tests/ui/mutex_atomic.rs:64:22
+  --> 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: aborting due to 13 previous errors
+error: using a `Mutex` where an atomic would do
+  --> tests/ui/mutex_atomic.rs:65:35
+   |
+LL |     let (funky_mtx): Mutex<u64> = 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<u64> = Mutex::new(0);
+LL +     let (funky_mtx) = std::sync::atomic::AtomicU64::new(0);
+   |
+
+error: aborting due to 14 previous errors
 

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<u32> = Mutex::new(0);
+    //~^ mutex_integer
+
+    // unfixable because we don't fix this `lock`
+    let mut guard = MTX.lock().unwrap();
+    *guard += 1;
+}

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<u32> = 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<u32> = Mutex::new(0);
+LL +     static MTX: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0);
+   |
+
+error: aborting due to 1 previous error
+