From 339cd14f27e9db4f7b040068abad8a20a0034fa8 Mon Sep 17 00:00:00 2001 From: avborhanian Date: Sun, 4 Jun 2023 10:33:13 -0700 Subject: [PATCH 1/4] Adds new lint `arc_with_non_send_or_sync` --- CHANGELOG.md | 1 + clippy_lints/src/arc_with_non_send_sync.rs | 71 ++++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + tests/ui/arc_with_non_send_sync.rs | 12 ++++ tests/ui/arc_with_non_send_sync.stderr | 11 ++++ 6 files changed, 98 insertions(+) create mode 100644 clippy_lints/src/arc_with_non_send_sync.rs create mode 100644 tests/ui/arc_with_non_send_sync.rs create mode 100644 tests/ui/arc_with_non_send_sync.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index a65ac4d46..a53567f25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4628,6 +4628,7 @@ Released 2018-09-13 [`almost_complete_range`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_complete_range [`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant +[`arc_with_non_send_sync`]: https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync [`arithmetic_side_effects`]: https://rust-lang.github.io/rust-clippy/master/index.html#arithmetic_side_effects [`as_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions [`as_ptr_cast_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_cast_mut diff --git a/clippy_lints/src/arc_with_non_send_sync.rs b/clippy_lints/src/arc_with_non_send_sync.rs new file mode 100644 index 000000000..2e14f81f8 --- /dev/null +++ b/clippy_lints/src/arc_with_non_send_sync.rs @@ -0,0 +1,71 @@ +use clippy_utils::diagnostics::span_lint_and_help; +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_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::sym; + +declare_clippy_lint! { + /// ### What it does. + /// 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. + /// + /// ### Example. + /// ```rust + /// use std::cell::RefCell; + /// use std::sync::Arc; + /// + /// fn main() { + /// // This is safe, as `i32` implements `Send` and `Sync`. + /// let a = Arc::new(42); + /// + /// // This is not safe, as `RefCell` does not implement `Sync`. + /// let b = Arc::new(RefCell::new(42)); + /// } + /// ``` + /// ``` + #[clippy::version = "1.72.0"] + pub ARC_WITH_NON_SEND_SYNC, + correctness, + "using `Arc` with a type that does not implement `Send` or `Sync`" +} +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 !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, &[])); + + 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", + ); + } + } + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index d014534ce..f82118e56 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -40,6 +40,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::allow_attributes::ALLOW_ATTRIBUTES_INFO, crate::almost_complete_range::ALMOST_COMPLETE_RANGE_INFO, crate::approx_const::APPROX_CONSTANT_INFO, + crate::arc_with_non_send_sync::ARC_WITH_NON_SEND_SYNC_INFO, crate::as_conversions::AS_CONVERSIONS_INFO, crate::asm_syntax::INLINE_ASM_X86_ATT_SYNTAX_INFO, crate::asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d53c6de54..2f5932104 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -68,6 +68,7 @@ mod renamed_lints; mod allow_attributes; mod almost_complete_range; mod approx_const; +mod arc_with_non_send_sync; mod as_conversions; mod asm_syntax; mod assertions_on_constants; @@ -1014,6 +1015,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(missing_fields_in_debug::MissingFieldsInDebug)); store.register_late_pass(|_| Box::new(endian_bytes::EndianBytes)); store.register_late_pass(|_| Box::new(redundant_type_annotations::RedundantTypeAnnotations)); + store.register_late_pass(|_| Box::new(arc_with_non_send_sync::ArcWithNonSendSync)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/arc_with_non_send_sync.rs b/tests/ui/arc_with_non_send_sync.rs new file mode 100644 index 000000000..4c2d84be5 --- /dev/null +++ b/tests/ui/arc_with_non_send_sync.rs @@ -0,0 +1,12 @@ +#![warn(clippy::arc_with_non_send_sync)] +#![allow(unused_variables)] +use std::cell::RefCell; +use std::sync::{Arc, Mutex}; + +fn main() { + // This is safe, as `i32` implements `Send` and `Sync`. + let a = Arc::new(42); + + // This is not safe, as `RefCell` does not implement `Sync`. + let b = Arc::new(RefCell::new(42)); +} diff --git a/tests/ui/arc_with_non_send_sync.stderr b/tests/ui/arc_with_non_send_sync.stderr new file mode 100644 index 000000000..99e38bcc1 --- /dev/null +++ b/tests/ui/arc_with_non_send_sync.stderr @@ -0,0 +1,11 @@ +error: usage of `Arc` where `T` is not `Send` or `Sync` + --> $DIR/arc_with_non_send_sync.rs:11:13 + | +LL | let b = Arc::new(RefCell::new(42)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider using `Rc` instead or wrapping `T` in a std::sync type like Mutex + = note: `-D clippy::arc-with-non-send-sync` implied by `-D warnings` + +error: aborting due to previous error + From 8330887e1b2272b14e334cb28fb85ccf0859f183 Mon Sep 17 00:00:00 2001 From: avborhanian Date: Wed, 7 Jun 2023 20:52:42 -0700 Subject: [PATCH 2/4] Updating documentation and lint formatting. --- clippy_lints/src/arc_with_non_send_sync.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/arc_with_non_send_sync.rs b/clippy_lints/src/arc_with_non_send_sync.rs index 2e14f81f8..ca2023ee0 100644 --- a/clippy_lints/src/arc_with_non_send_sync.rs +++ b/clippy_lints/src/arc_with_non_send_sync.rs @@ -17,10 +17,10 @@ declare_clippy_lint! { /// Wrapping a type in Arc doesn't add thread safety to the underlying data, so data races /// could occur when touching the underlying data. /// - /// ### Example. + /// ### Example /// ```rust - /// use std::cell::RefCell; - /// use std::sync::Arc; + /// # use std::cell::RefCell; + /// # use std::sync::Arc; /// /// fn main() { /// // This is safe, as `i32` implements `Send` and `Sync`. @@ -30,7 +30,6 @@ declare_clippy_lint! { /// let b = Arc::new(RefCell::new(42)); /// } /// ``` - /// ``` #[clippy::version = "1.72.0"] pub ARC_WITH_NON_SEND_SYNC, correctness, @@ -63,7 +62,7 @@ impl LateLintPass<'_> for ArcWithNonSendSync { "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", + `Mutex`", ); } } From 2f5d1c748a5447256048267d3fbc3533d6d7e126 Mon Sep 17 00:00:00 2001 From: avborhanian Date: Thu, 8 Jun 2023 00:22:04 -0700 Subject: [PATCH 3/4] Adding extra check to ignore generic args. --- clippy_lints/src/arc_with_non_send_sync.rs | 5 +++++ tests/ui/arc_with_non_send_sync.rs | 5 +++++ tests/ui/arc_with_non_send_sync.stderr | 4 ++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/arc_with_non_send_sync.rs b/clippy_lints/src/arc_with_non_send_sync.rs index ca2023ee0..13e938327 100644 --- a/clippy_lints/src/arc_with_non_send_sync.rs +++ b/clippy_lints/src/arc_with_non_send_sync.rs @@ -6,6 +6,7 @@ use if_chain::if_chain; use rustc_hir::{Expr, ExprKind}; use rustc_lint::LateContext; use rustc_lint::LateLintPass; +use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::symbol::sym; @@ -46,6 +47,10 @@ impl LateLintPass<'_> for ArcWithNonSendSync { 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 match arg_ty.kind() { + ty::Param(_) => false, + _ => true, + }; if !cx.tcx .lang_items() .sync_trait() diff --git a/tests/ui/arc_with_non_send_sync.rs b/tests/ui/arc_with_non_send_sync.rs index 4c2d84be5..ac786f68c 100644 --- a/tests/ui/arc_with_non_send_sync.rs +++ b/tests/ui/arc_with_non_send_sync.rs @@ -3,6 +3,11 @@ use std::cell::RefCell; use std::sync::{Arc, Mutex}; +fn foo(x: T) { + // Should not lint - purposefully ignoring generic args. + let a = Arc::new(x); +} + fn main() { // This is safe, as `i32` implements `Send` and `Sync`. let a = Arc::new(42); diff --git a/tests/ui/arc_with_non_send_sync.stderr b/tests/ui/arc_with_non_send_sync.stderr index 99e38bcc1..fc2fc5f93 100644 --- a/tests/ui/arc_with_non_send_sync.stderr +++ b/tests/ui/arc_with_non_send_sync.stderr @@ -1,10 +1,10 @@ error: usage of `Arc` where `T` is not `Send` or `Sync` - --> $DIR/arc_with_non_send_sync.rs:11:13 + --> $DIR/arc_with_non_send_sync.rs:16:13 | LL | let b = Arc::new(RefCell::new(42)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: consider using `Rc` instead or wrapping `T` in a std::sync type like Mutex + = help: consider using `Rc` instead or wrapping `T` in a std::sync type like `Mutex` = note: `-D clippy::arc-with-non-send-sync` implied by `-D warnings` error: aborting due to previous error From 20548eba6ceda0563b399f54b06d05b71950aa5c Mon Sep 17 00:00:00 2001 From: avborhanian Date: Thu, 8 Jun 2023 00:38:18 -0700 Subject: [PATCH 4/4] Swapping to matches macro. --- clippy_lints/src/arc_with_non_send_sync.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/clippy_lints/src/arc_with_non_send_sync.rs b/clippy_lints/src/arc_with_non_send_sync.rs index 13e938327..a1e44668e 100644 --- a/clippy_lints/src/arc_with_non_send_sync.rs +++ b/clippy_lints/src/arc_with_non_send_sync.rs @@ -47,10 +47,7 @@ impl LateLintPass<'_> for ArcWithNonSendSync { 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 match arg_ty.kind() { - ty::Param(_) => false, - _ => true, - }; + if !matches!(arg_ty.kind(), ty::Param(_)); if !cx.tcx .lang_items() .sync_trait()