Message-ID: <eba1cfdd-3756-4f8e-a2b2-e15b8b8b4a18@gmail.com>
Date: Sun, 8 Jun 2025 01:38:04 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Benno Lossin <lossin@...nel.org>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH] rust: add `assert_sync` function

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.

Powered by blists - more mailing lists