[PATCH] rust: add `assert_sync` function

[PATCH] rust: add `assert_sync` function

[Christian Schrefl]

Adds a new file `compile_assert.rs` for asserts during compile time and
add the `assert_sync` function to this file.

This will be used in `miscdevice` to avoid regression in case a `: Send`
bound falsely gets dropped in the future.

Suggested-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/rust-for-linux/20250530-b4-rust_miscdevice_registrationdata-v4-0-d313aafd7e59@gmail.com/T/#mdf3328834ce1d136daf836c9e089b5a8627a6d53
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
For now I've only added the function.

Some things that might make sense to do as well:
- Move `static_assert` into `compile_assert.rs`.
- Add `assert_sync` to prelude.
- Add `assert_send` as well.
- Use these asserts in various places around the kernel. (I'm not sure
where it would make sense)
---
 rust/kernel/compile_assert.rs | 24 ++++++++++++++++++++++++
 rust/kernel/lib.rs            |  1 +
 2 files changed, 25 insertions(+)

diff --git a/rust/kernel/compile_assert.rs b/rust/kernel/compile_assert.rs
new file mode 100644
index 0000000000000000000000000000000000000000..2a99de1ba919dc3952d7a1585869567a44106b44
--- /dev/null
+++ b/rust/kernel/compile_assert.rs
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Compile-time asserts.
+
+/// Asserts that the given type is [`Sync`]. This check is done at compile time and does nothing
+/// at runtime.
+///
+/// Note that this is only intended to avoid regressions and for sanity checks.
+///
+/// # Examples
+/// ```
+/// # use kernel::compile_assert::assert_sync;
+/// # use kernel::types::NotThreadSafe;
+///
+///
+/// // Do the assertion in a const block to make sure it won't be executed at runtime.
+/// const _:() = {
+///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
+///     // assert_sync::<NotThreadSafe>(); // Fails because `NotThreadSafe` is not `Sync`.
+/// };
+///
+/// ```
+#[inline(always)]
+pub const fn assert_sync<T: ?Sized + Sync>() {}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 7e227b52b4d8ff23d39d5cc9ad3ab57132c448d0..e1630e5079b2436eda6f8b71225bd5371af337b4 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -56,6 +56,7 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod compile_assert;
 pub mod cred;
 pub mod device;
 pub mod device_id;

---
base-commit: 7a17bbc1d952057898cb0739e60665908fbb8c72
change-id: 20250607-assert_sync-62fddc5a0adc

Best regards,
-- 
Christian Schrefl <chrisi.schrefl@gmail.com>

Re: [PATCH] rust: add `assert_sync` function

[Benno Lossin]

On Sat Jun 7, 2025 at 3:02 PM CEST, Christian Schrefl wrote:
> Adds a new file `compile_assert.rs` for asserts during compile time and
> add the `assert_sync` function to this file.
>
> This will be used in `miscdevice` to avoid regression in case a `: Send`
> bound falsely gets dropped in the future.
>
> Suggested-by: Benno Lossin <lossin@kernel.org>
> Link: https://lore.kernel.org/rust-for-linux/20250530-b4-rust_miscdevice_registrationdata-v4-0-d313aafd7e59@gmail.com/T/#mdf3328834ce1d136daf836c9e089b5a8627a6d53
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Thanks!

> ---
> For now I've only added the function.
>
> Some things that might make sense to do as well:
> - Move `static_assert` into `compile_assert.rs`.

Sounds reasonable.

> - Add `assert_sync` to prelude.

I don't think we need to do that. At least not yet.

> - Add `assert_send` as well.

Sounds like a good idea.

> - Use these asserts in various places around the kernel. (I'm not sure
> where it would make sense)
> ---
>  rust/kernel/compile_assert.rs | 24 ++++++++++++++++++++++++
>  rust/kernel/lib.rs            |  1 +
>  2 files changed, 25 insertions(+)
>
> diff --git a/rust/kernel/compile_assert.rs b/rust/kernel/compile_assert.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..2a99de1ba919dc3952d7a1585869567a44106b44
> --- /dev/null
> +++ b/rust/kernel/compile_assert.rs
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Compile-time asserts.
> +
> +/// Asserts that the given type is [`Sync`]. This check is done at compile time and does nothing
> +/// at runtime.
> +///
> +/// Note that this is only intended to avoid regressions and for sanity checks.
> +///
> +/// # Examples
> +/// ```
> +/// # use kernel::compile_assert::assert_sync;
> +/// # use kernel::types::NotThreadSafe;
> +///
> +///
> +/// // Do the assertion in a const block to make sure it won't be executed at runtime.
> +/// const _:() = {

s/_:()/_: ()/

> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
> +///     // assert_sync::<NotThreadSafe>(); // Fails because `NotThreadSafe` is not `Sync`.

Can you split this into two examples and mark the failing one with
`compile_fail`?

We also could provide a macro similar to [1].

[1]: https://docs.rs/static_assertions/latest/static_assertions/

---
Cheers,
Benno

> +/// };
> +///
> +/// ```
> +#[inline(always)]
> +pub const fn assert_sync<T: ?Sized + Sync>() {}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 7e227b52b4d8ff23d39d5cc9ad3ab57132c448d0..e1630e5079b2436eda6f8b71225bd5371af337b4 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -56,6 +56,7 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +pub mod compile_assert;
>  pub mod cred;
>  pub mod device;
>  pub mod device_id;
>
> ---
> base-commit: 7a17bbc1d952057898cb0739e60665908fbb8c72
> change-id: 20250607-assert_sync-62fddc5a0adc
>
> Best regards,

Re: [PATCH] rust: add `assert_sync` function

[Christian Schrefl]

On 07.06.25 5:42 PM, Benno Lossin wrote:
> On Sat Jun 7, 2025 at 3:02 PM CEST, Christian Schrefl wrote:
>> Adds a new file `compile_assert.rs` for asserts during compile time and
>> add the `assert_sync` function to this file.
>>
>> This will be used in `miscdevice` to avoid regression in case a `: Send`
>> bound falsely gets dropped in the future.
>>
>> Suggested-by: Benno Lossin <lossin@kernel.org>
>> Link: https://lore.kernel.org/rust-for-linux/20250530-b4-rust_miscdevice_registrationdata-v4-0-d313aafd7e59@gmail.com/T/#mdf3328834ce1d136daf836c9e089b5a8627a6d53
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> 
> Thanks!
> 
>> ---
>> For now I've only added the function.
>>
>> Some things that might make sense to do as well:
>> - Move `static_assert` into `compile_assert.rs`.
> 
> Sounds reasonable.
> 
>> - Add `assert_sync` to prelude.
> 
> I don't think we need to do that. At least not yet.

Alright

>> - Add `assert_send` as well.
> 
> Sounds like a good idea.

Should I already add this in V2 for this series?

> 
>> - Use these asserts in various places around the kernel. (I'm not sure
>> where it would make sense)
>> ---
>>  rust/kernel/compile_assert.rs | 24 ++++++++++++++++++++++++
>>  rust/kernel/lib.rs            |  1 +
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/rust/kernel/compile_assert.rs b/rust/kernel/compile_assert.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..2a99de1ba919dc3952d7a1585869567a44106b44
>> --- /dev/null
>> +++ b/rust/kernel/compile_assert.rs
>> @@ -0,0 +1,24 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Compile-time asserts.
>> +
>> +/// Asserts that the given type is [`Sync`]. This check is done at compile time and does nothing
>> +/// at runtime.
>> +///
>> +/// Note that this is only intended to avoid regressions and for sanity checks.
>> +///
>> +/// # Examples
>> +/// ```
>> +/// # use kernel::compile_assert::assert_sync;
>> +/// # use kernel::types::NotThreadSafe;
>> +///
>> +///
>> +/// // Do the assertion in a const block to make sure it won't be executed at runtime.
>> +/// const _:() = {
> 
> s/_:()/_: ()/

Fixed.

> 
>> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
>> +///     // assert_sync::<NotThreadSafe>(); // Fails because `NotThreadSafe` is not `Sync`.
> 
> Can you split this into two examples and mark the failing one with
> `compile_fail`?

I've tried it with `compile_fail` and it didn't work, I think
that's not supported in (kernel) doc tests yet. 

> 
> We also could provide a macro similar to [1].
>
> [1]: https://docs.rs/static_assertions/latest/static_assertions/

You mean the `assert_impl_*!` macros?

That might make sense, with macros we would not need to write
a const block to ensure its not executed at runtime (although
it's probably optimized out anyways). It would also mean that
we won't need a assert for every Trait, which seems nice.
So a macro sounds pretty good to me.

Cheers
Christian

Re: [PATCH] rust: add `assert_sync` function

[Miguel Ojeda]

On Sat, Jun 7, 2025 at 3:02 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> +/// Asserts that the given type is [`Sync`]. This check is done at compile time and does nothing
> +/// at runtime.

I would split the second sentence into its own paragraph, so that the
"short description" isn't long.

> +/// Note that this is only intended to avoid regressions and for sanity checks.

Hmm... I am not sure about this sentence. A macro may want to call
this to ensure something that is required for safety, for instance. Is
that the "sanity check" part? In any case, it sounds like the sentence
could be read as "this is not reliable for "other" things apart from
just sanity checks", which may be confusing.

Could we perhaps clarify?

> +/// # Examples
> +/// ```

Please add a newline between these.

> +///
> +///

These newlines should be removed, otherwise they will be rendered.

> +/// // Do the assertion in a const block to make sure it won't be executed at runtime.
> +/// const _:() = {
> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync

`Sync` and please use a period at the end. Also, I would suggest
following our usual style and putting it at the top, i.e.

    // Succeeds because `i32` is `Sync`.
    assert_sync::<i32>();

> +///
> +/// ```

This one can be removed too.

Cheers,
Miguel

Re: [PATCH] rust: add `assert_sync` function

[Miguel Ojeda]

On Sat, Jun 7, 2025 at 5:54 PM Christian Schrefl
<chrisi.schrefl@gmail.com> wrote:
>
> Should I already add this in V2 for this series?

I would do that in a separate patch (possibly in this series, or later on).

> I've tried it with `compile_fail` and it didn't work, I think
> that's not supported in (kernel) doc tests yet.

Yeah, it isn't -- please leave a normal comment to indicate it, so
that we can replace it with the real annotation in the future.

> That might make sense, with macros we would not need to write
> a const block to ensure its not executed at runtime (although
> it's probably optimized out anyways). It would also mean that

There is nothing to execute, no? It is `#[inline(always)]`, and even
if that wasn't enough, we always build with optimizations enabled.

But yeah, if needed, we may want to have macro(s) like that.

Thanks!

Cheers,
Miguel

Re: [PATCH] rust: add `assert_sync` function

[Benno Lossin]

On Sat Jun 7, 2025 at 5:54 PM CEST, Christian Schrefl wrote:
> On 07.06.25 5:42 PM, Benno Lossin wrote:
>> On Sat Jun 7, 2025 at 3:02 PM CEST, Christian Schrefl wrote:
>>> - Add `assert_send` as well.
>> 
>> Sounds like a good idea.
>
> Should I already add this in V2 for this series?

If you want to then sure, but we can also wait until we have a use-case.
Also, let's finish the discussion about the macro idea below.

>>> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
>>> +///     // assert_sync::<NotThreadSafe>(); // Fails because `NotThreadSafe` is not `Sync`.
>> 
>> Can you split this into two examples and mark the failing one with
>> `compile_fail`?
>
> I've tried it with `compile_fail` and it didn't work, I think
> that's not supported in (kernel) doc tests yet. 

Hmm, I thought that this worked... @Miguel any idea?

>> We also could provide a macro similar to [1].
>>
>> [1]: https://docs.rs/static_assertions/latest/static_assertions/
>
> You mean the `assert_impl_*!` macros?

Yes, but the others might also be useful from time to time.

> That might make sense, with macros we would not need to write
> a const block to ensure its not executed at runtime (although
> it's probably optimized out anyways).

It 100% will be optimized out.

> It would also mean that we won't need a assert for every Trait, which
> seems nice. So a macro sounds pretty good to me.

It depends, the macro impl needs to define its own function, which might
be inefficient if one uses it a lot. But there is no way to be generic
over traits, so there is no other way.

Let's see what the others think.

---
Cheers,
Benno

Re: [PATCH] rust: add `assert_sync` function

[Christian Schrefl]

On 07.06.25 8:11 PM, Benno Lossin wrote:
> On Sat Jun 7, 2025 at 5:54 PM CEST, Christian Schrefl wrote:
>> On 07.06.25 5:42 PM, Benno Lossin wrote:
>>> On Sat Jun 7, 2025 at 3:02 PM CEST, Christian Schrefl wrote:
>>>> - Add `assert_send` as well.
>>>
>>> Sounds like a good idea.
>>
>> Should I already add this in V2 for this series?
> 
> If you want to then sure, but we can also wait until we have a use-case.
> Also, let's finish the discussion about the macro idea below.
> 
>>>> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
>>>> +///     // assert_sync::<NotThreadSafe>(); // Fails because `NotThreadSafe` is not `Sync`.
>>>
>>> Can you split this into two examples and mark the failing one with
>>> `compile_fail`?
>>
>> I've tried it with `compile_fail` and it didn't work, I think
>> that's not supported in (kernel) doc tests yet. 
> 
> Hmm, I thought that this worked... @Miguel any idea?
> 
>>> We also could provide a macro similar to [1].
>>>
>>> [1]: https://docs.rs/static_assertions/latest/static_assertions/
>>
>> You mean the `assert_impl_*!` macros?
> 
> Yes, but the others might also be useful from time to time.
> >> That might make sense, with macros we would not need to write
>> a const block to ensure its not executed at runtime (although
>> it's probably optimized out anyways).
> 
> It 100% will be optimized out.
> 
>> It would also mean that we won't need a assert for every Trait, which
>> seems nice. So a macro sounds pretty good to me.
> 
> It depends, the macro impl needs to define its own function, which might
> be inefficient if one uses it a lot. But there is no way to be generic
> over traits, so there is no other way.
> 
> Let's see what the others think.

The error messages in the macro are slightly worse:
error[E0277]: `*mut ()` cannot be shared between threads safely
   --> rust/kernel/compile_assert.rs:40:18
    |
40  | assert_impl_all!(NotThreadSafe: Sync); // Fails because `NotThreadSafe` is not `Sync`
    |                  ^^^^^^^^^^^^^ `*mut ()` cannot be shared between threads safely
    |
    = help: within `PhantomData<*mut ()>`, the trait `Sync` is not implemented for `*mut ()`, which is required by `PhantomData<*mut ()>: Sync`
note: required because it appears within the type `PhantomData<*mut ()>`
   --> /home/chrisi/.rustup/toolchains/1.78-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/marker.rs:740:12
    |
740 | pub struct PhantomData<T: ?Sized>;
    |            ^^^^^^^^^^^
note: required by a bound in `assert_impl`
   --> rust/kernel/compile_assert.rs:34:48
    |
34  |             const fn assert_impl<T: ?Sized $(+ $trait)+>() {}
    |                                                ^^^^^^ required by this bound in `assert_impl`
...
40  | assert_impl_all!(NotThreadSafe: Sync); // Fails because `NotThreadSafe` is not `Sync`
    | ------------------------------------- in this macro invocation
    = note: this error originates in the macro `assert_impl_all` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 1 previous error

compared to the function:

error[E0277]: `*mut ()` cannot be shared between threads safely
   --> rust/kernel/compile_assert.rs:28:31
    |
28  | const _: () = { assert_sync::<NotThreadSafe>() };
    |                               ^^^^^^^^^^^^^ `*mut ()` cannot be shared between threads safely
    |
    = help: within `PhantomData<*mut ()>`, the trait `Sync` is not implemented for `*mut ()`, which is required by `PhantomData<*mut ()>: Sync`
note: required because it appears within the type `PhantomData<*mut ()>`
   --> /home/chrisi/.rustup/toolchains/1.78-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/marker.rs:740:12
    |
740 | pub struct PhantomData<T: ?Sized>;
    |            ^^^^^^^^^^^
note: required by a bound in `assert_sync`
   --> rust/kernel/compile_assert.rs:26:38
    |
26  | pub const fn assert_sync<T: ?Sized + Sync>() {}
    |                                      ^^^^ required by this bound in `assert_sync`

I guess I'll keep it as a function for now.

Cheers
Christian

Re: [PATCH] rust: add `assert_sync` function

[Christian Schrefl]

On 07.06.25 7:29 PM, Miguel Ojeda wrote:
> On Sat, Jun 7, 2025 at 3:02 PM Christian Schrefl
> <chrisi.schrefl@gmail.com> wrote:
>>
>> +/// Asserts that the given type is [`Sync`]. This check is done at compile time and does nothing
>> +/// at runtime.
> 
> I would split the second sentence into its own paragraph, so that the
> "short description" isn't long.

Alright.

> 
>> +/// Note that this is only intended to avoid regressions and for sanity checks.
> 
> Hmm... I am not sure about this sentence. A macro may want to call
> this to ensure something that is required for safety, for instance. Is
> that the "sanity check" part? In any case, it sounds like the sentence
> could be read as "this is not reliable for "other" things apart from
> just sanity checks", which may be confusing.
> 
> Could we perhaps clarify?

I added this because for some reason I was convinced that it would
be possible to cause post-monomorphization errors with this, but I 
not realize that's not a thing. I guess I'll just drop this sentence.

>> +/// # Examples
>> +/// ```
> 
> Please add a newline between these.> 
>> +///
>> +///
> 
> These newlines should be removed, otherwise they will be rendered.
> 
>> +/// // Do the assertion in a const block to make sure it won't be executed at runtime.
>> +/// const _:() = {
>> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
> 
> `Sync` and please use a period at the end. Also, I would suggest
> following our usual style and putting it at the top, i.e.
> 
>     // Succeeds because `i32` is `Sync`.
>     assert_sync::<i32>();
> 
>> +///
>> +/// ```
> 
> This one can be removed too.

Fixed these

Cheers
Christian

Re: [PATCH] rust: add `assert_sync` function

[Benno Lossin]

On Sat Jun 7, 2025 at 9:20 PM CEST, Christian Schrefl wrote:
> On 07.06.25 8:11 PM, Benno Lossin wrote:
>> On Sat Jun 7, 2025 at 5:54 PM CEST, Christian Schrefl wrote:
>>> On 07.06.25 5:42 PM, Benno Lossin wrote:
>>>> On Sat Jun 7, 2025 at 3:02 PM CEST, Christian Schrefl wrote:
>>>>> - Add `assert_send` as well.
>>>>
>>>> Sounds like a good idea.
>>>
>>> Should I already add this in V2 for this series?
>> 
>> If you want to then sure, but we can also wait until we have a use-case.
>> Also, let's finish the discussion about the macro idea below.
>> 
>>>>> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
>>>>> +///     // assert_sync::<NotThreadSafe>(); // Fails because `NotThreadSafe` is not `Sync`.
>>>>
>>>> Can you split this into two examples and mark the failing one with
>>>> `compile_fail`?
>>>
>>> I've tried it with `compile_fail` and it didn't work, I think
>>> that's not supported in (kernel) doc tests yet. 
>> 
>> Hmm, I thought that this worked... @Miguel any idea?
>> 
>>>> We also could provide a macro similar to [1].
>>>>
>>>> [1]: https://docs.rs/static_assertions/latest/static_assertions/
>>>
>>> You mean the `assert_impl_*!` macros?
>> 
>> Yes, but the others might also be useful from time to time.
>> >> That might make sense, with macros we would not need to write
>>> a const block to ensure its not executed at runtime (although
>>> it's probably optimized out anyways).
>> 
>> It 100% will be optimized out.
>> 
>>> It would also mean that we won't need a assert for every Trait, which
>>> seems nice. So a macro sounds pretty good to me.
>> 
>> It depends, the macro impl needs to define its own function, which might
>> be inefficient if one uses it a lot. But there is no way to be generic
>> over traits, so there is no other way.
>> 
>> Let's see what the others think.
>
> The error messages in the macro are slightly worse:
> error[E0277]: `*mut ()` cannot be shared between threads safely
>    --> rust/kernel/compile_assert.rs:40:18
>     |
> 40  | assert_impl_all!(NotThreadSafe: Sync); // Fails because `NotThreadSafe` is not `Sync`
>     |                  ^^^^^^^^^^^^^ `*mut ()` cannot be shared between threads safely
>     |
>     = help: within `PhantomData<*mut ()>`, the trait `Sync` is not implemented for `*mut ()`, which is required by `PhantomData<*mut ()>: Sync`
> note: required because it appears within the type `PhantomData<*mut ()>`
>    --> /home/chrisi/.rustup/toolchains/1.78-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/marker.rs:740:12
>     |
> 740 | pub struct PhantomData<T: ?Sized>;
>     |            ^^^^^^^^^^^
> note: required by a bound in `assert_impl`
>    --> rust/kernel/compile_assert.rs:34:48
>     |
> 34  |             const fn assert_impl<T: ?Sized $(+ $trait)+>() {}
>     |                                                ^^^^^^ required by this bound in `assert_impl`
> ...
> 40  | assert_impl_all!(NotThreadSafe: Sync); // Fails because `NotThreadSafe` is not `Sync`
>     | ------------------------------------- in this macro invocation
>     = note: this error originates in the macro `assert_impl_all` (in Nightly builds, run with -Z macro-backtrace for more info)
>
> error: aborting due to 1 previous error
>
> compared to the function:
>
> error[E0277]: `*mut ()` cannot be shared between threads safely
>    --> rust/kernel/compile_assert.rs:28:31
>     |
> 28  | const _: () = { assert_sync::<NotThreadSafe>() };
>     |                               ^^^^^^^^^^^^^ `*mut ()` cannot be shared between threads safely
>     |
>     = help: within `PhantomData<*mut ()>`, the trait `Sync` is not implemented for `*mut ()`, which is required by `PhantomData<*mut ()>: Sync`
> note: required because it appears within the type `PhantomData<*mut ()>`
>    --> /home/chrisi/.rustup/toolchains/1.78-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/marker.rs:740:12
>     |
> 740 | pub struct PhantomData<T: ?Sized>;
>     |            ^^^^^^^^^^^
> note: required by a bound in `assert_sync`
>    --> rust/kernel/compile_assert.rs:26:38
>     |
> 26  | pub const fn assert_sync<T: ?Sized + Sync>() {}
>     |                                      ^^^^ required by this bound in `assert_sync`
>
> I guess I'll keep it as a function for now.

Can we improve this by using a proc-macro instead and manipulating the
span? I honestly don't think the error is too bad.

---
Cheers,
Benno

Re: [PATCH] rust: add `assert_sync` function

[Christian Schrefl]

On 08.06.25 12:31 AM, Benno Lossin wrote:
> On Sat Jun 7, 2025 at 9:20 PM CEST, Christian Schrefl wrote:
>> On 07.06.25 8:11 PM, Benno Lossin wrote:
>>> On Sat Jun 7, 2025 at 5:54 PM CEST, Christian Schrefl wrote:
>>>> On 07.06.25 5:42 PM, Benno Lossin wrote:
>>>>> On Sat Jun 7, 2025 at 3:02 PM CEST, Christian Schrefl wrote:
>>>>>> - Add `assert_send` as well.
>>>>>
>>>>> Sounds like a good idea.
>>>>
>>>> Should I already add this in V2 for this series?
>>>
>>> If you want to then sure, but we can also wait until we have a use-case.
>>> Also, let's finish the discussion about the macro idea below.
>>>
>>>>>> +///     assert_sync::<i32>(); // Succeeds because `i32` is Sync
>>>>>> +///     // assert_sync::<NotThreadSafe>(); // Fails because `NotThreadSafe` is not `Sync`.
>>>>>
>>>>> Can you split this into two examples and mark the failing one with
>>>>> `compile_fail`?
>>>>
>>>> I've tried it with `compile_fail` and it didn't work, I think
>>>> that's not supported in (kernel) doc tests yet. 
>>>
>>> Hmm, I thought that this worked... @Miguel any idea?
>>>
>>>>> We also could provide a macro similar to [1].
>>>>>
>>>>> [1]: https://docs.rs/static_assertions/latest/static_assertions/
>>>>
>>>> You mean the `assert_impl_*!` macros?
>>>
>>> Yes, but the others might also be useful from time to time.
>>>>> That might make sense, with macros we would not need to write
>>>> a const block to ensure its not executed at runtime (although
>>>> it's probably optimized out anyways).
>>>
>>> It 100% will be optimized out.
>>>
>>>> It would also mean that we won't need a assert for every Trait, which
>>>> seems nice. So a macro sounds pretty good to me.
>>>
>>> It depends, the macro impl needs to define its own function, which might
>>> be inefficient if one uses it a lot. But there is no way to be generic
>>> over traits, so there is no other way.
>>>
>>> Let's see what the others think.
>>
>> The error messages in the macro are slightly worse:
>> error[E0277]: `*mut ()` cannot be shared between threads safely
>>    --> rust/kernel/compile_assert.rs:40:18
>>     |
>> 40  | assert_impl_all!(NotThreadSafe: Sync); // Fails because `NotThreadSafe` is not `Sync`
>>     |                  ^^^^^^^^^^^^^ `*mut ()` cannot be shared between threads safely
>>     |
>>     = help: within `PhantomData<*mut ()>`, the trait `Sync` is not implemented for `*mut ()`, which is required by `PhantomData<*mut ()>: Sync`
>> note: required because it appears within the type `PhantomData<*mut ()>`
>>    --> /home/chrisi/.rustup/toolchains/1.78-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/marker.rs:740:12
>>     |
>> 740 | pub struct PhantomData<T: ?Sized>;
>>     |            ^^^^^^^^^^^
>> note: required by a bound in `assert_impl`
>>    --> rust/kernel/compile_assert.rs:34:48
>>     |
>> 34  |             const fn assert_impl<T: ?Sized $(+ $trait)+>() {}
>>     |                                                ^^^^^^ required by this bound in `assert_impl`
>> ...
>> 40  | assert_impl_all!(NotThreadSafe: Sync); // Fails because `NotThreadSafe` is not `Sync`
>>     | ------------------------------------- in this macro invocation
>>     = note: this error originates in the macro `assert_impl_all` (in Nightly builds, run with -Z macro-backtrace for more info)
>>
>> error: aborting due to 1 previous error
>>
>> compared to the function:
>>
>> error[E0277]: `*mut ()` cannot be shared between threads safely
>>    --> rust/kernel/compile_assert.rs:28:31
>>     |
>> 28  | const _: () = { assert_sync::<NotThreadSafe>() };
>>     |                               ^^^^^^^^^^^^^ `*mut ()` cannot be shared between threads safely
>>     |
>>     = help: within `PhantomData<*mut ()>`, the trait `Sync` is not implemented for `*mut ()`, which is required by `PhantomData<*mut ()>: Sync`
>> note: required because it appears within the type `PhantomData<*mut ()>`
>>    --> /home/chrisi/.rustup/toolchains/1.78-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/marker.rs:740:12
>>     |
>> 740 | pub struct PhantomData<T: ?Sized>;
>>     |            ^^^^^^^^^^^
>> note: required by a bound in `assert_sync`
>>    --> rust/kernel/compile_assert.rs:26:38
>>     |
>> 26  | pub const fn assert_sync<T: ?Sized + Sync>() {}
>>     |                                      ^^^^ required by this bound in `assert_sync`
>>
>> I guess I'll keep it as a function for now.
> 
> Can we improve this by using a proc-macro instead and manipulating the
> span? I honestly don't think the error is too bad.

I don't see any point in paying the compile time hit for a proc macro.

The Error is not that bad, just a bit worse. I just don't really see the
point since this is only really need for marker traits and realistically
only for `Send` and `Sync`. Also the macro would create a function
definition for every invocation which would be a (very) small compile time
hit. So I think that we should just add the `Send` and `Sync` functions for
now and reconsider changing to a macro once/if more than these two is
actually needed. 

Cheers
Christian.

Re: [PATCH] rust: add `assert_sync` function

[Benno Lossin]

On Sun Jun 8, 2025 at 1:38 AM CEST, Christian Schrefl wrote:
> I don't see any point in paying the compile time hit for a proc macro.
>
> The Error is not that bad, just a bit worse. I just don't really see the
> point since this is only really need for marker traits and realistically
> only for `Send` and `Sync`. Also the macro would create a function
> definition for every invocation which would be a (very) small compile time
> hit. So I think that we should just add the `Send` and `Sync` functions for
> now and reconsider changing to a macro once/if more than these two is
> actually needed. 

Sounds good.

---
Cheers,
Benno