From 4939a716e80214edd19dab6b678fce3e769388f5 Mon Sep 17 00:00:00 2001 From: Alex Macleod Date: Tue, 4 Jul 2023 16:58:02 +0000 Subject: [PATCH] arc_with_non_send_sync: reword and move to suspicious --- clippy_lints/src/arc_with_non_send_sync.rs | 71 +++++++++++----------- tests/ui/arc_with_non_send_sync.rs | 12 ++-- tests/ui/arc_with_non_send_sync.stderr | 33 ++++++++-- 3 files changed, 73 insertions(+), 43 deletions(-) diff --git a/clippy_lints/src/arc_with_non_send_sync.rs b/clippy_lints/src/arc_with_non_send_sync.rs index 62313df9f90ec..98ee8a9a8806c 100644 --- a/clippy_lints/src/arc_with_non_send_sync.rs +++ b/clippy_lints/src/arc_with_non_send_sync.rs @@ -1,12 +1,11 @@ -use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::last_path_segment; use clippy_utils::ty::{implements_trait, is_type_diagnostic_item}; -use if_chain::if_chain; - use rustc_hir::{Expr, ExprKind}; use rustc_lint::LateContext; use rustc_lint::LateLintPass; use rustc_middle::ty; +use rustc_middle::ty::print::with_forced_trimmed_paths; use rustc_middle::ty::GenericArgKind; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::symbol::sym; @@ -16,8 +15,8 @@ declare_clippy_lint! { /// This lint warns when you use `Arc` with a type that does not implement `Send` or `Sync`. /// /// ### Why is this bad? - /// Wrapping a type in Arc doesn't add thread safety to the underlying data, so data races - /// could occur when touching the underlying data. + /// `Arc` is only `Send`/`Sync` when `T` is [both `Send` and `Sync`](https://doc.rust-lang.org/std/sync/struct.Arc.html#impl-Send-for-Arc%3CT%3E), + /// either `T` should be made `Send + Sync` or an `Rc` should be used instead of an `Arc` /// /// ### Example /// ```rust @@ -25,16 +24,17 @@ declare_clippy_lint! { /// # use std::sync::Arc; /// /// fn main() { - /// // This is safe, as `i32` implements `Send` and `Sync`. + /// // This is fine, as `i32` implements `Send` and `Sync`. /// let a = Arc::new(42); /// - /// // This is not safe, as `RefCell` does not implement `Sync`. + /// // `RefCell` is `!Sync`, so either the `Arc` should be replaced with an `Rc` + /// // or the `RefCell` replaced with something like a `RwLock` /// let b = Arc::new(RefCell::new(42)); /// } /// ``` #[clippy::version = "1.72.0"] pub ARC_WITH_NON_SEND_SYNC, - correctness, + suspicious, "using `Arc` with a type that does not implement `Send` or `Sync`" } declare_lint_pass!(ArcWithNonSendSync => [ARC_WITH_NON_SEND_SYNC]); @@ -42,35 +42,38 @@ declare_lint_pass!(ArcWithNonSendSync => [ARC_WITH_NON_SEND_SYNC]); impl LateLintPass<'_> for ArcWithNonSendSync { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { let ty = cx.typeck_results().expr_ty(expr); - if_chain! { - if is_type_diagnostic_item(cx, ty, sym::Arc); - if let ExprKind::Call(func, [arg]) = expr.kind; - if let ExprKind::Path(func_path) = func.kind; - if last_path_segment(&func_path).ident.name == sym::new; - if let arg_ty = cx.typeck_results().expr_ty(arg); + if is_type_diagnostic_item(cx, ty, sym::Arc) + && let ExprKind::Call(func, [arg]) = expr.kind + && let ExprKind::Path(func_path) = func.kind + && last_path_segment(&func_path).ident.name == sym::new + && let arg_ty = cx.typeck_results().expr_ty(arg) // make sure that the type is not and does not contain any type parameters - if arg_ty.walk().all(|arg| { + && arg_ty.walk().all(|arg| { !matches!(arg.unpack(), GenericArgKind::Type(ty) if matches!(ty.kind(), ty::Param(_))) - }); - if !cx.tcx - .lang_items() - .sync_trait() - .map_or(false, |id| implements_trait(cx, arg_ty, id, &[])) || - !cx.tcx - .get_diagnostic_item(sym::Send) - .map_or(false, |id| implements_trait(cx, arg_ty, id, &[])); + }) + && let Some(send) = cx.tcx.get_diagnostic_item(sym::Send) + && let Some(sync) = cx.tcx.lang_items().sync_trait() + && let [is_send, is_sync] = [send, sync].map(|id| implements_trait(cx, arg_ty, id, &[])) + && !(is_send && is_sync) + { + span_lint_and_then( + cx, + ARC_WITH_NON_SEND_SYNC, + expr.span, + "usage of an `Arc` that is not `Send` or `Sync`", + |diag| with_forced_trimmed_paths!({ + if !is_send { + diag.note(format!("the trait `Send` is not implemented for `{arg_ty}`")); + } + if !is_sync { + diag.note(format!("the trait `Sync` is not implemented for `{arg_ty}`")); + } + + diag.note(format!("required for `{ty}` to implement `Send` and `Sync`")); - then { - span_lint_and_help( - cx, - ARC_WITH_NON_SEND_SYNC, - expr.span, - "usage of `Arc` where `T` is not `Send` or `Sync`", - None, - "consider using `Rc` instead or wrapping `T` in a std::sync type like \ - `Mutex`", - ); - } + diag.help("consider using an `Rc` instead or wrapping the inner type with a `Mutex`"); + } + )); } } } diff --git a/tests/ui/arc_with_non_send_sync.rs b/tests/ui/arc_with_non_send_sync.rs index 9d5849277c204..b6fcca0a7919e 100644 --- a/tests/ui/arc_with_non_send_sync.rs +++ b/tests/ui/arc_with_non_send_sync.rs @@ -12,9 +12,13 @@ fn issue11076() { } fn main() { - // This is safe, as `i32` implements `Send` and `Sync`. - let a = Arc::new(42); + let _ = Arc::new(42); - // This is not safe, as `RefCell` does not implement `Sync`. - let b = Arc::new(RefCell::new(42)); + // !Sync + let _ = Arc::new(RefCell::new(42)); + let mutex = Mutex::new(1); + // !Send + let _ = Arc::new(mutex.lock().unwrap()); + // !Send + !Sync + let _ = Arc::new(&42 as *const i32); } diff --git a/tests/ui/arc_with_non_send_sync.stderr b/tests/ui/arc_with_non_send_sync.stderr index 520c21b565ba5..7633b38dfb597 100644 --- a/tests/ui/arc_with_non_send_sync.stderr +++ b/tests/ui/arc_with_non_send_sync.stderr @@ -1,11 +1,34 @@ -error: usage of `Arc` where `T` is not `Send` or `Sync` - --> $DIR/arc_with_non_send_sync.rs:19:13 +error: usage of an `Arc` that is not `Send` or `Sync` + --> $DIR/arc_with_non_send_sync.rs:18:13 | -LL | let b = Arc::new(RefCell::new(42)); +LL | let _ = Arc::new(RefCell::new(42)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider using `Rc` instead or wrapping `T` in a std::sync type like `Mutex` + = note: the trait `Sync` is not implemented for `RefCell` + = note: required for `Arc>` to implement `Send` and `Sync` + = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex` = note: `-D clippy::arc-with-non-send-sync` implied by `-D warnings` -error: aborting due to previous error +error: usage of an `Arc` that is not `Send` or `Sync` + --> $DIR/arc_with_non_send_sync.rs:21:13 + | +LL | let _ = Arc::new(mutex.lock().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: the trait `Send` is not implemented for `MutexGuard<'_, i32>` + = note: required for `Arc>` to implement `Send` and `Sync` + = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex` + +error: usage of an `Arc` that is not `Send` or `Sync` + --> $DIR/arc_with_non_send_sync.rs:23:13 + | +LL | let _ = Arc::new(&42 as *const i32); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: the trait `Send` is not implemented for `*const i32` + = note: the trait `Sync` is not implemented for `*const i32` + = note: required for `Arc<*const i32>` to implement `Send` and `Sync` + = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex` + +error: aborting due to 3 previous errors