rust: lint improvements
Subsystems (7)
Miguel Ojeda
maintainercover rust: lint improvements
Hi all,
This is a series that contains a series of lint-related things:
- Cleanups and other improvements and fixes, including removing allows that are not needed anymore for different reasons and a workaround for dbg_macro detection.
- The enablement of some safety lints so that the toolchain enforces that we write // SAFETY comments and # Safety sections properly.
- The addition of .clippy.toml, which allows us to take advantage of a few configuration options.
- Start using the new #[expect(...)] feature and add documentation on it as well as lints in general.
Overall, this should improve the quality of the code and documentation as well as reduce the time needed in reviews.
I want to mention Trevor's nice work on lints from a while ago [1]. I think we should still do something like that: discuss which lints we would like to have one-by-one and start enabling them (and perhaps have a file like Trevor proposed etc.).
For the moment, though, I am sending these, since we would like to have at least the safety-related ones enabled soon [2]: now that more code and developers are joining, it sounds like a good time to start enforcing it -- it should make new Rust kernel developers aware of the need of writing them, which has proven to be a common request from reviewers.
If needed, the series can be applied partially or split, but most of it should be fairly uncontroversial.
Miguel Ojeda (19):
rust: workqueue: remove unneeded ``#[allow(clippy::new_ret_no_self)]`
rust: sort global Rust flags
rust: types: avoid repetition in {As,From}Bytes impls
rust: enable clippy::undocumented_unsafe_blocks lint
rust: enable clippy::unnecessary_safety_comment lint
rust: enable clippy::unnecessary_safety_doc lint
rust: enable clippy::ignored_unit_patterns lint
rust: enable rustdoc::unescaped_backticks lint
rust: init: remove unneeded #[allow(clippy::disallowed_names)]
rust: sync: remove unneeded
`#[allow(clippy::non_send_fields_in_send_ty)]`
rust: introduce .clippy.toml
rust: replace clippy::dbg_macro with disallowed_macros
rust: rbtree: fix SAFETY comments that should be # Safety sections
rust: provide proper code documentation titles
rust: enable Clippy's check-private-items
Documentation: rust: add coding guidelines on lints
rust: start using the #[expect(...)] attribute
Documentation: rust: discuss #[expect(...)] in the guidelines
rust: std_vendor: simplify { .. macro! .. } with inner attributes
.clippy.toml | 9 ++ .gitignore | 1 + Documentation/rust/coding-guidelines.rst | 139 +++++++++++++++++++++++ MAINTAINERS | 1 + Makefile | 15 ++- rust/Makefile | 5 +- rust/bindings/lib.rs | 1 + rust/kernel/alloc/allocator.rs | 2 + rust/kernel/error.rs | 11 +- rust/kernel/init.rs | 30 ++--- rust/kernel/init/__internal.rs | 11 +- rust/kernel/init/macros.rs | 18 ++- rust/kernel/ioctl.rs | 2 +- rust/kernel/lib.rs | 1 + rust/kernel/list.rs | 1 + rust/kernel/list/arc_field.rs | 2 +- rust/kernel/print.rs | 5 +- rust/kernel/rbtree.rs | 9 +- rust/kernel/std_vendor.rs | 16 ++- rust/kernel/str.rs | 7 +- rust/kernel/sync/arc.rs | 2 +- rust/kernel/sync/arc/std_vendor.rs | 2 + rust/kernel/sync/condvar.rs | 1 - rust/kernel/sync/lock.rs | 6 +- rust/kernel/types.rs | 74 ++++++------ rust/kernel/workqueue.rs | 9 +- rust/uapi/lib.rs | 1 + samples/rust/rust_print.rs | 1 + scripts/Makefile.build | 2 +- 29 files changed, 293 insertions(+), 91 deletions(-) create mode 100644 .clippy.toml
base-commit: 68d3b6aa08708bb3907c2c13eaf4b3ccf4805160 --
2.46.0
Miguel Ojeda
maintainerPATCH 1/19 rust: workqueue: remove unneeded ``#[allow(clippy::new_ret_no_self)]`
Perform the same clean commit b2516f7af9d2 ("rust: kernel: remove #[allow(clippy::new_ret_no_self)]") did for a case that appeared in workqueue in parallel in commit 7324b88975c5 ("rust: workqueue: add helper for defining work_struct fields"):
Clippy triggered a false positive on its new_ret_no_self lint when using the pin_init! macro. Since Rust 1.67.0, that does not happen anymore, since Clippy learnt to not warn about -> impl Trait<Self> [1][2].
The kernel nowadays uses Rust 1.72.1, thus remove the #[allow].
--- rust/kernel/workqueue.rs | 1 - 1 file changed, 1 deletion(-)
rust/kernel/workqueue.rs +0 -1
@@ -366,7 +366,6 @@ unsafe impl<T: ?Sized, const ID: u64> Sync for Work<T, ID> {}impl <T : ?Sized ,const ID :u64 >Work <T ,ID > {/// Creates a new instance of [`Work`]. #[inline] -#[allow(clippy::new_ret_no_self)] pub fn new (name: &'staticCStr , key: &'staticLockClassKey ) ->impl PinInit <Self >where T :WorkItem <ID >,
Signature · 1 line
2.46.0
Re: [PATCH 01/19] rust: workqueue: remove unneeded ``#[allow(clippy::new_ret_no_self)]`
Quoted 11 lines · Miguel Ojeda
Perform the same clean commit
b2516f7af9d2("rust: kernel: remove#[allow(clippy::new_ret_no_self)]") did for a case that appeared in workqueue in parallel in commit7324b88975c5("rust: workqueue: add helper for definingwork_structfields"):Clippy triggered a false positive on its
new_ret_no_selflint when using thepin_init!macro. Since Rust 1.67.0, that does not happen anymore, since Clippy learnt to not warn about-> impl Trait<Self>[1][2].
Quoted 6 lines
The kernel nowadays uses Rust 1.72.1, thus remove the
#[allow].Signed-off-by Miguel Ojeda <[email protected]>
Re: [PATCH 01/19] rust: workqueue: remove unneeded ``#[allow(clippy::new_ret_no_self)]`
In reply to Miguel OjedaQuoted 11 lines · Miguel Ojeda
Perform the same clean commit
b2516f7af9d2("rust: kernel: remove#[allow(clippy::new_ret_no_self)]") did for a case that appeared in workqueue in parallel in commit7324b88975c5("rust: workqueue: add helper for definingwork_structfields"):Clippy triggered a false positive on its
new_ret_no_selflint when using thepin_init!macro. Since Rust 1.67.0, that does not happen anymore, since Clippy learnt to not warn about-> impl Trait<Self>[1][2].
Quoted 6 lines
The kernel nowadays uses Rust 1.72.1, thus remove the
#[allow].Signed-off-by Miguel Ojeda <[email protected]>
Miguel Ojeda
maintainerPATCH 2/19 rust: sort global Rust flags
In reply to Miguel OjedaSort the global Rust flags so that it is easier to follow along when we have more, like this patch series does.
--- Makefile | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Makefile +4 -4
@@ -445,18 +445,18 @@ KBUILD_USERLDFLAGS := $(USERLDFLAGS) # host programs. export rust_common_flags := --edition=2021 \ -Zbinary_dep_depinfo=y \ - -Dunsafe_op_in_unsafe_fn \ -Dnon_ascii_idents \ + -Dunsafe_op_in_unsafe_fn \ + -Wmissing_docs \ -Wrust_2018_idioms \ -Wunreachable_pub \ - -Wmissing_docs \ - -Wrustdoc::missing_crate_level_docs \ -Wclippy::all \ + -Wclippy::dbg_macro \ -Wclippy::mut_mut \ -Wclippy::needless_bitwise_bool \ -Wclippy::needless_continue \ -Wclippy::no_mangle_with_rust_abi \ - -Wclippy::dbg_macro + -Wrustdoc::missing_crate_level_docs KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) -I $(srctree)/scripts/include
Signature · 1 line
2.46.0
Re: [PATCH 02/19] rust: sort global Rust flags
Quoted 5 lines · Miguel Ojeda
Sort the global Rust flags so that it is easier to follow along when we have more, like this patch series does.
Signed-off-by Miguel Ojeda <[email protected]>
Re: [PATCH 02/19] rust: sort global Rust flags
In reply to Miguel OjedaQuoted 5 lines · Miguel Ojeda
Sort the global Rust flags so that it is easier to follow along when we have more, like this patch series does.
Signed-off-by Miguel Ojeda <[email protected]>
Miguel Ojeda
maintainerPATCH 3/19 rust: types: avoid repetition in `{As,From}Bytes` impls
In reply to Miguel OjedaIn order to provide // SAFETY comments for every unsafe impl, we would need to repeat them, which is not very useful and would be harder to read.
We could perhaps allow the lint (ideally within a small module), but we can take the chance to avoid the repetition of the impls themselves too by using a small local macro, like in other places where we have had to do this sort of thing.
Thus add the straightforward impl_{from,as}bytes! macros and use them to implement FromBytes.
This, in turn, will allow us in the next patch to place a // SAFETY comment that defers to the actual invocation of the macro.
--- rust/kernel/types.rs | 68 +++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 33 deletions(-)
rust/kernel/types.rs +35 -33
@@ -481,21 +481,22 @@ pub enum Either<L, R> {/// All bit-patterns must be valid for this type. This type must not have interior mutability. pub unsafe trait FromBytes {} -// SAFETY: All bit patterns are acceptable values of the types below. -unsafe impl FromBytes for u8 {} -unsafe impl FromBytes for u16 {} -unsafe impl FromBytes for u32 {} -unsafe impl FromBytes for u64 {} -unsafe impl FromBytes for usize {} -unsafe impl FromBytes for i8 {} -unsafe impl FromBytes for i16 {} -unsafe impl FromBytes for i32 {} -unsafe impl FromBytes for i64 {} -unsafe impl FromBytes for isize {} -// SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit -// patterns are also acceptable for arrays of that type. -unsafe impl <T :FromBytes >FromBytes for [T ] {} -unsafe impl <T :FromBytes ,const N :usize >FromBytes for [T ;N ] {} +macro_rules! impl_frombytes { + ($($({$($generics:tt)*})? $t:ty, )*) => { + $(unsafe impl $($($generics)*)?FromBytes for $t {})* + }; +} + +impl_frombytes! { +// SAFETY: All bit patterns are acceptable values of the types below. +u8 ,u16 ,u32 ,u64 ,usize , +i8 ,i16 ,i32 ,i64 ,isize , + +// SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit +// patterns are also acceptable for arrays of that type. + {<T :FromBytes >} [T ], + {<T :FromBytes ,const N :usize >} [T ;N ], +}/// Types that can be viewed as an immutable slice of initialized bytes. ///
@@ -514,21 +515,22 @@ unsafe impl<T: FromBytes> FromBytes for [T] {}/// mutability. pub unsafe trait AsBytes {} -// SAFETY: Instances of the following types have no uninitialized portions. -unsafe impl AsBytes for u8 {} -unsafe impl AsBytes for u16 {} -unsafe impl AsBytes for u32 {} -unsafe impl AsBytes for u64 {} -unsafe impl AsBytes for usize {} -unsafe impl AsBytes for i8 {} -unsafe impl AsBytes for i16 {} -unsafe impl AsBytes for i32 {} -unsafe impl AsBytes for i64 {} -unsafe impl AsBytes for isize {} -unsafe impl AsBytes for bool {} -unsafe impl AsBytes for char {} -unsafe impl AsBytes for str {} -// SAFETY: If individual values in an array have no uninitialized portions, then the array itself -// does not have any uninitialized portions either. -unsafe impl <T :AsBytes >AsBytes for [T ] {} -unsafe impl <T :AsBytes ,const N :usize >AsBytes for [T ;N ] {} +macro_rules! impl_asbytes { + ($($({$($generics:tt)*})? $t:ty, )*) => { + $(unsafe impl $($($generics)*)?AsBytes for $t {})* + }; +} + +impl_asbytes! { +// SAFETY: Instances of the following types have no uninitialized portions. +u8 ,u16 ,u32 ,u64 ,usize , +i8 ,i16 ,i32 ,i64 ,isize , +bool , +char , +str , + +// SAFETY: If individual values in an array have no uninitialized portions, then the array +// itself does not have any uninitialized portions either. + {<T :AsBytes >} [T ], + {<T :AsBytes ,const N :usize >} [T ;N ], +}
Signature · 1 line
2.46.0
Re: [PATCH 03/19] rust: types: avoid repetition in `{As,From}Bytes` impls
Quoted 9 lines · Miguel Ojeda
In order to provide
// SAFETYcomments for everyunsafe impl, we would need to repeat them, which is not very useful and would be harder to read.We could perhaps allow the lint (ideally within a small module), but we can take the chance to avoid the repetition of the
impls themselves too by using a small local macro, like in other places where we have had to do this sort of thing.
Quoted 7 lines
Thus add the straightforward
impl_{from,as}bytes!macros and use them to implementFromBytes.This, in turn, will allow us in the next patch to place a
// SAFETYcomment that defers to the actual invocation of the macro.Signed-off-by Miguel Ojeda <[email protected]>
Re: [PATCH 03/19] rust: types: avoid repetition in `{As,From}Bytes` impls
In reply to Miguel OjedaQuoted 9 lines · Miguel Ojeda
In order to provide
// SAFETYcomments for everyunsafe impl, we would need to repeat them, which is not very useful and would be harder to read.We could perhaps allow the lint (ideally within a small module), but we can take the chance to avoid the repetition of the
impls themselves too by using a small local macro, like in other places where we have had to do this sort of thing.
Quoted 7 lines
Thus add the straightforward
impl_{from,as}bytes!macros and use them to implementFromBytes.This, in turn, will allow us in the next patch to place a
// SAFETYcomment that defers to the actual invocation of the macro.Signed-off-by Miguel Ojeda <[email protected]>
Miguel Ojeda
maintainerPATCH 4/19 rust: enable `clippy::undocumented_unsafe_blocks` lint
In reply to Miguel OjedaChecking 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.
--- Makefile | 1 + rust/bindings/lib.rs | 1 + rust/kernel/alloc/allocator.rs | 2 ++ rust/kernel/error.rs | 9 ++++++--- rust/kernel/init.rs | 5 +++++ rust/kernel/init/__internal.rs | 2 ++ rust/kernel/init/macros.rs | 9 +++++++++ rust/kernel/list.rs | 1 + rust/kernel/print.rs | 2 ++ rust/kernel/str.rs | 7 ++++--- rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/lock.rs | 6 +++--- rust/kernel/types.rs | 4 ++++ rust/kernel/workqueue.rs | 4 ++++ rust/uapi/lib.rs | 1 + 15 files changed, 46 insertions(+), 10 deletions(-)
Makefile +1 -0
@@ -456,6 +456,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::needless_bitwise_bool \ -Wclippy::needless_continue \ -Wclippy::no_mangle_with_rust_abi \ + -Wclippy::undocumented_unsafe_blocks \ -Wrustdoc::missing_crate_level_docs KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \
rust/bindings/lib.rs +1 -0
@@ -25,6 +25,7 @@ )]#[allow(dead_code)] +#[allow(clippy::undocumented_unsafe_blocks)] mod bindings_raw {// Use glob import here to expose all helpers. // Symbols defined within the module will take precedence to the glob import.
rust/kernel/alloc/allocator.rs +2 -0
@@ -31,6 +31,7 @@ pub(crate) unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: Funsafe { bindings::krealloc (ptras *const core::ffi::c_void , size, flags.0 )as *mut u8 } } +// SAFETY: TODO. unsafe impl GlobalAlloc for KernelAllocator {unsafe fn alloc (&self, layout:Layout ) -> *mut u8 {// SAFETY: `ptr::null_mut()` is null and `layout` has a non-zero size by the function safety
@@ -39,6 +40,7 @@ unsafe fn alloc(&self, layout: Layout) -> *mut u8 { }unsafe fn dealloc (&self, ptr: *mut u8 , _layout:Layout ) { +// SAFETY: TODO. unsafe { bindings::kfree (ptras *const core::ffi::c_void ); }
rust/kernel/error.rs +6 -3
@@ -171,9 +171,11 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {match self.name () {// Print out number if no name can be found. None => f.debug_tuple ("Error" ).field (&-self.0 ).finish (), -// SAFETY: These strings are ASCII-only. Some (name) => f - .debug_tuple (unsafe { core::str::from_utf8_unchecked (name) }) + .debug_tuple ( +// SAFETY: These strings are ASCII-only. +unsafe { core::str::from_utf8_unchecked (name) }, + ) .finish (), } }
@@ -277,6 +279,8 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {if unsafe { bindings::IS_ERR (const_ptr) } {// SAFETY: The FFI function does not deref the pointer. let err =unsafe { bindings::PTR_ERR (const_ptr) }; + +#[allow(clippy::unnecessary_cast)] // CAST: If `IS_ERR()` returns `true`, // then `PTR_ERR()` is guaranteed to return a // negative value greater-or-equal to `-bindings::MAX_ERRNO`,
@@ -286,7 +290,6 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {// // SAFETY: `IS_ERR()` ensures `err` is a // negative value greater-or-equal to `-bindings::MAX_ERRNO`. -#[allow(clippy::unnecessary_cast)] return Err (unsafe {Error ::from_errno_unchecked (erras core::ffi::c_int ) }); }Ok (ptr)
rust/kernel/init.rs +5 -0
@@ -541,6 +541,7 @@ macro_rules! stack_try_pin_init {/// } /// pin_init!(&this in Buf { /// buf: [0; 64], +/// // SAFETY: TODO. /// ptr: unsafe { addr_of_mut!((*this.as_ptr()).buf).cast() }, /// pin: PhantomPinned, /// });
@@ -875,6 +876,7 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {/// } /// /// let foo = pin_init!(Foo { +/// // SAFETY: TODO. /// raw <- unsafe { /// Opaque::ffi_init(|s| { /// init_foo(s);
@@ -1162,6 +1164,7 @@ pub fn pin_init_array_from_fn<I, const N: usize, T, E>(// 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 (()) }
@@ -1170,6 +1173,7 @@ unsafe fn __init(self, slot: *mut T) -> Result<(), E> {// 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) } } }
@@ -1411,6 +1415,7 @@ pub fn zeroed<T: Zeroable>() -> impl Init<T> {macro_rules! impl_zeroable { ($($({$($generics:tt)*})? $t:ty, )*) => { +// SAFETY: Safety comments written in the macro invocation. $(unsafe impl $($($generics)*)?Zeroable for $t {})* }; }
rust/kernel/init/__internal.rs +2 -0
@@ -112,10 +112,12 @@ fn clone(&self) -> Self {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 >;
rust/kernel/init/macros.rs +9 -0
@@ -513,6 +513,7 @@ fn drop($($sig:tt)*) { } ), ) => { +// SAFETY: TODO. unsafe $($impl_sig)* {// Inherit all attributes and the type/ident tokens for the signature. $(#[$($attr)*])*
@@ -872,6 +873,7 @@ unsafe fn __pin_data() -> Self::PinData { } } +// SAFETY: TODO. unsafe impl<$($impl_generics)*> $crate::init::__internal::PinData for __ThePinData<$($ty_generics)*> where $($whr)*
@@ -997,6 +999,7 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*> slot: *mut $p_type, init: impl $crate::init::PinInit<$p_type, E>, ) -> ::core::result::Result<(), E> { +// SAFETY: TODO. unsafe { $crate::init::PinInit ::__pinned_init (init, slot) } } )*
@@ -1007,6 +1010,7 @@ impl<$($impl_generics)*> $pin_data<$($ty_generics)*> slot: *mut $type, init: impl $crate::init::Init<$type, E>, ) -> ::core::result::Result<(), E> { +// SAFETY: TODO. unsafe { $crate::init::Init ::__init (init, slot) } } )*
@@ -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::init::__internal::$has_data;// Here we abuse `paste!` to retokenize `$t`. Declarative macros have some internal
@@ -1176,6 +1182,7 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}let init =move |slot| -> ::core::result::Result <(), $err> {init (slot).map (|__InitOk| ()) }; +// SAFETY: TODO. let init = unsafe { $crate::init::$construct_closure::<_, $err>(init) }; init }};
@@ -1324,6 +1331,8 @@ fn assert_zeroable<T: $crate::init::Zeroable>(_: *mut T) {}// 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
rust/kernel/list.rs +1 -0
@@ -354,6 +354,7 @@ pub fn pop_front(&mut self) -> Option<ListArc<T, ID>> {/// /// `item` must not be in a different linked list (with the same id). pub unsafe fn remove (&mut self, item: &T ) ->Option <ListArc <T ,ID >> { +// SAFETY: TODO. let mut item =unsafe {ListLinks ::fields (T ::view_links (item)) };// SAFETY: The user provided a reference, and reference are never dangling. //
rust/kernel/print.rs +2 -0
@@ -23,6 +23,7 @@use fmt::Write ;// SAFETY: The C contract guarantees that `buf` is valid if it's less than `end`. let mut w =unsafe {RawFormatter ::from_ptrs (buf.cast (), end.cast ()) }; +// SAFETY: TODO. let _ = w.write_fmt (unsafe { *(ptras *const fmt::Arguments <'_>) }); w.pos ().cast () }
@@ -102,6 +103,7 @@ pub unsafe fn call_printk( ) {// `_printk` does not seem to fail in any path. #[cfg( CONFIG_PRINTK )] +// SAFETY: TODO. unsafe { bindings::_printk ( format_string.as_ptr() as _,
rust/kernel/str.rs +4 -3
@@ -162,10 +162,10 @@ pub const fn len(&self) -> usize {/// Returns the length of this string with `NUL`. #[inline] pub const fn len_with_nul (&self) ->usize { -// SAFETY: This is one of the invariant of `CStr`. -// We add a `unreachable_unchecked` here to hint the optimizer that -// the value returned from this function is non-zero. if self.0 .is_empty () { +// SAFETY: This is one of the invariant of `CStr`. +// We add a `unreachable_unchecked` here to hint the optimizer that +// the value returned from this function is non-zero. unsafe { core::hint::unreachable_unchecked () }; } self.0 .len ()
@@ -301,6 +301,7 @@ pub fn to_str(&self) -> Result<&str, core::str::Utf8Error> {/// ``` #[inline] pub unsafe fn as_str_unchecked (&self) -> &str { +// SAFETY: TODO. unsafe { core::str::from_utf8_unchecked (self.as_bytes ()) } }
rust/kernel/sync/condvar.rs +1 -1
@@ -92,8 +92,8 @@ pub struct CondVar { _pin: PhantomPinned, } -// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread. #[allow(clippy::non_send_fields_in_send_ty)] +// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread. unsafe impl Send for CondVar {}// SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on multiple threads
rust/kernel/sync/lock.rs +3 -3
@@ -150,9 +150,9 @@ pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {// SAFETY: The caller owns the lock, so it is safe to unlock it. unsafe {B ::unlock (self.lock.state.get (), &self.state) }; -// SAFETY: The lock was just unlocked above and is being relocked now. -let _relock = -ScopeGuard ::new (||unsafe {B ::relock (self.lock.state.get (), &mut self.state) }); +let _relock =ScopeGuard ::new (|| +// SAFETY: The lock was just unlocked above and is being relocked now. +unsafe {B ::relock (self.lock.state.get (), &mut self.state) });cb () }
rust/kernel/types.rs +4 -0
@@ -410,6 +410,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {/// /// struct Empty {} /// +/// # // SAFETY: TODO. /// unsafe impl AlwaysRefCounted for Empty { /// fn inc_ref(&self) {} /// unsafe fn dec_ref(_obj: NonNull<Self>) {}
@@ -417,6 +418,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {/// /// let mut data = Empty {}; /// let ptr = NonNull::<Empty>::new(&mut data as *mut _).unwrap(); +/// # // SAFETY: TODO. /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) }; /// let raw_ptr: NonNull<Empty> = ARef::into_raw(data_ref); ///
@@ -483,6 +485,7 @@ pub unsafe trait FromBytes {}macro_rules! impl_frombytes { ($($({$($generics:tt)*})? $t:ty, )*) => { +// SAFETY: Safety comments written in the macro invocation. $(unsafe impl $($($generics)*)?FromBytes for $t {})* }; }
@@ -517,6 +520,7 @@ pub unsafe trait AsBytes {}macro_rules! impl_asbytes { ($($({$($generics:tt)*})? $t:ty, )*) => { +// SAFETY: Safety comments written in the macro invocation. $(unsafe impl $($($generics)*)?AsBytes for $t {})* }; }
rust/kernel/workqueue.rs +4 -0
@@ -519,6 +519,7 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ impl{T} HasWork<Self> for ClosureWork<T> { self.work } } +// SAFETY: TODO. unsafe impl <T ,const ID :u64 >WorkItemPointer <ID >for Arc <T >where T :WorkItem <ID ,Pointer =Self >,
@@ -536,6 +537,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T> } } +// SAFETY: TODO. unsafe impl <T ,const ID :u64 >RawWorkItem <ID >for Arc <T >where T :WorkItem <ID ,Pointer =Self >,
@@ -564,6 +566,7 @@ unsafe fn __enqueue<F>(self, queue_work_on: F) -> Self::EnqueueOutput } } +// SAFETY: TODO. unsafe impl <T ,const ID :u64 >WorkItemPointer <ID >for Pin <Box <T >>where T :WorkItem <ID ,Pointer =Self >,
@@ -583,6 +586,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<Box<T>> } } +// SAFETY: TODO. unsafe impl <T ,const ID :u64 >RawWorkItem <ID >for Pin <Box <T >>where T :WorkItem <ID ,Pointer =Self >,
rust/uapi/lib.rs +1 -0
@@ -14,6 +14,7 @@#![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))] #![allow( clippy::all, + clippy::undocumented_unsafe_blocks, dead_code, missing_docs, non_camel_case_types,
Signature · 1 line
2.46.0
Re: [PATCH 04/19] rust: enable `clippy::undocumented_unsafe_blocks` lint
Quoted 15 lines · Miguel Ojeda
Checking that we are not missing any
// SAFETYcomments in ourunsafeblocks 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 theundocumented_unsafe_blockslint [2].Even though the lint has a few false positives, e.g. in some cases where attributes appear between the comment and the
unsafeblock [3], there are workarounds and the lint seems quite usable already.Thus enable the lint now.
Quoted 8 lines
We still have a few cases to clean up, so just allow those for the moment by writing a
TODOcomment -- some of those may be good candidates for new contributors.
Re: [PATCH 04/19] rust: enable `clippy::undocumented_unsafe_blocks` lint
In reply to Miguel OjedaQuoted 34 lines · Miguel Ojeda
Checking that we are not missing any
// SAFETYcomments in ourunsafeblocks 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 theundocumented_unsafe_blockslint [2].Even though the lint has a few false positives, e.g. in some cases where attributes appear between the comment and the
unsafeblock [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
TODOcomment -- some of those may be good candidates for new contributors.Signed-off-by Miguel Ojeda <[email protected]>rust/kernel/error.rs +2
-0@@ -277,6 +279,8 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {if unsafe { bindings::IS_ERR (const_ptr) } {// SAFETY: The FFI function does not deref the pointer. let err =unsafe { bindings::PTR_ERR (const_ptr) }; + +#[allow(clippy::unnecessary_cast)] // CAST: If `IS_ERR()` returns `true`, // then `PTR_ERR()` is guaranteed to return a // negative value greater-or-equal to `-bindings::MAX_ERRNO`,
rust/kernel/error.rs pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> { · line 294
Quoted 6 lines
rust/kernel/error.rs +0
-1@@ -286,7 +290,6 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> {// // SAFETY: `IS_ERR()` ensures `err` is a // negative value greater-or-equal to `-bindings::MAX_ERRNO`. -#[allow(clippy::unnecessary_cast)] return Err (unsafe {Error ::from_errno_unchecked (erras core::ffi::c_int ) }); }
There are a couple places where the attributes move like this - did this come from an older clippy version? clippy used to warn about this but accepts it by default since [1]. (Still works of course, I just think it looks nicer to have the attributes next to their statements).
Either way:
Re: [PATCH 04/19] rust: enable `clippy::undocumented_unsafe_blocks` lint
Quoted 5 lines · Trevor Gross
There are a couple places where the attributes move like this - did this come from an older clippy version? clippy used to warn about this but accepts it by default since [1]. (Still works of course, I just think it looks nicer to have the attributes next to their statements).
I started this series about a long time ago, but I don't think old Clippy is the reason -- I took a look again at this a couple months ago and created this issue about it due to these lines of code, including some examples and test cases:
https://github.com/rust-lang/rust-clippy/issues/13189
For instance:
// SAFETY: ...
#[allow(clippy::unnecessary_cast)]
return Err(unsafe { Error::from_errno_unchecked(err as core::ffi::c_int) });This will not work, i.e. it does not see the // SAFETY:. However, putting the #[allow] on top to let the comment be closer will work (like in the patch). This will also work:
return Err(
// SAFETY: ...
#[allow(clippy::unnecessary_cast)]
unsafe { Error::from_errno_unchecked(err as core::ffi::c_int) }
);There has been no reply in the issue so far, but if you have any comments/insights on whether those cases should or should not work, that would be great.
Cheers,
Miguel
Miguel Ojeda
maintainerPATCH 5/19 rust: enable `clippy::unnecessary_safety_comment` lint
In reply to Miguel OjedaIn Rust 1.67.0, Clippy added the unnecessary_safety_comment lint [1], which is the "inverse" of undocumented_unsafe_blocks: it finds places where safe code has a // SAFETY comment attached.
The lint currently finds 3 places where we had such mistakes, thus it seems already quite useful.
Thus clean those and enable it.
--- Makefile | 1 + rust/kernel/sync/arc.rs | 2 +- rust/kernel/workqueue.rs | 4 ++-- 3 files changed, 4 insertions(+), 3 deletions(-)
Makefile +1 -0
@@ -457,6 +457,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::needless_continue \ -Wclippy::no_mangle_with_rust_abi \ -Wclippy::undocumented_unsafe_blocks \ + -Wclippy::unnecessary_safety_comment \ -Wrustdoc::missing_crate_level_docs KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \
rust/kernel/sync/arc.rs +1 -1
@@ -341,7 +341,7 @@ fn into_foreign(self) -> *const core::ffi::c_void { }unsafe fn borrow <'a>(ptr: *const core::ffi::c_void ) ->ArcBorrow <'a,T > { -// SAFETY: By the safety requirement of this function, we know that `ptr` came from +// By the safety requirement of this function, we know that `ptr` came from // a previous call to `Arc::into_foreign`. let inner =NonNull ::new (ptras *mut ArcInner <T >).unwrap ();
rust/kernel/workqueue.rs +2 -2
@@ -526,7 +526,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T> T: HasWork<T, ID>, {unsafe extern "C" fn run (ptr: *mut bindings::work_struct ) { -// SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`. +// The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`. let ptr = ptras *mut Work <T ,ID >;// SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`. let ptr =unsafe {T ::work_container_of (ptr) };
@@ -573,7 +573,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<Box<T>> T: HasWork<T, ID>, {unsafe extern "C" fn run (ptr: *mut bindings::work_struct ) { -// SAFETY: The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`. +// The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`. let ptr = ptras *mut Work <T ,ID >;// SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`. let ptr =unsafe {T ::work_container_of (ptr) };
Signature · 1 line
2.46.0
Re: [PATCH 05/19] rust: enable `clippy::unnecessary_safety_comment` lint
Quoted 5 lines · Miguel Ojeda
In Rust 1.67.0, Clippy added the
unnecessary_safety_commentlint [1], which is the "inverse" ofundocumented_unsafe_blocks: it finds places where safe code has a// SAFETYcomment attached.
Quoted 7 lines
The lint currently finds 3 places where we had such mistakes, thus it seems already quite useful.
Thus clean those and enable it.
Re: [PATCH 05/19] rust: enable `clippy::unnecessary_safety_comment` lint
In reply to Miguel OjedaQuoted 4 lines
In Rust 1.67.0, Clippy added the
unnecessary_safety_commentlint [1], which is the "inverse" ofundocumented_unsafe_blocks: it finds places where safe code has a// SAFETYcomment attached.
Quoted 8 lines
The lint currently finds 3 places where we had such mistakes, thus it seems already quite useful.
Thus clean those and enable it.
Signed-off-by Miguel Ojeda <[email protected]>---
Re: [PATCH 05/19] rust: enable `clippy::unnecessary_safety_comment` lint
In reply to Miguel OjedaQuoted 5 lines · Miguel Ojeda
In Rust 1.67.0, Clippy added the
unnecessary_safety_commentlint [1], which is the "inverse" ofundocumented_unsafe_blocks: it finds places where safe code has a// SAFETYcomment attached.
Quoted 7 lines
The lint currently finds 3 places where we had such mistakes, thus it seems already quite useful.
Thus clean those and enable it.
Miguel Ojeda
maintainerPATCH 6/19 rust: enable `clippy::unnecessary_safety_doc` lint
In reply to Miguel OjedaIn Rust 1.67.0, Clippy added the unnecessary_safety_doc lint [1], which is similar to unnecessary_safety_comment, but for # Safety sections, i.e. safety preconditions in the documentation.
This is something that should not happen with our coding guidelines in mind. Thus enable the lint to have it machine-checked.
--- Makefile | 1 + 1 file changed, 1 insertion(+)
Makefile +1 -0
@@ -458,6 +458,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::no_mangle_with_rust_abi \ -Wclippy::undocumented_unsafe_blocks \ -Wclippy::unnecessary_safety_comment \ + -Wclippy::unnecessary_safety_doc \ -Wrustdoc::missing_crate_level_docs KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \
Signature · 1 line
2.46.0
Re: [PATCH 06/19] rust: enable `clippy::unnecessary_safety_doc` lint
Quoted 10 lines · Miguel Ojeda
In Rust 1.67.0, Clippy added the
unnecessary_safety_doclint [1], which is similar tounnecessary_safety_comment, but for# Safetysections, i.e. safety preconditions in the documentation.This is something that should not happen with our coding guidelines in mind. Thus enable the lint to have it machine-checked.
Re: [PATCH 06/19] rust: enable `clippy::unnecessary_safety_doc` lint
In reply to Miguel OjedaQuoted 10 lines · Miguel Ojeda
In Rust 1.67.0, Clippy added the
unnecessary_safety_doclint [1], which is similar tounnecessary_safety_comment, but for# Safetysections, i.e. safety preconditions in the documentation.This is something that should not happen with our coding guidelines in mind. Thus enable the lint to have it machine-checked.
Miguel Ojeda
maintainerPATCH 7/19 rust: enable `clippy::ignored_unit_patterns` lint
In reply to Miguel OjedaIn Rust 1.73.0, Clippy introduced the ignored_unit_patterns lint [1]:
Quoted 3 lines
Matching with
()explicitly instead of_outlines the fact that the pattern contains no data. Also it would detect a type change that_would ignore.
There is only a single case that requires a change:
error: matching over () is more explicit
--> rust/kernel/types.rs:176:45
|
176 | ScopeGuard::new_with_data((), move |_| cleanup())
| ^ help: use () instead of _: ()
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ignored_unit_patterns
= note: requested on the command line with -D clippy::ignored-unit-patterns
Thus clean it up and enable the lint -- no functional change intended.
--- Makefile | 1 + rust/kernel/types.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
Makefile +1 -0
@@ -452,6 +452,7 @@ export rust_common_flags := --edition=2021 \ -Wunreachable_pub \ -Wclippy::all \ -Wclippy::dbg_macro \ + -Wclippy::ignored_unit_patterns \ -Wclippy::mut_mut \ -Wclippy::needless_bitwise_bool \ -Wclippy::needless_continue \
rust/kernel/types.rs +1 -1
@@ -225,7 +225,7 @@ pub fn dismiss(mut self) -> T {impl ScopeGuard <(),fn (())> {/// Creates a new guarded object with the given cleanup function. pub fn new (cleanup:impl FnOnce ()) ->ScopeGuard <(),impl FnOnce (())> { -ScopeGuard ::new_with_data ((),move |_|cleanup ()) +ScopeGuard ::new_with_data ((),move |()|cleanup ()) } }
Signature · 1 line
2.46.0
Re: [PATCH 07/19] rust: enable `clippy::ignored_unit_patterns` lint
Quoted 18 lines · Miguel Ojeda
In Rust 1.73.0, Clippy introduced the
ignored_unit_patternslint [1]:Quoted 3 lines
Matching with
()explicitly instead of_outlines the fact that the pattern contains no data. Also it would detect a type change that_would ignore.There is only a single case that requires a change:
error: matching over
()is more explicit
-->rust/kernel/types.rs:176:45
|
176 |ScopeGuard::new_with_data((), move |_| cleanup())
| ^ help: use()instead of_:()
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ignored_unit_patterns
= note: requested on the command line with-D clippy::ignored-unit-patterns
Quoted 4 lines
Thus clean it up and enable the lint -- no functional change intended.
Re: [PATCH 07/19] rust: enable `clippy::ignored_unit_patterns` lint
In reply to Miguel OjedaQuoted 18 lines · Miguel Ojeda
In Rust 1.73.0, Clippy introduced the
ignored_unit_patternslint [1]:Quoted 3 lines
Matching with
()explicitly instead of_outlines the fact that the pattern contains no data. Also it would detect a type change that_would ignore.There is only a single case that requires a change:
error: matching over
()is more explicit
-->rust/kernel/types.rs:176:45
|
176 |ScopeGuard::new_with_data((), move |_| cleanup())
| ^ help: use()instead of_:()
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ignored_unit_patterns
= note: requested on the command line with-D clippy::ignored-unit-patterns
Quoted 4 lines
Thus clean it up and enable the lint -- no functional change intended.
Miguel Ojeda
maintainerPATCH 8/19 rust: enable `rustdoc::unescaped_backticks` lint
In reply to Miguel OjedaIn Rust 1.71.0, rustdoc added the unescaped_backticks lint, which detects what are typically typos in Markdown formatting regarding inline code [1], e.g. from the Rust standard library:
/// ... to `deref`/`deref_mut`` must ...
/// ... use [`from_mut`]`. Specifically, ...It does not seem to have almost any false positives, from the experience of enabling it in the Rust standard library [2], which will be checked starting with Rust 1.82.0. The maintainers also confirmed it is ready to be used.
Thus enable it.
--- Makefile | 3 ++- rust/Makefile | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-)
Makefile +2 -1
@@ -460,7 +460,8 @@ export rust_common_flags := --edition=2021 \ -Wclippy::undocumented_unsafe_blocks \ -Wclippy::unnecessary_safety_comment \ -Wclippy::unnecessary_safety_doc \ - -Wrustdoc::missing_crate_level_docs + -Wrustdoc::missing_crate_level_docs \ + -Wrustdoc::unescaped_backticks KBUILD_HOSTCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(HOST_LFS_CFLAGS) \ $(HOSTCFLAGS) -I $(srctree)/scripts/include
rust/Makefile +4 -1
@@ -61,7 +61,7 @@ alloc-cfgs = \ quiet_cmd_rustdoc = RUSTDOC $(if $(rustdoc_host),H, ) $< cmd_rustdoc = \ OBJTREE=$(abspath $(objtree)) \ - $(RUSTDOC) $(if $(rustdoc_host),$(rust_common_flags),$(rust_flags)) \ + $(RUSTDOC) $(filter-out $(skip_flags),$(if $(rustdoc_host),$(rust_common_flags),$(rust_flags))) \ $(rustc_target_flags) -L$(objtree)/$(obj) \ -Zunstable-options --generate-link-to-definition \ --output $(rustdoc_output) \
@@ -98,6 +98,9 @@ rustdoc-macros: private rustc_target_flags = --crate-type proc-macro \ rustdoc-macros: $(src)/macros/lib.rs FORCE +$(call if_changed,rustdoc) +# Starting with Rust 1.82.0, skipping `-Wrustdoc::unescaped_backticks` should +# not be needed -- see https://github.com/rust-lang/rust/pull/128307. +rustdoc-core: private skip_flags = -Wrustdoc::unescaped_backticks rustdoc-core: private rustc_target_flags = $(core-cfgs) rustdoc-core: $(RUST_LIB_SRC)/core/src/lib.rs FORCE +$(call if_changed,rustdoc)
Signature · 1 line
2.46.0
Re: [PATCH 08/19] rust: enable `rustdoc::unescaped_backticks` lint
Quoted 14 lines · Miguel Ojeda
In Rust 1.71.0,
rustdocadded theunescaped_backtickslint, which detects what are typically typos in Markdown formatting regarding inline code [1], e.g. from the Rust standard library:/// ... to `deref`/`deref_mut`` must ... /// ... use [`from_mut`]`. Specifically, ...It does not seem to have almost any false positives, from the experience of enabling it in the Rust standard library [2], which will be checked starting with Rust 1.82.0. The maintainers also confirmed it is ready to be used.
Quoted 5 lines
Thus enable it.
Re: [PATCH 08/19] rust: enable `rustdoc::unescaped_backticks` lint
In reply to Miguel OjedaQuoted 14 lines · Miguel Ojeda
In Rust 1.71.0,
rustdocadded theunescaped_backtickslint, which detects what are typically typos in Markdown formatting regarding inline code [1], e.g. from the Rust standard library:/// ... to `deref`/`deref_mut`` must ... /// ... use [`from_mut`]`. Specifically, ...It does not seem to have almost any false positives, from the experience of enabling it in the Rust standard library [2], which will be checked starting with Rust 1.82.0. The maintainers also confirmed it is ready to be used.
Quoted 5 lines
Thus enable it.
rust/Makefile rustdoc-macros: private rustc_target_flags = --crate-type proc-macro \ · line 103
Quoted 3 lines
rust/Makefile +3
-0+# Starting with Rust 1.82.0, skipping `-Wrustdoc::unescaped_backticks` should +# not be needed -- see https://github.com/rust-lang/rust/pull/128307. +rustdoc-core: private skip_flags = -Wrustdoc::unescaped_backticks
Maybe we should use something like FIXME(msrv) so things like this are easy to find?
Re: [PATCH 08/19] rust: enable `rustdoc::unescaped_backticks` lint
Quoted 3 lines · Trevor Gross
Maybe we should use something like FIXME(msrv) so things like this are easy to find?
It would be nice to standardize on something. As long as the version number is there, I think it is OK even without a standard notation, since it will be easy to find while grepping for cleanups on the version upgrade.
Relatedly, Benno even proposed failing the build [1].
Cheers,
Miguel
Miguel Ojeda
maintainerPATCH 9/19 rust: init: remove unneeded `#[allow(clippy::disallowed_names)]`
In reply to Miguel OjedaThese few cases, unlike others in the same file, did not need the allow.
Thus clean them up.
--- rust/kernel/init.rs | 4 ---- 1 file changed, 4 deletions(-)
rust/kernel/init.rs +0 -4
@@ -87,7 +87,6 @@//! To declare an init macro/function you just return an [`impl PinInit<T, E>`]: //! //! ```rust -//! # #![allow(clippy::disallowed_names)] //! # use kernel::{sync::Mutex, new_mutex, init::PinInit, try_pin_init}; //! #[pin_data] //! struct DriverData {
@@ -368,7 +367,6 @@ macro_rules! stack_try_pin_init {/// The syntax is almost identical to that of a normal `struct` initializer: /// /// ```rust -/// # #![allow(clippy::disallowed_names)] /// # use kernel::{init, pin_init, macros::pin_data, init::*}; /// # use core::pin::Pin; /// #[pin_data]
@@ -413,7 +411,6 @@ macro_rules! stack_try_pin_init {/// To create an initializer function, simply declare it like this: /// /// ```rust -/// # #![allow(clippy::disallowed_names)] /// # use kernel::{init, pin_init, init::*}; /// # use core::pin::Pin; /// # #[pin_data]
@@ -468,7 +465,6 @@ macro_rules! stack_try_pin_init {/// They can also easily embed it into their own `struct`s: /// /// ```rust -/// # #![allow(clippy::disallowed_names)] /// # use kernel::{init, pin_init, macros::pin_data, init::*}; /// # use core::pin::Pin; /// # #[pin_data]
Signature · 1 line
2.46.0
Re: [PATCH 09/19] rust: init: remove unneeded `#[allow(clippy::disallowed_names)]`
Quoted 6 lines · Miguel Ojeda
These few cases, unlike others in the same file, did not need the
allow.Thus clean them up.
Signed-off-by Miguel Ojeda <[email protected]>
Re: [PATCH 09/19] rust: init: remove unneeded `#[allow(clippy::disallowed_names)]`
In reply to Miguel OjedaQuoted 6 lines · Miguel Ojeda
These few cases, unlike others in the same file, did not need the
allow.Thus clean them up.
Signed-off-by Miguel Ojeda <[email protected]>
Miguel Ojeda
maintainerPATCH 10/19 rust: sync: remove unneeded `#[allow(clippy::non_send_fields_in_send_ty)]`
In reply to Miguel OjedaRust 1.58.0 (before Rust was merged into the kernel) made Clippy's non_send_fields_in_send_ty lint part of the suspicious lint group for a brief window of time [1] until the minor version 1.58.1 got released a week after, where the lint was moved back to nursery.
By that time, we had already upgraded to that Rust version, and thus we had allowed the lint here for CondVar.
Nowadays, Clippy's non_send_fields_in_send_ty would still trigger here if it were enabled.
Moreover, if enabled, Lock<T, B> and Task would also require an allow. Therefore, it does not seem like someone is actually enabling it (in, e.g., a custom flags build).
Finally, the lint does not appear to have had major improvements since then [2].
Thus remove the allow since it is unneeded.
--- rust/kernel/sync/condvar.rs | 1 - 1 file changed, 1 deletion(-)
rust/kernel/sync/condvar.rs +0 -1
@@ -92,7 +92,6 @@ pub struct CondVar { _pin: PhantomPinned, } -#[allow(clippy::non_send_fields_in_send_ty)] // SAFETY: `CondVar` only uses a `struct wait_queue_head`, which is safe to use on any thread. unsafe impl Send for CondVar {}
Signature · 1 line
2.46.0
Re: [PATCH 10/19] rust: sync: remove unneeded `#[allow(clippy::non_send_fields_in_send_ty)]`
Quoted 16 lines · Miguel Ojeda
Rust 1.58.0 (before Rust was merged into the kernel) made Clippy's
non_send_fields_in_send_tylint part of thesuspiciouslint group for a brief window of time [1] until the minor version 1.58.1 got released a week after, where the lint was moved back tonursery.By that time, we had already upgraded to that Rust version, and thus we had
allowed the lint here forCondVar.Nowadays, Clippy's
non_send_fields_in_send_tywould still trigger here if it were enabled.Moreover, if enabled,
Lock<T, B>andTaskwould also require anallow. Therefore, it does not seem like someone is actually enabling it (in, e.g., a custom flags build).
Quoted 8 lines
Finally, the lint does not appear to have had major improvements since then [2].
Thus remove the
allowsince it is unneeded.
Re: [PATCH 10/19] rust: sync: remove unneeded `#[allow(clippy::non_send_fields_in_send_ty)]`
In reply to Miguel OjedaQuoted 16 lines · Miguel Ojeda
Rust 1.58.0 (before Rust was merged into the kernel) made Clippy's
non_send_fields_in_send_tylint part of thesuspiciouslint group for a brief window of time [1] until the minor version 1.58.1 got released a week after, where the lint was moved back tonursery.By that time, we had already upgraded to that Rust version, and thus we had
allowed the lint here forCondVar.Nowadays, Clippy's
non_send_fields_in_send_tywould still trigger here if it were enabled.Moreover, if enabled,
Lock<T, B>andTaskwould also require anallow. Therefore, it does not seem like someone is actually enabling it (in, e.g., a custom flags build).
Quoted 8 lines
Finally, the lint does not appear to have had major improvements since then [2].
Thus remove the
allowsince it is unneeded.
Miguel Ojeda
maintainerPATCH 11/19 rust: introduce `.clippy.toml`
In reply to Miguel OjedaSome Clippy lints can be configured/tweaked. We will use these knobs to our advantage in later commits.
This is done via a configuration file, .clippy.toml [1]. The file is currently unstable. This may be a problem in the future, but we can adapt as needed. In addition, we proposed adding Clippy to the Rust CI's RFL job [2], so we should be able to catch issues pre-merge.
Thus introduce the file.
--- .clippy.toml | 1 + .gitignore | 1 + MAINTAINERS | 1 + Makefile | 3 +++ 4 files changed, 6 insertions(+) create mode 100644 .clippy.toml
.clippy.toml new file +1 -0
@@ -0,0 +1 @@ +# SPDX-License-Identifier: GPL-2.0
.gitignore +1 -0
@@ -102,6 +102,7 @@ modules.order # We don't want to ignore the following even if they are dot-files # !.clang-format +!.clippy.toml !.cocciconfig !.editorconfig !.get_maintainer.ignore
MAINTAINERS +1 -0
@@ -19922,6 +19922,7 @@ B: https://github.com/Rust-for-Linux/linux/issues C: zulip://rust-for-linux.zulipchat.com P: https://rust-for-linux.com/contributing T: git https://github.com/Rust-for-Linux/linux.git rust-next +F: .clippy.toml F: Documentation/rust/ F: rust/ F: samples/rust/
Makefile +3 -0
@@ -590,6 +590,9 @@ endif # Allows the usage of unstable features in stable compilers. export RUSTC_BOOTSTRAP := 1 +# Allows finding `.clippy.toml` in out-of-srctree builds. +export CLIPPY_CONF_DIR := $(srctree) + export ARCH SRCARCH CONFIG_SHELL BASH HOSTCC KBUILD_HOSTCFLAGS CROSS_COMPILE LD CC HOSTPKG_CONFIG export RUSTC RUSTDOC RUSTFMT RUSTC_OR_CLIPPY_QUIET RUSTC_OR_CLIPPY BINDGEN export HOSTRUSTC KBUILD_HOSTRUSTFLAGS
Signature · 1 line
2.46.0
Re: [PATCH 11/19] rust: introduce `.clippy.toml`
Quoted 9 lines · Miguel Ojeda
Some Clippy lints can be configured/tweaked. We will use these knobs to our advantage in later commits.
This is done via a configuration file,
.clippy.toml[1]. The file is currently unstable. This may be a problem in the future, but we can adapt as needed. In addition, we proposed adding Clippy to the Rust CI's RFL job [2], so we should be able to catch issues pre-merge.
Quoted 5 lines
Thus introduce the file.
Signed-off-by Miguel Ojeda <[email protected]>
Re: [PATCH 11/19] rust: introduce `.clippy.toml`
In reply to Miguel OjedaQuoted 9 lines · Miguel Ojeda
Some Clippy lints can be configured/tweaked. We will use these knobs to our advantage in later commits.
This is done via a configuration file,
.clippy.toml[1]. The file is currently unstable. This may be a problem in the future, but we can adapt as needed. In addition, we proposed adding Clippy to the Rust CI's RFL job [2], so we should be able to catch issues pre-merge.
Quoted 5 lines
Thus introduce the file.
Signed-off-by Miguel Ojeda <[email protected]>
Miguel Ojeda
maintainerPATCH 12/19 rust: replace `clippy::dbg_macro` with `disallowed_macros`
In reply to Miguel OjedaBack when we used Rust 1.60.0 (before Rust was merged in the kernel), we added -Wclippy::dbg_macro to the compilation flags. This worked great with our custom dbg! macro (vendored from std, but slightly modified to use the kernel printing facilities).
However, in the very next version, 1.61.0, it stopped working [1] since the lint started to use a Rust diagnostic item rather than a path to find the dbg! macro [1]. This behavior remains until the current nightly (1.83.0).
Therefore, currently, the dbg_macro is not doing anything, which explains why we can invoke dbg! in samples/rust/rust_print.rs, as well as why changing the #[allow()]s to #[expect()]s in std_vendor.rs` doctests does not work since they are not fulfilled.
One possible workaround is using rustc_attrs like the standard library does. However, this is intended to be internal, and we just started supporting several Rust compiler versions, so it is best to avoid it.
Therefore, instead, use disallowed_macros. It is a stable lint and is more flexible (in that we can provide different macros), although its diagnostic message(s) are not as nice as the specialized one (yet), and does not allow to set different lint levels per macro/path [2].
In turn, this requires allowing the (intentional) dbg! use in the sample, as one would have expected.
Finally, in a single case, the allow is fixed to be an inner attribute, since otherwise it was not being applied.
--- .clippy.toml | 6 ++++++ Makefile | 1 - rust/kernel/std_vendor.rs | 10 +++++----- samples/rust/rust_print.rs | 1 + 4 files changed, 12 insertions(+), 6 deletions(-)
.clippy.toml +6 -0
@@ -1 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 + +disallowed-macros = [ + # The `clippy::dbg_macro` lint only works with `std::dbg!`, thus we simulate + # it here, see: https://github.com/rust-lang/rust-clippy/issues/11303. + { path = "kernel::dbg", reason = "the `dbg!` macro is intended as a debugging tool" }, +]
Makefile +0 -1
@@ -451,7 +451,6 @@ export rust_common_flags := --edition=2021 \ -Wrust_2018_idioms \ -Wunreachable_pub \ -Wclippy::all \ - -Wclippy::dbg_macro \ -Wclippy::ignored_unit_patterns \ -Wclippy::mut_mut \ -Wclippy::needless_bitwise_bool \
rust/kernel/std_vendor.rs +5 -5
@@ -14,7 +14,7 @@/// /// ```rust /// let a = 2; -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] /// let b = dbg!(a * 2) + 1; /// // ^-- prints: [src/main.rs:2] a * 2 = 4 /// assert_eq!(b, 5);
@@ -52,7 +52,7 @@/// With a method call: /// /// ```rust -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] /// fn foo(n: usize) { /// if dbg!(n.checked_sub(4)).is_some() { /// // ...
@@ -71,7 +71,7 @@/// Naive factorial implementation: /// /// ```rust -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] /// # { /// fn factorial(n: u32) -> u32 { /// if dbg!(n <= 1) {
@@ -118,7 +118,7 @@/// a tuple (and return it, too): /// /// ``` -/// # #[allow(clippy::dbg_macro)] +/// # #![allow(clippy::disallowed_macros)] /// assert_eq!(dbg!(1usize, 2u32), (1, 2)); /// ``` ///
@@ -127,7 +127,7 @@/// invocations. You can use a 1-tuple directly if you need one: /// /// ``` -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] /// # { /// assert_eq!(1, dbg!(1u32,)); // trailing comma ignored /// assert_eq!((1,), dbg!((1u32,))); // 1-tuple
samples/rust/rust_print.rs +1 -0
@@ -15,6 +15,7 @@struct RustPrint ; +#[allow(clippy::disallowed_macros)] fn arc_print () ->Result {use kernel::sync::*;
Signature · 1 line
2.46.0
Re: [PATCH 12/19] rust: replace `clippy::dbg_macro` with `disallowed_macros`
Quoted 26 lines · Miguel Ojeda
.clippy.toml +6
-0@@ -1 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 + +disallowed-macros = [ + # The `clippy::dbg_macro` lint only works with `std::dbg!`, thus we simulate + # it here, see: https://github.com/rust-lang/rust-clippy/issues/11303. + { path = "kernel::dbg", reason = "the `dbg!` macro is intended as a debugging tool" }, +]rust/kernel/std_vendor.rs +3
-3@@ -14,7 +14,7 @@ -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] @@ -52,7 +52,7 @@ -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] @@ -71,7 +71,7 @@ -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)]
rust/kernel/std_vendor.rs line 124
Quoted 8 lines
rust/kernel/std_vendor.rs +1
-1@@ -118,7 +118,7 @@/// a tuple (and return it, too): /// /// ``` -/// # #[allow(clippy::dbg_macro)] +/// # #![allow(clippy::disallowed_macros)] /// assert_eq!(dbg!(1usize, 2u32), (1, 2)); /// ``` ///
For what it is worth, the exclamation mark did surprise me.
rust/kernel/std_vendor.rs line 132
Quoted 8 lines
rust/kernel/std_vendor.rs +1
-1@@ -127,7 +127,7 @@/// /// ``` -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] /// # { /// assert_eq!(1, dbg!(1u32,)); // trailing comma ignored /// assert_eq!((1,), dbg!((1u32,))); // 1-tuple
Groeten
Geert Stappers
Signature · 1 line
Silence is hard to parse
Gary Guo
maintainerRe: [PATCH 12/19] rust: replace `clippy::dbg_macro` with `disallowed_macros`
Quoted 29 lines · On Thu, 5 Sep 2024 07:20:46 +0200 Geert Stappers <[email protected]> wrote:
Quoted 28 lines · Miguel Ojeda
.clippy.toml +6
-0@@ -1 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 + +disallowed-macros = [ + # The `clippy::dbg_macro` lint only works with `std::dbg!`, thus we simulate + # it here, see: https://github.com/rust-lang/rust-clippy/issues/11303. + { path = "kernel::dbg", reason = "the `dbg!` macro is intended as a debugging tool" }, +]rust/kernel/std_vendor.rs +3
-3@@ -14,7 +14,7 @@ -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] @@ -52,7 +52,7 @@ -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] @@ -71,7 +71,7 @@ -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] @@ -118,7 +118,7 @@/// a tuple (and return it, too): ///
Quoted 8 lines
rust/kernel/std_vendor.rs line 124
Quoted 6 lines
rust/kernel/std_vendor.rs +1
-1/// ``` -/// # #[allow(clippy::dbg_macro)] +/// # #![allow(clippy::disallowed_macros)] /// assert_eq!(dbg!(1usize, 2u32), (1, 2)); /// ``` /// For what it is worth, the exclamation mark did surprise me.
#[] would apply to the next item/statement, and #![] would apply to the surrouding scope. In this case there's just a single statement so they should be equivalent.
Miguel, is this change from #[] to #![] intentional?
Best,
Gary
Quoted 14 lines
rust/kernel/std_vendor.rs line 132
Quoted 10 lines
rust/kernel/std_vendor.rs +1
-1@@ -127,7 +127,7 @@/// /// ``` -/// # #[allow(clippy::dbg_macro)] +/// # #[allow(clippy::disallowed_macros)] /// # { /// assert_eq!(1, dbg!(1u32,)); // trailing comma ignored /// assert_eq!((1,), dbg!((1u32,))); // 1-tuple Groeten
Geert Stappers
Re: [PATCH 12/19] rust: replace `clippy::dbg_macro` with `disallowed_macros`
Quoted 6 lines · Gary Guo
#[]would apply to the next item/statement, and#![]would apply to the surrouding scope. In this case there's just a single statement so they should be equivalent.Miguel, is this change from
#[]to#![]intentional?
Yeah, it is intentional -- it is what the last paragraph in the commit message refers to.
I think this is e.g. https://github.com/rust-lang/rust/issues/87391
and https://github.com/rust-lang/rust-clippy/issues/10355:
note: the built-in attribute expect will be ignored, since it's
applied to the macro invocation assert_eq
In addition, disallowed_macros also has some false negatives I noticed back when I started playing with this:
https://github.com/rust-lang/rust-clippy/issues/11431
Cheers,
Miguel
Miguel Ojeda
maintainerPATCH 13/19 rust: rbtree: fix `SAFETY` comments that should be `# Safety` sections
In reply to Miguel OjedaThe tag SAFETY is used for safety comments, i.e. // SAFETY, while a Safety section is used for safety preconditions in code documentation, i.e. /// # Safety.
Fix the three instances recently added in rbtree that Clippy would have normally caught in a public item, so that we can enable checking of private items in the next commit.
--- rust/kernel/rbtree.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
rust/kernel/rbtree.rs +6 -3
@@ -884,7 +884,8 @@ fn get_neighbor_raw(&self, direction: Direction) -> Option<NonNull<bindings::rb_NonNull ::new (neighbor) } -/// SAFETY: +/// # Safety +/// /// - `node` must be a valid pointer to a node in an [`RBTree`]. /// - The caller has immutable access to `node` for the duration of 'b. unsafe fn to_key_value <'b>(node:NonNull <bindings::rb_node >) -> (&'bK , &'bV ) {
@@ -894,7 +895,8 @@ unsafe fn to_key_value<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, &'b V) { (k,unsafe { &*v }) } -/// SAFETY: +/// # Safety +/// /// - `node` must be a valid pointer to a node in an [`RBTree`]. /// - The caller has mutable access to `node` for the duration of 'b. unsafe fn to_key_value_mut <'b>(node:NonNull <bindings::rb_node >) -> (&'bK , &'bmut V ) {
@@ -904,7 +906,8 @@ unsafe fn to_key_value_mut<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, &'b (k,unsafe { &mut *v }) } -/// SAFETY: +/// # Safety +/// /// - `node` must be a valid pointer to a node in an [`RBTree`]. /// - The caller has immutable access to the key for the duration of 'b. unsafe fn to_key_value_raw <'b>(node:NonNull <bindings::rb_node >) -> (&'bK , *mut V ) {
Signature · 1 line
2.46.0
Re: [PATCH 13/19] rust: rbtree: fix `SAFETY` comments that should be `# Safety` sections
Quoted 11 lines · Miguel Ojeda
The tag
SAFETYis used for safety comments, i.e.// SAFETY, while aSafetysection is used for safety preconditions in code documentation, i.e./// # Safety.Fix the three instances recently added in
rbtreethat Clippy would have normally caught in a public item, so that we can enable checking of private items in the next commit.Fixes 98c14e40e07a ("rust: rbtree: add cursor")Signed-off-by Miguel Ojeda <[email protected]>
Re: [PATCH 13/19] rust: rbtree: fix `SAFETY` comments that should be `# Safety` sections
In reply to Miguel OjedaQuoted 11 lines · Miguel Ojeda
The tag
SAFETYis used for safety comments, i.e.// SAFETY, while aSafetysection is used for safety preconditions in code documentation, i.e./// # Safety.Fix the three instances recently added in
rbtreethat Clippy would have normally caught in a public item, so that we can enable checking of private items in the next commit.Fixes 98c14e40e07a ("rust: rbtree: add cursor")Signed-off-by Miguel Ojeda <[email protected]>
Miguel Ojeda
maintainerPATCH 14/19 rust: provide proper code documentation titles
In reply to Miguel OjedaRust 1.82.0's Clippy is introducing [1][2] a new warn-by-default lint, too_long_first_doc_paragraph [3], which is intended to catch titles of code documentation items that are too long (likely because no title was provided and the item documentation starts with a paragraph).
This lint does not currently trigger anywhere, but it does detect a couple cases we had in private cases if checking for private items gets enabled (which we will do in the next commit):
error: first doc comment paragraph is too long
--> rust/kernel/init/__internal.rs:18:1
|
18 | / /// This is the module-internal type implementing PinInit and Init. It is unsafe to create this 19 | | /// type, since the closure needs to fulfill the same safety requirement as the
20 | | /// __pinned_init/__init functions.
| |_
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph
= note: -D clippy::too-long-first-doc-paragraph implied by -D warnings
= help: to override -D warnings add #[allow(clippy::too_long_first_doc_paragraph)]
error: first doc comment paragraph is too long
--> rust/kernel/sync/arc/std_vendor.rs:3:1
|
3 | / //! The contents of this file come from the Rust standard library, hosted in
4 | | //! the <https://github.com/rust-lang/rust> repository, licensed under
5 | | //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
6 | | //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
| |_
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph
Thus clean the two instances we hit and enable the lint.
In addition, since we have a second std_vendor.rs file with a similar header, do the same there too (even if that one does not trigger the lint, because it is doc(hidden)).
--- rust/kernel/init/__internal.rs | 7 ++++--- rust/kernel/std_vendor.rs | 2 ++ rust/kernel/sync/arc/std_vendor.rs | 2 ++ 3 files changed, 8 insertions(+), 3 deletions(-)
rust/kernel/init/__internal.rs +4 -3
@@ -15,9 +15,10 @@/// [this table]: https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns pub (super )type Invariant <T > =PhantomData <fn (*mut T ) -> *mut T >; -/// This is the module-internal type implementing `PinInit` and `Init`. It is unsafe to create this -/// type, since the closure needs to fulfill the same safety requirement as the -/// `__pinned_init`/`__init` functions. +/// Module-internal type implementing `PinInit` and `Init`. +/// +/// It is unsafe to create this type, since the closure needs to fulfill the same safety +/// requirement as the `__pinned_init`/`__init` functions. pub (crate )struct InitClosure <F ,T : ?Sized ,E >(pub (crate )F ,pub (crate )Invariant <(E ,T )>);// SAFETY: While constructing the `InitClosure`, the user promised that it upholds the
rust/kernel/std_vendor.rs +2 -0
@@ -1,5 +1,7 @@// SPDX-License-Identifier: Apache-2.0 OR MIT +//! Rust standard library vendored code. +//! //! The contents of this file come from the Rust standard library, hosted in //! the <https://github.com/rust-lang/rust> repository, licensed under //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
rust/kernel/sync/arc/std_vendor.rs +2 -0
@@ -1,5 +1,7 @@// SPDX-License-Identifier: Apache-2.0 OR MIT +//! Rust standard library vendored code. +//! //! The contents of this file come from the Rust standard library, hosted in //! the <https://github.com/rust-lang/rust> repository, licensed under //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
Signature · 1 line
2.46.0
Re: [PATCH 14/19] rust: provide proper code documentation titles
Quoted 35 lines · Miguel Ojeda
Rust 1.82.0's Clippy is introducing [1][2] a new warn-by-default lint,
too_long_first_doc_paragraph[3], which is intended to catch titles of code documentation items that are too long (likely because no title was provided and the item documentation starts with a paragraph).This lint does not currently trigger anywhere, but it does detect a couple cases we had in private cases if checking for private items gets enabled (which we will do in the next commit):
error: first doc comment paragraph is too long
-->rust/kernel/init/__internal.rs:18:1
|
18 | / /// This is the module-internal type implementingPinInitandInit. It is unsafe to create this 19 | | /// type, since the closure needs to fulfill the same safety requirement as the
20 | | ///__pinned_init/__initfunctions.
| |_
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph
= note:-D clippy::too-long-first-doc-paragraphimplied by-D warnings
= help: to override-D warningsadd#[allow(clippy::too_long_first_doc_paragraph)]error: first doc comment paragraph is too long
-->rust/kernel/sync/arc/std_vendor.rs:3:1
|
3 | / //! The contents of this file come from the Rust standard library, hosted in
4 | | //! the <https://github.com/rust-lang/rust> repository, licensed under
5 | | //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
6 | | //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
| |_
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraphThus clean the two instances we hit and enable the lint.
Quoted 8 lines
In addition, since we have a second
std_vendor.rsfile with a similar header, do the same there too (even if that one does not trigger the lint, because it isdoc(hidden)).
Re: [PATCH 14/19] rust: provide proper code documentation titles
In reply to Miguel OjedaQuoted 9 lines · Miguel Ojeda
Rust 1.82.0's Clippy is introducing [1][2] a new warn-by-default lint,
too_long_first_doc_paragraph[3], which is intended to catch titles of code documentation items that are too long (likely because no title was provided and the item documentation starts with a paragraph).This lint does not currently trigger anywhere, but it does detect a couple cases we had in private cases if checking for private items gets enabled (which we will do in the next commit):
I think some words got flipped around, should this say something like "...a couple private cases we had in case checking..."?
Quoted 25 lines
error: first doc comment paragraph is too long
-->rust/kernel/init/__internal.rs:18:1
|
18 | / /// This is the module-internal type implementingPinInitandInit. It is unsafe to create this 19 | | /// type, since the closure needs to fulfill the same safety requirement as the
20 | | ///__pinned_init/__initfunctions.
| |_
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph
= note:-D clippy::too-long-first-doc-paragraphimplied by-D warnings
= help: to override-D warningsadd#[allow(clippy::too_long_first_doc_paragraph)]error: first doc comment paragraph is too long
-->rust/kernel/sync/arc/std_vendor.rs:3:1
|
3 | / //! The contents of this file come from the Rust standard library, hosted in
4 | | //! the <https://github.com/rust-lang/rust> repository, licensed under
5 | | //! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
6 | | //! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
| |_
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraphThus clean the two instances we hit and enable the lint.
Quoted 8 lines
In addition, since we have a second
std_vendor.rsfile with a similar header, do the same there too (even if that one does not trigger the lint, because it isdoc(hidden)).
Re: [PATCH 14/19] rust: provide proper code documentation titles
Quoted 3 lines · Trevor Gross
I think some words got flipped around, should this say something like "...a couple private cases we had in case checking..."?
Nice catch, fixed in the version I am will apply -- thanks!
Cheers,
Miguel
Miguel Ojeda
maintainerPATCH 15/19 rust: enable Clippy's `check-private-items`
In reply to Miguel OjedaIn Rust 1.76.0, Clippy added the check-private-items lint configuration option. When turned on (the default is off), it makes several lints check private items as well.
In our case, it affects two lints we have enabled [1]:missing_safety_doc and unnecessary_safety_doc.
It also seems to affect the new too_long_first_doc_paragraph lint [2], even though the documentation does not mention it.
Thus allow the few instances remaining we currently hit and enable the lint.
--- .clippy.toml | 2 ++ rust/kernel/init.rs | 1 + rust/kernel/init/__internal.rs | 2 ++ rust/kernel/init/macros.rs | 1 + rust/kernel/print.rs | 1 + 5 files changed, 7 insertions(+)
.clippy.toml +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 +check-private-items = true + disallowed-macros = [ # The `clippy::dbg_macro` lint only works with `std::dbg!`, thus we simulate # it here, see: https://github.com/rust-lang/rust-clippy/issues/11303.
rust/kernel/init.rs +1 -0
@@ -125,6 +125,7 @@//! use core::{ptr::addr_of_mut, marker::PhantomPinned, pin::Pin}; //! # mod bindings { //! # #![allow(non_camel_case_types)] +//! # #![allow(clippy::missing_safety_doc)] //! # pub struct foo; //! # pub unsafe fn init_foo(_ptr: *mut foo) {} //! # pub unsafe fn destroy_foo(_ptr: *mut foo) {}
rust/kernel/init/__internal.rs +2 -0
@@ -54,6 +54,7 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {pub unsafe trait HasPinData {type PinData :PinData ; +#[allow(clippy::missing_safety_doc)] unsafe fn __pin_data () ->Self ::PinData ; }
@@ -83,6 +84,7 @@ fn make_closure<F, O, E>(self, f: F) -> Fpub unsafe trait HasInitData {type InitData :InitData ; +#[allow(clippy::missing_safety_doc)] unsafe fn __init_data () ->Self ::InitData ; }
rust/kernel/init/macros.rs +1 -0
@@ -989,6 +989,7 @@ fn drop(&mut self) {// // The functions are `unsafe` to prevent accidentally calling them. #[allow(dead_code)] +#[allow(clippy::missing_safety_doc)] impl<$($impl_generics)*> $pin_data<$($ty_generics)*> where $($whr)* {
rust/kernel/print.rs +1 -0
@@ -14,6 +14,7 @@use crate ::str::RawFormatter ;// Called from `vsprintf` with format specifier `%pA`. +#[allow(clippy::missing_safety_doc)] #[no_mangle] unsafe extern "C" fn rust_fmt_argument ( buf: *mut c_char,
Signature · 1 line
2.46.0
Re: [PATCH 15/19] rust: enable Clippy's `check-private-items`
Quoted 11 lines · Miguel Ojeda
In Rust 1.76.0, Clippy added the
check-private-itemslint configuration option. When turned on (the default is off), it makes several lints check private items as well.In our case, it affects two lints we have enabled [1]:
missing_safety_docandunnecessary_safety_doc.It also seems to affect the new
too_long_first_doc_paragraphlint [2], even though the documentation does not mention it.
Quoted 6 lines
Thus allow the few instances remaining we currently hit and enable the lint.
Re: [PATCH 15/19] rust: enable Clippy's `check-private-items`
In reply to Miguel OjedaQuoted 11 lines · Miguel Ojeda
In Rust 1.76.0, Clippy added the
check-private-itemslint configuration option. When turned on (the default is off), it makes several lints check private items as well.In our case, it affects two lints we have enabled [1]:
missing_safety_docandunnecessary_safety_doc.It also seems to affect the new
too_long_first_doc_paragraphlint [2], even though the documentation does not mention it.
Quoted 6 lines
Thus allow the few instances remaining we currently hit and enable the lint.
Miguel Ojeda
maintainerPATCH 16/19 Documentation: rust: add coding guidelines on lints
In reply to Miguel OjedaIn the C side, disabling diagnostics locally, i.e. within the source code, is rare (at least in the kernel). Sometimes warnings are manipulated via the flags at the translation unit level, but that is about it.
In Rust, it is easier to change locally the "level" of lints (e.g. allowing them locally). In turn, this means it is easier to globally enable more lints that may trigger a few false positives here and there that need to be allowed ocally, but that generally can spot issues or bugs.
Thus document this.
--- Documentation/rust/coding-guidelines.rst | 29 ++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Documentation/rust/coding-guidelines.rst +29 -0
@@ -227,3 +227,32 @@ The equivalent in Rust may look like (ignoring documentation): That is, the equivalent of ``GPIO_LINE_DIRECTION_IN`` would be referred to as ``gpio::LineDirection::In``. In particular, it should not be named ``gpio::gpio_line_direction::GPIO_LINE_DIRECTION_IN``. + + +Lints +----- + +In Rust, it is possible to ``allow`` particular warnings (diagnostics, lints) +locally, making the compiler ignore instances of a given warning within a given +function, module, block, etc. + +It is similar to ``#pragma GCC diagnostic push`` + ``ignored`` + ``pop`` in C: + +.. code-block:: c + + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wunused-function" + static void f(void) {} + #pragma GCC diagnostic pop + +But way less verbose: + +.. code-block:: rust + + #[allow(dead_code)] + fn f() {} + +By that virtue, it makes it possible to comfortably enable more diagnostics by +default (i.e. outside ``W=`` levels). In particular, those that may have some +false positives but that are otherwise quite useful to keep enabled to catch +potential mistakes.
Signature · 1 line
2.46.0
Re: [PATCH 16/19] Documentation: rust: add coding guidelines on lints
Quoted 11 lines · Miguel Ojeda
In the C side, disabling diagnostics locally, i.e. within the source code, is rare (at least in the kernel). Sometimes warnings are manipulated via the flags at the translation unit level, but that is about it.
In Rust, it is easier to change locally the "level" of lints (e.g. allowing them locally). In turn, this means it is easier to globally enable more lints that may trigger a few false positives here and there that need to be allowed ocally, but that generally can spot issues or bugs.
Quoted 3 lines
Thus document this.
Signed-off-by Miguel Ojeda <[email protected]>
Wow, does C really not have an easier way to do it?
Re: [PATCH 16/19] Documentation: rust: add coding guidelines on lints
Quoted 2 lines · Alice Ryhl
Wow, does C really not have an easier way to do it?
Yeah, it would be nice to get a similar system in C.
There are targeted attributes that can annotate certain things, like [[maybe_unused]] in C23 (and vendor attributes too like our __maybe_unused macro), so -Wunused-function is not really the best example in that sense -- I will think of a better one (it was nice to use the same as in the other examples I wrote for expect later on, which is why I used it).
But, as far as I am aware, there is no way to handle lints (and levels and so on) in a simple and consistent way like Rust does.
Cheers,
Miguel
Re: [PATCH 16/19] Documentation: rust: add coding guidelines on lints
Quoted 17 lines · Miguel Ojeda
Quoted 2 lines · Alice Ryhl
Wow, does C really not have an easier way to do it?
Yeah, it would be nice to get a similar system in C.
There are targeted attributes that can annotate certain things, like
[[maybe_unused]]in C23 (and vendor attributes too like our__maybe_unusedmacro), so-Wunused-functionis not really the best example in that sense -- I will think of a better one (it was nice to use the same as in the other examples I wrote forexpectlater on, which is why I used it).
Quoted 2 lines
But, as far as I am aware, there is no way to handle lints (and levels and so on) in a simple and consistent way like Rust does.
You can always hide the pragmas behind a macro:
https://gcc.godbolt.org/z/WTEaYWW8c
It’s not perfect, because warning names sometimes differ between GCC and Clang, among other reasons.
Re: [PATCH 16/19] Documentation: rust: add coding guidelines on lints
Quoted 6 lines · comex
You can always hide the pragmas behind a macro:
https://gcc.godbolt.org/z/WTEaYWW8c
It’s not perfect, because warning names sometimes differ between GCC and Clang, among other reasons.
That could be an interesting option for some projects, but yeah, as you say, I think it is far from ideal. It breaks clang-format, wrapping functions/structs/... would look weird, and generally the syntax gets on the way of the "normal" code (so even for statements one would probably want to keep the start and end on their own lines).
Cheers,
Miguel
Re: [PATCH 16/19] Documentation: rust: add coding guidelines on lints
In reply to Miguel OjedaQuoted 4 lines · Miguel Ojeda
example in that sense -- I will think of a better one (it was nice to use the same as in the other examples I wrote for
expectlater on, which is why I used it).
I added a footnote in the version I am applying to be able to keep using the same example (lint) everywhere.
Cheers,
Miguel
Re: [PATCH 16/19] Documentation: rust: add coding guidelines on lints
In reply to Miguel OjedaQuoted 10 lines · Miguel Ojeda
In the C side, disabling diagnostics locally, i.e. within the source code, is rare (at least in the kernel). Sometimes warnings are manipulated via the flags at the translation unit level, but that is about it.
In Rust, it is easier to change locally the "level" of lints (e.g. allowing them locally). In turn, this means it is easier to globally enable more lints that may trigger a few false positives here and there that need to be allowed ocally, but that generally can spot issues or bugs.
s/ocally/locally
Quoted 34 lines
Thus document this.
Signed-off-by Miguel Ojeda <[email protected]>--- Documentation/rust/coding-guidelines.rst | 29 ++++++++++++++++++++++++ 1 file changed, 29 insertions(+)Documentation/rust/coding-guidelines.rst +19
-0@@ -227,3 +227,32 @@ The equivalent in Rust may look like (ignoring documentation): That is, the equivalent of ``GPIO_LINE_DIRECTION_IN`` would be referred to as ``gpio::LineDirection::In``. In particular, it should not be named ``gpio::gpio_line_direction::GPIO_LINE_DIRECTION_IN``. + + +Lints +----- + +In Rust, it is possible to ``allow`` particular warnings (diagnostics, lints) +locally, making the compiler ignore instances of a given warning within a given +function, module, block, etc. + +It is similar to ``#pragma GCC diagnostic push`` + ``ignored`` + ``pop`` in C: + +.. code-block:: c + + #pragma GCC diagnostic push + #pragma GCC diagnostic ignored "-Wunused-function" + static void f(void) {} + #pragma GCC diagnostic pop + +But way less verbose:
Documentation/rust/coding-guidelines.rst The equivalent in Rust may look like (ignoring documentation): · line 258
Quoted 10 lines
Documentation/rust/coding-guidelines.rst +10
-0+ +.. code-block:: rust + + #[allow(dead_code)] + fn f() {} + +By that virtue, it makes it possible to comfortably enable more diagnostics by +default (i.e. outside ``W=`` levels). In particular, those that may have some +false positives but that are otherwise quite useful to keep enabled to catch +potential mistakes.
It may be good to link to the docs on this,
https://doc.rust-lang.org/stable/reference/attributes/diagnostics.html.
Either way:
Re: [PATCH 16/19] Documentation: rust: add coding guidelines on lints
Quoted 2 lines · Trevor Gross
s/ocally/locally
Fixed in the version I am applying -- thanks!
Quoted 2 lines
It may be good to link to the docs on this,
https://doc.rust-lang.org/stable/reference/attributes/diagnostics.html.
Done too!
Cheers,
Miguel
Miguel Ojeda
maintainerPATCH 17/19 rust: start using the `#[expect(...)]` attribute
In reply to Miguel OjedaIn Rust, it is possible to allow particular warnings (diagnostics, lints) locally, making the compiler ignore instances of a given warning within a given function, module, block, etc.
It is similar to #pragma GCC diagnostic push + ignored + pop in C:
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-function" static void f(void) {}
#pragma GCC diagnostic pop
But way less verbose:
#[allow(dead_code)]
fn f() {}By that virtue, it makes it possible to comfortably enable more diagnostics by default (i.e. outside W= levels) that may have some false positives but that are otherwise quite useful to keep enabled to catch potential mistakes.
The #[expect(...)] attribute [1] takes this further, and makes the compiler warn if the diagnostic was _not_ produced. For instance, the following will ensure that, when f() is called somewhere, we will have to remove the attribute:
#[expect(dead_code)]
fn f() {}If we do not, we get a warning from the compiler:
warning: this lint expectation is unfulfilled
--> x.rs:3:10
|
3 | #[expect(dead_code)]
| ^^^^^^^^^
|
= note: `#[warn(unfulfilled_lint_expectations)]` on by defaultThis means that expects do not get forgotten when they are not needed.
See the next commit for more details, nuances on its usage and documentation on the feature.
The attribute requires the lint_reasons [2] unstable feature, but it is becoming stable in 1.81.0 (to be released on 2024-09-05) and it has already been useful to clean things up in this patch series, finding cases where the allows should not have been there.
Thus, enable lint_reasons and convert some of our allows to expects where possible.
This feature was also an example of the ongoing collaboration between Rust and the kernel -- we tested it in the kernel early on and found an issue that was quickly resolved [3].
--- rust/kernel/error.rs | 2 +- rust/kernel/init.rs | 22 +++++++++++----------- rust/kernel/init/__internal.rs | 4 ++-- rust/kernel/init/macros.rs | 10 +++++----- rust/kernel/ioctl.rs | 2 +- rust/kernel/lib.rs | 1 + rust/kernel/list/arc_field.rs | 2 +- rust/kernel/print.rs | 4 ++-- rust/kernel/std_vendor.rs | 10 +++++----- samples/rust/rust_print.rs | 2 +- scripts/Makefile.build | 2 +- 11 files changed, 31 insertions(+), 30 deletions(-)
rust/kernel/error.rs +1 -1
@@ -133,7 +133,7 @@ pub(crate) fn to_blk_status(self) -> bindings::blk_status_t { }/// Returns the error encoded as a pointer. -#[allow(dead_code)] +#[expect(dead_code)] pub (crate )fn to_ptr <T >(self) -> *mut T {#[cfg_attr(target_pointer_width = "32" , allow(clippy::useless_conversion))] // SAFETY: `self.0` is a valid error due to its invariant.
rust/kernel/init.rs +11 -11
@@ -35,7 +35,7 @@//! that you need to write `<-` instead of `:` for fields that you want to initialize in-place. //! //! ```rust -//! # #![allow(clippy::disallowed_names)] +//! # #![expect(clippy::disallowed_names)] //! use kernel::sync::{new_mutex, Mutex}; //! # use core::pin::Pin; //! #[pin_data]
@@ -55,7 +55,7 @@//! (or just the stack) to actually initialize a `Foo`: //! //! ```rust -//! # #![allow(clippy::disallowed_names)] +//! # #![expect(clippy::disallowed_names)] //! # use kernel::sync::{new_mutex, Mutex}; //! # use core::pin::Pin; //! # #[pin_data]
@@ -120,12 +120,12 @@//! `slot` gets called. //! //! ```rust -//! # #![allow(unreachable_pub, clippy::disallowed_names)] +//! # #![expect(unreachable_pub, clippy::disallowed_names)] //! use kernel::{init, types::Opaque}; //! use core::{ptr::addr_of_mut, marker::PhantomPinned, pin::Pin}; //! # mod bindings { -//! # #![allow(non_camel_case_types)] -//! # #![allow(clippy::missing_safety_doc)] +//! # #![expect(non_camel_case_types)] +//! # #![expect(clippy::missing_safety_doc)] //! # pub struct foo; //! # pub unsafe fn init_foo(_ptr: *mut foo) {} //! # pub unsafe fn destroy_foo(_ptr: *mut foo) {}
@@ -238,7 +238,7 @@/// # Examples /// /// ```rust -/// # #![allow(clippy::disallowed_names)] +/// # #![expect(clippy::disallowed_names)] /// # use kernel::{init, macros::pin_data, pin_init, stack_pin_init, init::*, sync::Mutex, new_mutex}; /// # use core::pin::Pin; /// #[pin_data]
@@ -290,7 +290,7 @@ macro_rules! stack_pin_init {/// # Examples /// /// ```rust,ignore -/// # #![allow(clippy::disallowed_names)] +/// # #![expect(clippy::disallowed_names)] /// # use kernel::{init, pin_init, stack_try_pin_init, init::*, sync::Mutex, new_mutex}; /// # use macros::pin_data; /// # use core::{alloc::AllocError, pin::Pin};
@@ -316,7 +316,7 @@ macro_rules! stack_pin_init {/// ``` /// /// ```rust,ignore -/// # #![allow(clippy::disallowed_names)] +/// # #![expect(clippy::disallowed_names)] /// # use kernel::{init, pin_init, stack_try_pin_init, init::*, sync::Mutex, new_mutex}; /// # use macros::pin_data; /// # use core::{alloc::AllocError, pin::Pin};
@@ -438,7 +438,7 @@ macro_rules! stack_try_pin_init {/// Users of `Foo` can now create it like this: /// /// ```rust -/// # #![allow(clippy::disallowed_names)] +/// # #![expect(clippy::disallowed_names)] /// # use kernel::{init, pin_init, macros::pin_data, init::*}; /// # use core::pin::Pin; /// # #[pin_data]
@@ -852,7 +852,7 @@ pub unsafe trait PinInit<T: ?Sized, E = Infallible>: Sized {/// # Examples /// /// ```rust -/// # #![allow(clippy::disallowed_names)] +/// # #![expect(clippy::disallowed_names)] /// use kernel::{types::Opaque, init::pin_init_from_closure}; /// #[repr(C)] /// struct RawFoo([u8; 16]);
@@ -964,7 +964,7 @@ pub unsafe trait Init<T: ?Sized, E = Infallible>: PinInit<T, E> {/// # Examples /// /// ```rust -/// # #![allow(clippy::disallowed_names)] +/// # #![expect(clippy::disallowed_names)] /// use kernel::{types::Opaque, init::{self, init_from_closure}}; /// struct Foo { /// buf: [u8; 1_000_000],
rust/kernel/init/__internal.rs +2 -2
@@ -54,7 +54,7 @@ unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {pub unsafe trait HasPinData {type PinData :PinData ; -#[allow(clippy::missing_safety_doc)] +#[expect(clippy::missing_safety_doc)] unsafe fn __pin_data () ->Self ::PinData ; }
@@ -84,7 +84,7 @@ fn make_closure<F, O, E>(self, f: F) -> Fpub unsafe trait HasInitData {type InitData :InitData ; -#[allow(clippy::missing_safety_doc)] +#[expect(clippy::missing_safety_doc)] unsafe fn __init_data () ->Self ::InitData ; }
rust/kernel/init/macros.rs +5 -5
@@ -182,13 +182,13 @@//! // Normally `Drop` bounds do not have the correct semantics, but for this purpose they do //! // (normally people want to know if a type has any kind of drop glue at all, here we want //! // to know if it has any kind of custom drop glue, which is exactly what this bound does). -//! #[allow(drop_bounds)] +//! #[expect(drop_bounds)] //! impl<T: ::core::ops::Drop> MustNotImplDrop for T {} //! impl<T> MustNotImplDrop for Bar<T> {} //! // Here comes a convenience check, if one implemented `PinnedDrop`, but forgot to add it to //! // `#[pin_data]`, then this will error with the same mechanic as above, this is not needed //! // for safety, but a good sanity check, since no normal code calls `PinnedDrop::drop`. -//! #[allow(non_camel_case_types)] +//! #[expect(non_camel_case_types)] //! trait UselessPinnedDropImpl_you_need_to_specify_PinnedDrop {} //! impl< //! T: ::kernel::init::PinnedDrop,
@@ -925,14 +925,14 @@ impl<'__pin, $($impl_generics)*> ::core::marker::Unpin for $name<$($ty_generics)// `Drop`. Additionally we will implement this trait for the struct leading to a conflict, // if it also implements `Drop` trait MustNotImplDrop {} -#[allow(drop_bounds)] +#[expect(drop_bounds)] impl <T : ::core::ops::Drop >MustNotImplDrop for T {} impl<$($impl_generics)*> MustNotImplDrop for $name<$($ty_generics)*> where $($whr)* {}// We also take care to prevent users from writing a useless `PinnedDrop` implementation. // They might implement `PinnedDrop` correctly for the struct, but forget to give // `PinnedDrop` as the parameter to `#[pin_data]`. -#[allow(non_camel_case_types)] +#[expect(non_camel_case_types)] trait UselessPinnedDropImpl_you_need_to_specify_PinnedDrop {}impl <T : $crate::init::PinnedDrop >UselessPinnedDropImpl_you_need_to_specify_PinnedDrop for T {}
@@ -989,7 +989,7 @@ fn drop(&mut self) {// // The functions are `unsafe` to prevent accidentally calling them. #[allow(dead_code)] -#[allow(clippy::missing_safety_doc)] +#[expect(clippy::missing_safety_doc)] impl<$($impl_generics)*> $pin_data<$($ty_generics)*> where $($whr)* {
rust/kernel/ioctl.rs +1 -1
@@ -4,7 +4,7 @@//! //! C header: [`include/asm-generic/ioctl.h`](srctree/include/asm-generic/ioctl.h) -#![allow(non_snake_case)] +#![expect(non_snake_case)] use crate ::build_assert;
rust/kernel/lib.rs +1 -0
@@ -14,6 +14,7 @@#![no_std] #![feature(coerce_unsized)] #![feature(dispatch_from_dyn)] +#![feature(lint_reasons)] #![feature(new_uninit)] #![feature(receiver_trait)] #![feature(unsize)]
rust/kernel/list/arc_field.rs +1 -1
@@ -56,7 +56,7 @@ pub unsafe fn assert_ref(&self) -> &T {/// /// The caller must have mutable access to the `ListArc<ID>` containing the struct with this /// field for the duration of the returned reference. -#[allow(clippy::mut_from_ref)] +#[expect(clippy::mut_from_ref)] pub unsafe fn assert_mut (&self) -> &mut T {// SAFETY: The caller has exclusive access to the `ListArc`, so they also have exclusive // access to this field.
rust/kernel/print.rs +2 -2
@@ -14,7 +14,7 @@use crate ::str::RawFormatter ;// Called from `vsprintf` with format specifier `%pA`. -#[allow(clippy::missing_safety_doc)] +#[expect(clippy::missing_safety_doc)] #[no_mangle] unsafe extern "C" fn rust_fmt_argument ( buf: *mut c_char,
@@ -140,7 +140,7 @@ pub fn call_printk_cont(args: fmt::Arguments<'_>) {#[doc(hidden)] #[cfg(not(testlib))] #[macro_export] -#[allow(clippy::crate_in_macro_def)] +#[expect(clippy::crate_in_macro_def)] macro_rules! print_macro ( // The non-continuation cases (most of them, e.g. `INFO`). ($format_string:path, false, $($arg:tt)+) => (
rust/kernel/std_vendor.rs +5 -5
@@ -16,7 +16,7 @@/// /// ```rust /// let a = 2; -/// # #[allow(clippy::disallowed_macros)] +/// # #[expect(clippy::disallowed_macros)] /// let b = dbg!(a * 2) + 1; /// // ^-- prints: [src/main.rs:2] a * 2 = 4 /// assert_eq!(b, 5);
@@ -54,7 +54,7 @@/// With a method call: /// /// ```rust -/// # #[allow(clippy::disallowed_macros)] +/// # #[expect(clippy::disallowed_macros)] /// fn foo(n: usize) { /// if dbg!(n.checked_sub(4)).is_some() { /// // ...
@@ -73,7 +73,7 @@/// Naive factorial implementation: /// /// ```rust -/// # #[allow(clippy::disallowed_macros)] +/// # #[expect(clippy::disallowed_macros)] /// # { /// fn factorial(n: u32) -> u32 { /// if dbg!(n <= 1) {
@@ -120,7 +120,7 @@/// a tuple (and return it, too): /// /// ``` -/// # #![allow(clippy::disallowed_macros)] +/// # #![expect(clippy::disallowed_macros)] /// assert_eq!(dbg!(1usize, 2u32), (1, 2)); /// ``` ///
@@ -129,7 +129,7 @@/// invocations. You can use a 1-tuple directly if you need one: /// /// ``` -/// # #[allow(clippy::disallowed_macros)] +/// # #[expect(clippy::disallowed_macros)] /// # { /// assert_eq!(1, dbg!(1u32,)); // trailing comma ignored /// assert_eq!((1,), dbg!((1u32,))); // 1-tuple
samples/rust/rust_print.rs +1 -1
@@ -15,7 +15,7 @@struct RustPrint ; -#[allow(clippy::disallowed_macros)] +#[expect(clippy::disallowed_macros)] fn arc_print () ->Result {use kernel::sync::*;
scripts/Makefile.build +1 -1
@@ -263,7 +263,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE # Compile Rust sources (.rs) # --------------------------------------------------------------------------- -rust_allowed_features := new_uninit +rust_allowed_features := lint_reasons,new_uninit # `--out-dir` is required to avoid temporaries being created by `rustc` in the # current working directory, which may be not accessible in the out-of-tree
Signature · 1 line
2.46.0
Re: [PATCH 17/19] rust: start using the `#[expect(...)]` attribute
Quoted 57 lines · Miguel Ojeda
In Rust, it is possible to
allowparticular warnings (diagnostics, lints) locally, making the compiler ignore instances of a given warning within a given function, module, block, etc.It is similar to
#pragma GCC diagnostic push+ignored+popin C:#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-function" static void f(void) {}
#pragma GCC diagnostic popBut way less verbose:
#[allow(dead_code)] fn f() {}By that virtue, it makes it possible to comfortably enable more diagnostics by default (i.e. outside
W=levels) that may have some false positives but that are otherwise quite useful to keep enabled to catch potential mistakes.The
#[expect(...)]attribute [1] takes this further, and makes the compiler warn if the diagnostic was _not_ produced. For instance, the following will ensure that, whenf()is called somewhere, we will have to remove the attribute:#[expect(dead_code)] fn f() {}If we do not, we get a warning from the compiler:
warning: this lint expectation is unfulfilled --> x.rs:3:10 | 3 | #[expect(dead_code)] | ^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by defaultThis means that
expects do not get forgotten when they are not needed.See the next commit for more details, nuances on its usage and documentation on the feature.
The attribute requires the
lint_reasons[2] unstable feature, but it is becoming stable in 1.81.0 (to be released on 2024-09-05) and it has already been useful to clean things up in this patch series, finding cases where theallows should not have been there.Thus, enable
lint_reasonsand convert some of ourallows toexpects where possible.This feature was also an example of the ongoing collaboration between Rust and the kernel -- we tested it in the kernel early on and found an issue that was quickly resolved [3].
Quoted 6 lines
Cc Fridtjof Stoldt <[email protected]>Cc Urgau <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>
Re: [PATCH 17/19] rust: start using the `#[expect(...)]` attribute
In reply to Miguel OjedaQuoted 57 lines · Miguel Ojeda
In Rust, it is possible to
allowparticular warnings (diagnostics, lints) locally, making the compiler ignore instances of a given warning within a given function, module, block, etc.It is similar to
#pragma GCC diagnostic push+ignored+popin C:#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-function" static void f(void) {}
#pragma GCC diagnostic popBut way less verbose:
#[allow(dead_code)] fn f() {}By that virtue, it makes it possible to comfortably enable more diagnostics by default (i.e. outside
W=levels) that may have some false positives but that are otherwise quite useful to keep enabled to catch potential mistakes.The
#[expect(...)]attribute [1] takes this further, and makes the compiler warn if the diagnostic was _not_ produced. For instance, the following will ensure that, whenf()is called somewhere, we will have to remove the attribute:#[expect(dead_code)] fn f() {}If we do not, we get a warning from the compiler:
warning: this lint expectation is unfulfilled --> x.rs:3:10 | 3 | #[expect(dead_code)] | ^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by defaultThis means that
expects do not get forgotten when they are not needed.See the next commit for more details, nuances on its usage and documentation on the feature.
The attribute requires the
lint_reasons[2] unstable feature, but it is becoming stable in 1.81.0 (to be released on 2024-09-05) and it has already been useful to clean things up in this patch series, finding cases where theallows should not have been there.Thus, enable
lint_reasonsand convert some of ourallows toexpects where possible.This feature was also an example of the ongoing collaboration between Rust and the kernel -- we tested it in the kernel early on and found an issue that was quickly resolved [3].
Quoted 6 lines
Cc Fridtjof Stoldt <[email protected]>Cc Urgau <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>
Miguel Ojeda
maintainerPATCH 18/19 Documentation: rust: discuss `#[expect(...)]` in the guidelines
In reply to Miguel OjedaDiscuss #[expect(...)] in the Lints sections of the coding guidelines document, which is an upcoming feature in Rust 1.81.0, and explain that it is generally to be preferred over allow unless there is a reason not to use it (e.g. conditional compilation being involved).
--- Documentation/rust/coding-guidelines.rst | 110 +++++++++++++++++++++++ 1 file changed, 110 insertions(+)
Documentation/rust/coding-guidelines.rst +110 -0
@@ -256,3 +256,113 @@ By that virtue, it makes it possible to comfortably enable more diagnostics by default (i.e. outside ``W=`` levels). In particular, those that may have some false positives but that are otherwise quite useful to keep enabled to catch potential mistakes. + +On top of that, Rust provides the ``expect`` attribute which takes this further. +It makes the compiler warn if the warning was not produced. For instance, the +following will ensure that, when ``f()`` is called somewhere, we will have to +remove the attribute: + +.. code-block:: rust + + #[expect(dead_code)] + fn f() {} + +If we do not, we get a warning from the compiler:: + + warning: this lint expectation is unfulfilled + --> x.rs:3:10 + | + 3 | #[expect(dead_code)] + | ^^^^^^^^^ + | + = note: `#[warn(unfulfilled_lint_expectations)]` on by default + +This means that ``expect``\ s do not get forgotten when they are not needed, which +may happen in several situations, e.g.: + +- Temporary attributes added while developing. + +- Improvements in lints in the compiler, Clippy or custom tools which may + remove a false positive. + +- When the lint is not needed anymore because it was expected that it would be + removed at some point, such as the ``dead_code`` example above. + +It also increases the visibility of the remaining ``allow``\ s and reduces the +chance of misapplying one. + +Thus prefer ``except`` over ``allow`` unless: + +- The lint attribute is intended to be temporary, e.g. while developing. + +- Conditional compilation triggers the warning in some cases but not others. + + If there are only a few cases where the warning triggers (or does not + trigger) compared to the total number of cases, then one may consider using + a conditional ``expect`` (i.e. ``cfg_attr(..., expect(...))``). Otherwise, + it is likely simpler to just use ``allow``. + +- Inside macros, when the different invocations may create expanded code that + triggers the warning in some cases but not in others. + +- When code may trigger a warning for some architectures but not others, such + as an ``as`` cast to a C FFI type. + +As a more developed example, consider for instance this program: + +.. code-block:: rust + + fn g() {} + + fn main() { + #[cfg(CONFIG_X)] + g(); + } + +Here, function ``g()`` is dead code if ``CONFIG_X`` is not set. Can we use +``expect`` here? + +.. code-block:: rust + + #[expect(dead_code)] + fn g() {} + + fn main() { + #[cfg(CONFIG_X)] + g(); + } + +This would emit a lint if ``CONFIG_X`` is set, since it is not dead code in that +configuration. Therefore, in cases like this, we cannot use ``expect`` as-is. + +A simple possibility is using ``allow``: + +.. code-block:: rust + + #[allow(dead_code)] + fn g() {} + + fn main() { + #[cfg(CONFIG_X)] + g(); + } + +An alternative would be using a conditional ``expect``: + +.. code-block:: rust + + #[cfg_attr(not(CONFIG_X), expect(dead_code))] + fn g() {} + + fn main() { + #[cfg(CONFIG_X)] + g(); + } + +This would ensure that, if someone introduces another call to ``g()`` somewhere +(e.g. unconditionally), then it would be spotted that it is not dead code +anymore. However, the ``cfg_attr`` is more complex than a simple ``allow``. + +Therefore, it is likely that it is not worth using conditional ``expect``\ s when +more than one or two configurations are involved or when the lint may be +triggered due to non-local changes (such as ``dead_code``).
Signature · 1 line
2.46.0
Trevor Gross
reviewerRe: [PATCH 18/19] Documentation: rust: discuss `#[expect(...)]` in the guidelines
Quoted 7 lines · Miguel Ojeda
Discuss
#[expect(...)]in the Lints sections of the coding guidelines document, which is an upcoming feature in Rust 1.81.0, and explain that it is generally to be preferred overallowunless there is a reason not to use it (e.g. conditional compilation being involved).Signed-off-by Miguel Ojeda <[email protected]>
Would it be good to mention that a reason can be specified with reason = "..."? I don't think we use this anywhere yet.
Re: [PATCH 18/19] Documentation: rust: discuss `#[expect(...)]` in the guidelines
Quoted 3 lines · Trevor Gross
Would it be good to mention that a reason can be specified with
reason = "..."? I don't think we use this anywhere yet.
I mainly wanted to introduce things "slowly" :)
But I would be happy if we consistently use reason, I think it is a good idea to document those, and perhaps we can get to the point where we go for clippy::allow_attributes_without_reason too.
Relatedly, I also thought about clippy::allow_attributes, but due to conditional compilation and the other reasons mentioned in the docs added in the series, it is likely not something we can easily do.
Another tangent: I find the reason syntax a bit too "busy". I think in some cases we may just want to write things as if they were "normal comments" (and multiline and so on). Highlighting in particular ways could help perhaps. It would have been nice to have something like // ALLOW on top of the attribute, similar to // SAFETY, and have the compiler understand that directly like Clippy nowadays does for // SAFETY, but I guess there are downsides.
Cheers,
Miguel
Miguel Ojeda
maintainerPATCH 19/19 rust: std_vendor: simplify `{ .. macro! .. }` with inner attributes
In reply to Miguel OjedaIt is cleaner to have a single inner attribute rather than needing several hidden lines to wrap the macro invocations.
Thus simplify them.
--- rust/kernel/std_vendor.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
rust/kernel/std_vendor.rs +2 -6
@@ -73,8 +73,7 @@/// Naive factorial implementation: /// /// ```rust -/// # #[expect(clippy::disallowed_macros)] -/// # { +/// # #![expect(clippy::disallowed_macros)] /// fn factorial(n: u32) -> u32 { /// if dbg!(n <= 1) { /// dbg!(1)
@@ -84,7 +83,6 @@/// } /// /// dbg!(factorial(4)); -/// # } /// ``` /// /// This prints to the kernel log:
@@ -129,11 +127,9 @@/// invocations. You can use a 1-tuple directly if you need one: /// /// ``` -/// # #[expect(clippy::disallowed_macros)] -/// # { +/// # #![expect(clippy::disallowed_macros)] /// assert_eq!(1, dbg!(1u32,)); // trailing comma ignored /// assert_eq!((1,), dbg!((1u32,))); // 1-tuple -/// # } /// ``` /// /// [`std::dbg`]: https://doc.rust-lang.org/std/macro.dbg.html
Signature · 1 line
2.46.0
Alice Ryhl
maintainerRe: [PATCH 19/19] rust: std_vendor: simplify `{ .. macro! .. }` with inner attributes
Quoted 7 lines · Miguel Ojeda
It is cleaner to have a single inner attribute rather than needing several hidden lines to wrap the macro invocations.
Thus simplify them.
Signed-off-by Miguel Ojeda <[email protected]>
Are we ok with changing std_vendor?
Alice
Re: [PATCH 19/19] rust: std_vendor: simplify `{ .. macro! .. }` with inner attributes
Quoted 2 lines · Alice Ryhl
Are we ok with changing
std_vendor?
Yeah, to some degree, i.e. the code is already adapted slightly (it is not intended to be a 1:1 copy). As long as we keep it more or less in sync with upstream, it should be fine.
Even if we had to diverge, it should not be a big deal, but it would be nice to be able to pick up improvements if any, e.g. there is let else being used now upstream, which we could replicate (I will create a "good first issue" for that).
(These allows/expects are not there in the original anyway, for that reason)
Cheers,
Miguel
Re: [PATCH 19/19] rust: std_vendor: simplify `{ .. macro! .. }` with inner attributes
Quoted 14 lines · Miguel Ojeda
Quoted 2 lines · Alice Ryhl
Are we ok with changing
std_vendor?Yeah, to some degree, i.e. the code is already adapted slightly (it is not intended to be a 1:1 copy). As long as we keep it more or less in sync with upstream, it should be fine.
Even if we had to diverge, it should not be a big deal, but it would be nice to be able to pick up improvements if any, e.g. there is
let elsebeing used now upstream, which we could replicate (I will create a "good first issue" for that).
Quoted 4 lines
(These allows/expects are not there in the original anyway, for that reason)
Cheers,
Miguel
Re: [PATCH 19/19] rust: std_vendor: simplify `{ .. macro! .. }` with inner attributes
In reply to Miguel OjedaQuoted 7 lines · Miguel Ojeda
It is cleaner to have a single inner attribute rather than needing several hidden lines to wrap the macro invocations.
Thus simplify them.
Signed-off-by Miguel Ojeda <[email protected]>---
Quoted 31 lines · On Wed, 4 Sep 2024 22:43:28 +0200 Miguel Ojeda <[email protected]> wrote:
Hi all,
This is a series that contains a series of lint-related things:
- Cleanups and other improvements and fixes, including removing
allows that are not needed anymore for different reasons and a workaround fordbg_macrodetection.- The enablement of some safety lints so that the toolchain enforces that we write
// SAFETYcomments and# Safetysections properly.- The addition of
.clippy.toml, which allows us to take advantage of a few configuration options.- Start using the new
#[expect(...)]feature and add documentation on it as well as lints in general.Overall, this should improve the quality of the code and documentation as well as reduce the time needed in reviews.
I want to mention Trevor's nice work on lints from a while ago [1]. I think we should still do something like that: discuss which lints we would like to have one-by-one and start enabling them (and perhaps have a file like Trevor proposed etc.).
For the moment, though, I am sending these, since we would like to have at least the safety-related ones enabled soon [2]: now that more code and developers are joining, it sounds like a good time to start enforcing it -- it should make new Rust kernel developers aware of the need of writing them, which has proven to be a common request from reviewers.
Quoted 5 lines
If needed, the series can be applied partially or split, but most of it should be fairly uncontroversial.
Built with clippy locally and it's all clean.
Apart from one #[]/#![] modification - which I think is fine as long as it's intentional, everything LGTM.
Quoted 59 lines
Miguel Ojeda (19):
rust: workqueue: remove unneeded ``#[allow(clippy::new_ret_no_self)]`
rust: sort global Rust flags
rust: types: avoid repetition in{As,From}Bytesimpls
rust: enableclippy::undocumented_unsafe_blockslint
rust: enableclippy::unnecessary_safety_commentlint
rust: enableclippy::unnecessary_safety_doclint
rust: enableclippy::ignored_unit_patternslint
rust: enablerustdoc::unescaped_backtickslint
rust: init: remove unneeded#[allow(clippy::disallowed_names)]
rust: sync: remove unneeded`#[allow(clippy::non_send_fields_in_send_ty)]`rust: introduce
.clippy.toml
rust: replaceclippy::dbg_macrowithdisallowed_macros
rust: rbtree: fixSAFETYcomments that should be# Safetysections
rust: provide proper code documentation titles
rust: enable Clippy'scheck-private-items
Documentation: rust: add coding guidelines on lints
rust: start using the#[expect(...)]attribute
Documentation: rust: discuss#[expect(...)]in the guidelines
rust:std_vendor: simplify{ .. macro! .. }with inner attributes.clippy.toml | 9 ++ .gitignore | 1 + Documentation/rust/coding-guidelines.rst | 139 +++++++++++++++++++++++ MAINTAINERS | 1 + Makefile | 15 ++- rust/Makefile | 5 +- rust/bindings/lib.rs | 1 + rust/kernel/alloc/allocator.rs | 2 + rust/kernel/error.rs | 11 +- rust/kernel/init.rs | 30 ++--- rust/kernel/init/__internal.rs | 11 +- rust/kernel/init/macros.rs | 18 ++- rust/kernel/ioctl.rs | 2 +- rust/kernel/lib.rs | 1 + rust/kernel/list.rs | 1 + rust/kernel/list/arc_field.rs | 2 +- rust/kernel/print.rs | 5 +- rust/kernel/rbtree.rs | 9 +- rust/kernel/std_vendor.rs | 16 ++- rust/kernel/str.rs | 7 +- rust/kernel/sync/arc.rs | 2 +- rust/kernel/sync/arc/std_vendor.rs | 2 + rust/kernel/sync/condvar.rs | 1 - rust/kernel/sync/lock.rs | 6 +- rust/kernel/types.rs | 74 ++++++------ rust/kernel/workqueue.rs | 9 +- rust/uapi/lib.rs | 1 + samples/rust/rust_print.rs | 1 + scripts/Makefile.build | 2 +- 29 files changed, 293 insertions(+), 91 deletions(-) create mode 100644 .clippy.tomlbase-commit:
68d3b6aa08708bb3907c2c13eaf4b3ccf4805160--
2.46.0
Miguel Ojeda
AppliedQuoted 32 lines · Miguel Ojeda
Hi all,
This is a series that contains a series of lint-related things:
- Cleanups and other improvements and fixes, including removing
allows that are not needed anymore for different reasons and a workaround fordbg_macrodetection.- The enablement of some safety lints so that the toolchain enforces that we write
// SAFETYcomments and# Safetysections properly.- The addition of
.clippy.toml, which allows us to take advantage of a few configuration options.- Start using the new
#[expect(...)]feature and add documentation on it as well as lints in general.Overall, this should improve the quality of the code and documentation as well as reduce the time needed in reviews.
I want to mention Trevor's nice work on lints from a while ago [1]. I think we should still do something like that: discuss which lints we would like to have one-by-one and start enabling them (and perhaps have a file like Trevor proposed etc.).
For the moment, though, I am sending these, since we would like to have at least the safety-related ones enabled soon [2]: now that more code and developers are joining, it sounds like a good time to start enforcing it -- it should make new Rust kernel developers aware of the need of writing them, which has proven to be a common request from reviewers.
Quoted 5 lines
If needed, the series can be applied partially or split, but most of it should be fairly uncontroversial.
Applied to rust-next -- thanks everyone!
Cheers,
Miguel