Auto merge of rust-lang#8075 - Qwaz:non_send_fields_documentation, r=…

…xFrednet Clarify the purpose of the non_send lint PR 2/2 for issue rust-lang#8045. Tried to tone down the warning message and clarify the intention of the lint. Specifically, I added a description that this lint tries to detect "types that are not safe to be sent to another thread". changelog: none r? `@xFrednet`
flip1995 · Dec 7, 2021 · ecae70f · ecae70f
2 parents 1c7df44 + 5d63a28
commit ecae70f
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 56 deletions.
diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs
@@ -220,7 +220,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(non_copy_const::DECLARE_INTERIOR_MUTABLE_CONST),
     LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS),
     LintId::of(non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS),
-    LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY),
     LintId::of(octal_escapes::OCTAL_ESCAPES),
     LintId::of(open_options::NONSENSICAL_OPEN_OPTIONS),
     LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP),

diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs
@@ -18,6 +18,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![
     LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN),
     LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL),
     LintId::of(mutex_atomic::MUTEX_INTEGER),
+    LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY),
     LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES),
     LintId::of(option_if_let_else::OPTION_IF_LET_ELSE),
     LintId::of(path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),

diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs
@@ -15,7 +15,6 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec!
     LintId::of(loops::MUT_RANGE_BOUND),
     LintId::of(methods::SUSPICIOUS_MAP),
     LintId::of(mut_key::MUTABLE_KEY_TYPE),
-    LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY),
     LintId::of(octal_escapes::OCTAL_ESCAPES),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL),
     LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),

diff --git a/clippy_lints/src/non_send_fields_in_send_ty.rs b/clippy_lints/src/non_send_fields_in_send_ty.rs
@@ -13,24 +13,28 @@ use rustc_span::sym;
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Warns about fields in struct implementing `Send` that are neither `Send` nor `Copy`.
+    /// This lint warns about a `Send` implementation for a type that
+    /// contains fields that are not safe to be sent across threads.
+    /// It tries to detect fields that can cause a soundness issue
+    /// when sent to another thread (e.g., `Rc`) while allowing `!Send` fields
+    /// that are expected to exist in a `Send` type, such as raw pointers.
     ///
     /// ### Why is this bad?
-    /// Sending the struct to another thread will transfer the ownership to
-    /// the new thread by dropping in the current thread during the transfer.
-    /// This causes soundness issues for non-`Send` fields, as they are also
-    /// dropped and might not be set up to handle this.
+    /// Sending the struct to another thread effectively sends all of its fields,
+    /// and the fields that do not implement `Send` can lead to soundness bugs
+    /// such as data races when accessed in a thread
+    /// that is different from the thread that created it.
     ///
     /// See:
     /// * [*The Rustonomicon* about *Send and Sync*](https://doc.rust-lang.org/nomicon/send-and-sync.html)
     /// * [The documentation of `Send`](https://doc.rust-lang.org/std/marker/trait.Send.html)
     ///
     /// ### Known Problems
-    /// Data structures that contain raw pointers may cause false positives.
-    /// They are sometimes safe to be sent across threads but do not implement
-    /// the `Send` trait. This lint has a heuristic to filter out basic cases
-    /// such as `Vec<*const T>`, but it's not perfect. Feel free to create an
-    /// issue if you have a suggestion on how this heuristic can be improved.
+    /// This lint relies on heuristics to distinguish types that are actually
+    /// unsafe to be sent across threads and `!Send` types that are expected to
+    /// exist in  `Send` type. Its rule can filter out basic cases such as
+    /// `Vec<*const T>`, but it's not perfect. Feel free to create an issue if
+    /// you have a suggestion on how this heuristic can be improved.
     ///
     /// ### Example
     /// ```rust,ignore
@@ -46,8 +50,8 @@ declare_clippy_lint! {
     /// or specify correct bounds on generic type parameters (`T: Send`).
     #[clippy::version = "1.57.0"]
     pub NON_SEND_FIELDS_IN_SEND_TY,
-    suspicious,
-    "there is field that does not implement `Send` in a `Send` struct"
+    nursery,
+    "there is a field that is not safe to be sent to another thread in a `Send` struct"
 }
 
 #[derive(Copy, Clone)]
@@ -120,14 +124,14 @@ impl<'tcx> LateLintPass<'tcx> for NonSendFieldInSendTy {
                         NON_SEND_FIELDS_IN_SEND_TY,
                         item.span,
                         &format!(
-                            "this implementation is unsound, as some fields in `{}` are `!Send`",
+                            "some fields in `{}` are not safe to be sent to another thread",
                             snippet(cx, hir_impl.self_ty.span, "Unknown")
                         ),
                         |diag| {
                             for field in non_send_fields {
                                 diag.span_note(
                                     field.def.span,
-                                    &format!("the type of field `{}` is `!Send`", field.def.ident.name),
+                                    &format!("it is not safe to send field `{}` to another thread", field.def.ident.name),
                                 );
 
                                 match field.generic_params.len() {

diff --git a/tests/ui-toml/strict_non_send_fields_in_send_ty/test.stderr b/tests/ui-toml/strict_non_send_fields_in_send_ty/test.stderr
@@ -1,86 +1,86 @@
-error: this implementation is unsound, as some fields in `NoGeneric` are `!Send`
+error: some fields in `NoGeneric` are not safe to be sent to another thread
   --> $DIR/test.rs:11:1
    |
 LL | unsafe impl Send for NoGeneric {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::non-send-fields-in-send-ty` implied by `-D warnings`
-note: the type of field `rc_is_not_send` is `!Send`
+note: it is not safe to send field `rc_is_not_send` to another thread
   --> $DIR/test.rs:8:5
    |
 LL |     rc_is_not_send: Rc<String>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: use a thread-safe type that implements `Send`
 
-error: this implementation is unsound, as some fields in `MultiField<T>` are `!Send`
+error: some fields in `MultiField<T>` are not safe to be sent to another thread
   --> $DIR/test.rs:19:1
    |
 LL | unsafe impl<T> Send for MultiField<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `field1` is `!Send`
+note: it is not safe to send field `field1` to another thread
   --> $DIR/test.rs:14:5
    |
 LL |     field1: T,
    |     ^^^^^^^^^
    = help: add `T: Send` bound in `Send` impl
-note: the type of field `field2` is `!Send`
+note: it is not safe to send field `field2` to another thread
   --> $DIR/test.rs:15:5
    |
 LL |     field2: T,
    |     ^^^^^^^^^
    = help: add `T: Send` bound in `Send` impl
-note: the type of field `field3` is `!Send`
+note: it is not safe to send field `field3` to another thread
   --> $DIR/test.rs:16:5
    |
 LL |     field3: T,
    |     ^^^^^^^^^
    = help: add `T: Send` bound in `Send` impl
 
-error: this implementation is unsound, as some fields in `MyOption<T>` are `!Send`
+error: some fields in `MyOption<T>` are not safe to be sent to another thread
   --> $DIR/test.rs:26:1
    |
 LL | unsafe impl<T> Send for MyOption<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `0` is `!Send`
+note: it is not safe to send field `0` to another thread
   --> $DIR/test.rs:22:12
    |
 LL |     MySome(T),
    |            ^
    = help: add `T: Send` bound in `Send` impl
 
-error: this implementation is unsound, as some fields in `HeuristicTest` are `!Send`
+error: some fields in `HeuristicTest` are not safe to be sent to another thread
   --> $DIR/test.rs:41:1
    |
 LL | unsafe impl Send for HeuristicTest {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `field1` is `!Send`
+note: it is not safe to send field `field1` to another thread
   --> $DIR/test.rs:34:5
    |
 LL |     field1: Vec<*const NonSend>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: use a thread-safe type that implements `Send`
-note: the type of field `field2` is `!Send`
+note: it is not safe to send field `field2` to another thread
   --> $DIR/test.rs:35:5
    |
 LL |     field2: [*const NonSend; 3],
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: use a thread-safe type that implements `Send`
-note: the type of field `field3` is `!Send`
+note: it is not safe to send field `field3` to another thread
   --> $DIR/test.rs:36:5
    |
 LL |     field3: (*const NonSend, *const NonSend, *const NonSend),
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: use a thread-safe type that implements `Send`
-note: the type of field `field4` is `!Send`
+note: it is not safe to send field `field4` to another thread
   --> $DIR/test.rs:37:5
    |
 LL |     field4: (*const NonSend, Rc<u8>),
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: use a thread-safe type that implements `Send`
-note: the type of field `field5` is `!Send`
+note: it is not safe to send field `field5` to another thread
   --> $DIR/test.rs:38:5
    |
 LL |     field5: Vec<Vec<*const NonSend>>,

diff --git a/tests/ui/non_send_fields_in_send_ty.stderr b/tests/ui/non_send_fields_in_send_ty.stderr
@@ -1,166 +1,166 @@
-error: this implementation is unsound, as some fields in `RingBuffer<T>` are `!Send`
+error: some fields in `RingBuffer<T>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:16:1
    |
 LL | unsafe impl<T> Send for RingBuffer<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::non-send-fields-in-send-ty` implied by `-D warnings`
-note: the type of field `data` is `!Send`
+note: it is not safe to send field `data` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:11:5
    |
 LL |     data: Vec<UnsafeCell<T>>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^
    = help: add bounds on type parameter `T` that satisfy `Vec<UnsafeCell<T>>: Send`
 
-error: this implementation is unsound, as some fields in `MvccRwLock<T>` are `!Send`
+error: some fields in `MvccRwLock<T>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:24:1
    |
 LL | unsafe impl<T> Send for MvccRwLock<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `lock` is `!Send`
+note: it is not safe to send field `lock` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:21:5
    |
 LL |     lock: Mutex<Box<T>>,
    |     ^^^^^^^^^^^^^^^^^^^
    = help: add bounds on type parameter `T` that satisfy `Mutex<Box<T>>: Send`
 
-error: this implementation is unsound, as some fields in `ArcGuard<RC, T>` are `!Send`
+error: some fields in `ArcGuard<RC, T>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:32:1
    |
 LL | unsafe impl<RC, T: Send> Send for ArcGuard<RC, T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `head` is `!Send`
+note: it is not safe to send field `head` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:29:5
    |
 LL |     head: Arc<RC>,
    |     ^^^^^^^^^^^^^
    = help: add bounds on type parameter `RC` that satisfy `Arc<RC>: Send`
 
-error: this implementation is unsound, as some fields in `DeviceHandle<T>` are `!Send`
+error: some fields in `DeviceHandle<T>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:48:1
    |
 LL | unsafe impl<T: UsbContext> Send for DeviceHandle<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `context` is `!Send`
+note: it is not safe to send field `context` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:44:5
    |
 LL |     context: T,
    |     ^^^^^^^^^^
    = help: add `T: Send` bound in `Send` impl
 
-error: this implementation is unsound, as some fields in `NoGeneric` are `!Send`
+error: some fields in `NoGeneric` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:55:1
    |
 LL | unsafe impl Send for NoGeneric {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `rc_is_not_send` is `!Send`
+note: it is not safe to send field `rc_is_not_send` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:52:5
    |
 LL |     rc_is_not_send: Rc<String>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: use a thread-safe type that implements `Send`
 
-error: this implementation is unsound, as some fields in `MultiField<T>` are `!Send`
+error: some fields in `MultiField<T>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:63:1
    |
 LL | unsafe impl<T> Send for MultiField<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `field1` is `!Send`
+note: it is not safe to send field `field1` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:58:5
    |
 LL |     field1: T,
    |     ^^^^^^^^^
    = help: add `T: Send` bound in `Send` impl
-note: the type of field `field2` is `!Send`
+note: it is not safe to send field `field2` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:59:5
    |
 LL |     field2: T,
    |     ^^^^^^^^^
    = help: add `T: Send` bound in `Send` impl
-note: the type of field `field3` is `!Send`
+note: it is not safe to send field `field3` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:60:5
    |
 LL |     field3: T,
    |     ^^^^^^^^^
    = help: add `T: Send` bound in `Send` impl
 
-error: this implementation is unsound, as some fields in `MyOption<T>` are `!Send`
+error: some fields in `MyOption<T>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:70:1
    |
 LL | unsafe impl<T> Send for MyOption<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `0` is `!Send`
+note: it is not safe to send field `0` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:66:12
    |
 LL |     MySome(T),
    |            ^
    = help: add `T: Send` bound in `Send` impl
 
-error: this implementation is unsound, as some fields in `MultiParam<A, B>` are `!Send`
+error: some fields in `MultiParam<A, B>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:82:1
    |
 LL | unsafe impl<A, B> Send for MultiParam<A, B> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `vec` is `!Send`
+note: it is not safe to send field `vec` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:79:5
    |
 LL |     vec: Vec<(A, B)>,
    |     ^^^^^^^^^^^^^^^^
    = help: add bounds on type parameters `A, B` that satisfy `Vec<(A, B)>: Send`
 
-error: this implementation is unsound, as some fields in `HeuristicTest` are `!Send`
+error: some fields in `HeuristicTest` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:100:1
    |
 LL | unsafe impl Send for HeuristicTest {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `field4` is `!Send`
+note: it is not safe to send field `field4` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:95:5
    |
 LL |     field4: (*const NonSend, Rc<u8>),
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: use a thread-safe type that implements `Send`
 
-error: this implementation is unsound, as some fields in `AttrTest3<T>` are `!Send`
+error: some fields in `AttrTest3<T>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:119:1
    |
 LL | unsafe impl<T> Send for AttrTest3<T> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `0` is `!Send`
+note: it is not safe to send field `0` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:114:11
    |
 LL |     Enum2(T),
    |           ^
    = help: add `T: Send` bound in `Send` impl
 
-error: this implementation is unsound, as some fields in `Complex<P, u32>` are `!Send`
+error: some fields in `Complex<P, u32>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:127:1
    |
 LL | unsafe impl<P> Send for Complex<P, u32> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `field1` is `!Send`
+note: it is not safe to send field `field1` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:123:5
    |
 LL |     field1: A,
    |     ^^^^^^^^^
    = help: add `P: Send` bound in `Send` impl
 
-error: this implementation is unsound, as some fields in `Complex<Q, MutexGuard<'static, bool>>` are `!Send`
+error: some fields in `Complex<Q, MutexGuard<'static, bool>>` are not safe to be sent to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:130:1
    |
 LL | unsafe impl<Q: Send> Send for Complex<Q, MutexGuard<'static, bool>> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
-note: the type of field `field2` is `!Send`
+note: it is not safe to send field `field2` to another thread
   --> $DIR/non_send_fields_in_send_ty.rs:124:5
    |
 LL |     field2: B,