Skip to content

Commit 33c34fb

Browse files
committed
Auto merge of #7709 - Qwaz:drop_non_send, r=xFrednet
Implement `non_send_field_in_send_ty` lint changelog: Implement [`non_send_fields_in_send_ty`] lint Fixes #7703
2 parents da3b4b4 + fb0353b commit 33c34fb

13 files changed

+682
-1
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2898,6 +2898,7 @@ Released 2018-09-13
28982898
[`no_effect`]: https://rust-lang.github.io/rust-clippy/master/index.html#no_effect
28992899
[`non_ascii_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_ascii_literal
29002900
[`non_octal_unix_permissions`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions
2901+
[`non_send_fields_in_send_ty`]: https://rust-lang.github.io/rust-clippy/master/index.html#non_send_fields_in_send_ty
29012902
[`nonminimal_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
29022903
[`nonsensical_open_options`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonsensical_open_options
29032904
[`nonstandard_macro_braces`]: https://rust-lang.github.io/rust-clippy/master/index.html#nonstandard_macro_braces

clippy_lints/src/lib.mods.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ mod no_effect;
149149
mod non_copy_const;
150150
mod non_expressive_names;
151151
mod non_octal_unix_permissions;
152+
mod non_send_fields_in_send_ty;
152153
mod nonstandard_macro_braces;
153154
mod open_options;
154155
mod option_env_unwrap;

clippy_lints/src/lib.register_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,7 @@ store.register_lints(&[
366366
non_expressive_names::MANY_SINGLE_CHAR_NAMES,
367367
non_expressive_names::SIMILAR_NAMES,
368368
non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS,
369+
non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY,
369370
nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES,
370371
open_options::NONSENSICAL_OPEN_OPTIONS,
371372
option_env_unwrap::OPTION_ENV_UNWRAP,

clippy_lints/src/lib.register_nursery.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![
1616
LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN),
1717
LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL),
1818
LintId::of(mutex_atomic::MUTEX_INTEGER),
19+
LintId::of(non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY),
1920
LintId::of(nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES),
2021
LintId::of(option_if_let_else::OPTION_IF_LET_ELSE),
2122
LintId::of(path_buf_push_overwrite::PATH_BUF_PUSH_OVERWRITE),

clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
535535
store.register_late_pass(move || Box::new(feature_name::FeatureName));
536536
store.register_late_pass(move || Box::new(iter_not_returning_iterator::IterNotReturningIterator));
537537
store.register_late_pass(move || Box::new(if_then_panic::IfThenPanic));
538+
let enable_raw_pointer_heuristic_for_send = conf.enable_raw_pointer_heuristic_for_send;
539+
store.register_late_pass(move || Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(enable_raw_pointer_heuristic_for_send)));
538540
}
539541

540542
#[rustfmt::skip]
Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,238 @@
1+
use clippy_utils::diagnostics::span_lint_and_then;
2+
use clippy_utils::is_lint_allowed;
3+
use clippy_utils::source::snippet;
4+
use clippy_utils::ty::{implements_trait, is_copy};
5+
use rustc_ast::ImplPolarity;
6+
use rustc_hir::def_id::DefId;
7+
use rustc_hir::{FieldDef, Item, ItemKind, Node};
8+
use rustc_lint::{LateContext, LateLintPass};
9+
use rustc_middle::ty::{self, subst::GenericArgKind, Ty};
10+
use rustc_session::{declare_tool_lint, impl_lint_pass};
11+
use rustc_span::sym;
12+
13+
declare_clippy_lint! {
14+
/// ### What it does
15+
/// Warns about fields in struct implementing `Send` that are neither `Send` nor `Copy`.
16+
///
17+
/// ### Why is this bad?
18+
/// Sending the struct to another thread will transfer the ownership to
19+
/// the new thread by dropping in the current thread during the transfer.
20+
/// This causes soundness issues for non-`Send` fields, as they are also
21+
/// dropped and might not be set up to handle this.
22+
///
23+
/// See:
24+
/// * [*The Rustonomicon* about *Send and Sync*](https://doc.rust-lang.org/nomicon/send-and-sync.html)
25+
/// * [The documentation of `Send`](https://doc.rust-lang.org/std/marker/trait.Send.html)
26+
///
27+
/// ### Known Problems
28+
/// Data structures that contain raw pointers may cause false positives.
29+
/// They are sometimes safe to be sent across threads but do not implement
30+
/// the `Send` trait. This lint has a heuristic to filter out basic cases
31+
/// such as `Vec<*const T>`, but it's not perfect. Feel free to create an
32+
/// issue if you have a suggestion on how this heuristic can be improved.
33+
///
34+
/// ### Example
35+
/// ```rust,ignore
36+
/// struct ExampleStruct<T> {
37+
/// rc_is_not_send: Rc<String>,
38+
/// unbounded_generic_field: T,
39+
/// }
40+
///
41+
/// // This impl is unsound because it allows sending `!Send` types through `ExampleStruct`
42+
/// unsafe impl<T> Send for ExampleStruct<T> {}
43+
/// ```
44+
/// Use thread-safe types like [`std::sync::Arc`](https://doc.rust-lang.org/std/sync/struct.Arc.html)
45+
/// or specify correct bounds on generic type parameters (`T: Send`).
46+
pub NON_SEND_FIELDS_IN_SEND_TY,
47+
nursery,
48+
"there is field that does not implement `Send` in a `Send` struct"
49+
}
50+
51+
#[derive(Copy, Clone)]
52+
pub struct NonSendFieldInSendTy {
53+
enable_raw_pointer_heuristic: bool,
54+
}
55+
56+
impl NonSendFieldInSendTy {
57+
pub fn new(enable_raw_pointer_heuristic: bool) -> Self {
58+
Self {
59+
enable_raw_pointer_heuristic,
60+
}
61+
}
62+
}
63+
64+
impl_lint_pass!(NonSendFieldInSendTy => [NON_SEND_FIELDS_IN_SEND_TY]);
65+
66+
impl<'tcx> LateLintPass<'tcx> for NonSendFieldInSendTy {
67+
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
68+
let ty_allowed_in_send = if self.enable_raw_pointer_heuristic {
69+
ty_allowed_with_raw_pointer_heuristic
70+
} else {
71+
ty_allowed_without_raw_pointer_heuristic
72+
};
73+
74+
// Checks if we are in `Send` impl item.
75+
// We start from `Send` impl instead of `check_field_def()` because
76+
// single `AdtDef` may have multiple `Send` impls due to generic
77+
// parameters, and the lint is much easier to implement in this way.
78+
if_chain! {
79+
if let Some(send_trait) = cx.tcx.get_diagnostic_item(sym::send_trait);
80+
if let ItemKind::Impl(hir_impl) = &item.kind;
81+
if let Some(trait_ref) = &hir_impl.of_trait;
82+
if let Some(trait_id) = trait_ref.trait_def_id();
83+
if send_trait == trait_id;
84+
if let ImplPolarity::Positive = hir_impl.polarity;
85+
if let Some(ty_trait_ref) = cx.tcx.impl_trait_ref(item.def_id);
86+
if let self_ty = ty_trait_ref.self_ty();
87+
if let ty::Adt(adt_def, impl_trait_substs) = self_ty.kind();
88+
then {
89+
let mut non_send_fields = Vec::new();
90+
91+
let hir_map = cx.tcx.hir();
92+
for variant in &adt_def.variants {
93+
for field in &variant.fields {
94+
if_chain! {
95+
if let Some(field_hir_id) = field
96+
.did
97+
.as_local()
98+
.map(|local_def_id| hir_map.local_def_id_to_hir_id(local_def_id));
99+
if !is_lint_allowed(cx, NON_SEND_FIELDS_IN_SEND_TY, field_hir_id);
100+
if let field_ty = field.ty(cx.tcx, impl_trait_substs);
101+
if !ty_allowed_in_send(cx, field_ty, send_trait);
102+
if let Node::Field(field_def) = hir_map.get(field_hir_id);
103+
then {
104+
non_send_fields.push(NonSendField {
105+
def: field_def,
106+
ty: field_ty,
107+
generic_params: collect_generic_params(cx, field_ty),
108+
})
109+
}
110+
}
111+
}
112+
}
113+
114+
if !non_send_fields.is_empty() {
115+
span_lint_and_then(
116+
cx,
117+
NON_SEND_FIELDS_IN_SEND_TY,
118+
item.span,
119+
&format!(
120+
"this implementation is unsound, as some fields in `{}` are `!Send`",
121+
snippet(cx, hir_impl.self_ty.span, "Unknown")
122+
),
123+
|diag| {
124+
for field in non_send_fields {
125+
diag.span_note(
126+
field.def.span,
127+
&format!("the type of field `{}` is `!Send`", field.def.ident.name),
128+
);
129+
130+
match field.generic_params.len() {
131+
0 => diag.help("use a thread-safe type that implements `Send`"),
132+
1 if is_ty_param(field.ty) => diag.help(&format!("add `{}: Send` bound in `Send` impl", field.ty)),
133+
_ => diag.help(&format!(
134+
"add bounds on type parameter{} `{}` that satisfy `{}: Send`",
135+
if field.generic_params.len() > 1 { "s" } else { "" },
136+
field.generic_params_string(),
137+
snippet(cx, field.def.ty.span, "Unknown"),
138+
)),
139+
};
140+
}
141+
},
142+
);
143+
}
144+
}
145+
}
146+
}
147+
}
148+
149+
struct NonSendField<'tcx> {
150+
def: &'tcx FieldDef<'tcx>,
151+
ty: Ty<'tcx>,
152+
generic_params: Vec<Ty<'tcx>>,
153+
}
154+
155+
impl<'tcx> NonSendField<'tcx> {
156+
fn generic_params_string(&self) -> String {
157+
self.generic_params
158+
.iter()
159+
.map(ToString::to_string)
160+
.collect::<Vec<_>>()
161+
.join(", ")
162+
}
163+
}
164+
165+
/// Given a type, collect all of its generic parameters.
166+
/// Example: `MyStruct<P, Box<Q, R>>` => `vec![P, Q, R]`
167+
fn collect_generic_params<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Vec<Ty<'tcx>> {
168+
ty.walk(cx.tcx)
169+
.filter_map(|inner| match inner.unpack() {
170+
GenericArgKind::Type(inner_ty) => Some(inner_ty),
171+
_ => None,
172+
})
173+
.filter(|&inner_ty| is_ty_param(inner_ty))
174+
.collect()
175+
}
176+
177+
/// Be more strict when the heuristic is disabled
178+
fn ty_allowed_without_raw_pointer_heuristic<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, send_trait: DefId) -> bool {
179+
if implements_trait(cx, ty, send_trait, &[]) {
180+
return true;
181+
}
182+
183+
if is_copy(cx, ty) && !contains_raw_pointer(cx, ty) {
184+
return true;
185+
}
186+
187+
false
188+
}
189+
190+
/// Heuristic to allow cases like `Vec<*const u8>`
191+
fn ty_allowed_with_raw_pointer_heuristic<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, send_trait: DefId) -> bool {
192+
if implements_trait(cx, ty, send_trait, &[]) || is_copy(cx, ty) {
193+
return true;
194+
}
195+
196+
// The type is known to be `!Send` and `!Copy`
197+
match ty.kind() {
198+
ty::Tuple(_) => ty
199+
.tuple_fields()
200+
.all(|ty| ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait)),
201+
ty::Array(ty, _) | ty::Slice(ty) => ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait),
202+
ty::Adt(_, substs) => {
203+
if contains_raw_pointer(cx, ty) {
204+
// descends only if ADT contains any raw pointers
205+
substs.iter().all(|generic_arg| match generic_arg.unpack() {
206+
GenericArgKind::Type(ty) => ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait),
207+
// Lifetimes and const generics are not solid part of ADT and ignored
208+
GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => true,
209+
})
210+
} else {
211+
false
212+
}
213+
},
214+
// Raw pointers are `!Send` but allowed by the heuristic
215+
ty::RawPtr(_) => true,
216+
_ => false,
217+
}
218+
}
219+
220+
/// Checks if the type contains any raw pointers in substs (including nested ones).
221+
fn contains_raw_pointer<'tcx>(cx: &LateContext<'tcx>, target_ty: Ty<'tcx>) -> bool {
222+
for ty_node in target_ty.walk(cx.tcx) {
223+
if_chain! {
224+
if let GenericArgKind::Type(inner_ty) = ty_node.unpack();
225+
if let ty::RawPtr(_) = inner_ty.kind();
226+
then {
227+
return true;
228+
}
229+
}
230+
}
231+
232+
false
233+
}
234+
235+
/// Returns `true` if the type is a type parameter such as `T`.
236+
fn is_ty_param(target_ty: Ty<'_>) -> bool {
237+
matches!(target_ty.kind(), ty::Param(_))
238+
}

clippy_lints/src/utils/conf.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,10 @@ define_Conf! {
284284
///
285285
/// The list of unicode scripts allowed to be used in the scope.
286286
(allowed_scripts: Vec<String> = vec!["Latin".to_string()]),
287+
/// Lint: NON_SEND_FIELDS_IN_SEND_TY.
288+
///
289+
/// Whether to apply the raw pointer heuristic to determine if a type is `Send`.
290+
(enable_raw_pointer_heuristic_for_send: bool = true),
287291
}
288292

289293
/// Search for the configuration file.
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
enable-raw-pointer-heuristic-for-send = false
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
#![warn(clippy::non_send_fields_in_send_ty)]
2+
#![feature(extern_types)]
3+
4+
use std::rc::Rc;
5+
6+
// Basic tests should not be affected
7+
pub struct NoGeneric {
8+
rc_is_not_send: Rc<String>,
9+
}
10+
11+
unsafe impl Send for NoGeneric {}
12+
13+
pub struct MultiField<T> {
14+
field1: T,
15+
field2: T,
16+
field3: T,
17+
}
18+
19+
unsafe impl<T> Send for MultiField<T> {}
20+
21+
pub enum MyOption<T> {
22+
MySome(T),
23+
MyNone,
24+
}
25+
26+
unsafe impl<T> Send for MyOption<T> {}
27+
28+
// All fields are disallowed when raw pointer heuristic is off
29+
extern "C" {
30+
type NonSend;
31+
}
32+
33+
pub struct HeuristicTest {
34+
field1: Vec<*const NonSend>,
35+
field2: [*const NonSend; 3],
36+
field3: (*const NonSend, *const NonSend, *const NonSend),
37+
field4: (*const NonSend, Rc<u8>),
38+
field5: Vec<Vec<*const NonSend>>,
39+
}
40+
41+
unsafe impl Send for HeuristicTest {}
42+
43+
fn main() {}

0 commit comments

Comments
 (0)