Enable various new clippy lints #1025

tgross35 · 2023-07-16T09:18:38Z

This applies a most of what is discussed in #349.

Additionally, I enabled a large subset of lints from pedantic that shouldn't be too annoying, but should improve consistency and correctness.

This patchset can be reviewed per-commit, each patch should build correctly. A few things I need feedback on:

Which lints should be enabled? I enabled quite a few that I think should be reasonable, but will adjust as needed
There are some // SAFETY: TODO comments in rust/kernel/init. I'm not too familiar with how this portion - could somebody more familiar to fill in those blanks and generally verify the safety comments in init
Should the patchset be merged or split up more? I'll probably send the first commit separate since that's the only one that touches non-Rust things, but intend to send the others together

Currently, 'rustc_common_flags' lives in the top-level 'Makefile'. This means that we need to touch this whenever adjustments to Rust's CLI configuration are desired, such as changing what lints Clippy should emit. This patch moves these flags to a separate file within 'rust/' so that necessary changes can be made without affecting 'Makefile'. It copies the behavior currently used with 'bindgen_parameters', which accepts comments. Additionally, 'let_unit_value` is no longer specifically named as it is now part of the 'style' group [1]. [1]: rust-lang/rust-clippy#8563 Signed-off-by: Trevor Gross <[email protected]>

This patch selects a subset of lints from Clippy's 'pedantic' group and enables them. The only lints enabled here are those that do not show any errors with any of the files we currently have. Signed-off-by: Trevor Gross <[email protected]>

This patch enables the 'undocumented_unsafe_blocks' lint and adds comments where needed for this lint to pass. Clippy will now deny unsafe code that does not have an associated '// SAFETY: ' comment, which helps improve the maintainability of potentially dangerous code. Signed-off-by: Trevor Gross <[email protected]>

This patch enables three trivial lints and adds corrections so existing code passes. Lints added were: - 'expl_impl_clone_on_copy': check for implementing `Clone` when `Copy` already exists. - 'explicit_deref_methods': check for usage of `.deref()` to encourage cleaner code. - 'trivially_copy_pass_by_ref': check for small types that would be easier passed by value than by reference. Signed-off-by: Trevor Gross <[email protected]>

- 'ptr_as_ptr': enforce that '*const -> *const' and '*mut -> *mut' conversions be done via '.cast()' rather than 'as', to avoid accidental changes in mutability. - 'transmute_ptr_to_ptr': detect transumtes that could be written as lessi dangerous casts. Signed-off-by: Trevor Gross <[email protected]>

- 'manual_assert': detect combined usage of 'if' and 'panic' when 'assert' would have been more straightforward. - 'redundant_else': enforce that blocks are not nested within 'else' after conditional control flow statments. - 'semicolon_if_nothing_returned': enforce consistent style that makes it more clear that a statement is not used. Signed-off-by: Trevor Gross <[email protected]>

…for_...' - 'items_after_statements': prevent confusing scope since items and statements do not follow the same rules. - 'redundant_closure_for_method_calls': simplify closures that call a single method. This is the associated function equivalent of 'redundant_closure'. Signed-off-by: Trevor Gross <[email protected]>

ojeda · 2023-07-16T09:42:44Z

rust/rust_common_flags

+--edition=2021
+-Zbinary_dep_depinfo=y
+
+# Standard rust lints


Perhaps "rustc lints"?

ojeda · 2023-07-16T09:43:24Z

rust/rust_common_flags

+-Dclippy::needless_bitwise_bool
+-Dclippy::needless_continue
+
+# Clippy lints that we only warn on


Ideally, we would explain why we only warn on, but we can do that later.

Well I didn't exactly know the reasoning :) what is it?

For dbg_macro (and todo I guess), it is to avoid annoying people while developing. They could avoid CLIPPY=1, but the intention is that you can use it all the time (except when building production kernels, since Clippy does not guarantee to generate the same code).

Also, perhaps consider if others should be -W too for similar reasons. Or perhaps whether the reasoning is bad to being with! :)

y86-dev · 2023-07-16T09:45:36Z

rust/kernel/init/__internal.rs

@@ -7,6 +7,8 @@
 //! - `macros/pin_data.rs`
 //! - `macros/pinned_drop.rs`

+#![allow(clippy::undocumented_unsafe_blocks)]


I think we should also document these here.

It's only misssing two:

unsafe impl<T: ?Sized> InitData for AllData<T>

and

unsafe impl<T: ?Sized> HasInitData for T

Would you mind providing the comments for them?

y86-dev · 2023-07-16T09:47:42Z

rust/kernel/init.rs

-        // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
-        // get the correct type inference here:
+
+        // SAFETY: Since we are in the `if false` branch, this will never get executed. We abuse
+        // `slot` to get the correct type inference here:


I would leave the original comment and just add SAFETY: this code is never executed.

y86-dev · 2023-07-16T09:48:33Z

rust/kernel/init.rs

+        // SAFETY: forgetting the guard is intentional
        unsafe { $crate::init::__internal::DropGuard::forget($field) };


You can leave these (forgetting guards) with a SAFETY: TODO, since this part will be rewritten in an update soon and then no unsafe will be needed.

y86-dev · 2023-07-16T09:56:55Z

rust/kernel/init.rs

@@ -891,6 +896,7 @@ macro_rules! try_init {
        // no possibility of returning without `unsafe`.
        struct __InitOk;
        // Get the init data from the supplied type.
+        // SAFETY: `HasInitData` is a marker trait which we are allowed to use in this context


Suggested change

// SAFETY: `HasInitData` is a marker trait which we are allowed to use in this context

// SAFETY: we are in one of the macros permitted to call `__init_data()` .

y86-dev · 2023-07-16T09:57:12Z

rust/kernel/init.rs

-        // Since we are in the `if false` branch, this will never get executed. We abuse `slot` to
-        // get the correct type inference here:
+        // SAFETY: Since we are in the `if false` branch, this will never get
+        // executed. We abuse `slot` to get the correct type inference here:


same as above

y86-dev · 2023-07-16T09:57:35Z

rust/kernel/init.rs

+/// # Safety
+///
+/// This can only be used on types that meet `Zeroable`'s guidelines


Suggested change

/// # Safety

///

/// This can only be used on types that meet `Zeroable`'s guidelines

/// # Safety

///

/// This can only be used on types that meet `Zeroable`'s safety contract.

y86-dev · 2023-07-16T09:59:00Z

rust/kernel/init/macros.rs

@@ -974,6 +977,7 @@ macro_rules! __pin_data {
                    slot: *mut $type,
                    init: impl $crate::init::Init<$type, E>,
                ) -> ::core::result::Result<(), E> {
+                    // SAFETY: `slot` is valid per this function's contract


The function has no # Safety section, so it effectively has no contract. I think we should just give it the same contract as Init::__init.

y86-dev · 2023-07-16T10:00:38Z

rust/kernel/init.rs

@@ -621,6 +621,7 @@ macro_rules! try_pin_init {
        // no possibility of returning without `unsafe`.
        struct __InitOk;
        // Get the pin data from the supplied type.
+        // SAFETY: TODO


Suggested change

// SAFETY: TODO

// SAFETY: we are in one of the macros permitted to call `__pin_data()` .

ojeda · 2023-07-16T10:14:07Z

rust/rust_common_flags

+# FIXME: enable this at the next version bump. Disabled because of false
+# positive in macros: https://github.com/rust-lang/rust-clippy/issues/10084
+# -Dclippy::unnecessary_safety_comment


I would explicitly mention the version it has started working instead; or otherwise not mention the FIXME part if we don't know.

(We are upgrading to 1.70.0/1.71.0 soon)

Also, this should probably be in another commit.

ojeda · 2023-07-16T10:16:26Z

This patch moves these flags to a separate file within 'rust/' so that necessary changes can be made without affecting 'Makefile'.

I would add a note why this would be desirable, e.g. to be cleaner and/or because your later goal is to add new lints or to split the groups into individual lints which would take quite a bit of space.

Also, please use include $(srctree)/scripts/Makefile.rust if possible.

Additionally, 'let_unit_value` is no longer specifically named as it is now part of the 'style' group [1].

This should be a separate commit.

Should the patchset be merged or split up more? I'll probably send the first commit separate since that's the only one that touches non-Rust things, but intend to send the others together

Yeah, please split them up more. You can split the move into a first patch, which allows to trivially check that the contents are the same with e.g. Git's --color-moved. Then you can do incremental changes in the new file, like the removal of let_unit_value.

Similarly, the commits like rust: clippy: enable 'undocumented_unsafe_blocks' lint should be either split into individual lints (if they require few / trivial changes to the code), so that they are as minimal as possible; or they should be split into commits that clean the code first before enabling the lint (if they are non-trivial changes).

Which lints should be enabled? I enabled quite a few that I think should be reasonable, but will adjust as needed

Hard to say, but you are definitely enabling a lot of them! :)

For the "pedantic" group ones, perhaps you can try to enable them in the rust branch, to see how they would behave for a bigger amount of Rust kernel code, because we don't have much so far, and perhaps there are some that would already conflict.

Also, I think it would be good to understand why they are not already part of other Clippy groups already, i.e. why are they considered pedantic? Are they controversial? Do they conflict with rustfmt? Do they have corner cases? ...?

ojeda · 2023-07-16T10:28:57Z

rust/rust_common_flags

+-Wmissing_docs
+-Drustdoc::missing_crate_level_docs
+
+# Clippy groups


A different patch series should split these into individual lints, unless there is a way to prevent Clippy from breaking compilation due to newer lints.

Perhaps we should enable individual them with -D, and keep the groups with -W? That way the new ones would be warnings only (assuming the flags work that way).

tgross35 · 2023-07-17T07:32:24Z

I split off the first commit here #1026, I think I will get that prepared to send to the list before I refactor this PR too much.

Thanks for all the suggestions - after the Makefile is a bit more figured out, I will figure out how best to split this up and make the requested changes.

For the "pedantic" group ones, perhaps you can try to enable them in the rust branch, to see how they would behave for a bigger amount of Rust kernel code, because we don't have much so far, and perhaps there are some that would already conflict.

Excellent thought, I will try that out.

Also, I think it would be good to understand why they are not already part of other Clippy groups already, i.e. why are they considered pedantic? Are they controversial? Do they conflict with rustfmt? Do they have corner cases? ...?

If I recall correctly, they're mostly lints that can be noisy or have more false positives. Usually I compile all my code with pedantic - there are quite a few lints that make for more readable code after you've done a lot of refactoring (flat_map_option, manual_ok_or, borrow_as_ptr ) and some things that can be easy footguns (cast_possible_truncation, float_cmp). When I do the updates, I'll try to add justification for why I applied the ones I did.

I have also considered whether it might be good to have separate sets of lints somehow, so that e.g. rust/kernel would be checked to the most strict standards but everything else would use the standard set of lints unless they opt in (our own "safety levels") . I know some early discussion (like here) there was a lot of talk about (1) not allowing panics, and (2) avoiding the bugs of wraparounds. These things aren't 100% avoidable but lints like arithmetic_side_effects cast_possible_truncation cast_lossless, panic, cast_possible_wrap, cast_precision_loss, and cast_sign_loss (some restriction some pedantic) go an extremely long way toward preventing these things. They would be noisy now... but I wonder if some form of those very restrictive lints may be worth it for the most critical components. Not something that has to be decided here of course, just food for thought.

ojeda · 2023-07-17T11:15:59Z

I have also considered whether it might be good to have separate sets of lints somehow, so that e.g. rust/kernel would be checked to the most strict standards but everything else would use the standard set of lints unless they opt in (our own "safety levels")

I think it is best that, as much as possible, the majority of the kernel code uses the same set. The code is not yet written (or, for some of it, merged), so we are in the best situation possible to enforce these things (assuming they make sense and are not overly strict, of course).

There may be some differences for e.g. host programs vs. kernel code, or for vendored code, of course. But apart from that, we should try to make the style (and the lints, in a sense) as consistent as possible.

(1) not allowing panics, and (2) avoiding the bugs of wraparounds. These things aren't 100% avoidable but lints like arithmetic_side_effects cast_possible_truncation cast_lossless, panic, cast_possible_wrap, cast_precision_loss, and cast_sign_loss (some restriction some pedantic) go an extremely long way toward preventing these things. They would be noisy now... but I wonder if some form of those very restrictive lints may be worth it for the most critical components

I think, in general, if a lint is worth it for core components, then they are likely worth it for everything. In fact, the inverted situation can also happen: that some lints are worth it only for non-core components.

For instance, consider the panic lint. If we do have a panic! macro invocation in the kernel crate, then it must be for very good reason. Thus having the lint enabled means we will likely have to add a local allow() for each instance of panic!, which may not be too useful unless it states a reason. However, for e.g. drivers (considering out-of-tree ones too), we may want to lint it, because they really should not be doing it.

Having said that, for core code, even if we don't lint, what we may want is to require certain explanations to explain why something is OK, i.e. rather than disallow it. We have a couple CAST comments currently, which were the start of that idea, following the approach of the SAFETY ones. We could also have a PANIC one, applied to .unwrap(), panic!, assert! and so on; and then a lint to check we are not missing them.

In other words, instead of having, say, the unwrap_unused lint enabled, we would enable one that requires a PANIC comment to explain the unwrap(). It could thought of as a generalization of undocumented_unsafe_blocks -- so perhaps I should add it to rust-lang/rust-clippy#8031.

Having said that, this overlaps with the unstable lint_reasons, i.e. reason = "..." in #[allow(...)] and friends. I don't particularly like having to write the reasons inside a string inside an attribute, though, since it is really intended mostly as a comment for readers of the code. For instance, would we want

#[allow(unsafe_code, reason = "...")]
unsafe { ... }

vs.

// SAFETY: ...
unsafe { ... }

? (in some cases, multiline with lists etc.).

And for macro invocations like panic!, will #[allow(...)] work? It seems currently we would need to wrap it:

#[allow(clippy::panic, reason = "...")]
{ panic!(); }

vs.

// PANIC: ...
panic!();

Perhaps some food for thought for rust-lang/rust#54503 -- I see the original RFC had some thoughts about comments in https://github.com/rust-lang/rfcs/blob/master/text/2383-lint-reasons.md#optional--yet-another-comment-syntax, but they are more about writing longer-form comments for a given attribute, rather than having a comment be interpreted as the allow itself, which is what PANIC and SAFETY comments are, in a sense.

fbq · 2023-07-18T00:02:21Z

rust/kernel/sync/arc.rs

@@ -610,24 +610,24 @@ impl<T: ?Sized> DerefMut for UniqueArc<T> {

 impl<T: fmt::Display + ?Sized> fmt::Display for UniqueArc<T> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        fmt::Display::fmt(self.deref(), f)
+        fmt::Display::fmt(&self, f)


Hmm.. I don't think this is an improve, &self is of type &&UniqueArc<T>, it's not very straight-forwards why does the compiler coerce it to a &T. We could do the same as std:

fmt::Display::fmt(&**self, f);

But honestly, I don't think it's better.. could you explain what problem the old code has?

There is no particular problem other than keeping consistency, this comes from the explicit_deref_methods lint. I suspect that this is why std does what it does (I neglected to check what they do before applying this).

I am indifferent about this lint and agree that there are many times where it makes more sense to be explicit, so I don't mind dropping it. However, even unrelated to the lint, maybe it would be good to match what std does, just to avoid any possible confusion when syncing std -> kernel

tgross35 added 7 commits July 16, 2023 02:25

ojeda reviewed Jul 16, 2023

View reviewed changes

rust/rust_common_flags

--edition=2021

-Zbinary_dep_depinfo=y

# Standard rust lints

Copy link

Member

ojeda Jul 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps "rustc lints"?

ojeda reviewed Jul 16, 2023

View reviewed changes

y86-dev reviewed Jul 16, 2023

View reviewed changes

ojeda reviewed Jul 16, 2023

View reviewed changes

tgross35 mentioned this pull request Jul 17, 2023

Split Rust arguments into a new scripts/Makefile.rust #1026

Closed

fbq reviewed Jul 18, 2023

View reviewed changes

ojeda force-pushed the rust-next branch from bee23a1 to 341faf2 Compare July 19, 2023 18:06

ojeda force-pushed the rust-next branch from bd83e43 to 49a9ef7 Compare August 7, 2023 09:39

ojeda force-pushed the rust-next branch from 71004ce to 4af84c6 Compare August 21, 2023 12:32

ojeda force-pushed the rust-next branch from 2392e2a to e3c3d34 Compare February 18, 2024 20:22

ojeda force-pushed the rust-next branch from a419846 to 6d1c22d Compare August 21, 2024 22:16

ojeda force-pushed the rust-next branch 3 times, most recently from c9b5ce6 to ce1c54f Compare October 9, 2024 22:37

ojeda force-pushed the rust-next branch 3 times, most recently from 9ee7197 to 6ce162a Compare October 15, 2024 21:11

ojeda force-pushed the rust-next branch from 0675624 to d072acd Compare November 11, 2024 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable various new clippy lints #1025

Enable various new clippy lints #1025

tgross35 commented Jul 16, 2023

ojeda Jul 16, 2023

ojeda Jul 16, 2023

tgross35 Jul 16, 2023

ojeda Jul 16, 2023

y86-dev Jul 16, 2023

tgross35 Jul 16, 2023 •

edited

Loading

y86-dev Jul 16, 2023

y86-dev Jul 16, 2023

y86-dev Jul 16, 2023

y86-dev Jul 16, 2023

y86-dev Jul 16, 2023

y86-dev Jul 16, 2023

y86-dev Jul 16, 2023

ojeda Jul 16, 2023

ojeda commented Jul 16, 2023

ojeda Jul 16, 2023

tgross35 commented Jul 17, 2023

ojeda commented Jul 17, 2023 •

edited

Loading

fbq Jul 18, 2023

tgross35 Jul 18, 2023

		// SAFETY: forgetting the guard is intentional
		unsafe { $crate::init::__internal::DropGuard::forget($field) };

	// SAFETY: `HasInitData` is a marker trait which we are allowed to use in this context
	// SAFETY: we are in one of the macros permitted to call `__init_data()` .

	// SAFETY: TODO
	// SAFETY: we are in one of the macros permitted to call `__pin_data()` .

Enable various new clippy lints #1025

Are you sure you want to change the base?

Enable various new clippy lints #1025

Conversation

tgross35 commented Jul 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojeda commented Jul 16, 2023

Choose a reason for hiding this comment

tgross35 commented Jul 17, 2023

ojeda commented Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 Jul 16, 2023 •

edited

Loading

ojeda commented Jul 17, 2023 •

edited

Loading