Re: [PATCH 08/11] rust: macros: convert `#[kunit_tests]` macro to use `syn`

[Gary Guo <gary@garyguo.net>]

On Sun, 4 Jan 2026 18:38:53 -0500
Tamir Duberstein <tamird@gmail.com> wrote:

> On Thu, Dec 11, 2025 at 2:30 PM Gary Guo <gary@kernel.org> wrote:
> >
> > From: Gary Guo <gary@garyguo.net>
> >
> > This allows significant cleanups.
> >
> > Signed-off-by: Gary Guo <gary@garyguo.net>
> > ---
> >  rust/macros/kunit.rs | 274 +++++++++++++++++++------------------------
> >  rust/macros/lib.rs   |   6 +-
> >  2 files changed, 123 insertions(+), 157 deletions(-)
> >
> > diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
> > index 7427c17ee5f5c..516219f5b1356 100644
> > --- a/rust/macros/kunit.rs
> > +++ b/rust/macros/kunit.rs
> > ...
> > +    // Make the entire module gated behind `CONFIG_KUNIT`.
> > +    module
> > +        .attrs
> > +        .insert(0, parse_quote!(#[cfg(CONFIG_KUNIT="y")]));  
> 
> Does this need to be the first attribute? I think it can just be
> pushed to the end.

This is the current behaviour, and I think it should be the first.
Other attributes shouldn't need to be processed if kunit is not enabled.

> 
> 
> >
> > -    // Add `#[cfg(CONFIG_KUNIT="y")]` before the module declaration.
> > -    let config_kunit = "#[cfg(CONFIG_KUNIT=\"y\")]".to_owned().parse().unwrap();
> > -    tokens.insert(
> > -        0,
> > -        TokenTree::Group(Group::new(Delimiter::None, config_kunit)),
> > -    );
> > +    let mut processed_items = Vec::new();
> > +    let mut test_cases = Vec::new();
> >
> >      // Generate the test KUnit test suite and a test case for each `#[test]`.
> > +    //
> >      // The code generated for the following test module:
> >      //
> >      // ```
> > @@ -110,98 +79,93 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
> >      //
> >      // ::kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
> >      // ```
> > -    let mut kunit_macros = "".to_owned();
> > -    let mut test_cases = "".to_owned();
> > -    let mut assert_macros = "".to_owned();
> > -    let path = crate::helpers::file();
> > -    let num_tests = tests.len();
> > -    for (test, cfg_attr) in tests {
> > -        let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{test}");
> > -        // Append any `cfg` attributes the user might have written on their tests so we don't
> > -        // attempt to call them when they are `cfg`'d out. An extra `use` is used here to reduce
> > -        // the length of the assert message.
> > -        let kunit_wrapper = format!(
> > -            r#"unsafe extern "C" fn {kunit_wrapper_fn_name}(_test: *mut ::kernel::bindings::kunit)
> > -            {{
> > -                (*_test).status = ::kernel::bindings::kunit_status_KUNIT_SKIPPED;
> > -                {cfg_attr} {{
> > -                    (*_test).status = ::kernel::bindings::kunit_status_KUNIT_SUCCESS;
> > -                    use ::kernel::kunit::is_test_result_ok;
> > -                    assert!(is_test_result_ok({test}()));
> > +    //
> > +    // Non-function items (e.g. imports) are preserved.
> > +    for item in module_items {
> > +        let Item::Fn(mut f) = item else {
> > +            processed_items.push(item);
> > +            continue;
> > +        };
> > +
> > +        // TODO: Replace below with `extract_if` when MSRV is bumped above 1.85.
> > +        // Remove `#[test]` attributes applied on the function and count if any.  
> 
> What does "count if any" mean here?

Sorry, I think the last half of the sentence was missing. I meant
"count if any is removed".

> 
> > +        if !f.attrs.iter().any(|attr| attr.path().is_ident("test")) {
> > +            processed_items.push(Item::Fn(f));
> > +            continue;
> > +        }
> > +        f.attrs.retain(|attr| !attr.path().is_ident("test"));  
> 
> Can this code be something like this:
> 
>         let before = f.attrs.len();
>         f.attrs.retain(|attr| !attr.path().is_ident("test"));
>         let after = f.attrs.len();
> 
>         if after == before {
>             processed_items.push(Item::Fn(f));
>             continue;
>         }

Indeed looks better.

> 
> > +
> > +        let test = f.sig.ident.clone();
> > +
> > +        // Retrieve `#[cfg]` applied on the function which needs to be present on derived items too.
> > +        let cfg_attrs: Vec<_> = f
> > +            .attrs
> > +            .iter()
> > +            .filter(|attr| attr.path().is_ident("cfg"))
> > +            .cloned()
> > +            .collect();
> > +
> > +        // Before the test, override usual `assert!` and `assert_eq!` macros with ones that call
> > +        // KUnit instead.
> > +        let test_str = test.to_string();
> > +        let path = crate::helpers::file();
> > +        processed_items.push(parse_quote! {
> > +            #[allow(unused)]
> > +            macro_rules! assert {
> > +                ($cond:expr $(,)?) => {{
> > +                    kernel::kunit_assert!(#test_str, #path, 0, $cond);
> > +                }}
> > +            }
> > +        });
> > +        processed_items.push(parse_quote! {
> > +            #[allow(unused)]
> > +            macro_rules! assert_eq {
> > +                ($left:expr, $right:expr $(,)?) => {{
> > +                    kernel::kunit_assert_eq!(#test_str, #path, 0, $left, $right);
> >                  }}
> > -            }}"#,
> > +            }
> > +        });  
> 
> Am I reading this right that the macros will be repeatedly redefined
> before each test? Could we put them inside each test body instead?

Indeed. The `test_str` is different for each test so this needs to be
redefined for each test. I experimented about better approaches but
none of them are too satisfactory. I think the existing approach is not
unreasonable.

Putting them into test body sounds more reasonable (I suppose the
existing impl doesn't want to parse functions), but I'll leave it to
the future as this commit is focusing on converting with minimal
behavioural change.

Best,
Gary