[PATCH 1/3] rust: use the `build_error!` macro, not the hidden function

[PATCH 1/3] rust: use the `build_error!` macro, not the hidden function

[Miguel Ojeda]

Code and some examples were using the function, rather than the macro. The
macro is what is documented.

Thus move users to the macro.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/block/mq/operations.rs |  2 +-
 rust/kernel/net/phy.rs             | 18 +++++++++---------
 rust/macros/lib.rs                 |  8 ++++----
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
index c8646d0d9866..962f16a5a530 100644
--- a/rust/kernel/block/mq/operations.rs
+++ b/rust/kernel/block/mq/operations.rs
@@ -35,7 +35,7 @@ pub trait Operations: Sized {
     /// Called by the kernel to poll the device for completed requests. Only
     /// used for poll queues.
     fn poll() -> bool {
-        crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
+        crate::build_error!(crate::error::VTABLE_DEFAULT_ERROR)
     }
 }
 
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index beb62ec712c3..f488f6c55e9a 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -587,17 +587,17 @@ pub trait Driver {
 
     /// Issues a PHY software reset.
     fn soft_reset(_dev: &mut Device) -> Result {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Sets up device-specific structures during discovery.
     fn probe(_dev: &mut Device) -> Result {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Probes the hardware to determine what abilities it has.
     fn get_features(_dev: &mut Device) -> Result {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Returns true if this is a suitable driver for the given phydev.
@@ -609,32 +609,32 @@ fn match_phy_device(_dev: &Device) -> bool {
     /// Configures the advertisement and resets auto-negotiation
     /// if auto-negotiation is enabled.
     fn config_aneg(_dev: &mut Device) -> Result {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Determines the negotiated speed and duplex.
     fn read_status(_dev: &mut Device) -> Result<u16> {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Suspends the hardware, saving state if needed.
     fn suspend(_dev: &mut Device) -> Result {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Resumes the hardware, restoring state if needed.
     fn resume(_dev: &mut Device) -> Result {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Overrides the default MMD read function for reading a MMD register.
     fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Overrides the default MMD write function for writing a MMD register.
     fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
-        kernel::build_error(VTABLE_DEFAULT_ERROR)
+        kernel::build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Callback for notification of link change.
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index 4ab94e44adfe..1a30c8075ebd 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -123,12 +123,12 @@ pub fn module(ts: TokenStream) -> TokenStream {
 /// used on the Rust side, it should not be possible to call the default
 /// implementation. This is done to ensure that we call the vtable methods
 /// through the C vtable, and not through the Rust vtable. Therefore, the
-/// default implementation should call `kernel::build_error`, which prevents
+/// default implementation should call `kernel::build_error!`, which prevents
 /// calls to this function at compile time:
 ///
 /// ```compile_fail
 /// # // Intentionally missing `use`s to simplify `rusttest`.
-/// kernel::build_error(VTABLE_DEFAULT_ERROR)
+/// kernel::build_error!(VTABLE_DEFAULT_ERROR)
 /// ```
 ///
 /// Note that you might need to import [`kernel::error::VTABLE_DEFAULT_ERROR`].
@@ -145,11 +145,11 @@ pub fn module(ts: TokenStream) -> TokenStream {
 /// #[vtable]
 /// pub trait Operations: Send + Sync + Sized {
 ///     fn foo(&self) -> Result<()> {
-///         kernel::build_error(VTABLE_DEFAULT_ERROR)
+///         kernel::build_error!(VTABLE_DEFAULT_ERROR)
 ///     }
 ///
 ///     fn bar(&self) -> Result<()> {
-///         kernel::build_error(VTABLE_DEFAULT_ERROR)
+///         kernel::build_error!(VTABLE_DEFAULT_ERROR)
 ///     }
 /// }
 ///

base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
-- 
2.47.0

[PATCH 2/3] rust: kernel: move `build_error` hidden function to prevent mistakes

[Miguel Ojeda]

Users were using the hidden exported `kernel::build_error` function
instead of the intended `kernel::build_error!` macro, e.g. see the
previous commit.

To force to use the macro, move it into the `build_assert` module,
thus making it a compilation error and avoiding a collision in the same
"namespace". Using the function now would require typing the module name
(which is hidden), not just a single character.

Now attempting to use the function will trigger this error with the
right suggestion by the compiler:

      error[E0423]: expected function, found macro `kernel::build_error`
      --> samples/rust/rust_minimal.rs:29:9
         |
      29 |         kernel::build_error();
         |         ^^^^^^^^^^^^^^^^^^^ not a function
         |
      help: use `!` to invoke the macro
         |
      29 |         kernel::build_error!();
         |                            +

An alternative would be using an alias, but it would be more complex
and moving it into the module seems right since it belongs there and
reduces the amount of code at the crate root.

Keep the `#[doc(hidden)]` inside `build_assert` in case the module is
not hidden in the future.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/build_assert.rs | 11 +++++++----
 rust/kernel/lib.rs          |  6 ++----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/build_assert.rs b/rust/kernel/build_assert.rs
index 9e37120bc69c..347ba5ce50f4 100644
--- a/rust/kernel/build_assert.rs
+++ b/rust/kernel/build_assert.rs
@@ -2,6 +2,9 @@
 
 //! Build-time assert.
 
+#[doc(hidden)]
+pub use build_error::build_error;
+
 /// Fails the build if the code path calling `build_error!` can possibly be executed.
 ///
 /// If the macro is executed in const context, `build_error!` will panic.
@@ -23,10 +26,10 @@
 #[macro_export]
 macro_rules! build_error {
     () => {{
-        $crate::build_error("")
+        $crate::build_assert::build_error("")
     }};
     ($msg:expr) => {{
-        $crate::build_error($msg)
+        $crate::build_assert::build_error($msg)
     }};
 }
 
@@ -73,12 +76,12 @@ macro_rules! build_error {
 macro_rules! build_assert {
     ($cond:expr $(,)?) => {{
         if !$cond {
-            $crate::build_error(concat!("assertion failed: ", stringify!($cond)));
+            $crate::build_assert::build_error(concat!("assertion failed: ", stringify!($cond)));
         }
     }};
     ($cond:expr, $msg:expr) => {{
         if !$cond {
-            $crate::build_error($msg);
+            $crate::build_assert::build_error($msg);
         }
     }};
 }
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index bf8d7f841f94..73e33a41ea04 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -32,7 +32,8 @@
 pub mod alloc;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
-mod build_assert;
+#[doc(hidden)]
+pub mod build_assert;
 pub mod device;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
@@ -66,9 +67,6 @@
 pub use macros;
 pub use uapi;
 
-#[doc(hidden)]
-pub use build_error::build_error;
-
 /// Prefix to appear before log messages printed from within the `kernel` crate.
 const __LOG_PREFIX: &[u8] = b"rust_kernel\0";
 
-- 
2.47.0

[PATCH 3/3] rust: add `build_error!` to the prelude

[Miguel Ojeda]

The sibling `build_assert!` is already in the prelude, it makes sense
that a "core"/"language" facility like this is part of the prelude and
users should not be defining their own one (thus there should be no risk
of future name collisions and we would want to be aware of them anyway).

Thus add `build_error!` into the prelude.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 rust/kernel/block/mq/operations.rs |  3 ++-
 rust/kernel/build_assert.rs        |  1 -
 rust/kernel/net/phy.rs             | 18 +++++++++---------
 rust/kernel/prelude.rs             |  2 +-
 rust/macros/lib.rs                 |  8 ++++----
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs
index 962f16a5a530..864ff379dc91 100644
--- a/rust/kernel/block/mq/operations.rs
+++ b/rust/kernel/block/mq/operations.rs
@@ -9,6 +9,7 @@
     block::mq::request::RequestDataWrapper,
     block::mq::Request,
     error::{from_result, Result},
+    prelude::*,
     types::ARef,
 };
 use core::{marker::PhantomData, sync::atomic::AtomicU64, sync::atomic::Ordering};
@@ -35,7 +36,7 @@ pub trait Operations: Sized {
     /// Called by the kernel to poll the device for completed requests. Only
     /// used for poll queues.
     fn poll() -> bool {
-        crate::build_error!(crate::error::VTABLE_DEFAULT_ERROR)
+        build_error!(crate::error::VTABLE_DEFAULT_ERROR)
     }
 }
 
diff --git a/rust/kernel/build_assert.rs b/rust/kernel/build_assert.rs
index 347ba5ce50f4..6331b15d7c4d 100644
--- a/rust/kernel/build_assert.rs
+++ b/rust/kernel/build_assert.rs
@@ -14,7 +14,6 @@
 /// # Examples
 ///
 /// ```
-/// # use kernel::build_error;
 /// #[inline]
 /// fn foo(a: usize) -> usize {
 ///     a.checked_add(1).unwrap_or_else(|| build_error!("overflow"))
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index f488f6c55e9a..654311a783e9 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -587,17 +587,17 @@ pub trait Driver {
 
     /// Issues a PHY software reset.
     fn soft_reset(_dev: &mut Device) -> Result {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Sets up device-specific structures during discovery.
     fn probe(_dev: &mut Device) -> Result {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Probes the hardware to determine what abilities it has.
     fn get_features(_dev: &mut Device) -> Result {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Returns true if this is a suitable driver for the given phydev.
@@ -609,32 +609,32 @@ fn match_phy_device(_dev: &Device) -> bool {
     /// Configures the advertisement and resets auto-negotiation
     /// if auto-negotiation is enabled.
     fn config_aneg(_dev: &mut Device) -> Result {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Determines the negotiated speed and duplex.
     fn read_status(_dev: &mut Device) -> Result<u16> {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Suspends the hardware, saving state if needed.
     fn suspend(_dev: &mut Device) -> Result {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Resumes the hardware, restoring state if needed.
     fn resume(_dev: &mut Device) -> Result {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Overrides the default MMD read function for reading a MMD register.
     fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Overrides the default MMD write function for writing a MMD register.
     fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
-        kernel::build_error!(VTABLE_DEFAULT_ERROR)
+        build_error!(VTABLE_DEFAULT_ERROR)
     }
 
     /// Callback for notification of link change.
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index 8bdab9aa0d16..ed076b3062ae 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -19,7 +19,7 @@
 #[doc(no_inline)]
 pub use macros::{module, pin_data, pinned_drop, vtable, Zeroable};
 
-pub use super::build_assert;
+pub use super::{build_assert, build_error};
 
 // `super::std_vendor` is hidden, which makes the macro inline for some reason.
 #[doc(no_inline)]
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index 1a30c8075ebd..d61bc6a56425 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -123,12 +123,12 @@ pub fn module(ts: TokenStream) -> TokenStream {
 /// used on the Rust side, it should not be possible to call the default
 /// implementation. This is done to ensure that we call the vtable methods
 /// through the C vtable, and not through the Rust vtable. Therefore, the
-/// default implementation should call `kernel::build_error!`, which prevents
+/// default implementation should call `build_error!`, which prevents
 /// calls to this function at compile time:
 ///
 /// ```compile_fail
 /// # // Intentionally missing `use`s to simplify `rusttest`.
-/// kernel::build_error!(VTABLE_DEFAULT_ERROR)
+/// build_error!(VTABLE_DEFAULT_ERROR)
 /// ```
 ///
 /// Note that you might need to import [`kernel::error::VTABLE_DEFAULT_ERROR`].
@@ -145,11 +145,11 @@ pub fn module(ts: TokenStream) -> TokenStream {
 /// #[vtable]
 /// pub trait Operations: Send + Sync + Sized {
 ///     fn foo(&self) -> Result<()> {
-///         kernel::build_error!(VTABLE_DEFAULT_ERROR)
+///         build_error!(VTABLE_DEFAULT_ERROR)
 ///     }
 ///
 ///     fn bar(&self) -> Result<()> {
-///         kernel::build_error!(VTABLE_DEFAULT_ERROR)
+///         build_error!(VTABLE_DEFAULT_ERROR)
 ///     }
 /// }
 ///
-- 
2.47.0

Re: [PATCH 1/3] rust: use the `build_error!` macro, not the hidden function

[Alice Ryhl]

On Sat, Nov 23, 2024 at 11:29 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Code and some examples were using the function, rather than the macro. The
> macro is what is documented.
>
> Thus move users to the macro.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

I've introduced a few more instances of this in the miscdevices
series, so you probably want to amend this to also fix those.

Either way:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 2/3] rust: kernel: move `build_error` hidden function to prevent mistakes

[Alice Ryhl]

On Sat, Nov 23, 2024 at 11:29 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Users were using the hidden exported `kernel::build_error` function
> instead of the intended `kernel::build_error!` macro, e.g. see the
> previous commit.
>
> To force to use the macro, move it into the `build_assert` module,
> thus making it a compilation error and avoiding a collision in the same
> "namespace". Using the function now would require typing the module name
> (which is hidden), not just a single character.
>
> Now attempting to use the function will trigger this error with the
> right suggestion by the compiler:
>
>       error[E0423]: expected function, found macro `kernel::build_error`
>       --> samples/rust/rust_minimal.rs:29:9
>          |
>       29 |         kernel::build_error();
>          |         ^^^^^^^^^^^^^^^^^^^ not a function
>          |
>       help: use `!` to invoke the macro
>          |
>       29 |         kernel::build_error!();
>          |                            +
>
> An alternative would be using an alias, but it would be more complex
> and moving it into the module seems right since it belongs there and
> reduces the amount of code at the crate root.
>
> Keep the `#[doc(hidden)]` inside `build_assert` in case the module is
> not hidden in the future.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index bf8d7f841f94..73e33a41ea04 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -32,7 +32,8 @@
>  pub mod alloc;
>  #[cfg(CONFIG_BLOCK)]
>  pub mod block;
> -mod build_assert;
> +#[doc(hidden)]
> +pub mod build_assert;

You could also put #![doc(hidden)] at the top of build_assert.rs to
simplify the lib.rs list. Not sure what is best.

Alice

Re: [PATCH 3/3] rust: add `build_error!` to the prelude

[Alice Ryhl]

On Sat, Nov 23, 2024 at 11:29 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> The sibling `build_assert!` is already in the prelude, it makes sense
> that a "core"/"language" facility like this is part of the prelude and
> users should not be defining their own one (thus there should be no risk
> of future name collisions and we would want to be aware of them anyway).
>
> Thus add `build_error!` into the prelude.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 2/3] rust: kernel: move `build_error` hidden function to prevent mistakes

[Miguel Ojeda]

On Mon, Nov 25, 2024 at 10:14 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> You could also put #![doc(hidden)] at the top of build_assert.rs to
> simplify the lib.rs list. Not sure what is best.

Yeah, not sure. We used outer attributes for the rest of
`doc(hidden)`s, including in the same file, so it is probably best to
keep it consistent, unless we decide to move all to inner ones.

Thanks!

Cheers,
Miguel

Re: [PATCH 2/3] rust: kernel: move `build_error` hidden function to prevent mistakes

[Alice Ryhl]

On Mon, Nov 25, 2024 at 10:23 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Mon, Nov 25, 2024 at 10:14 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > You could also put #![doc(hidden)] at the top of build_assert.rs to
> > simplify the lib.rs list. Not sure what is best.
>
> Yeah, not sure. We used outer attributes for the rest of
> `doc(hidden)`s, including in the same file, so it is probably best to
> keep it consistent, unless we decide to move all to inner ones.

It might actually make sense to move all of them, I think. I find that
they make the list difficult to edit.

But feel free to pick whichever one you prefer for this change. My RB
applies with or without it :)

Alice

Re: [PATCH 1/3] rust: use the `build_error!` macro, not the hidden function

[Miguel Ojeda]

On Sat, Nov 23, 2024 at 11:29 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> Code and some examples were using the function, rather than the macro. The
> macro is what is documented.
>
> Thus move users to the macro.
>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Applied series to `rust-next` -- thanks!

    [ Applied the change to the new miscdevice cases. - Miguel ]

    [ Applied the change to the new miscdevice cases. - Miguel ]

Cheers,
Miguel