[v3] rust: macros: improve `#[vtable]` documentation

  Traits marked with `#[vtable]` need to provide default implementations
for optional functions. The C side represents these with `NULL` in the
vtable, so the default functions are never actually called. We do not
want to replicate the default behavior from C in Rust, because that is
not maintainable. Therefore we should use `build_error` in those default
implementations. The error message for that is provided at
`kernel::error::VTABLE_DEFAULT_ERROR`.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
v2 -> v3:
- don't hide the import of the constant in the example
- fixed "function" typo
- improve paragraph about optional functions
- do not remove the `Send + Sync + Sized` bounds on the example

v1 -> v2:
- removed imperative mode in the paragraph describing optional
  functions.

 rust/kernel/error.rs |  4 ++++
 rust/macros/lib.rs   | 37 ++++++++++++++++++++++++++++++-------
 2 files changed, 34 insertions(+), 7 deletions(-)


base-commit: 3857af38e57a80b15b994e19d1f4301cac796481
  

On 23/10/26 08:19PM, Benno Lossin wrote:
> Traits marked with `#[vtable]` need to provide default implementations
> for optional functions. The C side represents these with `NULL` in the
> vtable, so the default functions are never actually called. We do not
> want to replicate the default behavior from C in Rust, because that is
> not maintainable. Therefore we should use `build_error` in those default
> implementations. The error message for that is provided at
> `kernel::error::VTABLE_DEFAULT_ERROR`.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> v2 -> v3:
> - don't hide the import of the constant in the example
> - fixed "function" typo
> - improve paragraph about optional functions
> - do not remove the `Send + Sync + Sized` bounds on the example
> 
> v1 -> v2:
> - removed imperative mode in the paragraph describing optional
>   functions.
> 
>  rust/kernel/error.rs |  4 ++++
>  rust/macros/lib.rs   | 37 ++++++++++++++++++++++++++++++-------
>  2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 05fcab6abfe6..1373cde025ef 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -335,3 +335,7 @@ pub(crate) fn from_result<T, F>(f: F) -> T
>          Err(e) => T::from(e.to_errno() as i16),
>      }
>  }
> +
> +/// Error message for calling a default function of a [`#[vtable]`](macros::vtable) trait.
> +pub const VTABLE_DEFAULT_ERROR: &str =
> +    "This function must not be called, see the #[vtable] documentation.";
> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
> index c42105c2ff96..917a51183c23 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -87,27 +87,48 @@ pub fn module(ts: TokenStream) -> TokenStream {
>  /// implementation could just return `Error::EINVAL`); Linux typically use C
>  /// `NULL` pointers to represent these functions.
>  ///
> -/// This attribute is intended to close the gap. Traits can be declared and
> -/// implemented with the `#[vtable]` attribute, and a `HAS_*` associated constant
> -/// will be generated for each method in the trait, indicating if the implementor
> -/// has overridden a method.
> +/// This attribute closes that gap. A trait can be annotated with the `#[vtable]` attribute.
> +/// Implementers of the trait will then also have to annotate the trait with `#[vtable]`. This
> +/// attribute generates a `HAS_*` associated constant bool for each method in the trait that is set
> +/// to true if the implementer has overridden the associated method.

The above paragraph seems to be wrapped at 100 characters while the
paragraph below seems to be wrapped at 80 characters.

Cheers,
Ariel
> +///
> +/// For a trait method to be optional, it must have a default implementation.
> /// This is also the case for traits annotated with `#[vtable]`, but in this
> +/// case the default implementation will never be executed. The reason for this
> +/// is that the functions will be called through function pointers installed in
> +/// C side vtables. When an optional method is not implemented on a `#[vtable]`
> +/// trait, a NULL entry is installed in the vtable. Thus the default
> +/// implementation is never called. Since these traits are not designed to be
> +/// 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
> +/// calls to this function at compile time:
> +///
> +/// ```compile_fail
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
> +/// kernel::build_error(VTABLE_DEFAULT_ERROR)
> +/// ```
> +///
> +/// note that you might need to import [`kernel::error::VTABLE_DEFAULT_ERROR`].
>  ///
> -/// This attribute is not needed if all methods are required.
> +/// This macro should not be used when all functions are required.
>  ///
>  /// # Examples
>  ///
>  /// ```ignore
> +/// use kernel::error::VTABLE_DEFAULT_ERROR;
>  /// use kernel::prelude::*;
>  ///
>  /// // Declares a `#[vtable]` trait
>  /// #[vtable]
>  /// pub trait Operations: Send + Sync + Sized {
>  ///     fn foo(&self) -> Result<()> {
> -///         Err(EINVAL)
> +///         kernel::build_error(VTABLE_DEFAULT_ERROR)
>  ///     }
>  ///
>  ///     fn bar(&self) -> Result<()> {
> -///         Err(EINVAL)
> +///         kernel::build_error(VTABLE_DEFAULT_ERROR)
>  ///     }
>  /// }
>  ///
> @@ -125,6 +146,8 @@ pub fn module(ts: TokenStream) -> TokenStream {
>  /// assert_eq!(<Foo as Operations>::HAS_FOO, true);
>  /// assert_eq!(<Foo as Operations>::HAS_BAR, false);
>  /// ```
> +///
> +/// [`kernel::error::VTABLE_DEFAULT_ERROR`]: ../kernel/error/constant.VTABLE_DEFAULT_ERROR.html
>  #[proc_macro_attribute]
>  pub fn vtable(attr: TokenStream, ts: TokenStream) -> TokenStream {
>      vtable::vtable(attr, ts)
> 
> base-commit: 3857af38e57a80b15b994e19d1f4301cac796481
> -- 
> 2.41.0
> 
>
  

On 10/26/23 17:19, Benno Lossin wrote:
> Traits marked with `#[vtable]` need to provide default implementations
> for optional functions. The C side represents these with `NULL` in the
> vtable, so the default functions are never actually called. We do not
> want to replicate the default behavior from C in Rust, because that is
> not maintainable. Therefore we should use `build_error` in those default
> implementations. The error message for that is provided at
> `kernel::error::VTABLE_DEFAULT_ERROR`.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> v2 -> v3:
> - don't hide the import of the constant in the example
> - fixed "function" typo
> - improve paragraph about optional functions
> - do not remove the `Send + Sync + Sized` bounds on the example
> 
> v1 -> v2:
> - removed imperative mode in the paragraph describing optional
>    functions.
> 
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
  

On 26 Oct 2023, at 22:19, Benno Lossin wrote:

> Traits marked with `#[vtable]` need to provide default implementations
> for optional functions. The C side represents these with `NULL` in the
> vtable, so the default functions are never actually called. We do not
> want to replicate the default behavior from C in Rust, because that is
> not maintainable. Therefore we should use `build_error` in those default
> implementations. The error message for that is provided at
> `kernel::error::VTABLE_DEFAULT_ERROR`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> ---
> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
> index c42105c2ff96..917a51183c23 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -87,27 +87,48 @@ pub fn module(ts: TokenStream) -> TokenStream {
>  /// implementation could just return `Error::EINVAL`); Linux typically use C
>  /// `NULL` pointers to represent these functions.
>  ///
> -/// This attribute is intended to close the gap. Traits can be declared and
> -/// implemented with the `#[vtable]` attribute, and a `HAS_*` associated constant
> -/// will be generated for each method in the trait, indicating if the implementor
> -/// has overridden a method.
> +/// This attribute closes that gap. A trait can be annotated with the `#[vtable]` attribute.
> +/// Implementers of the trait will then also have to annotate the trait with `#[vtable]`. This
> +/// attribute generates a `HAS_*` associated constant bool for each method in the trait that is set
> +/// to true if the implementer has overridden the associated method.
> +///
> +/// For a trait method to be optional, it must have a default implementation.
> +/// This is also the case for traits annotated with `#[vtable]`, but in this
> +/// case the default implementation will never be executed. The reason for this
> +/// is that the functions will be called through function pointers installed in
> +/// C side vtables. When an optional method is not implemented on a `#[vtable]`
> +/// trait, a NULL entry is installed in the vtable. Thus the default
> +/// implementation is never called. Since these traits are not designed to be
> +/// 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
> +/// calls to this function at compile time:
In the future it would be nice to have something like `#[default]` or `#[optional]` to automatically derive the implementation.
> +///
> +/// ```compile_fail
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
> +/// kernel::build_error(VTABLE_DEFAULT_ERROR)
> +/// ```
> +///
> +/// note that you might need to import [`kernel::error::VTABLE_DEFAULT_ERROR`].
>  ///
> -/// This attribute is not needed if all methods are required.
> +/// This macro should not be used when all functions are required.
Reviewed-by: Finn Behrens <me@kloenk.dev>
  

On 10/27/23 10:02, Finn Behrens wrote:
> 
> 
> On 26 Oct 2023, at 22:19, Benno Lossin wrote:
> 
>> Traits marked with `#[vtable]` need to provide default implementations
>> for optional functions. The C side represents these with `NULL` in the
>> vtable, so the default functions are never actually called. We do not
>> want to replicate the default behavior from C in Rust, because that is
>> not maintainable. Therefore we should use `build_error` in those default
>> implementations. The error message for that is provided at
>> `kernel::error::VTABLE_DEFAULT_ERROR`.
>>
>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>> ---
>> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
>> index c42105c2ff96..917a51183c23 100644
>> --- a/rust/macros/lib.rs
>> +++ b/rust/macros/lib.rs
>> @@ -87,27 +87,48 @@ pub fn module(ts: TokenStream) -> TokenStream {
>>  /// implementation could just return `Error::EINVAL`); Linux typically use C
>>  /// `NULL` pointers to represent these functions.
>>  ///
>> -/// This attribute is intended to close the gap. Traits can be declared and
>> -/// implemented with the `#[vtable]` attribute, and a `HAS_*` associated constant
>> -/// will be generated for each method in the trait, indicating if the implementor
>> -/// has overridden a method.
>> +/// This attribute closes that gap. A trait can be annotated with the `#[vtable]` attribute.
>> +/// Implementers of the trait will then also have to annotate the trait with `#[vtable]`. This
>> +/// attribute generates a `HAS_*` associated constant bool for each method in the trait that is set
>> +/// to true if the implementer has overridden the associated method.
>> +///
>> +/// For a trait method to be optional, it must have a default implementation.
>> +/// This is also the case for traits annotated with `#[vtable]`, but in this
>> +/// case the default implementation will never be executed. The reason for this
>> +/// is that the functions will be called through function pointers installed in
>> +/// C side vtables. When an optional method is not implemented on a `#[vtable]`
>> +/// trait, a NULL entry is installed in the vtable. Thus the default
>> +/// implementation is never called. Since these traits are not designed to be
>> +/// 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
>> +/// calls to this function at compile time:
> In the future it would be nice to have something like `#[default]` or `#[optional]` to automatically derive the implementation.

I brought this up in the discussion on zulip [1]. But Wedson argued that
macros make it more magical and less easy to understand. So for the time
being, I just wanted to improve the docs.

[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.60bool.3A.3Athen.60.20helper/near/395659471
  

On 10/26/23 23:12, Ariel Miculas (amiculas) wrote:
> On 23/10/26 08:19PM, Benno Lossin wrote:
>> Traits marked with `#[vtable]` need to provide default implementations
>> for optional functions. The C side represents these with `NULL` in the
>> vtable, so the default functions are never actually called. We do not
>> want to replicate the default behavior from C in Rust, because that is
>> not maintainable. Therefore we should use `build_error` in those default
>> implementations. The error message for that is provided at
>> `kernel::error::VTABLE_DEFAULT_ERROR`.
>>
>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
>> ---
>> v2 -> v3:
>> - don't hide the import of the constant in the example
>> - fixed "function" typo
>> - improve paragraph about optional functions
>> - do not remove the `Send + Sync + Sized` bounds on the example
>>
>> v1 -> v2:
>> - removed imperative mode in the paragraph describing optional
>>   functions.
>>
>>  rust/kernel/error.rs |  4 ++++
>>  rust/macros/lib.rs   | 37 ++++++++++++++++++++++++++++++-------
>>  2 files changed, 34 insertions(+), 7 deletions(-)
>>
>> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
>> index 05fcab6abfe6..1373cde025ef 100644
>> --- a/rust/kernel/error.rs
>> +++ b/rust/kernel/error.rs
>> @@ -335,3 +335,7 @@ pub(crate) fn from_result<T, F>(f: F) -> T
>>          Err(e) => T::from(e.to_errno() as i16),
>>      }
>>  }
>> +
>> +/// Error message for calling a default function of a [`#[vtable]`](macros::vtable) trait.
>> +pub const VTABLE_DEFAULT_ERROR: &str =
>> +    "This function must not be called, see the #[vtable] documentation.";
>> diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
>> index c42105c2ff96..917a51183c23 100644
>> --- a/rust/macros/lib.rs
>> +++ b/rust/macros/lib.rs
>> @@ -87,27 +87,48 @@ pub fn module(ts: TokenStream) -> TokenStream {
>>  /// implementation could just return `Error::EINVAL`); Linux typically use C
>>  /// `NULL` pointers to represent these functions.
>>  ///
>> -/// This attribute is intended to close the gap. Traits can be declared and
>> -/// implemented with the `#[vtable]` attribute, and a `HAS_*` associated constant
>> -/// will be generated for each method in the trait, indicating if the implementor
>> -/// has overridden a method.
>> +/// This attribute closes that gap. A trait can be annotated with the `#[vtable]` attribute.
>> +/// Implementers of the trait will then also have to annotate the trait with `#[vtable]`. This
>> +/// attribute generates a `HAS_*` associated constant bool for each method in the trait that is set
>> +/// to true if the implementer has overridden the associated method.
> 
> The above paragraph seems to be wrapped at 100 characters while the
> paragraph below seems to be wrapped at 80 characters.

Oh I forgot about that. Miguel, would it be reasonable for you to fix
this when picking the patch?
  

On Fri, Oct 27, 2023 at 11:32 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> Oh I forgot about that. Miguel, would it be reasonable for you to fix
> this when picking the patch?

Yeah, no worries.

Thanks!

Cheers,
Miguel
  

On 10/26/23 22:19, Benno Lossin wrote:
> Traits marked with `#[vtable]` need to provide default implementations
> for optional functions. The C side represents these with `NULL` in the
> vtable, so the default functions are never actually called. We do not
> want to replicate the default behavior from C in Rust, because that is
> not maintainable. Therefore we should use `build_error` in those default
> implementations. The error message for that is provided at
> `kernel::error::VTABLE_DEFAULT_ERROR`.
> 
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

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

Benno Lossin <benno.lossin@proton.me> writes:

> Traits marked with `#[vtable]` need to provide default implementations
> for optional functions. The C side represents these with `NULL` in the
> vtable, so the default functions are never actually called. We do not
> want to replicate the default behavior from C in Rust, because that is
> not maintainable. Therefore we should use `build_error` in those default
> implementations. The error message for that is provided at
> `kernel::error::VTABLE_DEFAULT_ERROR`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>
  

On Thu, Oct 26, 2023 at 10:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> Traits marked with `#[vtable]` need to provide default implementations
> for optional functions. The C side represents these with `NULL` in the
> vtable, so the default functions are never actually called. We do not
> want to replicate the default behavior from C in Rust, because that is
> not maintainable. Therefore we should use `build_error` in those default
> implementations. The error message for that is provided at
> `kernel::error::VTABLE_DEFAULT_ERROR`.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

Applied to `rust-next` (with the requested wrapping applied and
capitalized sentence).

Thanks everyone!

Cheers,
Miguel