Skip to content

Commit 5a1391d

Browse files
jsgfjsgf
committed
Address review comments
1 parent fc17293 commit 5a1391d

File tree

3 files changed

+64
-57
lines changed

3 files changed

+64
-57
lines changed

‎clippy_lints/src/volatile_composites.rs‎

Lines changed: 28 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
use clippy_utils::diagnostics::span_lint;
22
use clippy_utils::sym;
3+
use clippy_utils::ty::is_type_diagnostic_item;
34
use rustc_hir::{Expr, ExprKind};
45
use rustc_lint::{LateContext, LateLintPass};
5-
use rustc_middle::ty::{self, Ty};
6+
use rustc_middle::ty::layout::LayoutOf;
7+
use rustc_middle::ty::{self, Ty, TypeVisitableExt};
68
use rustc_session::declare_lint_pass;
79

810
declare_clippy_lint! {
@@ -50,60 +52,37 @@ declare_clippy_lint! {
5052
/// }
5153
/// }
5254
/// ```
53-
#[clippy::version = "1.91.0"]
55+
#[clippy::version = "1.92.0"]
5456
pub VOLATILE_COMPOSITES,
5557
nursery,
5658
"warn about volatile read/write applied to composite types"
5759
}
5860
declare_lint_pass!(VolatileComposites => [VOLATILE_COMPOSITES]);
5961

60-
// functions:
61-
// core::ptr::{read_volatile,write_volatile}
62-
// methods:
63-
// pointer::{read_volatile,write_volatile}
64-
// NonNull::{read_volatile,write_volatile}
65-
66-
// primitive type:
67-
// unit, [iu]{8,16,32,64,128?}, f{32,64}, thin pointer, usize, isize, bool, char
68-
// C enum with primitive repr
69-
// #[repr(transparent)] wrapper of above
70-
71-
// Zero-sized types are intrinsically safe to use volatile on since they won't
72-
// actually generate *any* loads or stores. But this is also used to skip zero
73-
// fields of #[repr(transparent)] structures.
62+
/// Zero-sized types are intrinsically safe to use volatile on since they won't
63+
/// actually generate *any* loads or stores. But this is also used to skip zero-sized
64+
/// fields of `#[repr(transparent)]` structures.
7465
fn is_zero_sized_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
75-
if let Ok(ty) = cx.tcx.try_normalize_erasing_regions(cx.typing_env(), ty)
76-
&& let Ok(layout) = cx.tcx.layout_of(cx.typing_env().as_query_input(ty))
77-
{
78-
layout.layout.size().bytes() == 0
79-
} else {
80-
false
81-
}
66+
cx.layout_of(ty).is_ok_and(|layout| layout.is_zst())
8267
}
8368

84-
// Make sure the raw pointer has no metadata
85-
fn is_narrow_raw_ptr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
86-
if let ty::RawPtr(_inner, _) = ty.kind() {
87-
ty.pointee_metadata_ty_or_projection(cx.tcx).is_unit()
88-
} else {
89-
false
69+
/// A thin raw pointer or reference.
70+
fn is_narrow_ptr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
71+
match ty.kind() {
72+
ty::RawPtr(inner, _) | ty::Ref(_, inner, _) => inner.has_trivial_sizedness(cx.tcx, ty::SizedTraitKind::Sized),
73+
_ => false,
9074
}
9175
}
9276

93-
// Enum with some fixed representation and no data-carrying variants
77+
/// Enum with some fixed representation and no data-carrying variants.
9478
fn is_enum_repr_c<'tcx>(_cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
95-
if let ty::Adt(adt_def, _args) = ty.kind()
96-
&& adt_def.is_enum()
97-
&& adt_def.repr().inhibit_struct_field_reordering()
98-
{
99-
adt_def.is_payloadfree()
100-
} else {
101-
false
102-
}
79+
ty.ty_adt_def().is_some_and(|adt_def| {
80+
adt_def.is_enum() && adt_def.repr().inhibit_struct_field_reordering() && adt_def.is_payloadfree()
81+
})
10382
}
10483

105-
// #[repr(transparent)] structures are also OK if the only non-zero
106-
// sized field contains a volatile-safe type
84+
/// `#[repr(transparent)]` structures are also OK if the only non-zero
85+
/// sized field contains a volatile-safe type.
10786
fn is_struct_repr_transparent<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
10887
if let ty::Adt(adt_def, args) = ty.kind()
10988
&& adt_def.is_struct()
@@ -123,8 +102,8 @@ fn is_struct_repr_transparent<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> boo
123102
}
124103
}
125104

126-
// SIMD can be useful to get larger atomic loads/stores, though this is still
127-
// pretty machine-dependent.
105+
/// SIMD can be useful to get larger single loads/stores, though this is still
106+
/// pretty machine-dependent.
128107
fn is_simd_repr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
129108
if let ty::Adt(adt_def, _args) = ty.kind()
130109
&& adt_def.is_struct()
@@ -137,21 +116,19 @@ fn is_simd_repr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
137116
}
138117
}
139118

140-
// We can't know about a generic type, so just let it pass to avoid noise
141-
fn is_generic<'tcx>(_cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
142-
ty.flags().intersects(ty::TypeFlags::HAS_PARAM)
143-
}
144-
119+
/// Top-level predicate for whether a type is volatile-safe or not.
145120
fn is_volatile_safe_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
146121
ty.is_primitive()
147-
|| is_narrow_raw_ptr(cx, ty)
122+
|| is_narrow_ptr(cx, ty)
148123
|| is_zero_sized_ty(cx, ty)
149124
|| is_enum_repr_c(cx, ty)
150125
|| is_simd_repr(cx, ty)
151126
|| is_struct_repr_transparent(cx, ty)
152-
|| is_generic(cx, ty)
127+
// We can't know about a generic type, so just let it pass to avoid noise
128+
|| ty.has_non_region_param()
153129
}
154130

131+
/// Print diagnostic for volatile read/write on non-volatile-safe types.
155132
fn report_volatile_safe<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, ty: Ty<'tcx>) {
156133
if !is_volatile_safe_ty(cx, ty) {
157134
span_lint(
@@ -177,7 +154,7 @@ impl<'tcx> LateLintPass<'tcx> for VolatileComposites {
177154
// Raw pointers
178155
ty::RawPtr(innerty, _) => report_volatile_safe(cx, expr, *innerty),
179156
// std::ptr::NonNull
180-
ty::Adt(adt_def, args) if cx.tcx.is_diagnostic_item(sym::NonNull, adt_def.did()) => {
157+
ty::Adt(_, args) if is_type_diagnostic_item(cx, self_ty, sym::NonNull) => {
181158
report_volatile_safe(cx, expr, args.type_at(0));
182159
},
183160
_ => (),
@@ -192,7 +169,7 @@ impl<'tcx> LateLintPass<'tcx> for VolatileComposites {
192169
cx.tcx.get_diagnostic_name(def_id),
193170
Some(sym::ptr_read_volatile | sym::ptr_write_volatile)
194171
)
195-
&& let ty::RawPtr(ptrty, _) = cx.typeck_results().expr_ty(arg_ptr).kind()
172+
&& let ty::RawPtr(ptrty, _) = cx.typeck_results().expr_ty_adjusted(arg_ptr).kind()
196173
{
197174
report_volatile_safe(cx, expr, *ptrty);
198175
}

‎tests/ui/volatile_composites.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,16 @@ fn main() {
100100
thing.write_volatile(&"goodbye".into()); // OK
101101
}
102102

103+
// Zero size types OK
104+
unsafe {
105+
struct Empty;
106+
107+
(0xdead as *mut Empty).write_volatile(Empty); // OK
108+
// Note that this is OK because Wrapper<Empty> is itself ZST, not because of the repr transparent
109+
// handling tested below.
110+
(0xdead as *mut Wrapper<Empty>).write_volatile(Wrapper((), Empty, ())); // OK
111+
}
112+
103113
// Via repr transparent newtype
104114
unsafe {
105115
(0xdead as *mut Wrapper<usize>).write_volatile(Wrapper((), 123, ())); // OK
@@ -127,6 +137,14 @@ fn main() {
127137
//~^ volatile_composites
128138
}
129139

140+
// Plain pointers and pointers with lifetimes are OK
141+
unsafe {
142+
let v: u32 = 123;
143+
let rv: &u32 = &v;
144+
145+
(0xdead as *mut &u32).write_volatile(rv); // OK
146+
}
147+
130148
// C-style enums are OK
131149
unsafe {
132150
// Bad: need some specific repr
@@ -187,6 +205,12 @@ fn main() {
187205
unsafe {
188206
do_device_write::<MyDevRegisters>(0xdead as *mut _, Default::default()); // OK
189207
}
208+
209+
let mut s = String::from("foo");
210+
unsafe {
211+
std::ptr::write_volatile(&mut s, String::from("bar"));
212+
//~^ volatile_composites
213+
}
190214
}
191215

192216
// Generic OK

‎tests/ui/volatile_composites.stderr‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,34 +50,40 @@ LL | let _regs = ptr.read_volatile();
5050
| ^^^^^^^^^^^^^^^^^^^
5151

5252
error: type `Wrapper<MyDevRegisters>` is not volatile-compatible
53-
--> tests/ui/volatile_composites.rs:108:9
53+
--> tests/ui/volatile_composites.rs:118:9
5454
|
5555
LL | (0xdead as *mut Wrapper<MyDevRegisters>).write_volatile(Wrapper((), MyDevRegisters::default(), ()));
5656
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5757

5858
error: type `*const [u32]` is not volatile-compatible
59-
--> tests/ui/volatile_composites.rs:126:9
59+
--> tests/ui/volatile_composites.rs:136:9
6060
|
6161
LL | (0xdead as *mut *const [u32]).write_volatile(wideptr);
6262
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6363

6464
error: type `main::PlainEnum` is not volatile-compatible
65-
--> tests/ui/volatile_composites.rs:139:9
65+
--> tests/ui/volatile_composites.rs:157:9
6666
|
6767
LL | (0xdead as *mut PlainEnum).write_volatile(PlainEnum::A);
6868
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
6969

7070
error: type `main::SumType` is not volatile-compatible
71-
--> tests/ui/volatile_composites.rs:167:9
71+
--> tests/ui/volatile_composites.rs:185:9
7272
|
7373
LL | (0xdead as *mut SumType).write_volatile(SumType::C);
7474
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
7575

7676
error: type `main::ReprSumType` is not volatile-compatible
77-
--> tests/ui/volatile_composites.rs:177:9
77+
--> tests/ui/volatile_composites.rs:195:9
7878
|
7979
LL | (0xdead as *mut ReprSumType).write_volatile(ReprSumType::C);
8080
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
8181

82-
error: aborting due to 13 previous errors
82+
error: type `std::string::String` is not volatile-compatible
83+
--> tests/ui/volatile_composites.rs:211:9
84+
|
85+
LL | std::ptr::write_volatile(&mut s, String::from("bar"));
86+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
87+
88+
error: aborting due to 14 previous errors
8389

0 commit comments

Comments
 (0)