diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cb2755be0ee..647cdc9bf96f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6833,6 +6833,8 @@ Released 2018-09-13 [`return_self_not_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#return_self_not_must_use [`reverse_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#reverse_range_loop [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges +[`rwlock_atomic`]: https://rust-lang.github.io/rust-clippy/master/index.html#rwlock_atomic +[`rwlock_integer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rwlock_integer [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition [`same_item_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push [`same_name_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_name_method diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index a754eea31165..d3b8ad44c7c3 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -662,6 +662,8 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::returns::LET_AND_RETURN_INFO, crate::returns::NEEDLESS_RETURN_INFO, crate::returns::NEEDLESS_RETURN_WITH_QUESTION_MARK_INFO, + crate::rwlock_atomic::RWLOCK_ATOMIC_INFO, + crate::rwlock_atomic::RWLOCK_INTEGER_INFO, crate::same_name_method::SAME_NAME_METHOD_INFO, crate::self_named_constructors::SELF_NAMED_CONSTRUCTORS_INFO, crate::semicolon_block::SEMICOLON_INSIDE_BLOCK_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4542105d3277..82505f855828 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -316,6 +316,7 @@ mod replace_box; mod reserve_after_initialization; mod return_self_not_must_use; mod returns; +mod rwlock_atomic; mod same_name_method; mod self_named_constructors; mod semicolon_block; @@ -848,6 +849,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co Box::new(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)), Box::new(|_| Box::new(volatile_composites::VolatileComposites)), Box::new(|_| Box::::default()), + Box::new(|_| Box::new(rwlock_atomic::RwLock)), // add late passes here, used by `cargo dev new_lint` ]; store.late_passes.extend(late_lints); diff --git a/clippy_lints/src/rwlock_atomic.rs b/clippy_lints/src/rwlock_atomic.rs new file mode 100644 index 000000000000..6529a773b57d --- /dev/null +++ b/clippy_lints/src/rwlock_atomic.rs @@ -0,0 +1,190 @@ +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::{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; + +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `RwLock` where an atomic will do. + /// + /// ### Why restrict this? + /// Using a RwLock just to make access to a plain bool or + /// reference sequential is shooting flies with cannons. + /// `std::sync::atomic::AtomicBool` and `std::sync::atomic::AtomicPtr` are leaner and + /// faster. + /// + /// On the other hand, `RwLock`s are, in general, easier to + /// verify correctness. An atomic does not behave the same as + /// an equivalent RwLock. + /// + /// ### Example + /// ```no_run + /// # let y = true; + /// # use std::sync::RwLock; + /// let x = RwLock::new(&y); + /// ``` + /// + /// Use instead: + /// ```no_run + /// # let y = true; + /// # use std::sync::atomic::AtomicBool; + /// let x = AtomicBool::new(y); + /// ``` + #[clippy::version = "1.90.0"] + pub RWLOCK_ATOMIC, + restriction, + "using a RwLock where an atomic value could be used instead." +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of `RwLock` where `X` is an integral + /// type. + /// + /// ### Why restrict this? + /// Using a RwLock just to make access to a plain integer + /// sequential is + /// shooting flies with cannons. `std::sync::atomic::AtomicUsize` is leaner and faster. + /// + /// On the other hand, `RwLock`s are, in general, easier to + /// verify correctness. An atomic does not behave the same as + /// an equivalent RwLock. + /// + /// ### Example + /// ```no_run + /// # use std::sync::RwLock; + /// let x = RwLock::new(0usize); + /// ``` + /// + /// Use instead: + /// ```no_run + /// # use std::sync::atomic::AtomicUsize; + /// let x = AtomicUsize::new(0usize); + /// ``` + #[clippy::version = "1.90.0"] + pub RWLOCK_INTEGER, + restriction, + "using a RwLock for an integer type" +} + +declare_lint_pass!(RwLock => [RWLOCK_ATOMIC, RWLOCK_INTEGER]); + +// NOTE: we don't use `check_expr` because that would make us lint every _use_ of such RwLocks, not +// just their definitions +impl<'tcx> LateLintPass<'tcx> for RwLock { + 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 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 `: RwLock` (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 rwlock: RwLock = RwLock::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::RwLock) + && let rwlock_param = subst.type_at(0) + && let Some(atomic_name) = get_atomic_name(rwlock_param) + { + let msg = "using a `RwLock` where an atomic would do"; + let diag = |diag: &mut Diag<'_, _>| { + // if `expr = RwLock::new(arg)`, we can try emitting a suggestion + if let ExprKind::Call(qpath, [arg]) = expr.kind + && let ExprKind::Path(QPath::TypeRelative(_rwlock, 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 `RwLock<()>`"); + }; + match *rwlock_param.kind() { + ty::Uint(t) if t != UintTy::Usize => span_lint_and_then(cx, RWLOCK_INTEGER, expr.span, msg, diag), + ty::Int(t) if t != IntTy::Isize => span_lint_and_then(cx, RWLOCK_INTEGER, expr.span, msg, diag), + _ => span_lint_and_then(cx, RWLOCK_ATOMIC, expr.span, msg, diag), + } + } +} + +fn get_atomic_name(ty: Ty<'_>) -> Option<&'static str> { + match ty.kind() { + ty::Bool => Some("AtomicBool"), + ty::Uint(uint_ty) => { + match uint_ty { + UintTy::U8 => Some("AtomicU8"), + UintTy::U16 => Some("AtomicU16"), + UintTy::U32 => Some("AtomicU32"), + UintTy::U64 => Some("AtomicU64"), + UintTy::Usize => Some("AtomicUsize"), + // `AtomicU128` is unstable and only available on a few platforms: https://github.com/rust-lang/rust/issues/99069 + UintTy::U128 => None, + } + }, + ty::Int(int_ty) => { + match int_ty { + IntTy::I8 => Some("AtomicI8"), + IntTy::I16 => Some("AtomicI16"), + IntTy::I32 => Some("AtomicI32"), + IntTy::I64 => Some("AtomicI64"), + IntTy::Isize => Some("AtomicIsize"), + // `AtomicU128` is unstable and only available on a few platforms: https://github.com/rust-lang/rust/issues/99069 + IntTy::I128 => None, + } + }, + // `AtomicPtr` only accepts `*mut T` + ty::RawPtr(_, Mutability::Mut) => Some("AtomicPtr"), + _ => None, + } +} diff --git a/tests/ui/explicit_write_in_test.stderr b/tests/ui/explicit_write_in_test.stderr deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/tests/ui/rwlock_atomic.fixed b/tests/ui/rwlock_atomic.fixed new file mode 100644 index 000000000000..24cdc3127e24 --- /dev/null +++ b/tests/ui/rwlock_atomic.fixed @@ -0,0 +1,49 @@ +#![warn(clippy::rwlock_integer)] +#![warn(clippy::rwlock_atomic)] +#![allow(clippy::borrow_as_ptr)] + +use std::sync::RwLock; + +fn main() { + let _ = std::sync::atomic::AtomicBool::new(true); + //~^ rwlock_atomic + + let _ = std::sync::atomic::AtomicUsize::new(5usize); + //~^ rwlock_atomic + + let _ = std::sync::atomic::AtomicIsize::new(9isize); + //~^ rwlock_atomic + + let mut x = 4u32; + // `AtomicPtr` only accepts `*mut T`, so this should not lint + let _ = RwLock::new(&x as *const u32); + + let _ = std::sync::atomic::AtomicPtr::new(&mut x as *mut u32); + //~^ rwlock_atomic + + let _ = std::sync::atomic::AtomicU32::new(0u32); + //~^ rwlock_integer + + let _ = std::sync::atomic::AtomicI32::new(0i32); + //~^ rwlock_integer + + let _ = RwLock::new(0f32); // there are no float atomics, so this should not lint + let _ = std::sync::atomic::AtomicU8::new(0u8); + //~^ rwlock_integer + + let _ = std::sync::atomic::AtomicI16::new(0i16); + //~^ rwlock_integer + + let _x = std::sync::atomic::AtomicI8::new(0); + //~^ rwlock_integer + + const X: i64 = 0; + let _ = std::sync::atomic::AtomicI64::new(X); + //~^ rwlock_integer + + // there are no 128 atomics, so these two should not lint + { + let _ = RwLock::new(0u128); + let _x: RwLock = RwLock::new(0); + } +} diff --git a/tests/ui/rwlock_atomic.rs b/tests/ui/rwlock_atomic.rs new file mode 100644 index 000000000000..51bc775a9e38 --- /dev/null +++ b/tests/ui/rwlock_atomic.rs @@ -0,0 +1,49 @@ +#![warn(clippy::rwlock_integer)] +#![warn(clippy::rwlock_atomic)] +#![allow(clippy::borrow_as_ptr)] + +use std::sync::RwLock; + +fn main() { + let _ = RwLock::new(true); + //~^ rwlock_atomic + + let _ = RwLock::new(5usize); + //~^ rwlock_atomic + + let _ = RwLock::new(9isize); + //~^ rwlock_atomic + + let mut x = 4u32; + // `AtomicPtr` only accepts `*mut T`, so this should not lint + let _ = RwLock::new(&x as *const u32); + + let _ = RwLock::new(&mut x as *mut u32); + //~^ rwlock_atomic + + let _ = RwLock::new(0u32); + //~^ rwlock_integer + + let _ = RwLock::new(0i32); + //~^ rwlock_integer + + let _ = RwLock::new(0f32); // there are no float atomics, so this should not lint + let _ = RwLock::new(0u8); + //~^ rwlock_integer + + let _ = RwLock::new(0i16); + //~^ rwlock_integer + + let _x: RwLock = RwLock::new(0); + //~^ rwlock_integer + + const X: i64 = 0; + let _ = RwLock::new(X); + //~^ rwlock_integer + + // there are no 128 atomics, so these two should not lint + { + let _ = RwLock::new(0u128); + let _x: RwLock = RwLock::new(0); + } +} diff --git a/tests/ui/rwlock_atomic.stderr b/tests/ui/rwlock_atomic.stderr new file mode 100644 index 000000000000..da2cd2bbb619 --- /dev/null +++ b/tests/ui/rwlock_atomic.stderr @@ -0,0 +1,91 @@ +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:8:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + = note: `-D clippy::rwlock-atomic` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::rwlock_atomic)]` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:11:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:14:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:21:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:24:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + = note: `-D clippy::rwlock-integer` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::rwlock_integer)]` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:27:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:31:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:34:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:37:26 + | +LL | let _x: RwLock = RwLock::new(0); + | ^^^^^^^^^^^^^^ + | + = help: if you just want the locking behavior and not the internal type, consider using `RwLock<()>` +help: try + | +LL - let _x: RwLock = RwLock::new(0); +LL + let _x = std::sync::atomic::AtomicI8::new(0); + | + +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic.rs:41:13 + | +LL | let _ = RwLock::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 `RwLock<()>` + +error: aborting due to 10 previous errors + diff --git a/tests/ui/rwlock_atomic_unfixiable.rs b/tests/ui/rwlock_atomic_unfixiable.rs new file mode 100644 index 000000000000..e1045ab08a7c --- /dev/null +++ b/tests/ui/rwlock_atomic_unfixiable.rs @@ -0,0 +1,13 @@ +//@no-rustfix +#![warn(clippy::rwlock_atomic, clippy::rwlock_integer)] + +use std::sync::RwLock; + +fn none_issue_yet() { + static MTX: RwLock = RwLock::new(0); + //~^ rwlock_integer + + // unfixable because we don't fix this `write` + let mut guard = MTX.write().unwrap(); + *guard += 1; +} diff --git a/tests/ui/rwlock_atomic_unfixiable.stderr b/tests/ui/rwlock_atomic_unfixiable.stderr new file mode 100644 index 000000000000..da78f9e2b3da --- /dev/null +++ b/tests/ui/rwlock_atomic_unfixiable.stderr @@ -0,0 +1,17 @@ +error: using a `RwLock` where an atomic would do + --> tests/ui/rwlock_atomic_unfixiable.rs:7:31 + | +LL | static MTX: RwLock = RwLock::new(0); + | ^^^^^^^^^^^^^^ + | + = help: if you just want the locking behavior and not the internal type, consider using `RwLock<()>` + = note: `-D clippy::rwlock-integer` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::rwlock_integer)]` +help: try + | +LL - static MTX: RwLock = RwLock::new(0); +LL + static MTX: std::sync::atomic::AtomicU32 = std::sync::atomic::AtomicU32::new(0); + | + +error: aborting due to 1 previous error +