[PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Miguel Ojeda]

Starting with the upcoming Rust 1.86.0, Clippy's `needless_continue` lint
complains about the last statement of a loop [1], including cases like:

    while ... {
        match ... {
            ... if ... => {
                ...
                return ...;
            }
            _ => continue,
        }
    }

as well as nested `match`es in a loop.

Thus clean them up.

Link: https://github.com/rust-lang/rust-clippy/pull/13891 [1]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
To be honest, I am not sure we want this.

The lint can find cases that should be simplified, and it has been a nice lint
so far, but somehow I feel that using `continue` shows the intent better when
it is alone in an arm like that, and I am not sure we want to force people to
try to find other ways to write the code either, in cases when that applies.

If others feel this reads worse, then I would be happy to disable the lint and
open an issue upstream to keep the cases that are more clear cut.

 rust/macros/helpers.rs | 2 +-
 rust/macros/kunit.rs   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
index a3ee27e29a6f..bfa3aa7441d2 100644
--- a/rust/macros/helpers.rs
+++ b/rust/macros/helpers.rs
@@ -81,7 +81,7 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
                 }
                 return None;
             }
-            _ => continue,
+            _ => (),
         }
     }
     None
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
index 4f553ecf40c0..63f79e5ac290 100644
--- a/rust/macros/kunit.rs
+++ b/rust/macros/kunit.rs
@@ -54,7 +54,7 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
                     };
                     tests.push(test_name);
                 }
-                _ => continue,
+                _ => (),
             },
             _ => (),
         }

base-commit: 08733088b566b58283f0f12fb73f5db6a9a9de30
--
2.49.0

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Benno Lossin]

On Wed Apr 2, 2025 at 12:12 AM CEST, Miguel Ojeda wrote:
> Starting with the upcoming Rust 1.86.0, Clippy's `needless_continue` lint
> complains about the last statement of a loop [1], including cases like:
>
>     while ... {
>         match ... {
>             ... if ... => {
>                 ...
>                 return ...;
>             }
>             _ => continue,
>         }
>     }
>
> as well as nested `match`es in a loop.
>
> Thus clean them up.
>
> Link: https://github.com/rust-lang/rust-clippy/pull/13891 [1]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
> To be honest, I am not sure we want this.
>
> The lint can find cases that should be simplified, and it has been a nice lint
> so far, but somehow I feel that using `continue` shows the intent better when
> it is alone in an arm like that, and I am not sure we want to force people to
> try to find other ways to write the code either, in cases when that applies.
>
> If others feel this reads worse, then I would be happy to disable the lint and
> open an issue upstream to keep the cases that are more clear cut.

I'm not too sure about this change, if the loop is longer than one
screen, it makes a lot of sense to have a `continue` instead of `()`,
since one might not see that there is nothing after the `match`.

I also think that an explicit `continue` is nicer from a expressability
standpoint. So I think we should keep them.

---
Cheers,
Benno

>  rust/macros/helpers.rs | 2 +-
>  rust/macros/kunit.rs   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Miguel Ojeda]

On Wed, Apr 2, 2025 at 1:53 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> I'm not too sure about this change, if the loop is longer than one
> screen, it makes a lot of sense to have a `continue` instead of `()`,
> since one might not see that there is nothing after the `match`.
>
> I also think that an explicit `continue` is nicer from a expressability
> standpoint. So I think we should keep them.

Yeah, I feel similarly. It is sad because the lint did find other
cases in the past that were useful.

If nobody else cares about keeping this one enabled, then I will send
the alternative patch. Hopefully we can re-enable it at some point in
the future.

Cheers,
Miguel

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Tamir Duberstein]

On Wed, Apr 2, 2025 at 7:15 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Apr 2, 2025 at 1:53 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > I'm not too sure about this change, if the loop is longer than one
> > screen, it makes a lot of sense to have a `continue` instead of `()`,
> > since one might not see that there is nothing after the `match`.
> >
> > I also think that an explicit `continue` is nicer from a expressability
> > standpoint. So I think we should keep them.
>
> Yeah, I feel similarly. It is sad because the lint did find other
> cases in the past that were useful.
>
> If nobody else cares about keeping this one enabled, then I will send
> the alternative patch. Hopefully we can re-enable it at some point in
> the future.

Rather than disabling globally, why not `#[expect]` these instances
with a reason?

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Miguel Ojeda]

On Wed, Apr 2, 2025 at 3:59 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Rather than disabling globally, why not `#[expect]` these instances
> with a reason?

That is an option sometimes, yeah, but in this case, writing those
lines is also a burden -- one that is, I would say, worse than just
using `()`.

It would also need to be `allow` here, not `expect`, because older
versions do not complain, which makes it even worse...

So it is all about what a lint gives us in exchange of those false
positives, and about how much time people would need to spend on it. I
have always supported adding lints (I think I added this one, long
ago, in fact), but I don't want that we overdo it either, so I am
happy disabling it if it is going to be too painful.

Cheers,
Miguel

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Tamir Duberstein]

On Wed, Apr 2, 2025 at 11:27 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Apr 2, 2025 at 3:59 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Rather than disabling globally, why not `#[expect]` these instances
> > with a reason?
>
> That is an option sometimes, yeah, but in this case, writing those
> lines is also a burden -- one that is, I would say, worse than just
> using `()`.
>
> It would also need to be `allow` here, not `expect`, because older
> versions do not complain, which makes it even worse...

👍

> So it is all about what a lint gives us in exchange of those false
> positives, and about how much time people would need to spend on it. I
> have always supported adding lints (I think I added this one, long
> ago, in fact), but I don't want that we overdo it either, so I am
> happy disabling it if it is going to be too painful.

The counterfactual is hard to prove - you don't know what true
positives the lint would catch. In my opinion disabling lints is
risking throwing the baby out with the bathwater.

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Miguel Ojeda]

On Wed, Apr 2, 2025 at 6:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> The counterfactual is hard to prove - you don't know what true
> positives the lint would catch. In my opinion disabling lints is
> risking throwing the baby out with the bathwater.

It is true that it is not easy to know what we will exactly lose right
now (apart from what it claims in the docs and its examples), but one
can easily test enabling it in a couple cycles and we would have some
data from kernel code.

To be clear, disabling now does not mean forever -- we can reevaluate
with that data and possible improvements to the lint that happened
meanwhile (sometimes they get improved or split due to feedback).

By the way, the lint is in "pedantic" in Clippy and disabled by
default -- so we are "only" disabling w.r.t. what we do nowadays.

In any case, my main concern is cost: we already require a lot from
Rust kernel developers, typically more than in C, and while one goal
of the project is trying to see how far we can go on being strict
about things like lints, I worry we may overdo it at times.

Cheers,
Miguel

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Tamir Duberstein]

On Wed, Apr 2, 2025 at 4:29 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Apr 2, 2025 at 6:41 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > The counterfactual is hard to prove - you don't know what true
> > positives the lint would catch. In my opinion disabling lints is
> > risking throwing the baby out with the bathwater.
>
> It is true that it is not easy to know what we will exactly lose right
> now (apart from what it claims in the docs and its examples), but one
> can easily test enabling it in a couple cycles and we would have some
> data from kernel code.
>
> To be clear, disabling now does not mean forever -- we can reevaluate
> with that data and possible improvements to the lint that happened
> meanwhile (sometimes they get improved or split due to feedback).

My experience in industrial settings is that this happens very rarely.

> By the way, the lint is in "pedantic" in Clippy and disabled by
> default -- so we are "only" disabling w.r.t. what we do nowadays.

> In any case, my main concern is cost: we already require a lot from
> Rust kernel developers, typically more than in C, and while one goal
> of the project is trying to see how far we can go on being strict
> about things like lints, I worry we may overdo it at times.

👍

I don't have any new information to inject into this debate. It's a
judgement call.

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Miguel Ojeda]

On Wed, Apr 2, 2025 at 11:09 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> My experience in industrial settings is that this happens very rarely.

We enable lints from time to time -- this would be no different.

> 👍
>
> I don't have any new information to inject into this debate. It's a
> judgement call.

Thanks for the feedback, it was useful.

I submitted a new patch to disable it and sent the feedback upstream.

Cheers,
Miguel

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Danilo Krummrich]

On Wed, Apr 02, 2025 at 12:12:05AM +0200, Miguel Ojeda wrote:
> Starting with the upcoming Rust 1.86.0, Clippy's `needless_continue` lint
> complains about the last statement of a loop [1], including cases like:

Not related to the patch itself: Don't we need to disable new lints anyways?
Otherwise we'd get warning when compiling older kernel with newer compilers /
linters, no?

Re: [PATCH] rust: clean Rust 1.86.0 new `clippy::needless_continue` cases

[Miguel Ojeda]

On Wed, Apr 2, 2025 at 11:18 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Not related to the patch itself: Don't we need to disable new lints anyways?
> Otherwise we'd get warning when compiling older kernel with newer compilers /
> linters, no?

So far we are cleaning them up as they arrive (and backporting) and
deciding if we want to keep them or not etc. My plan is to eventually
have an explicit set of the ones we really, really want to keep clean,
since at some point it will be too much to clean (if the rate of
changes on Clippy is steady), and leave the rest to something like
`W=`.

In any case, `CLIPPY=1` is opt-in and the future can always give us
issues: even when we set an explicit list, sometimes lints have added
new cases (like in this patch), so we will need cleanups from time to
time either way.

Relatedly, we discussed at FOSDEM having Clippy be potentially
unconditionally used, but that would require changes on their side
first.

Cheers,
Miguel