Re: [PATCH v4 5/7] rust: alloc: add Vec::retain

[Alice Ryhl <aliceryhl@google.com>]

On Thu, May 01, 2025 at 01:30:20PM +0200, Danilo Krummrich wrote:
> On Thu, May 01, 2025 at 11:10:12AM +0000, Alice Ryhl wrote:
> > On Wed, Apr 30, 2025 at 06:26:05PM +0200, Danilo Krummrich wrote:
> > > On Tue, Apr 29, 2025 at 02:44:25PM +0000, Alice Ryhl wrote:
> > > > +#[macros::kunit_tests(rust_kvec_kunit)]
> > > > +mod tests {
> > > > +    use super::*;
> > > > +    use crate::prelude::*;
> > > > +
> > > > +    #[test]
> > > > +    fn test_kvec_retain() {
> > >
> > > Can we have this return a Result, like doctests can do?
> > 
> > I get warning when I try that:
> > 
> > warning: unused `core::result::Result` that must be used
> >     --> rust/kernel/alloc/kvec.rs:1232:1
> >      |
> > 1232 | #[macros::kunit_tests(rust_kvec_kunit)]
> >      | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >      |
> >      = note: this `Result` may be an `Err` variant, which should be handled
> >      = note: `#[warn(unused_must_use)]` on by default
> >      = note: this warning originates in the attribute macro `macros::kunit_tests`
> >              (in Nightly builds, run with -Z macro-backtrace for more info)
> 
> Yes, I'm aware, I tried playing with that myself. I really meant the question as
> I wrote, not as "Can you please change that?". :-) Sorry for the confusion.

One downside is that returning a Result doesn't show the line of code
where it failed.

> > > > +        /// Verify correctness for one specific function.
> > > > +        #[expect(clippy::needless_range_loop)]
> > > > +        fn verify(c: &[bool]) {
> > > > +            let mut vec1: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > > > +            let mut vec2: KVec<usize> = KVec::with_capacity(c.len(), GFP_KERNEL).unwrap();
> > > > +
> > > > +            for i in 0..c.len() {
> > > > +                vec1.push_within_capacity(i).unwrap();
> > > > +                if c[i] {
> > > > +                    vec2.push_within_capacity(i).unwrap();
> > > > +                }
> > > > +            }
> > > > +
> > > > +            vec1.retain(|i| c[*i]);
> > > > +
> > > > +            assert_eq!(vec1, vec2);
> > >
> > > Don't we have macros around kunit_assert!() and kunit_assert_eq() outside of
> > > doctests (i.e. dedicated kunit tests)?
> > >
> > > I much prefer their output over the kernel panic we get with the "normal"
> > > asserts, unwraps, etc.
> > >
> > > Consistently sticking to the same output on failure makes it easier to catch and
> > > easier to setup CI environments.
> > 
> > The documentation for those macros says "Public but hidden since it
> > should only be used from generated tests." so I don't think I'm supposed
> > to use them.
> 
> Same here, that's more a fundamental question, rather than something for you to
> change right away.
> 
> I really like the way doctests implement the assert macros and how they appear
> in the kernel log compared to panics through the "real" assert ones, unwraps,
> etc.
> 
> I also think that avoiding things that directly panic in doctests (i.e. example
> code) is the correct thing to do. For KUnit tests it's probably less important,
> since they don't directly serve as sample code.
> 
> So, I wonder what's our take on that. Do we want to have KUnit and doctests
> aligned? I think that'd be a good thing.

I think that both of these would be reasonable to fix. Also the fact
that I had to do #[macros::kunit_tests()] instead of just #[kunit_tests()].

Alice