Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable various new clippy lints #1025

Open
wants to merge 7 commits into
base: rust-next
Choose a base branch
from

Conversation

tgross35
Copy link
Collaborator

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:

  1. Which lints should be enabled? I enabled quite a few that I think should be reasonable, but will adjust as needed
  2. 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
  3. 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

tgross35 added 7 commits July 16, 2023 02:25
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 <tmgross@umich.edu>
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 <tmgross@umich.edu>
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 <tmgross@umich.edu>
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 <tmgross@umich.edu>
-   '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 <tmgross@umich.edu>
-   '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 <tmgross@umich.edu>
…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 <tmgross@umich.edu>
--edition=2021
-Zbinary_dep_depinfo=y

# Standard rust lints
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "rustc lints"?

-Dclippy::needless_bitwise_bool
-Dclippy::needless_continue

# Clippy lints that we only warn on
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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! :)

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

#![allow(clippy::undocumented_unsafe_blocks)]
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also document these here.

Copy link
Collaborator Author

@tgross35 tgross35 Jul 16, 2023

Choose a reason for hiding this comment

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

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?

Comment on lines -738 to +742
// 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:
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines +783 to 784
// SAFETY: forgetting the guard is intentional
unsafe { $crate::init::__internal::DropGuard::forget($field) };
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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()` .

Comment on lines -1002 to +1010
// 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:
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Comment on lines +1384 to +1386
/// # Safety
///
/// This can only be used on types that meet `Zeroable`'s guidelines
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines +73 to +75
# 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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

ojeda commented Jul 16, 2023

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? ...?

-Wmissing_docs
-Drustdoc::missing_crate_level_docs

# Clippy groups
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Collaborator Author

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
Copy link
Member

ojeda commented Jul 17, 2023

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants