From d413e157a5410b40eaa42decad6bf9d85a577a2d Mon Sep 17 00:00:00 2001 From: Yechan Bae Date: Tue, 28 Sep 2021 16:42:27 -0400 Subject: [PATCH] Look into tuple, array, ADT args in raw pointer heuristic --- clippy_lints/src/non_send_field_in_send_ty.rs | 49 ++++++++++++++++--- tests/ui/non_send_field_in_send_ty.rs | 17 +++++-- tests/ui/non_send_field_in_send_ty.stderr | 27 +++++++--- 3 files changed, 74 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/non_send_field_in_send_ty.rs b/clippy_lints/src/non_send_field_in_send_ty.rs index 0fa994cac..f800d3ff7 100644 --- a/clippy_lints/src/non_send_field_in_send_ty.rs +++ b/clippy_lints/src/non_send_field_in_send_ty.rs @@ -121,7 +121,7 @@ impl<'tcx> LateLintPass<'tcx> for NonSendFieldInSendTy { }; } }, - ) + ); } } } @@ -145,6 +145,8 @@ impl<'tcx> NonSendField<'tcx> { } } +/// Given a type, collect all of its generic parameters. +/// Example: MyStruct> => vec![P, Q, R] fn collect_generic_params<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Vec> { ty.walk(cx.tcx) .filter_map(|inner| match inner.unpack() { @@ -155,14 +157,47 @@ fn collect_generic_params<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Vec(cx: &LateContext<'tcx>, ty: Ty<'tcx>, send_trait: DefId) -> bool { - raw_pointer_in_ty_def(cx, ty) || implements_trait(cx, ty, send_trait, &[]) || is_copy(cx, ty) +/// Determine if the given type is allowed in an ADT that implements `Send` +fn ty_allowed_in_send(cx: &LateContext<'tcx>, ty: Ty<'tcx>, send_trait: DefId) -> bool { + // TODO: check configuration and call `ty_implements_send_or_copy()` or + // `ty_allowed_with_raw_pointer_heuristic()` + ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait) } -/// Returns `true` if the type itself is a raw pointer or has a raw pointer as a -/// generic parameter, e.g., `Vec<*const u8>`. -/// Note that it does not look into enum variants or struct fields. -fn raw_pointer_in_ty_def<'tcx>(cx: &LateContext<'tcx>, target_ty: Ty<'tcx>) -> bool { +/// Determine if the given type is `Send` or `Copy` +fn ty_implements_send_or_copy(cx: &LateContext<'tcx>, ty: Ty<'tcx>, send_trait: DefId) -> bool { + implements_trait(cx, ty, send_trait, &[]) || is_copy(cx, ty) +} + +/// Heuristic to allow cases like `Vec<*const u8>` +fn ty_allowed_with_raw_pointer_heuristic<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, send_trait: DefId) -> bool { + if ty_implements_send_or_copy(cx, ty, send_trait) { + true + } else { + // The type is known to be `!Send` and `!Copy` + match ty.kind() { + ty::Tuple(_) => ty + .tuple_fields() + .all(|ty| ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait)), + ty::Array(ty, _) | ty::Slice(ty) => ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait), + ty::Adt(_, substs) => { + if contains_raw_pointer(cx, ty) { + // descends only if ADT contains any raw pointers + substs.iter().all(|generic_arg| match generic_arg.unpack() { + GenericArgKind::Type(ty) => ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait), + GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => true, + }) + } else { + false + } + }, + ty::RawPtr(_) => true, + _ => false, + } + } +} + +fn contains_raw_pointer<'tcx>(cx: &LateContext<'tcx>, target_ty: Ty<'tcx>) -> bool { for ty_node in target_ty.walk(cx.tcx) { if_chain! { if let GenericArgKind::Type(inner_ty) = ty_node.unpack(); diff --git a/tests/ui/non_send_field_in_send_ty.rs b/tests/ui/non_send_field_in_send_ty.rs index cf4fc1fd9..f5e7abdec 100644 --- a/tests/ui/non_send_field_in_send_ty.rs +++ b/tests/ui/non_send_field_in_send_ty.rs @@ -76,16 +76,23 @@ pub struct MultiParam { unsafe impl Send for MultiParam {} -// Raw pointers are allowed +// Tests for raw pointer heuristic extern "C" { - type SomeFfiType; + type NonSend; } -pub struct FpTest { - vec: Vec<*const SomeFfiType>, +pub struct HeuristicTest { + // raw pointers are allowed + field1: Vec<*const NonSend>, + field2: [*const NonSend; 3], + field3: (*const NonSend, *const NonSend, *const NonSend), + // not allowed when it contains concrete `!Send` field + field4: (*const NonSend, Rc), + // nested raw pointer is also allowed + field5: Vec>, } -unsafe impl Send for FpTest {} +unsafe impl Send for HeuristicTest {} // Test attributes #[allow(clippy::non_send_field_in_send_ty)] diff --git a/tests/ui/non_send_field_in_send_ty.stderr b/tests/ui/non_send_field_in_send_ty.stderr index 52cc038b6..327ef9fc2 100644 --- a/tests/ui/non_send_field_in_send_ty.stderr +++ b/tests/ui/non_send_field_in_send_ty.stderr @@ -115,44 +115,57 @@ LL | vec: Vec<(A, B)>, | ^^^^^^^^^^^^^^^^ = help: add bounds on type parameters `A, B` that satisfy `std::vec::Vec<(A, B)>: Send` +error: this implementation is unsound, as some fields in `HeuristicTest` are `!Send` + --> $DIR/non_send_field_in_send_ty.rs:95:1 + | +LL | unsafe impl Send for HeuristicTest {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: the field `field4` has type `(*const NonSend, std::rc::Rc)` which is not `Send` + --> $DIR/non_send_field_in_send_ty.rs:90:5 + | +LL | field4: (*const NonSend, Rc), + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: use a thread-safe type that implements `Send` + error: this implementation is unsound, as some fields in `AttrTest3` are `!Send` - --> $DIR/non_send_field_in_send_ty.rs:107:1 + --> $DIR/non_send_field_in_send_ty.rs:114:1 | LL | unsafe impl Send for AttrTest3 {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: the field `0` has type `T` which is not `Send` - --> $DIR/non_send_field_in_send_ty.rs:102:11 + --> $DIR/non_send_field_in_send_ty.rs:109:11 | LL | Enum2(T), | ^ = help: add `T: Send` bound in `Send` impl error: this implementation is unsound, as some fields in `Complex` are `!Send` - --> $DIR/non_send_field_in_send_ty.rs:115:1 + --> $DIR/non_send_field_in_send_ty.rs:122:1 | LL | unsafe impl

Send for Complex {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: the field `field1` has type `P` which is not `Send` - --> $DIR/non_send_field_in_send_ty.rs:111:5 + --> $DIR/non_send_field_in_send_ty.rs:118:5 | LL | field1: A, | ^^^^^^^^^ = help: add `P: Send` bound in `Send` impl error: this implementation is unsound, as some fields in `Complex>` are `!Send` - --> $DIR/non_send_field_in_send_ty.rs:118:1 + --> $DIR/non_send_field_in_send_ty.rs:125:1 | LL | unsafe impl Send for Complex> {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | note: the field `field2` has type `std::sync::MutexGuard<'static, bool>` which is not `Send` - --> $DIR/non_send_field_in_send_ty.rs:112:5 + --> $DIR/non_send_field_in_send_ty.rs:119:5 | LL | field2: B, | ^^^^^^^^^ = help: use a thread-safe type that implements `Send` -error: aborting due to 11 previous errors +error: aborting due to 12 previous errors