[PATCH 12/19] rust: replace `clippy::dbg_macro` with `disallowed_macros`

Miguel Ojeda posted 19 patches 1 year, 3 months ago
[PATCH 12/19] rust: replace `clippy::dbg_macro` with `disallowed_macros`
Posted by Miguel Ojeda 1 year, 3 months ago
Back 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.

Link: https://github.com/rust-lang/rust-clippy/issues/11303 [1]
Link: https://github.com/rust-lang/rust-clippy/issues/11307 [2]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 .clippy.toml               |  6 ++++++
 Makefile                   |  1 -
 rust/kernel/std_vendor.rs  | 10 +++++-----
 samples/rust/rust_print.rs |  1 +
 4 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/.clippy.toml b/.clippy.toml
index f66554cd5c45..ad9f804fb677 100644
--- a/.clippy.toml
+++ b/.clippy.toml
@@ -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" },
+]
diff --git a/Makefile b/Makefile
index 234ab97de796..f236dd5fb6d9 100644
--- a/Makefile
+++ b/Makefile
@@ -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 \
diff --git a/rust/kernel/std_vendor.rs b/rust/kernel/std_vendor.rs
index 67bf9d37ddb5..085b23312c65 100644
--- a/rust/kernel/std_vendor.rs
+++ b/rust/kernel/std_vendor.rs
@@ -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
diff --git a/samples/rust/rust_print.rs b/samples/rust/rust_print.rs
index 6eabb0d79ea3..ed1137ab2018 100644
--- a/samples/rust/rust_print.rs
+++ b/samples/rust/rust_print.rs
@@ -15,6 +15,7 @@
 
 struct RustPrint;
 
+#[allow(clippy::disallowed_macros)]
 fn arc_print() -> Result {
     use kernel::sync::*;
 
-- 
2.46.0
Re: [PATCH 12/19] rust: replace `clippy::dbg_macro` with `disallowed_macros`
Posted by Geert Stappers 1 year, 3 months ago
On Wed, Sep 04, 2024 at 10:43:40PM +0200, Miguel Ojeda wrote:
> diff --git a/.clippy.toml b/.clippy.toml
> index f66554cd5c45..ad9f804fb677 100644
> --- a/.clippy.toml
> +++ b/.clippy.toml
> @@ -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" },
> +]
> diff --git a/rust/kernel/std_vendor.rs b/rust/kernel/std_vendor.rs
> index 67bf9d37ddb5..085b23312c65 100644
> --- a/rust/kernel/std_vendor.rs
> +++ b/rust/kernel/std_vendor.rs
> @@ -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):
>  ///
>  /// ```
> -/// # #[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.


> @@ -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
-- 
Silence is hard to parse
Re: [PATCH 12/19] rust: replace `clippy::dbg_macro` with `disallowed_macros`
Posted by Gary Guo 1 year, 3 months ago
On Thu, 5 Sep 2024 07:20:46 +0200
Geert Stappers <stappers@stappers.nl> wrote:

> On Wed, Sep 04, 2024 at 10:43:40PM +0200, Miguel Ojeda wrote:
> > diff --git a/.clippy.toml b/.clippy.toml
> > index f66554cd5c45..ad9f804fb677 100644
> > --- a/.clippy.toml
> > +++ b/.clippy.toml
> > @@ -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" },
> > +]
> > diff --git a/rust/kernel/std_vendor.rs b/rust/kernel/std_vendor.rs
> > index 67bf9d37ddb5..085b23312c65 100644
> > --- a/rust/kernel/std_vendor.rs
> > +++ b/rust/kernel/std_vendor.rs
> > @@ -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):
> >  ///
> >  /// ```
> > -/// # #[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

> 
> 
> > @@ -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`
Posted by Miguel Ojeda 1 year, 2 months ago
On Sun, Sep 15, 2024 at 1:10 AM Gary Guo <gary@garyguo.net> wrote:
>
> `#[]` 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