Skip to content

Commit

Permalink
rust: enable clippy::undocumented_unsafe_blocks lint
Browse files Browse the repository at this point in the history
Checking that we are not missing any `// SAFETY` comments in our `unsafe`
blocks is something we have wanted to do for a long time, as well as
cleaning up the remaining cases that were not documented [1].

Back when Rust for Linux started, this was something that could have
been done via a script, like Rust's `tidy`. Soon after, in Rust 1.58.0,
Clippy implemented the `undocumented_unsafe_blocks` lint [2].

Even though the lint has a few false positives, e.g. in some cases where
attributes appear between the comment and the `unsafe` block [3], there
are workarounds and the lint seems quite usable already.

Thus enable the lint now.

We still have a few cases to clean up, so just allow those for the moment
by writing a `TODO` comment -- some of those may be good candidates for
new contributors.

Link: Rust-for-Linux/linux#351 [1]
Link: https://rust-lang.github.io/rust-clippy/master/#/undocumented_unsafe_blocks [2]
Link: rust-lang/rust-clippy#13189 [3]
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20240904204347.168520-5-ojeda@kernel.org
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
(cherry picked from commit db4f72c904cb116e2bf56afdd67fc5167a607a7b)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
  • Loading branch information
ojeda authored and bonzini committed Dec 2, 2024
1 parent 9152123 commit cb3aeb7
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 0 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,6 @@ needless_bitwise_bool = "deny"
needless_continue = "deny"
needless_lifetimes = "deny"
no_mangle_with_rust_abi = "deny"
undocumented_unsafe_blocks = "deny"
unnecessary_safety_comment = "deny"
unnecessary_safety_doc = "deny"
2 changes: 2 additions & 0 deletions src/__internal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,12 @@ impl<T: ?Sized> Clone for AllData<T> {

impl<T: ?Sized> Copy for AllData<T> {}

// SAFETY: TODO.
unsafe impl<T: ?Sized> InitData for AllData<T> {
type Datee = T;
}

// SAFETY: TODO.
unsafe impl<T: ?Sized> HasInitData for T {
type InitData = AllData<T>;

Expand Down
4 changes: 4 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,7 @@ macro_rules! stack_try_pin_init {
///
/// let init = pin_init!(&this in Buf {
/// buf: [0; 64],
/// // SAFETY: TODO.
/// ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() },
/// pin: PhantomPinned,
/// });
Expand Down Expand Up @@ -1153,6 +1154,7 @@ where
// SAFETY: Every type can be initialized by-value.
unsafe impl<T, E> Init<T, E> for T {
unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
// SAFETY: TODO.
unsafe { slot.write(self) };
Ok(())
}
Expand All @@ -1161,6 +1163,7 @@ unsafe impl<T, E> Init<T, E> for T {
// SAFETY: Every type can be initialized by-value. `__pinned_init` calls `__init`.
unsafe impl<T, E> PinInit<T, E> for T {
unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
// SAFETY: TODO.
unsafe { self.__init(slot) }
}
}
Expand Down Expand Up @@ -1387,6 +1390,7 @@ macro_rules! impl_zeroable {
($($(#[$attr:meta])*$({$($generics:tt)*})? $t:ty, )*) => {
$(
$(#[$attr])*
// SAFETY: Safety comments written in the macro invocation.
unsafe impl$($($generics)*)? Zeroable for $t {}
)*
};
Expand Down
9 changes: 9 additions & 0 deletions src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ macro_rules! __pinned_drop {
}
),
) => {
// SAFETY: TODO.
unsafe $($impl_sig)* {
// Inherit all attributes and the type/ident tokens for the signature.
$(#[$($attr)*])*
Expand Down Expand Up @@ -872,6 +873,7 @@ macro_rules! __pin_data {
}
}

// SAFETY: TODO.
unsafe impl<$($impl_generics)*>
$crate::__internal::PinData for __ThePinData<$($ty_generics)*>
where $($whr)*
Expand Down Expand Up @@ -996,6 +998,7 @@ macro_rules! __pin_data {
slot: *mut $p_type,
init: impl $crate::PinInit<$p_type, E>,
) -> ::core::result::Result<(), E> {
// SAFETY: TODO.
unsafe { $crate::PinInit::__pinned_init(init, slot) }
}
)*
Expand All @@ -1005,6 +1008,7 @@ macro_rules! __pin_data {
slot: *mut $type,
init: impl $crate::Init<$type, E>,
) -> ::core::result::Result<(), E> {
// SAFETY: TODO.
unsafe { $crate::Init::__init(init, slot) }
}
)*
Expand Down Expand Up @@ -1121,6 +1125,8 @@ macro_rules! __init_internal {
// no possibility of returning without `unsafe`.
struct __InitOk;
// Get the data about fields from the supplied type.
//
// SAFETY: TODO.
let data = unsafe {
use $crate::__internal::$has_data;
// Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal
Expand Down Expand Up @@ -1176,6 +1182,7 @@ macro_rules! __init_internal {
let init = move |slot| -> ::core::result::Result<(), $err> {
init(slot).map(|__InitOk| ())
};
// SAFETY: TODO.
let init = unsafe { $crate::$construct_closure::<_, $err>(init) };
init
}};
Expand Down Expand Up @@ -1324,6 +1331,8 @@ macro_rules! __init_internal {
// Endpoint, nothing more to munch, create the initializer.
// Since we are in the closure that is never called, this will never get executed.
// We abuse `slot` to get the correct type inference here:
//
// SAFETY: TODO.
unsafe {
// Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal
// information that is associated to already parsed fragments, so a path fragment
Expand Down

0 comments on commit cb3aeb7

Please sign in to comment.