[PATCH v2] rust: macros: improve `#[vtable]` documentation

[PATCH v2] rust: macros: improve `#[vtable]` documentation

[Benno Lossin]

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>
---
v1 -> v2:
- removed imperative mode in the paragraph describing optional
  functions.

 rust/kernel/error.rs |  4 ++++
 rust/macros/lib.rs   | 32 ++++++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 8 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..daf1ef8baa62 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -87,27 +87,41 @@ 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 function to be optional, it must have a default implementation. But this default
+/// implementation will never be executed, since these functions are exclusively called from
+/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
+/// will execute the default behavior. Since it is not maintainable to replicate the default
+/// behavior in Rust, the default implementation should be:
+///
+/// ```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 function are required.
 ///
 /// # Examples
 ///
 /// ```ignore
+/// # use kernel::error::VTABLE_DEFAULT_ERROR;
 /// use kernel::prelude::*;
 ///
 /// // Declares a `#[vtable]` trait
 /// #[vtable]
-/// pub trait Operations: Send + Sync + Sized {
+/// pub trait Operations {
 ///     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 +139,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: a7135d10754760f0c038497b44c2c2f2b0fb5651
-- 
2.41.0

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Ariel Miculas (amiculas)]

On 23/10/19 05:15PM, 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>
> ---
> v1 -> v2:
> - removed imperative mode in the paragraph describing optional
>   functions.
> 
>  rust/kernel/error.rs |  4 ++++
>  rust/macros/lib.rs   | 32 ++++++++++++++++++++++++--------
>  2 files changed, 28 insertions(+), 8 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..daf1ef8baa62 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -87,27 +87,41 @@ 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 function to be optional, it must have a default implementation. But this default
> +/// implementation will never be executed, since these functions are exclusively called from
> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
> +/// will execute the default behavior. Since it is not maintainable to replicate the default
> +/// behavior in Rust, the default implementation should be:
> +///
> +/// ```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 function are required.

Typo: should be `all functions`
>  ///
>  /// # Examples
>  ///
>  /// ```ignore
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
>  /// use kernel::prelude::*;
>  ///
>  /// // Declares a `#[vtable]` trait
>  /// #[vtable]
> -/// pub trait Operations: Send + Sync + Sized {
> +/// pub trait Operations {
>  ///     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 +139,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: a7135d10754760f0c038497b44c2c2f2b0fb5651
> -- 
> 2.41.0
> 
> 
With the above typo fixed,
Reviewed-by: Ariel Miculas <amiculas@cisco.com>

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Andreas Hindborg (Samsung)]

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>
> ---
> v1 -> v2:
> - removed imperative mode in the paragraph describing optional
>   functions.
>
>  rust/kernel/error.rs |  4 ++++
>  rust/macros/lib.rs   | 32 ++++++++++++++++++++++++--------
>  2 files changed, 28 insertions(+), 8 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..daf1ef8baa62 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -87,27 +87,41 @@ 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 function to be optional, it must have a default implementation. But this default
> +/// implementation will never be executed, since these functions are exclusively called from
> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
> +/// will execute the default behavior. Since it is not maintainable to replicate the default
> +/// behavior in Rust, the default implementation should be:

How about this?:

For a Rust trait method to be optional, it must have a default
implementation. For a trait marked with `#[vtable]`, the default
implementation will not be executed, as the only way the trait methods
should be called is through function pointers installed in C side
vtables. When a trait implementation marked with `#[vtable]` is missing
a method, a `NULL` pointer will be installed in the corresponding C side
vtable, and thus the Rust default implementation can not be called. The
default implementation should be:

Not sure if it is more clear 🤷

> +///
> +/// ```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 function are required.
>  ///
>  /// # Examples
>  ///
>  /// ```ignore
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
>  /// use kernel::prelude::*;
>  ///
>  /// // Declares a `#[vtable]` trait
>  /// #[vtable]
> -/// pub trait Operations: Send + Sync + Sized {
> +/// pub trait Operations {
>  ///     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 +139,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: a7135d10754760f0c038497b44c2c2f2b0fb5651

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Alice Ryhl]

On 10/19/23 19:15, 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>

There is a minor nit below, and in reviews sent by others, but overall 
this looks fine to me.

>   /// # Examples
>   ///
>   /// ```ignore
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
>   /// use kernel::prelude::*;
I probably wouldn't hide the import of VTABLE_DEFAULT_ERROR from this 
example.

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Benno Lossin]

On 20.10.23 11:06, Andreas Hindborg (Samsung) wrote:
> Benno Lossin <benno.lossin@proton.me> writes:
>> +/// 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..daf1ef8baa62 100644
>> --- a/rust/macros/lib.rs
>> +++ b/rust/macros/lib.rs
>> @@ -87,27 +87,41 @@ 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 function to be optional, it must have a default implementation. But this default
>> +/// implementation will never be executed, since these functions are exclusively called from
>> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
>> +/// will execute the default behavior. Since it is not maintainable to replicate the default
>> +/// behavior in Rust, the default implementation should be:
> 
> How about this?:
> 
> For a Rust trait method to be optional, it must have a default
> implementation. For a trait marked with `#[vtable]`, the default
> implementation will not be executed, as the only way the trait methods
> should be called is through function pointers installed in C side
> vtables. When a trait implementation marked with `#[vtable]` is missing
> a method, a `NULL` pointer will be installed in the corresponding C side
> vtable, and thus the Rust default implementation can not be called. The
> default implementation should be:
> 
> Not sure if it is more clear 🤷

I think it misses the following important point: why is it not
possible to just replicate the default behavior?

What do you think of this?:

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. It is not possible to replicate the default behavior from C
in Rust, since that is not maintainable. The default implementaiton should
therefore call `kernel::build_error`, thus preventing calls to this
function at compile time:

-- 
Cheers,
Benno

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Benno Lossin]

On 21.10.23 14:45, Alice Ryhl wrote:
> On 10/19/23 19:15, 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>
> 
> There is a minor nit below, and in reviews sent by others, but overall
> this looks fine to me.
> 
>>    /// # Examples
>>    ///
>>    /// ```ignore
>> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
>>    /// use kernel::prelude::*;
> I probably wouldn't hide the import of VTABLE_DEFAULT_ERROR from this
> example.

What do you guys think of putting the const it in the prelude?

-- 
Cheers,
Benno

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Andreas Hindborg (Samsung)]

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

> On 20.10.23 11:06, Andreas Hindborg (Samsung) wrote:
>> Benno Lossin <benno.lossin@proton.me> writes:
>>> +/// 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..daf1ef8baa62 100644
>>> --- a/rust/macros/lib.rs
>>> +++ b/rust/macros/lib.rs
>>> @@ -87,27 +87,41 @@ 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 function to be optional, it must have a default implementation. But this default
>>> +/// implementation will never be executed, since these functions are exclusively called from
>>> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
>>> +/// will execute the default behavior. Since it is not maintainable to replicate the default
>>> +/// behavior in Rust, the default implementation should be:
>> 
>> How about this?:
>> 
>> For a Rust trait method to be optional, it must have a default
>> implementation. For a trait marked with `#[vtable]`, the default
>> implementation will not be executed, as the only way the trait methods
>> should be called is through function pointers installed in C side
>> vtables. When a trait implementation marked with `#[vtable]` is missing
>> a method, a `NULL` pointer will be installed in the corresponding C side
>> vtable, and thus the Rust default implementation can not be called. The
>> default implementation should be:
>> 
>> Not sure if it is more clear 🤷
>
> I think it misses the following important point: why is it not
> possible to just replicate the default behavior?
>
> What do you think of this?:
>
> 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.

> It is not possible to replicate the default behavior from C
> in Rust, since that is not maintainable.

I don't feel that this bit should be included. It's not a matter of
maintainability. Why would we reimplement something that is already
present in a subsystem? The functionality is already present, so we use
it.

> The default implementaiton should
> therefore call `kernel::build_error`, thus preventing calls to this
> function at compile time:

Otherwise I think it is good 👍

BR Andreas

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Benno Lossin]

On 23.10.23 10:30, Andreas Hindborg (Samsung) wrote:
> 
> Benno Lossin <benno.lossin@proton.me> writes:
> 
>> On 20.10.23 11:06, Andreas Hindborg (Samsung) wrote:
>>> Benno Lossin <benno.lossin@proton.me> writes:
>>>> +/// 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..daf1ef8baa62 100644
>>>> --- a/rust/macros/lib.rs
>>>> +++ b/rust/macros/lib.rs
>>>> @@ -87,27 +87,41 @@ 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 function to be optional, it must have a default implementation. But this default
>>>> +/// implementation will never be executed, since these functions are exclusively called from
>>>> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
>>>> +/// will execute the default behavior. Since it is not maintainable to replicate the default
>>>> +/// behavior in Rust, the default implementation should be:
>>>
>>> How about this?:
>>>
>>> For a Rust trait method to be optional, it must have a default
>>> implementation. For a trait marked with `#[vtable]`, the default
>>> implementation will not be executed, as the only way the trait methods
>>> should be called is through function pointers installed in C side
>>> vtables. When a trait implementation marked with `#[vtable]` is missing
>>> a method, a `NULL` pointer will be installed in the corresponding C side
>>> vtable, and thus the Rust default implementation can not be called. The
>>> default implementation should be:
>>>
>>> Not sure if it is more clear 🤷
>>
>> I think it misses the following important point: why is it not
>> possible to just replicate the default behavior?
>>
>> What do you think of this?:
>>
>> 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.
> 
>> It is not possible to replicate the default behavior from C
>> in Rust, since that is not maintainable.
> 
> I don't feel that this bit should be included. It's not a matter of
> maintainability. Why would we reimplement something that is already
> present in a subsystem? The functionality is already present, so we use
> it.

But we don't use it on the Rust side. You cannot write this:

     fn foo<T: Operations>(t: &T) -> Result<usize> {
         t.seek(0)?
     }

if the `seek` function is optional.

-- 
Cheers,
Benno

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Gary Guo]

On Thu, 19 Oct 2023 17:15:53 +0000
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>
> ---
> v1 -> v2:
> - removed imperative mode in the paragraph describing optional
>   functions.
> 
>  rust/kernel/error.rs |  4 ++++
>  rust/macros/lib.rs   | 32 ++++++++++++++++++++++++--------
>  2 files changed, 28 insertions(+), 8 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..daf1ef8baa62 100644
> --- a/rust/macros/lib.rs
> +++ b/rust/macros/lib.rs
> @@ -87,27 +87,41 @@ 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 function to be optional, it must have a default implementation. But this default
> +/// implementation will never be executed, since these functions are exclusively called from
> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
> +/// will execute the default behavior. Since it is not maintainable to replicate the default
> +/// behavior in Rust, the default implementation should be:
> +///
> +/// ```compile_fail
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
> +/// kernel::build_error(VTABLE_DEFAULT_ERROR)

Note that `build_error` function is considered impl detail and is
hidden. This should use the macro version instead:

kernel::build_error!(VTABLE_DEFAULT_ERROR)

Actually, the string here provides little use other than documentation,
since the string provided to build_error is only visible in const eval,
so this you might just omit that and write

kernel::build_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 function are required.
>  ///
>  /// # Examples
>  ///
>  /// ```ignore
> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
>  /// use kernel::prelude::*;
>  ///
>  /// // Declares a `#[vtable]` trait
>  /// #[vtable]
> -/// pub trait Operations: Send + Sync + Sized {
> +/// pub trait Operations {
>  ///     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 +139,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: a7135d10754760f0c038497b44c2c2f2b0fb5651

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Benno Lossin]

On 24.10.23 13:24, Gary Guo wrote:
> On Thu, 19 Oct 2023 17:15:53 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:

[...]

>> -/// 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 function to be optional, it must have a default implementation. But this default
>> +/// implementation will never be executed, since these functions are exclusively called from
>> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
>> +/// will execute the default behavior. Since it is not maintainable to replicate the default
>> +/// behavior in Rust, the default implementation should be:
>> +///
>> +/// ```compile_fail
>> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
>> +/// kernel::build_error(VTABLE_DEFAULT_ERROR)
> 
> Note that `build_error` function is considered impl detail and is
> hidden.

I see, we should mention that in the docs on `build_error`.

> This should use the macro version instead:
> 
> kernel::build_error!(VTABLE_DEFAULT_ERROR)

Is there a reason that it is a macro? Why is it re-exported in the
kernel crate? The macro could just use `::bulid_error::build_error()`.

> Actually, the string here provides little use other than documentation,

Sure, but that is the whole purpose of this patch.

> since the string provided to build_error is only visible in const eval,
> so this you might just omit that and write
> 
> kernel::build_error!()

Note that it is also useful for people who read the code, as they
can search for the constant and understand why it is a build error.

-- 
Cheers,
Benno

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Alice Ryhl]

On Mon, Oct 23, 2023 at 9:01 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 21.10.23 14:45, Alice Ryhl wrote:
> > On 10/19/23 19:15, 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>
> >
> > There is a minor nit below, and in reviews sent by others, but overall
> > this looks fine to me.
> >
> >>    /// # Examples
> >>    ///
> >>    /// ```ignore
> >> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
> >>    /// use kernel::prelude::*;
> > I probably wouldn't hide the import of VTABLE_DEFAULT_ERROR from this
> > example.
>
> What do you guys think of putting the const it in the prelude?

I think it's fine to just import it.

Alice

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Gary Guo]

On Tue, 24 Oct 2023 14:43:30 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> On 24.10.23 13:24, Gary Guo wrote:
> > On Thu, 19 Oct 2023 17:15:53 +0000
> > Benno Lossin <benno.lossin@proton.me> wrote:  
> 
> [...]
> 
> >> -/// 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 function to be optional, it must have a default implementation. But this default
> >> +/// implementation will never be executed, since these functions are exclusively called from
> >> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
> >> +/// will execute the default behavior. Since it is not maintainable to replicate the default
> >> +/// behavior in Rust, the default implementation should be:
> >> +///
> >> +/// ```compile_fail
> >> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
> >> +/// kernel::build_error(VTABLE_DEFAULT_ERROR)  
> > 
> > Note that `build_error` function is considered impl detail and is
> > hidden.  
> 
> I see, we should mention that in the docs on `build_error`.

Well, it's marked as `#[doc(hidden)]`...

> 
> > This should use the macro version instead:
> > 
> > kernel::build_error!(VTABLE_DEFAULT_ERROR)  
> 
> Is there a reason that it is a macro? Why is it re-exported in the
> kernel crate? The macro could just use `::bulid_error::build_error()`.

The original intention is to allow format strings, but Rust const
panic is not powerful enough to support it at the moment. Macro
allows higher flexibility.

It's re-exported so the macro can reference them (note that downstream
crates can't reference build_error directly). Perhaps I should put it
inside __private_reexport or something instead.

Re: [PATCH v2] rust: macros: improve `#[vtable]` documentation

[Benno Lossin]

On 25.10.23 21:14, Gary Guo wrote:
> On Tue, 24 Oct 2023 14:43:30 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> On 24.10.23 13:24, Gary Guo wrote:
>>> On Thu, 19 Oct 2023 17:15:53 +0000
>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> [...]
>>
>>>> -/// 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 function to be optional, it must have a default implementation. But this default
>>>> +/// implementation will never be executed, since these functions are exclusively called from
>>>> +/// callbacks from the C side. This is because the vtable will have a `NULL` entry and the C side
>>>> +/// will execute the default behavior. Since it is not maintainable to replicate the default
>>>> +/// behavior in Rust, the default implementation should be:
>>>> +///
>>>> +/// ```compile_fail
>>>> +/// # use kernel::error::VTABLE_DEFAULT_ERROR;
>>>> +/// kernel::build_error(VTABLE_DEFAULT_ERROR)
>>>
>>> Note that `build_error` function is considered impl detail and is
>>> hidden.
>>
>> I see, we should mention that in the docs on `build_error`.
> 
> Well, it's marked as `#[doc(hidden)]`...

Yes, but I did not even build the docs, but read it directly
inside of the `build_error` crate and thus I did not see the
`#[doc(hidden)]`.

>>> This should use the macro version instead:
>>>
>>> kernel::build_error!(VTABLE_DEFAULT_ERROR)
>>
>> Is there a reason that it is a macro? Why is it re-exported in the
>> kernel crate? The macro could just use `::bulid_error::build_error()`.
> 
> The original intention is to allow format strings, but Rust const
> panic is not powerful enough to support it at the moment. Macro
> allows higher flexibility.

That is what I thought. But should we then not always require a
string literal? So

     kernel::build_error!("{}", VTABLE_DEFAULT_ERROR)

> It's re-exported so the macro can reference them (note that downstream
> crates can't reference build_error directly). Perhaps I should put it
> inside __private_reexport or something instead.

I see, I did not know that they cannot reference build error directly.
Is that some limitation of the build system? If it is possible to not
re-export it, I think that would be better, otherwise just put it in
`__private`.

-- 
Cheers,
Benno